Conversation
4a7fc18 to
f39ef05
Compare
f39ef05 to
267785b
Compare
There was a problem hiding this comment.
I think it would be nice to have some indexing tests to ensure that we get the right data structures initially.
For example:
# Bar and CONST don't depend on anything. There are no code changes that
# can modify what they mean other than changing them directly
module Bar; end
CONST = 1
# Foo has no dependencies
module Foo
# This Bar depends on Foo
# The Baz depends on both Bar and Foo
class Bar::Baz
# CONST depends on Foo and Baz
CONST
end
endNote that the key is ensuring that the chain of dependents can be followed. For example, since Baz already depends on Bar, remembering that CONST depends on Baz is enough. We don't need to tie CONST to Bar. If Bar gets invalidated, it will invalidate Baz and that will in turn invalidate CONST.
| /// The three steps must run in this order: | ||
| /// 1. `invalidate` -- reads resolved names and declaration state to determine what to invalidate | ||
| /// 2. `remove_document_data` -- removes old refs/defs/names/strings from maps | ||
| /// 3. `extend` -- merges the new `LocalGraph` into the now-clean graph |
There was a problem hiding this comment.
No need to do it in this PR, but I think this method now deserves a more descriptive name. Maybe consume_document_changes or ingest_local_graph. It does a whole lot more than just "update".
| self.remove_definitions_for_document(&document); | ||
| let old_document = self.documents.remove(&uri_id); | ||
|
|
||
| self.invalidate(old_document.as_ref(), Some(&other)); |
There was a problem hiding this comment.
It might be worth adding a fast path for the initial indexing on boot (which can trigger no invalidation and no removal of data).
Maybe a boolean flag for skipping invalidation.
rust/rubydex/src/model/graph.rs
Outdated
| && declaration.remove_definition(def_id) | ||
| { | ||
| declaration.clear_diagnostics(); |
There was a problem hiding this comment.
This is doing two data removal operations: removing a definition and clearing diagnostics. Does it make sense to move this to remove_document_data? Or will that result in duplicate work?
| && let Some(nesting_id) = name_ref.nesting() | ||
| && let Some(NameRef::Resolved(resolved)) = self.names.get(nesting_id) |
There was a problem hiding this comment.
Can you clarify that document change this is accounting for? I'm having a hard time understanding it.
A constant reference was changed and we're enqueuing invalidation for the reference's nesting declaration.
There was a problem hiding this comment.
This is for
class Foo
include Bar # constant reference, when added we invalidate Foo entirely for now
endI've added comments for it.
rust/rubydex/src/model/graph.rs
Outdated
|
|
||
| self.declarations.remove(&decl_id); | ||
| } else { | ||
| // Ancestor-stale mode |
There was a problem hiding this comment.
Added more comments. Basically, it's triggered in
class Foo
include Bar # this is added/removed
endSo we simply change ancestors/descendents update in this branch.
| } | ||
| } | ||
|
|
||
| self.declarations.remove(&decl_id); |
There was a problem hiding this comment.
This is also doing data removal. Maybe this is fine, but I'm calling it out because the method documentation mentions a separation between invalidation and removal.
There was a problem hiding this comment.
I currently treat declaration removal as invalidation, kinda similar to unresolving a name. We remove (unresolve) the declaration here if we found it has no underlying definitions anymore.
The underlying materials (definitions, constant references, names...etc.) are only removed in remove_document_data.
| return; | ||
| }; | ||
|
|
||
| // Remove self from each ancestor's descendant set |
There was a problem hiding this comment.
I could be misunderstanding and maybe this was an existing bug, but removing self is not enough. The entire ancestor chain of self must be removed from descendants. However, we should absolutely not try to perform this removal because there are module deduping rules that you cannot possibly account for with a removal.
Whenever a declaration gets invalidated, we always need to invalidate the ancestors of all descendants. In both branches of this method.
There was a problem hiding this comment.
Both paths will update the ancestors. I've updated the document to make it more clear: invalidate_declaration either "remove/rebuild" or "update" a declaration. In both paths we update ancestors.
This also means there are optimization opportunities in both paths we can do later, which I also included in comments.
47c1b4c to
de13a32
Compare
Introduces a worklist-based invalidation engine that cascades changes through the graph when documents are updated or deleted. Uses ChildName/NestedName edges from the name_dependents index to propagate invalidation with two distinct modes: - Structural cascade (UnresolveName): declaration removed or scope broken - Ancestor cascade (UnresolveReferences): ancestor chain changed Replaces the has_unresolved_dependency runtime check with explicit invalidation variants determined at queue time.
de13a32 to
d560ba6
Compare
Extracted from #638 (incremental invalidation). This PR contains the core invalidation engine; the incremental resolver and
without_resolutionflag will follow as separate PRs.Replaces the old
remove_definitions_for_document+invalidate_ancestor_chainsapproach with a targeted invalidation engine. When a file is updated or deleted, the engine traces through thename_dependentsreverse index to invalidate only the affected declarations, names, and references — instead of requiring a full graph rebuild.How it works
update()anddelete_document()now run a three-step process:invalidate— seeds a worklist from old/new definitions and referencesremove_document_data— cleans up raw data (defs, refs, names, strings) from mapsextend— merges new content and accumulates pending work itemsThe worklist processes two kinds of items:
Declaration— either removes the declaration (cascading to members, singleton, descendants) or clears its ancestor chain and queues descendantsUnresolveName/UnresolveReferences— unresolves names or just their references depending on whether the structural dependency broke or only the ancestor context changedThe
name_dependentsreverse index (built during indexing inLocalGraph) maps each name to the definitions and references that depend on it, enabling efficient invalidation tracing without scanning the full graph.Note: the resolver still does
clear_declarations+ full rebuild. Wiring it to drainpending_workincrementally is a follow-up.