Add `name_dependents` reverse index for incremental invalidation by st0012 · Pull Request #646 · Shopify/rubydex

st0012 · 2026-03-05T22:00:59Z

Build a reverse index during indexing that tracks which definitions, references, and names depend on each NameId. Name-to-name edges encode the dependency type at registration time:

ChildName: parent_scope relationship (structural dependency)
NestedName: nesting relationship (reference-only dependency)
Definition/Reference: direct dependents of the name

This index will be consumed by the incremental invalidation engine to efficiently cascade changes without scanning the full graph.

vinistock · 2026-03-05T22:14:36Z

rust/rubydex/src/model/graph.rs

+    /// This name's `parent_scope` is the key name — structural dependency.
+    ChildName(NameId),
+    /// This name's `nesting` is the key name — reference-only dependency.
+    NestedName(NameId),


Is there a logic need for this distinction? Or could we just merge them into Name for dependent names?

In the next PR (#641), we will have something like:

NameDependent::ChildName(id) => queue.push(InvalidationItem::UnresolveName(*id)), NameDependent::NestedName(id) => queue.push(InvalidationItem::UnresolveReferences(*id)),

The main difference is that UnresolveReferences will only unresolve constant references:

class Foo; end class Bar Foo class Baz; end # Bar has [NestedName(Foo), ChildName(Baz)] end

When Bar's ancestors changes:

ChildName(Baz) will trigger a total invalidation on Baz

NestedName(Foo) will only invalide the Foo reference

If the ancestors of Bar change, all constant references inside of the namespace have to be invalidated. I suspect that we can merge these two because the information of what needs to be invalidated is already encoded in the hashmap.

If it's ok, I'd like to merge this as it is, and see if the invalidation algo looks better/worse without the 2nd enum after some rounds of reviews.

rust/rubydex/src/indexing/local_graph.rs

vinistock · 2026-03-05T22:18:20Z

rust/rubydex/src/indexing/ruby_indexer.rs

 }
+
+#[cfg(test)]
+mod name_dependent_tests {


Not opposed to it, but is it common in Rust to split tests groups in different modules?

Yes. ZJIT does this as well. IMO it's a nice way to scope test helpers.

It's too bad we didn't start out like that. We should've created modules for indexing each individual type of thing. Same for resolution, all ancestors tests could be separate.

Anyway, not worth the investment to refactor immediately, but something we may want later.

I opened #649 in case anyway wants to give it a try.

rust/rubydex/src/indexing/ruby_indexer.rs

vinistock

I'm still not 100% sure we need the two variants for names, but it looks good

rust/rubydex/src/indexing/local_graph.rs

rust/rubydex/src/indexing/ruby_indexer.rs

rust/rubydex/src/model/graph.rs

Build a reverse index during indexing that tracks which definitions, references, and names depend on each NameId. Name-to-name edges encode the dependency type at registration time: - ChildName: parent_scope relationship (structural dependency) - NestedName: nesting relationship (reference-only dependency) - Definition/Reference: direct dependents of the name This index will be consumed by the incremental invalidation engine to efficiently cascade changes without scanning the full graph.

Morriar · 2026-03-24T16:56:23Z

rust/rubydex/src/model/graph.rs

+        for (name_id, deps) in name_dependents {
+            let global_deps = self.name_dependents.entry(name_id).or_default();
+            for dep in deps {
+                if !global_deps.contains(&dep) {


The dedup check here is O(n) per dep, making the whole merge loop O(n²) for any name with many dependents. For the initial indexing pass where we're merging many local graphs, this could be costly on large codebases.

I wonder if we should use a HashSet during the merge and then convert back to Vec, or just rely on the fact that duplicates shouldn't occur at all if the local graph was built correctly (given that a local graph only covers one file)?

If duplicates are theoretically impossible, a debug_assert here would be more appropriate.

Morriar · 2026-03-24T16:56:23Z

rust/rubydex/src/test_utils/graph_test.rs

+    ///
+    /// Panics if no names match the given path.
+    #[must_use]
+    pub fn find_name_ids(&self, path: &str) -> Vec<NameId> {


Both GraphTest and LocalGraphTest have identical implementations of find_name_ids, name_dependents_for, name_str, and dependent_name_str. Any way we could avoid the duplication here? A shared trait or a helper module would keep these in sync automatically.

Morriar · 2026-03-24T16:56:23Z

rust/rubydex/src/test_utils/graph_test.rs

+                }
+                match parent {
+                    None => name_ref.parent_scope().as_ref().is_none(),
+                    Some(p) => name_ref.parent_scope().as_ref().is_some_and(|ps_id| {


Two thoughts:

find_name_ids with a path like "Bar::Baz" only checks one level up — it verifies the immediate parent's string is "Bar", but not that Bar is itself a top-level name. So Foo::Bar::Baz would also match "Bar::Baz" since the parent's str is "Bar" regardless of where Bar lives. Is that intentional?

Same question for LocalGraphTest::find_name_ids (same code).

st0012 self-assigned this Mar 5, 2026

st0012 force-pushed the add-name-dependents-index branch from 0993425 to 4fd9a14 Compare March 5, 2026 22:03

st0012 marked this pull request as ready for review March 5, 2026 22:03

st0012 requested a review from a team as a code owner March 5, 2026 22:03

vinistock reviewed Mar 5, 2026

View reviewed changes

st0012 force-pushed the add-name-dependents-index branch 4 times, most recently from 19b1837 to eac46bb Compare March 5, 2026 23:48

vinistock approved these changes Mar 6, 2026

View reviewed changes

rust/rubydex/src/indexing/local_graph.rs Outdated Show resolved Hide resolved

rust/rubydex/src/indexing/ruby_indexer.rs Outdated Show resolved Hide resolved

rust/rubydex/src/model/graph.rs Outdated Show resolved Hide resolved

rust/rubydex/src/model/graph.rs Outdated Show resolved Hide resolved

st0012 mentioned this pull request Mar 6, 2026

Use test modules to group tests & their helpers #649

Open

st0012 force-pushed the add-name-dependents-index branch 2 times, most recently from 3fc7d48 to 21785c9 Compare March 6, 2026 17:06

st0012 force-pushed the add-name-dependents-index branch from 21785c9 to 7ff0dbd Compare March 6, 2026 17:16

st0012 merged commit 74ea870 into main Mar 6, 2026
30 checks passed

st0012 deleted the add-name-dependents-index branch March 6, 2026 18:09

Morriar reviewed Mar 24, 2026

View reviewed changes

vinistock mentioned this pull request Mar 26, 2026

Support consuming document changes incrementally #330

Open

Conversation

st0012 commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vinistock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vinistock left a comment •

edited

Loading