From 3bc96fb68b850c6e114295e0b814b3b9250315bc Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Mon, 2 Mar 2026 19:55:00 +0000 Subject: [PATCH 1/2] Add unresolve primitives and declaration_to_names reverse index Add methods for unwinding name resolution during incremental updates: - `unresolve_name`: converts a Resolved name back to Unresolved - `unresolve_reference`: detaches a reference from its declaration and unresolves the underlying name - `remove_name`: removes a name and cleans up reverse indices - `detach_name_from_declaration`: shared helper for reverse index cleanup Add `declaration_to_names` reverse index that tracks which names resolve to each declaration. Populated during `record_resolved_name`, cleaned up by the unresolve/remove primitives. Integrate `unresolve_reference` into `remove_definitions_for_document` to properly detach references during document updates. --- rust/rubydex/src/model/graph.rs | 114 ++++++++++++++++++++++++++++++-- 1 file changed, 108 insertions(+), 6 deletions(-) diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index f56ad2f6..010477b7 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -40,6 +40,10 @@ pub struct Graph { /// The position encoding used for LSP line/column locations. Not related to the actual encoding of the file position_encoding: Encoding, + + /// Reverse index: for each declaration, which `NameId`s resolved to it. + /// Used during incremental invalidation to find names that need unresolving when a declaration is removed. + declaration_to_names: IdentityHashMap>, } impl Graph { @@ -54,6 +58,7 @@ impl Graph { constant_references: IdentityHashMap::default(), method_references: IdentityHashMap::default(), position_encoding: Encoding::default(), + declaration_to_names: IdentityHashMap::default(), } } @@ -462,6 +467,59 @@ impl Graph { &self.names } + /// Removes `name_id` from the `declaration_to_names` reverse index for the given declaration, + /// cleaning up the entry entirely if no names remain. + fn detach_name_from_declaration(&mut self, declaration_id: DeclarationId, name_id: NameId) { + if let Some(name_set) = self.declaration_to_names.get_mut(&declaration_id) { + name_set.remove(&name_id); + if name_set.is_empty() { + self.declaration_to_names.remove(&declaration_id); + } + } + } + + /// Converts a `Resolved` `NameRef` back to `Unresolved`, preserving the original `Name` data. + /// Returns the `DeclarationId` it was previously resolved to, if any. + fn unresolve_name(&mut self, name_id: NameId) -> Option { + let name_ref = self.names.get(&name_id)?; + + match name_ref { + NameRef::Resolved(resolved) => { + let declaration_id = *resolved.declaration_id(); + let name = resolved.name().clone(); + self.names.insert(name_id, NameRef::Unresolved(Box::new(name))); + self.detach_name_from_declaration(declaration_id, name_id); + Some(declaration_id) + } + NameRef::Unresolved(_) => None, + } + } + + /// Unresolves a constant reference: removes it from the target declaration's reference set + /// and unresolves its underlying name. + fn unresolve_reference(&mut self, reference_id: ReferenceId) -> Option { + let constant_ref = self.constant_references.get(&reference_id)?; + let name_id = *constant_ref.name_id(); + + if let Some(old_decl_id) = self.unresolve_name(name_id) { + if let Some(declaration) = self.declarations.get_mut(&old_decl_id) { + declaration.remove_reference(&reference_id); + } + Some(old_decl_id) + } else { + None + } + } + + /// Removes a name from the graph and cleans up the `declaration_to_names` reverse index. + fn remove_name(&mut self, name_id: NameId) { + if let Some(NameRef::Resolved(resolved)) = self.names.get(&name_id) { + self.detach_name_from_declaration(*resolved.declaration_id(), name_id); + } + + self.names.remove(&name_id); + } + /// Decrements the ref count for a name and removes it if the count reaches zero. /// /// This does not recursively untrack `parent_scope` or `nesting` names. @@ -469,7 +527,7 @@ impl Graph { if let Some(name_ref) = self.names.get_mut(&name_id) { let string_id = *name_ref.str(); if !name_ref.decrement_ref_count() { - self.names.remove(&name_id); + self.remove_name(name_id); } self.untrack_string(string_id); } @@ -579,6 +637,11 @@ impl Graph { let resolved_name = NameRef::Resolved(Box::new(ResolvedName::new(*unresolved, declaration_id))); self.names.insert(name_id, resolved_name); } + + self.declaration_to_names + .entry(declaration_id) + .or_default() + .insert(name_id); } NameRef::Resolved(_) => { // TODO: consider if this is a valid scenario with the resolution phase design. Either collect @@ -682,12 +745,9 @@ impl Graph { } for ref_id in document.constant_references() { + self.unresolve_reference(*ref_id); + if let Some(constant_ref) = self.constant_references.remove(ref_id) { - if let Some(NameRef::Resolved(resolved)) = self.names.get(constant_ref.name_id()) - && let Some(declaration) = self.declarations.get_mut(resolved.declaration_id()) - { - declaration.remove_reference(ref_id); - } self.untrack_name(*constant_ref.name_id()); } } @@ -731,6 +791,7 @@ impl Graph { self.invalidate_ancestor_chains(declarations_to_invalidate_ancestor_chains); for declaration_id in declarations_to_delete { + self.declaration_to_names.remove(&declaration_id); self.declarations.remove(&declaration_id); } @@ -1664,4 +1725,45 @@ mod tests { assert_eq!(12, context.graph().names.len()); assert_eq!(41, context.graph().strings.len()); } + + #[test] + fn deleting_sole_definition_removes_the_name_entirely() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "module Foo; end\nBar"); + context.index_uri("file:///bar.rb", "module Bar; end"); + context.resolve(); + + // Bar declaration should have 1 reference (from foo.rb) + let bar_decl = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap(); + assert_eq!(bar_decl.references().len(), 1); + + // Update foo.rb to remove the Bar reference + context.index_uri("file:///foo.rb", "module Foo; end"); + context.resolve(); + + let bar_decl = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap(); + assert!( + bar_decl.references().is_empty(), + "Reference to Bar should be detached from declaration" + ); + + // Delete bar.rb — the Bar name should be fully removed + let bar_name_id = Name::new(StringId::from("Bar"), ParentScope::None, None).id(); + context.index_uri("file:///bar.rb", ""); + context.resolve(); + + assert!( + context + .graph() + .declarations() + .get(&DeclarationId::from("Bar")) + .is_none(), + "Bar declaration should be removed" + ); + assert!( + context.graph().names().get(&bar_name_id).is_none(), + "Bar name should be removed from the names map" + ); + } } From 417635f3ea7452b5c3d840e5d15641aea2653d51 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Tue, 3 Mar 2026 20:19:49 +0000 Subject: [PATCH 2/2] Add incremental invalidation for the graph --- ext/rubydex/graph.c | 20 + lib/rubydex/graph.rb | 5 +- rust/rubydex-mcp/src/server.rs | 2 +- rust/rubydex-sys/src/graph_api.rs | 26 +- rust/rubydex/src/indexing.rs | 1 + rust/rubydex/src/indexing/local_graph.rs | 30 + rust/rubydex/src/main.rs | 2 +- rust/rubydex/src/model/declaration.rs | 4 + rust/rubydex/src/model/graph.rs | 1510 +++++++++++++++++++-- rust/rubydex/src/query.rs | 18 +- rust/rubydex/src/resolution.rs | 189 ++- rust/rubydex/src/test_utils/graph_test.rs | 23 +- test/graph_test.rb | 43 + 13 files changed, 1634 insertions(+), 239 deletions(-) diff --git a/ext/rubydex/graph.c b/ext/rubydex/graph.c index 905ee1d1..cb1a25cd 100644 --- a/ext/rubydex/graph.c +++ b/ext/rubydex/graph.c @@ -356,6 +356,24 @@ static VALUE rdxr_graph_set_encoding(VALUE self, VALUE encoding) { return Qnil; } +// Graph#without_resolution=: (Boolean) -> void +// Configures the graph to skip accumulating resolution work items +static VALUE rdxr_graph_set_without_resolution(VALUE self, VALUE without_resolution) { + void *graph; + TypedData_Get_Struct(self, void *, &graph_type, graph); + rdx_graph_set_without_resolution(graph, RTEST(without_resolution)); + return Qnil; +} + +// Graph#without_resolution?: () -> Boolean +// Returns whether the graph is configured to skip accumulating resolution work items +static VALUE rdxr_graph_without_resolution(VALUE self) { + void *graph; + TypedData_Get_Struct(self, void *, &graph_type, graph); + + return rdx_graph_without_resolution(graph) ? Qtrue : Qfalse; +} + // Graph#resolve_constant: (String, Array[String]) -> Declaration? // Runs the resolver on a single constant reference to determine what it points to static VALUE rdxr_graph_resolve_constant(VALUE self, VALUE const_name, VALUE nesting) { @@ -498,6 +516,8 @@ void rdxi_initialize_graph(VALUE mRubydex) { rb_define_method(cGraph, "[]", rdxr_graph_aref, 1); rb_define_method(cGraph, "search", rdxr_graph_search, 1); rb_define_method(cGraph, "encoding=", rdxr_graph_set_encoding, 1); + rb_define_method(cGraph, "without_resolution=", rdxr_graph_set_without_resolution, 1); + rb_define_method(cGraph, "without_resolution?", rdxr_graph_without_resolution, 0); rb_define_method(cGraph, "resolve_require_path", rdxr_graph_resolve_require_path, 2); rb_define_method(cGraph, "require_paths", rdxr_graph_require_paths, 1); } diff --git a/lib/rubydex/graph.rb b/lib/rubydex/graph.rb index 672f9ae9..e086b35a 100644 --- a/lib/rubydex/graph.rb +++ b/lib/rubydex/graph.rb @@ -16,9 +16,10 @@ class Graph #: String attr_accessor :workspace_path - #: (?workspace_path: String) -> void - def initialize(workspace_path: Dir.pwd) + #: (?workspace_path: String, ?without_resolution: bool) -> void + def initialize(workspace_path: Dir.pwd, without_resolution: false) @workspace_path = workspace_path + self.without_resolution = without_resolution end # Index all files and dependencies of the workspace that exists in `@workspace_path` diff --git a/rust/rubydex-mcp/src/server.rs b/rust/rubydex-mcp/src/server.rs index 5a0194c8..bcbaea5c 100644 --- a/rust/rubydex-mcp/src/server.rs +++ b/rust/rubydex-mcp/src/server.rs @@ -60,7 +60,7 @@ impl RubydexServer { } let mut resolver = rubydex::resolution::Resolver::new(&mut graph); - resolver.resolve_all(); + resolver.resolve(); eprintln!( "Rubydex indexed {} files, {} declarations", diff --git a/rust/rubydex-sys/src/graph_api.rs b/rust/rubydex-sys/src/graph_api.rs index d390ae5f..1520a2da 100644 --- a/rust/rubydex-sys/src/graph_api.rs +++ b/rust/rubydex-sys/src/graph_api.rs @@ -192,7 +192,7 @@ pub unsafe extern "C" fn rdx_graph_delete_document(pointer: GraphPointer, uri: * pub extern "C" fn rdx_graph_resolve(pointer: GraphPointer) { with_mut_graph(pointer, |graph| { let mut resolver = Resolver::new(graph); - resolver.resolve_all(); + resolver.resolve(); }); } @@ -221,6 +221,28 @@ pub unsafe extern "C" fn rdx_graph_set_encoding(pointer: GraphPointer, encoding_ true } +/// Configures the graph to skip accumulating resolution work items. +/// +/// # Safety +/// +/// Expects the graph pointer to be valid +#[unsafe(no_mangle)] +pub unsafe extern "C" fn rdx_graph_set_without_resolution(pointer: GraphPointer, without_resolution: bool) { + with_mut_graph(pointer, |graph| { + graph.set_without_resolution(without_resolution); + }); +} + +/// Returns whether the graph is configured to skip accumulating resolution work items. +/// +/// # Safety +/// +/// Expects the graph pointer to be valid +#[unsafe(no_mangle)] +pub unsafe extern "C" fn rdx_graph_without_resolution(pointer: GraphPointer) -> bool { + with_graph(pointer, |graph| graph.without_resolution()) +} + /// Creates a new iterator over declaration IDs by snapshotting the current set of IDs. /// /// # Safety @@ -577,7 +599,7 @@ mod tests { let mut graph = Graph::new(); graph.update(indexer.local_graph()); let mut resolver = Resolver::new(&mut graph); - resolver.resolve_all(); + resolver.resolve(); assert_eq!( 1, diff --git a/rust/rubydex/src/indexing.rs b/rust/rubydex/src/indexing.rs index f437941a..7f1f62cf 100644 --- a/rust/rubydex/src/indexing.rs +++ b/rust/rubydex/src/indexing.rs @@ -99,6 +99,7 @@ pub fn index_source(graph: &mut Graph, uri: &str, source: &str, language_id: &La } /// Indexes the given paths, reading the content from disk and populating the given `Graph` instance. +/// Pending work is accumulated on the graph; drained by the resolver. /// /// # Panics /// diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index 559270a5..fffe2444 100644 --- a/rust/rubydex/src/indexing/local_graph.rs +++ b/rust/rubydex/src/indexing/local_graph.rs @@ -3,6 +3,7 @@ use std::collections::hash_map::Entry; use crate::diagnostic::{Diagnostic, Rule}; use crate::model::definitions::Definition; use crate::model::document::Document; +use crate::model::graph::NameDependent; use crate::model::identity_maps::IdentityHashMap; use crate::model::ids::{DefinitionId, NameId, ReferenceId, StringId, UriId}; use crate::model::name::{Name, NameRef}; @@ -18,6 +19,7 @@ type LocalGraphParts = ( IdentityHashMap, IdentityHashMap, IdentityHashMap, + IdentityHashMap>, ); #[derive(Debug)] @@ -29,6 +31,7 @@ pub struct LocalGraph { names: IdentityHashMap, constant_references: IdentityHashMap, method_references: IdentityHashMap, + name_dependents: IdentityHashMap>, } impl LocalGraph { @@ -42,6 +45,7 @@ impl LocalGraph { names: IdentityHashMap::default(), constant_references: IdentityHashMap::default(), method_references: IdentityHashMap::default(), + name_dependents: IdentityHashMap::default(), } } @@ -70,6 +74,10 @@ impl LocalGraph { pub fn add_definition(&mut self, definition: Definition) -> DefinitionId { let definition_id = definition.id(); + if let Some(name_id) = definition.name_id() { + self.insert_name_dependent(*name_id, NameDependent::Definition(definition_id)); + } + if self.definitions.insert(definition_id, definition).is_some() { debug_assert!(false, "DefinitionId collision in local graph"); } @@ -133,6 +141,7 @@ impl LocalGraph { pub fn add_constant_reference(&mut self, reference: ConstantReference) -> ReferenceId { let reference_id = reference.id(); + self.insert_name_dependent(*reference.name_id(), NameDependent::Reference(reference_id)); if self.constant_references.insert(reference_id, reference).is_some() { debug_assert!(false, "ReferenceId collision in local graph"); @@ -172,6 +181,26 @@ impl LocalGraph { self.document.add_diagnostic(diagnostic); } + // Name dependents + + /// Registers a dependent under its own `name_id` and under the `nesting`/`parent_scope` + /// of that name, so invalidation can trace from any of those scopes. + fn insert_name_dependent(&mut self, name_id: NameId, dep: NameDependent) { + self.name_dependents.entry(name_id).or_default().push(dep); + + if let Some(name_ref) = self.names.get(&name_id) { + for owner_id in [name_ref.nesting().as_ref().copied(), name_ref.parent_scope().as_ref().copied()] + .into_iter() + .flatten() + { + let deps = self.name_dependents.entry(owner_id).or_default(); + if !deps.contains(&dep) { + deps.push(dep); + } + } + } + } + // Into parts #[must_use] @@ -184,6 +213,7 @@ impl LocalGraph { self.names, self.constant_references, self.method_references, + self.name_dependents, ) } } diff --git a/rust/rubydex/src/main.rs b/rust/rubydex/src/main.rs index ffe4098c..e3c37431 100644 --- a/rust/rubydex/src/main.rs +++ b/rust/rubydex/src/main.rs @@ -91,7 +91,7 @@ fn main() { time_it!(resolution, { let mut resolver = Resolver::new(&mut graph); - resolver.resolve_all(); + resolver.resolve(); }); if let Some(StopAfter::Resolution) = args.stop_after { diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index ebd76c0a..f754c52d 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/rust/rubydex/src/model/declaration.rs @@ -478,6 +478,10 @@ impl Namespace { all_namespaces!(self, it => it.member(str_id)) } + pub fn remove_member(&mut self, str_id: &StringId) -> Option { + all_namespaces!(self, it => it.remove_member(str_id)) + } + #[must_use] pub fn singleton_class(&self) -> Option<&DeclarationId> { all_namespaces!(self, it => it.singleton_class_id()) diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index 010477b7..bc619788 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -14,10 +14,41 @@ use crate::model::references::{ConstantReference, MethodRef}; use crate::model::string_ref::StringRef; use crate::stats; +/// An entity whose validity depends on a particular `NameId`. +/// Used as the value type in the `name_dependents` reverse index. +/// +/// Each dependent is stored under its own `name_id` **and** under the `nesting`/`parent_scope` +/// of that name. This lets us trace from a changed name directly to the affected refs/defs +/// without an intermediate `Name → Name` layer. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NameDependent { + Definition(DefinitionId), + Reference(ReferenceId), +} + +/// Items processed by the unified invalidation worklist. +enum InvalidationItem { + /// A declaration whose ancestor chain is stale, or that has become empty and needs removal. + Declaration(DeclarationId), + /// A name whose dependencies may have changed, needing cascade or reference re-evaluation. + Name(NameId), +} + pub static OBJECT_ID: LazyLock = LazyLock::new(|| DeclarationId::from("Object")); pub static MODULE_ID: LazyLock = LazyLock::new(|| DeclarationId::from("Module")); pub static CLASS_ID: LazyLock = LazyLock::new(|| DeclarationId::from("Class")); +/// A work item produced by graph mutations (update/delete) that needs resolution. +#[derive(Debug)] +pub enum Unit { + /// A definition that defines a constant and might require resolution + Definition(DefinitionId), + /// A constant reference that needs to be resolved + ConstantRef(ReferenceId), + /// A declaration whose ancestors need re-linearization + Ancestors(DeclarationId), +} + // The `Graph` is the global representation of the entire Ruby codebase. It contains all declarations and their // relationships #[derive(Default, Debug)] @@ -44,6 +75,19 @@ pub struct Graph { /// Reverse index: for each declaration, which `NameId`s resolved to it. /// Used during incremental invalidation to find names that need unresolving when a declaration is removed. declaration_to_names: IdentityHashMap>, + + /// Reverse index: for each `NameId`, which definitions and references depend on it. + /// Each dependent is stored under its own name AND under the `nesting`/`parent_scope` of that name. + /// Used during invalidation to efficiently find affected entities without scanning the full graph. + name_dependents: IdentityHashMap>, + + /// When true, work items from update/delete are discarded instead of accumulated. + /// Use for tools that only need definitions without resolution. + without_resolution: bool, + + /// Accumulated work items from update/delete operations. + /// Drained by `take_pending_work()` before resolution. + pending_work: Vec, } impl Graph { @@ -59,6 +103,9 @@ impl Graph { method_references: IdentityHashMap::default(), position_encoding: Encoding::default(), declaration_to_names: IdentityHashMap::default(), + name_dependents: IdentityHashMap::default(), + without_resolution: false, + pending_work: Vec::default(), } } @@ -163,11 +210,6 @@ impl Graph { .get(declaration_id) .is_some_and(|decl| decl.as_namespace().is_some()) } - - pub fn clear_declarations(&mut self) { - self.declarations.clear(); - } - // Returns an immutable reference to the definitions map #[must_use] pub fn definitions(&self) -> &IdentityHashMap { @@ -467,8 +509,11 @@ impl Graph { &self.names } - /// Removes `name_id` from the `declaration_to_names` reverse index for the given declaration, - /// cleaning up the entry entirely if no names remain. + /// Drains the accumulated work items, returning them for use by the resolver. + pub fn take_pending_work(&mut self) -> Vec { + std::mem::take(&mut self.pending_work) + } + fn detach_name_from_declaration(&mut self, declaration_id: DeclarationId, name_id: NameId) { if let Some(name_set) = self.declaration_to_names.get_mut(&declaration_id) { name_set.remove(&name_id); @@ -511,12 +556,13 @@ impl Graph { } } - /// Removes a name from the graph and cleans up the `declaration_to_names` reverse index. + /// Removes a name from the graph and cleans up all reverse indices that reference it. fn remove_name(&mut self, name_id: NameId) { if let Some(NameRef::Resolved(resolved)) = self.names.get(&name_id) { self.detach_name_from_declaration(*resolved.declaration_id(), name_id); } + self.name_dependents.remove(&name_id); self.names.remove(&name_id); } @@ -660,17 +706,28 @@ impl Graph { /// Handles the deletion of a document identified by `uri`. /// Returns the `UriId` of the removed document, or `None` if it didn't exist. + /// Pending work is accumulated internally; drained by the resolver. pub fn delete_document(&mut self, uri: &str) -> Option { let uri_id = UriId::from(uri); let document = self.documents.remove(&uri_id)?; - self.remove_definitions_for_document(&document); + let mut work = Vec::new(); + self.invalidate(Some(&document), None, &mut work); + self.remove_document_data(&document); + if !self.without_resolution { + self.pending_work.extend(work); + } Some(uri_id) } /// Merges everything in `other` into this Graph. This method is meant to merge all graph representations from /// different threads, but not meant to handle updates to the existing global representation - pub fn extend(&mut self, local_graph: LocalGraph) { - let (uri_id, document, definitions, strings, names, constant_references, method_references) = + /// + /// # Panics + /// + /// Panics if a name that was just inserted into the graph cannot be found when looking up + /// its `parent_scope`. + fn extend(&mut self, local_graph: LocalGraph, work: &mut Vec) { + let (uri_id, document, definitions, strings, names, constant_references, method_references, name_dependents) = local_graph.into_parts(); if self.documents.insert(uri_id, document).is_some() { @@ -705,9 +762,13 @@ impl Graph { if self.definitions.insert(definition_id, definition).is_some() { debug_assert!(false, "DefinitionId collision in global graph"); } + + work.push(Unit::Definition(definition_id)); } for (constant_ref_id, constant_ref) in constant_references { + work.push(Unit::ConstantRef(constant_ref_id)); + if self.constant_references.insert(constant_ref_id, constant_ref).is_some() { debug_assert!(false, "Constant ReferenceId collision in global graph"); } @@ -718,26 +779,98 @@ impl Graph { debug_assert!(false, "Method ReferenceId collision in global graph"); } } + + 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) { + global_deps.push(dep); + } + } + } } /// Updates the global representation with the information contained in `other`, handling deletions, insertions and - /// updates to existing entries + /// updates to existing entries. Pending work is accumulated internally; drained by the resolver. + /// + /// 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 pub fn update(&mut self, other: LocalGraph) { - // For each URI that was indexed through `other`, check what was discovered and update our current global - // representation let uri_id = other.uri_id(); - if let Some(document) = self.documents.remove(&uri_id) { - self.remove_definitions_for_document(&document); + let mut work = Vec::new(); + let old_document = self.documents.remove(&uri_id); + + self.invalidate(old_document.as_ref(), Some(&other), &mut work); + if let Some(doc) = &old_document { + self.remove_document_data(doc); + } + + self.extend(other, &mut work); + if !self.without_resolution { + self.pending_work.extend(work); + } + } + + /// Detaches old definitions from declarations, identifies declarations touched by the + /// new `LocalGraph`, and feeds everything into `invalidate_graph` as a single worklist. + /// + /// Constant reference detachment is deferred to `remove_document_data`, which runs after + /// invalidation. References unresolved during invalidation are already detached by + /// `unresolve_reference`; the rest are still resolved and detached during cleanup. + /// + /// Does NOT remove raw data (refs/defs/names/strings) from maps — the caller must + /// follow up with `remove_document_data` for that. + fn invalidate( + &mut self, + old_document: Option<&Document>, + new_local_graph: Option<&LocalGraph>, + work: &mut Vec, + ) { + let mut items: Vec = Vec::new(); + + // Detach old definitions from their declarations, queue affected directly + if let Some(document) = old_document { + for def_id in document.definitions() { + if let Some(declaration_id) = self.definition_id_to_declaration_id(*def_id).copied() + && let Some(declaration) = self.declarations.get_mut(&declaration_id) + && declaration.remove_definition(def_id) + { + declaration.clear_diagnostics(); + items.push(InvalidationItem::Declaration(declaration_id)); + } + } + } + + // Declarations touched by the new local graph + if let Some(lg) = new_local_graph { + for def in lg.definitions().values() { + if let Some(name_id) = def.name_id() + && let Some(NameRef::Resolved(resolved)) = self.names.get(name_id) + { + items.push(InvalidationItem::Declaration(*resolved.declaration_id())); + } + } + + for cr in lg.constant_references().values() { + if let Some(name_ref) = self.names.get(cr.name_id()) + && let Some(nesting_id) = name_ref.nesting() + && let Some(NameRef::Resolved(resolved)) = self.names.get(nesting_id) + { + items.push(InvalidationItem::Declaration(*resolved.declaration_id())); + } + } } - self.extend(other); + if !items.is_empty() { + self.invalidate_graph(items, work); + } } - // Removes all nodes and relationships associated to the given document. This is used to clean up stale data when a - // document changes or when a document is deleted and we need to clean up the memory. - // The document must already have been removed from `self.documents` before calling this. - fn remove_definitions_for_document(&mut self, document: &Document) { - // TODO: Remove method references from method declarations once method inference is implemented + /// Removes raw document data (refs, defs, names, strings) from maps. + /// Does not touch declarations or perform invalidation — that is handled by `invalidate`. + fn remove_document_data(&mut self, document: &Document) { for ref_id in document.method_references() { if let Some(method_ref) = self.method_references.remove(ref_id) { self.untrack_string(*method_ref.str()); @@ -748,119 +881,295 @@ impl Graph { self.unresolve_reference(*ref_id); if let Some(constant_ref) = self.constant_references.remove(ref_id) { + // Detach from target declaration. References unresolved during invalidation + // were already detached by `unresolve_reference`; this catches the rest. + if let Some(NameRef::Resolved(resolved)) = self.names.get(constant_ref.name_id()) + && let Some(declaration) = self.declarations.get_mut(resolved.declaration_id()) + { + declaration.remove_reference(ref_id); + } + + let dep = NameDependent::Reference(*ref_id); + self.remove_name_dependent_with_owners(*constant_ref.name_id(), dep); self.untrack_name(*constant_ref.name_id()); } } - // Vector of (owner_declaration_id, member_name_id) to delete after processing all definitions - let mut members_to_delete: Vec<(DeclarationId, StringId)> = Vec::new(); - let mut definitions_to_delete: Vec = Vec::new(); - let mut declarations_to_delete: Vec = Vec::new(); - let mut declarations_to_invalidate_ancestor_chains: Vec = Vec::new(); - for def_id in document.definitions() { - definitions_to_delete.push(*def_id); - - if let Some(declaration_id) = self.definition_id_to_declaration_id(*def_id).copied() - && let Some(declaration) = self.declarations.get_mut(&declaration_id) - && declaration.remove_definition(def_id) - { - declaration.clear_diagnostics(); - if declaration.as_namespace().is_some() { - declarations_to_invalidate_ancestor_chains.push(declaration_id); - } - - if declaration.has_no_definitions() { - let unqualified_str_id = StringId::from(&declaration.unqualified_name()); - members_to_delete.push((*declaration.owner_id(), unqualified_str_id)); - declarations_to_delete.push(declaration_id); - - if let Some(namespace) = declaration.as_namespace() - && let Some(singleton_id) = namespace.singleton_class() - { - declarations_to_delete.push(*singleton_id); - } - } - } - - if let Some(name_id) = self.definitions.get(def_id).unwrap().name_id() { + let definition = self.definitions.remove(def_id).unwrap(); + if let Some(name_id) = definition.name_id() { + let dep = NameDependent::Definition(*def_id); + self.remove_name_dependent_with_owners(*name_id, dep); self.untrack_name(*name_id); } + self.untrack_definition_strings(&definition); } + } - self.invalidate_ancestor_chains(declarations_to_invalidate_ancestor_chains); + /// Removes a specific dependent from `name_dependents[name_id]` and also from the + /// `nesting`/`parent_scope` owners of that name, since dependents are stored under all three. + fn remove_name_dependent_with_owners(&mut self, name_id: NameId, dependent: NameDependent) { + let owners: [Option; 2] = self + .names + .get(&name_id) + .map(|name_ref| [name_ref.nesting().as_ref().copied(), name_ref.parent_scope().as_ref().copied()]) + .unwrap_or_default(); - for declaration_id in declarations_to_delete { - self.declaration_to_names.remove(&declaration_id); - self.declarations.remove(&declaration_id); + self.remove_name_dependent(name_id, dependent); + for owner_id in owners.into_iter().flatten() { + self.remove_name_dependent(owner_id, dependent); } + } - // Clean up any members that pointed to declarations that were removed - for (owner_id, member_str_id) in members_to_delete { - // Remove the `if` and use `unwrap` once we are indexing RBS files to have `Object` - if let Some(owner) = self.declarations.get_mut(&owner_id) { - match owner { - Declaration::Namespace(Namespace::Class(owner)) => { - owner.remove_member(&member_str_id); - } - Declaration::Namespace(Namespace::SingletonClass(owner)) => { - owner.remove_member(&member_str_id); - } - Declaration::Namespace(Namespace::Module(owner)) => { - owner.remove_member(&member_str_id); - } - _ => {} // Nothing happens - } + /// Removes a specific dependent from the `name_dependents` entry for `name_id`, + /// cleaning up the entry if no dependents remain. + fn remove_name_dependent(&mut self, name_id: NameId, dependent: NameDependent) { + if let Some(deps) = self.name_dependents.get_mut(&name_id) { + deps.retain(|d| *d != dependent); + if deps.is_empty() { + self.name_dependents.remove(&name_id); } } + } - for def_id in definitions_to_delete { - let definition = self.definitions.remove(&def_id).unwrap(); - self.untrack_definition_strings(&definition); + /// Unified invalidation worklist. Processes declaration and name items in a single loop, + /// where processing one item can push new items back onto the queue. + fn invalidate_graph(&mut self, items: Vec, work: &mut Vec) { + let mut queue = items; + let mut visited_declarations = IdentityHashSet::::default(); + let mut visited_names = IdentityHashSet::::default(); + + while let Some(item) = queue.pop() { + match item { + InvalidationItem::Declaration(decl_id) => { + self.process_declaration(decl_id, &mut queue, &mut visited_declarations, work); + } + InvalidationItem::Name(name_id) => { + self.process_name(name_id, &mut queue, &mut visited_names, work); + } + } } } - fn invalidate_ancestor_chains(&mut self, initial_ids: Vec) { - let mut queue = initial_ids; - let mut visited = IdentityHashSet::::default(); + /// Processes a declaration in the invalidation worklist. + /// + /// A declaration should be removed if it has no definitions or if its owner has already + /// been removed from the graph (orphaned). Otherwise, its ancestor chain is cleared and + /// descendants are queued for the same treatment. + fn process_declaration( + &mut self, + decl_id: DeclarationId, + queue: &mut Vec, + visited_declarations: &mut IdentityHashSet, + work: &mut Vec, + ) { + let Some(decl) = self.declarations.get(&decl_id) else { + return; + }; + + let should_remove = decl.has_no_definitions() || !self.declarations.contains_key(decl.owner_id()); - while let Some(declaration_id) = queue.pop() { - if !visited.insert(declaration_id) { - continue; + if should_remove { + // Queue members + singleton for removal + if let Some(ns) = decl.as_namespace() { + if let Some(singleton_id) = ns.singleton_class() { + queue.push(InvalidationItem::Declaration(*singleton_id)); + } + for member_decl_id in ns.members().values() { + queue.push(InvalidationItem::Declaration(*member_decl_id)); + } + for descendant_id in ns.descendants() { + queue.push(InvalidationItem::Declaration(*descendant_id)); + } } - let namespace = self - .declarations_mut() - .get_mut(&declaration_id) - .unwrap() - .as_namespace_mut() - .expect("expected namespace declaration"); + // Unresolve names resolved to this declaration, cascade to dependents + if let Some(name_set) = self.declaration_to_names.remove(&decl_id) { + for name_id in name_set { + self.unresolve_name(name_id); + self.queue_dependent_names(name_id, queue); + } + } + + // Clean up owner membership and queue remaining definitions for re-resolution + if let Some(decl) = self.declarations.get(&decl_id) { + let unqualified_str_id = StringId::from(&decl.unqualified_name()); + let owner_id = *decl.owner_id(); + + for def_id in decl.definitions() { + work.push(Unit::Definition(*def_id)); + } + + if let Some(owner) = self.declarations.get_mut(&owner_id) + && let Some(ns) = owner.as_namespace_mut() + { + ns.remove_member(&unqualified_str_id); + } + } + + self.declarations.remove(&decl_id); + } else { + // Ancestor-stale mode + if !visited_declarations.insert(decl_id) { + return; + } + + let Some(namespace) = self.declarations.get_mut(&decl_id).and_then(|d| d.as_namespace_mut()) else { + return; + }; + // Remove self from each ancestor's descendant set for ancestor in &namespace.clone_ancestors() { - if let Ancestor::Complete(ancestor_id) = ancestor { - self.declarations_mut() - .get_mut(ancestor_id) - .unwrap() - .as_namespace_mut() - .unwrap() - .remove_descendant(&declaration_id); + if let Ancestor::Complete(ancestor_id) = ancestor + && let Some(anc_decl) = self.declarations.get_mut(ancestor_id) + && let Some(ns) = anc_decl.as_namespace_mut() + { + ns.remove_descendant(&decl_id); } } - let namespace = self - .declarations_mut() - .get_mut(&declaration_id) - .unwrap() - .as_namespace_mut() - .unwrap(); + let namespace = self.declarations.get_mut(&decl_id).unwrap().as_namespace_mut().unwrap(); namespace.for_each_descendant(|descendant_id| { - queue.push(*descendant_id); + queue.push(InvalidationItem::Declaration(*descendant_id)); }); namespace.clear_ancestors(); namespace.clear_descendants(); + + work.push(Unit::Ancestors(decl_id)); + + // Queue dependent names of resolved names (skipping seeds themselves) + if let Some(name_set) = self.declaration_to_names.get(&decl_id) { + for seed_name_id in name_set { + self.queue_dependent_names(*seed_name_id, queue); + } + } + } + } + + /// Processes a name in the invalidation worklist. + /// + /// Always propagates to `name_dependents`. Then checks whether the name needs full + /// structural cascade (nesting or parent scope dependency broken) or just reference + /// re-evaluation (ancestor context changed). + fn process_name( + &mut self, + name_id: NameId, + queue: &mut Vec, + visited_names: &mut IdentityHashSet, + work: &mut Vec, + ) { + if !visited_names.insert(name_id) { + return; + } + + let dependents: Vec = self.name_dependents.get(&name_id).cloned().unwrap_or_default(); + + // Propagate to names of dependents that are nested/scoped under this name + self.queue_dependent_names(name_id, queue); + + if self.has_unresolved_dependency(name_id) { + // Structural cascade: the name's resolution is invalid + if let Some(old_decl_id) = self.unresolve_name(name_id) { + for dep in &dependents { + match dep { + NameDependent::Reference(ref_id) => { + if let Some(decl) = self.declarations.get_mut(&old_decl_id) { + decl.remove_reference(ref_id); + } + work.push(Unit::ConstantRef(*ref_id)); + } + NameDependent::Definition(def_id) => { + work.push(Unit::Definition(*def_id)); + + if let Some(decl) = self.declarations.get_mut(&old_decl_id) { + decl.remove_definition(def_id); + } + + if self + .declarations + .get(&old_decl_id) + .is_some_and(Declaration::has_no_definitions) + { + queue.push(InvalidationItem::Declaration(old_decl_id)); + } + } + } + } + } + } else { + // Ancestor change only: re-evaluate constant references under this name + let is_resolved = matches!(self.names.get(&name_id), Some(NameRef::Resolved(_))); + + for dep in &dependents { + if let NameDependent::Reference(ref_id) = dep { + if is_resolved { + self.unresolve_reference(*ref_id); + } + work.push(Unit::ConstantRef(*ref_id)); + } + } + } + } + + /// Looks at `name_dependents[name_id]` and queues the `name_id` of each dependent + /// whose own name differs from `name_id` (i.e., dependents registered here via + /// `nesting`/`parent_scope` expansion). + fn queue_dependent_names(&self, name_id: NameId, queue: &mut Vec) { + if let Some(deps) = self.name_dependents.get(&name_id) { + for dep in deps { + let dep_name_id = match dep { + NameDependent::Reference(ref_id) => { + self.constant_references.get(ref_id).map(|r| *r.name_id()) + } + NameDependent::Definition(def_id) => { + self.definitions.get(def_id).and_then(|d| d.name_id().copied()) + } + }; + if let Some(dep_name_id) = dep_name_id + && dep_name_id != name_id + { + queue.push(InvalidationItem::Name(dep_name_id)); + } + } + } + } + + /// Returns true if the name's nesting or parent scope dependency has been broken, + /// meaning the name needs full structural cascade rather than just reference re-evaluation. + fn has_unresolved_dependency(&self, name_id: NameId) -> bool { + let Some(name_ref) = self.names.get(&name_id) else { + return false; + }; + + // Nesting is unresolved or removed: the lexical scope this name lives in is invalid + if let Some(nesting_id) = name_ref.nesting() + && matches!(self.names.get(nesting_id), Some(NameRef::Unresolved(_)) | None) + { + return true; + } + + // Parent scope is still resolved (pointing to a now-stale declaration) or was removed: + // the qualifier (e.g. `Foo` in `Foo::Bar`) may resolve differently + if let Some(parent_id) = name_ref.parent_scope().as_ref() + && matches!(self.names.get(parent_id), Some(NameRef::Resolved(_)) | None) + { + return true; } + + false + } + + /// Configures the graph to skip accumulating resolution work items. Use for tools that only + /// need definitions and don't intend to call `resolve()`. + pub fn set_without_resolution(&mut self, without_resolution: bool) { + self.without_resolution = without_resolution; + } + + #[must_use] + pub fn without_resolution(&self) -> bool { + self.without_resolution } /// Sets the encoding that should be used for transforming byte offsets into LSP code unit line/column positions @@ -982,8 +1291,14 @@ mod tests { use super::*; use crate::model::comment::Comment; use crate::model::declaration::Ancestors; + use crate::model::name::NameRef; use crate::test_utils::GraphTest; - use crate::{assert_descendants, assert_members_eq, assert_no_diagnostics, assert_no_members}; + use crate::{ + assert_alias_targets_contain, assert_ancestors_eq, assert_constant_reference_to, + assert_declaration_does_not_exist, assert_declaration_exists, assert_declaration_references_count_eq, + assert_descendants, assert_members_eq, assert_no_constant_alias_target, assert_no_diagnostics, + assert_no_members, + }; #[test] fn deleting_a_uri() { @@ -1726,44 +2041,987 @@ mod tests { assert_eq!(41, context.graph().strings.len()); } + // ========================================== + // Incremental invalidation tests + // ========================================== + #[test] - fn deleting_sole_definition_removes_the_name_entirely() { + fn ancestor_changes_invalidate_constant_references() { let mut context = GraphTest::new(); - context.index_uri("file:///foo.rb", "module Foo; end\nBar"); - context.index_uri("file:///bar.rb", "module Bar; end"); - context.resolve(); + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end - // Bar declaration should have 1 reference (from foo.rb) - let bar_decl = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap(); - assert_eq!(bar_decl.references().len(), 1); + module Bar + CONST = 2 + end + ", + ); + context.index_uri( + "file:///foo2.rb", + r" + class Baz + include Foo - // Update foo.rb to remove the Bar reference - context.index_uri("file:///foo.rb", "module Foo; end"); + CONST + end + ", + ); context.resolve(); - let bar_decl = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap(); + // Initially, CONST points to `Foo::CONST` + assert_constant_reference_to!(context, "Foo::CONST", "file:///foo2.rb:4:3-4:8"); + assert_declaration_references_count_eq!(context, "Foo::CONST", 1); + + // By adding a new file that prepends `Bar`, we change the ancestors of `Baz` and now `CONST` should point to + // `Bar::CONST` + context.index_uri( + "file:///foo3.rb", + r" + class Baz + prepend Bar + end + ", + ); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + + if context.graph().strings().get(name.str()).unwrap().as_str() == "CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + assert!( - bar_decl.references().is_empty(), - "Reference to Bar should be detached from declaration" + matches!(reference_name, NameRef::Unresolved(_)), + "Did not properly invalidate constant reference" ); + assert_declaration_references_count_eq!(context, "Foo::CONST", 0); + } + + #[test] + fn new_namespace_shadowing_include_target_invalidates_references() { + let mut context = GraphTest::new(); - // Delete bar.rb — the Bar name should be fully removed - let bar_name_id = Name::new(StringId::from("Bar"), ParentScope::None, None).id(); - context.index_uri("file:///bar.rb", ""); + context.index_uri( + "file:///foo.rb", + r" + module Foo + module Bar + module Baz + end + end + end + ", + ); + context.index_uri( + "file:///qux.rb", + r" + module Foo + module Bar + module Baz + class Qux + include Bar + end + end + end + end + ", + ); context.resolve(); + assert_constant_reference_to!(context, "Foo::Bar", "file:///qux.rb:5:17-5:20"); + assert_declaration_references_count_eq!(context, "Foo::Bar", 1); + assert_ancestors_eq!( + context, + "Foo::Bar::Baz::Qux", + ["Foo::Bar::Baz::Qux", "Foo::Bar", "Object"] + ); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + module Bar + module Baz + module Bar; end + end + end + end + ", + ); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "Bar" { + Some(name) + } else { + None + } + }) + .unwrap(); + assert!( - context - .graph() - .declarations() - .get(&DeclarationId::from("Bar")) - .is_none(), - "Bar declaration should be removed" + matches!(reference_name, NameRef::Unresolved(_)), + "Did not properly invalidate constant reference" + ); + assert_declaration_references_count_eq!(context, "Foo::Bar", 0); + let empty_ancestors: [&str; 0] = []; + assert_ancestors_eq!(context, "Foo::Bar::Baz::Qux", empty_ancestors); + } + + #[test] + fn deleting_include_file_invalidates_ancestors_and_references() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + + class Bar + CONST + end + ", + ); + context.index_uri( + "file:///bar.rb", + r" + class Bar + include Foo + end + ", ); + context.resolve(); + + assert_constant_reference_to!(context, "Foo::CONST", "file:///foo.rb:6:3-6:8"); + assert_declaration_references_count_eq!(context, "Foo::CONST", 1); + assert_ancestors_eq!(context, "Bar", ["Bar", "Foo", "Object"]); + + context.delete_uri("file:///bar.rb"); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + assert!( - context.graph().names().get(&bar_name_id).is_none(), - "Bar name should be removed from the names map" + matches!(reference_name, NameRef::Unresolved(_)), + "Did not properly invalidate constant reference" + ); + assert_declaration_references_count_eq!(context, "Foo::CONST", 0); + let empty_ancestors: [&str; 0] = []; + assert_ancestors_eq!(context, "Bar", empty_ancestors); + } + + #[test] + fn invalidating_constant_aliases() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + + class Bar + ALIAS_CONST = CONST + end + ", ); + context.index_uri( + "file:///bar.rb", + r" + class Bar + include Foo + end + ", + ); + context.resolve(); + + assert_alias_targets_contain!(context, "Bar::ALIAS_CONST", "Foo::CONST"); + + context.delete_uri("file:///bar.rb"); + + assert_no_constant_alias_target!(context, "Bar::ALIAS_CONST"); + } + + #[test] + fn new_constant_in_existing_chain_invalidates_references() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + + module Bar + end + ", + ); + context.index_uri( + "file:///foo2.rb", + r" + class Baz + include Foo + prepend Bar + + CONST + end + ", + ); + context.resolve(); + + assert_constant_reference_to!(context, "Foo::CONST", "file:///foo2.rb:5:3-5:8"); + assert_declaration_references_count_eq!(context, "Foo::CONST", 1); + + context.index_uri( + "file:///foo3.rb", + r" + module Bar + CONST = 2 + end + ", + ); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + + assert!( + matches!(reference_name, NameRef::Unresolved(_)), + "Did not properly invalidate constant reference" + ); + assert_declaration_references_count_eq!(context, "Foo::CONST", 0); + } + + #[test] + fn ancestor_changes_re_resolve_correctly() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + + module Bar + CONST = 2 + end + ", + ); + context.index_uri( + "file:///foo2.rb", + r" + class Baz + include Foo + + CONST + end + ", + ); + context.resolve(); + + assert_constant_reference_to!(context, "Foo::CONST", "file:///foo2.rb:4:3-4:8"); + assert_declaration_references_count_eq!(context, "Foo::CONST", 1); + + context.index_uri( + "file:///foo3.rb", + r" + class Baz + prepend Bar + end + ", + ); + + context.resolve(); + + assert_constant_reference_to!(context, "Bar::CONST", "file:///foo2.rb:4:3-4:8"); + assert_declaration_references_count_eq!(context, "Bar::CONST", 1); + assert_declaration_references_count_eq!(context, "Foo::CONST", 0); + } + + #[test] + fn invalidation_cascade_from_reference_to_declaration() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + module Bar + module Baz + end + end + end + ", + ); + context.index_uri( + "file:///foo2.rb", + r" + module Foo + include Bar + + class Baz::Qux + end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Foo::Bar::Baz::Qux"); + + context.index_uri( + "file:///foo3.rb", + r" + module Foo + module Baz + end + end + ", + ); + + assert_declaration_does_not_exist!(context, "Foo::Bar::Baz::Qux"); + } + + // ========================================== + // Edge case tests for robustness + // ========================================== + + #[test] + fn multiple_definitions_one_removed_declaration_survives() { + let mut context = GraphTest::new(); + + context.index_uri("file:///a.rb", "module Foo; end"); + context.index_uri("file:///b.rb", "module Foo; end"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_eq!(context.graph().get("Foo").unwrap().len(), 2); + + context.delete_uri("file:///a.rb"); + assert_declaration_exists!(context, "Foo"); + assert_eq!(context.graph().get("Foo").unwrap().len(), 1); + } + + #[test] + fn re_indexing_same_content_preserves_state() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + ", + ); + context.index_uri( + "file:///bar.rb", + r" + class Bar + include Foo + CONST + end + ", + ); + context.resolve(); + + assert_constant_reference_to!(context, "Foo::CONST", "file:///bar.rb:3:3-3:8"); + assert_ancestors_eq!(context, "Bar", ["Bar", "Foo", "Object"]); + + context.index_uri( + "file:///bar.rb", + r" + class Bar + include Foo + CONST + end + ", + ); + context.resolve(); + assert_constant_reference_to!(context, "Foo::CONST", "file:///bar.rb:3:3-3:8"); + assert_ancestors_eq!(context, "Bar", ["Bar", "Foo", "Object"]); + } + + #[test] + fn incremental_resolve_after_delete_and_re_add() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 1 + end + ", + ); + context.index_uri( + "file:///bar.rb", + r" + class Bar + include Foo + CONST + end + ", + ); + context.resolve(); + + assert_constant_reference_to!(context, "Foo::CONST", "file:///bar.rb:3:3-3:8"); + + context.delete_uri("file:///foo.rb"); + context.index_uri( + "file:///foo.rb", + r" + module Foo + CONST = 42 + end + ", + ); + + context.resolve(); + assert_constant_reference_to!(context, "Foo::CONST", "file:///bar.rb:3:3-3:8"); + } + + #[test] + fn deep_ancestor_chain_invalidation() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///a.rb", + r" + module A + DEEP_CONST = 1 + end + module B + include A + end + module C + include B + end + class D + include C + DEEP_CONST + end + ", + ); + context.resolve(); + + // DEEP_CONST resolves through D -> C -> B -> A + assert_constant_reference_to!(context, "A::DEEP_CONST", "file:///a.rb:12:3-12:13"); + + // Add a new file that changes C's ancestors + context.index_uri( + "file:///b.rb", + r" + module C + prepend B + end + ", + ); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "DEEP_CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + + assert!( + matches!(reference_name, NameRef::Unresolved(_)), + "Deep ancestor chain should invalidate constant references" + ); + } + + // ========================================== + // Regression tests: recursive declaration cleanup + // ========================================== + + #[test] + fn removing_namespace_declaration_cleans_up_member_methods() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + class Foo + def hello; end + def world; end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert!(context.graph().get("Foo#hello()").is_some()); + assert!(context.graph().get("Foo#world()").is_some()); + + context.delete_uri("file:///foo.rb"); + + assert!(context.graph().get("Foo").is_none()); + assert!(context.graph().get("Foo#hello()").is_none()); + assert!(context.graph().get("Foo#world()").is_none()); + } + + #[test] + fn removing_declaration_cascades_to_nested_members() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Outer + class Inner + CONST = 1 + def method_name; end + module Nested; end + end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Outer"); + assert_declaration_exists!(context, "Outer::Inner"); + assert_declaration_exists!(context, "Outer::Inner::Nested"); + assert!(context.graph().get("Outer::Inner").is_some()); + + context.delete_uri("file:///foo.rb"); + + assert!(context.graph().get("Outer").is_none()); + assert!(context.graph().get("Outer::Inner").is_none()); + assert!(context.graph().get("Outer::Inner::Nested").is_none()); + assert!(context.graph().get("Outer::Inner#method_name()").is_none()); + } + + #[test] + fn cascade_removes_declaration_with_singleton_and_members() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + module Foo + module Bar + class Baz + def self.class_method; end + CONST = 1 + end + end + end + ", + ); + context.index_uri( + "file:///bar.rb", + r" + module Foo + include Bar + + class Baz::Qux + def instance_method; end + end + end + ", + ); + context.resolve(); + + // Qux exists via inheritance (Foo includes Bar, so Baz is accessible) + assert_declaration_exists!(context, "Foo::Bar::Baz::Qux"); + + // Add a new Baz in Foo's lexical scope, invalidating the Baz reference + context.index_uri( + "file:///baz.rb", + r" + module Foo + module Baz + end + end + ", + ); + + // Qux and all its members should be gone (cascade from Baz reference invalidation) + assert_declaration_does_not_exist!(context, "Foo::Bar::Baz::Qux"); + assert!(context.graph().get("Foo::Bar::Baz::Qux#instance_method()").is_none()); + } + + #[test] + fn new_file_adding_superclass_invalidates_ancestors() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.index_uri("file:///bar.rb", "module Bar; end"); + context.resolve(); + + assert_ancestors_eq!(context, "Foo", ["Foo", "Object"]); + + // A new file reopens Foo with a superclass — ancestors must be invalidated + context.index_uri( + "file:///foo2.rb", + r" + class Foo < Bar + end + ", + ); + + let empty_ancestors: [&str; 0] = []; + assert_ancestors_eq!(context, "Foo", empty_ancestors); + + // After re-resolve, Foo should now inherit from Bar + // (Bar is a module, so Foo's chain is Foo → Bar → Object) + context.resolve(); + // Bar is a module so its ancestors are empty; Foo gets [Foo, Bar, Object] + // through the class parent resolution + let foo_decl = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap(); + assert!( + foo_decl.as_namespace().unwrap().has_complete_ancestors(), + "Foo ancestors should be complete after re-resolve" + ); + } + + #[test] + fn adding_include_resolves_previously_unresolved_references() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + class Foo + CONST + end + + module Bar + CONST = 1 + end + ", + ); + context.resolve(); + + // CONST is unresolved (Foo doesn't include Bar yet, CONST not found) + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + assert!(matches!(reference_name, NameRef::Unresolved(_))); + + context.index_uri( + "file:///foo_include.rb", + r" + class Foo + include Bar + end + ", + ); + + // After re-resolve, CONST should now resolve through Foo -> Bar + context.resolve(); + assert_constant_reference_to!(context, "Bar::CONST", "file:///foo.rb:2:3-2:8"); + assert_ancestors_eq!(context, "Foo", ["Foo", "Bar", "Object"]); + } + + #[test] + fn deleting_file_with_include_invalidates_constant_references() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + class Foo + CONST + end + + module Bar + CONST = 1 + end + ", + ); + context.index_uri( + "file:///foo_include.rb", + r" + class Foo + include Bar + end + ", + ); + context.resolve(); + + // CONST resolves through Foo -> Bar + assert_constant_reference_to!(context, "Bar::CONST", "file:///foo.rb:2:3-2:8"); + + // Delete the file that includes Bar — ancestors change, CONST should be invalidated + context.delete_uri("file:///foo_include.rb"); + + let reference_name = context + .graph() + .constant_references() + .values() + .find_map(|r| { + let name = context.graph().names().get(r.name_id()).unwrap(); + if context.graph().strings().get(name.str()).unwrap().as_str() == "CONST" { + Some(name) + } else { + None + } + }) + .unwrap(); + + assert!( + matches!(reference_name, NameRef::Unresolved(_)), + "Deleting include file should invalidate constant reference" + ); + assert_declaration_references_count_eq!(context, "Bar::CONST", 0); + } + + #[test] + fn re_indexing_preserves_constant_singleton_classes() { + let mut context = GraphTest::new(); + + // `EXTENDED = GENERAL + %w[...]` calls `+` on GENERAL, which makes the resolver + // create a singleton class for the GENERAL constant (to host the method receiver). + context.index_uri( + "file:///ns.rb", + r" + module Ns + GENERAL = %w[nodoc].freeze + EXTENDED = GENERAL + %w[arg args] + end + ", + ); + + context.index_uri( + "file:///child.rb", + r" + class Ns::Child + def name; end + end + ", + ); + + context.resolve(); + + // Singleton class for the constant exists after initial resolve. + // Ancestors include self, matching Ruby's singleton_class.ancestors behavior. + assert_declaration_exists!(context, "Ns::GENERAL::"); + assert_ancestors_eq!( + context, + "Ns::GENERAL::", + ["Ns::GENERAL::", "Module", "Object"] + ); + + context.index_uri( + "file:///child.rb", + r" + class Ns::Child + def name; end + def other; end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Ns::GENERAL::"); + assert_ancestors_eq!( + context, + "Ns::GENERAL::", + ["Ns::GENERAL::", "Module", "Object"] + ); + } + + #[test] + fn re_indexing_module_invalidates_compact_class_inside_it() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + class Foo; end + ", + ); + + context.index_uri( + "file:///m.rb", + r" + module M + class Foo::Bar + def bar; end + end + end + ", + ); + + context.resolve(); + + assert_declaration_exists!(context, "Foo::Bar"); + assert_ancestors_eq!(context, "Foo::Bar", ["Foo::Bar", "Object"]); + assert_members_eq!(context, "Foo::Bar", ["bar()"]); + + context.index_uri( + "file:///m.rb", + r" + module M + module Foo; end + + class Foo::Bar # Now the Foo in the class name resolves to M::Foo instead of the top-level Foo, changing the declaration's ancestors and members + def bar; end + end + end + ", + ); + context.resolve(); + + // Every declaration should be under M::Foo now + assert_declaration_exists!(context, "M::Foo::Bar"); + assert_ancestors_eq!(context, "M::Foo::Bar", ["M::Foo::Bar", "Object"]); + assert_members_eq!(context, "M::Foo::Bar", ["bar()"]); + } + + #[test] + fn invalidating_namespace_cascades_to_compact_class_and_its_members() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///foo.rb", + r" + class Foo + end + ", + ); + + context.index_uri( + "file:///bar.rb", + r" + class Foo::Bar + def bar; end + end + ", + ); + + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::Bar"); + assert_ancestors_eq!(context, "Foo::Bar", ["Foo::Bar", "Object"]); + assert_members_eq!(context, "Foo", ["Bar"]); + assert_members_eq!(context, "Foo::Bar", ["bar()"]); + + context.index_uri( + "file:///foo.rb", + r" + class Baz; end + + Foo = Baz + + class Foo::Bar + def bar; end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Baz::Bar"); + assert_ancestors_eq!(context, "Baz", ["Baz", "Object"]); + assert_ancestors_eq!(context, "Baz::Bar", ["Baz::Bar", "Object"]); + assert_members_eq!(context, "Baz", ["Bar"]); + assert_members_eq!(context, "Baz::Bar", ["bar()"]); + } + + #[test] + fn deleting_sole_definition_file_cascades_removal_through_nested_declarations() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///a.rb", + r" + module A + end + ", + ); + + context.index_uri( + "file:///b.rb", + r" + module A::B + end + ", + ); + + context.index_uri( + "file:///c.rb", + r" + class A::B::C + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "A"); + assert_declaration_exists!(context, "A::B"); + assert_declaration_exists!(context, "A::B::C"); + + context.delete_uri("file:///a.rb"); + + assert!(context.graph().get("A").is_none()); + assert!(context.graph().get("A::B").is_none()); + assert_declaration_does_not_exist!(context, "A::B::C"); + + context.resolve(); + assert!(context.graph().get("A").is_none()); + assert!(context.graph().get("A::B").is_none()); + assert!(context.graph().get("A::B::C").is_none()); } } diff --git a/rust/rubydex/src/query.rs b/rust/rubydex/src/query.rs index e8c364ec..89d52653 100644 --- a/rust/rubydex/src/query.rs +++ b/rust/rubydex/src/query.rs @@ -816,11 +816,19 @@ mod tests { Some(Name::new(StringId::from("Foo"), ParentScope::None, None).id()), ) .id(); - assert_completion_eq!( - context, - CompletionReceiver::Expression(name_id), - ["Foo::Bar", "$var", "Foo", "$var2", "Foo::Bar#bar_m()"] - ); + let mut candidates = completion_candidates( + context.graph(), + CompletionContext::new(CompletionReceiver::Expression(name_id)), + ) + .unwrap() + .iter() + .map(|id| context.graph().declarations().get(id).unwrap().name().to_string()) + .collect::>(); + candidates.sort(); + + let mut expected = vec!["Foo::Bar", "$var", "Foo", "$var2", "Foo::Bar#bar_m()"]; + expected.sort(); + assert_eq!(expected, candidates); } #[test] diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index 2877c6cf..c13b3642 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -10,21 +10,12 @@ use crate::model::{ Namespace, SingletonClassDeclaration, }, definitions::{Definition, Mixin, Receiver}, - graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID}, + graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID, Unit}, identity_maps::{IdentityHashMap, IdentityHashSet}, ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId}, name::{Name, NameRef, ParentScope}, }; -pub enum Unit { - /// A definition that defines a constant and might require resolution - Definition(DefinitionId), - /// A constant reference that needs to be resolved - ConstantRef(ReferenceId), - /// A list of ancestors that have been partially linearized and need to be retried - Ancestors(DeclarationId), -} - enum Outcome { /// The constant was successfully resolved to the given declaration ID. The second optional tuple element is a /// declaration that still needs to have its ancestors linearized @@ -81,56 +72,55 @@ impl<'a> Resolver<'a> { } } - /// Runs the resolution phase on the graph. The resolution phase is when 4 main pieces of information are computed: + /// Runs the resolution phase on the graph. The resolution phase is when 4 main pieces of information are + /// computed: /// /// 1. Declarations for all definitions /// 2. Members and ownership for all declarations /// 3. Resolution of all constant references /// 4. Inheritance relationships between declarations /// + /// Drains pending work accumulated by graph mutations (`update`/`delete_document`). + /// For the initial resolve, this contains all definitions and references. + /// For incremental resolves, only the invalidated subset. + /// /// # Panics /// /// Can panic if there's inconsistent data in the graph - pub fn resolve_all(&mut self) { - // TODO: temporary code while we don't have synchronization. We clear all declarations instead of doing the minimal - // amount of work - self.graph.clear_declarations(); - // Ensure that Object exists ahead of time so that we can associate top level declarations with the right membership + pub fn resolve(&mut self) { + let work = self.graph.take_pending_work(); + self.ensure_bootstrap_declarations(); + let other_ids = self.prepare_units(work); + self.run_resolution_loop(); + self.handle_remaining_definitions(other_ids); + } - { - self.graph.declarations_mut().insert( - *OBJECT_ID, - Declaration::Namespace(Namespace::Class(Box::new(ClassDeclaration::new( - "Object".to_string(), - *OBJECT_ID, - )))), - ); - self.graph.declarations_mut().insert( - *MODULE_ID, - Declaration::Namespace(Namespace::Class(Box::new(ClassDeclaration::new( - "Module".to_string(), - *OBJECT_ID, - )))), - ); - self.graph.declarations_mut().insert( - *CLASS_ID, - Declaration::Namespace(Namespace::Class(Box::new(ClassDeclaration::new( - "Class".to_string(), - *OBJECT_ID, - )))), - ); - } + /// Ensures Object, Module, and Class declarations exist in the graph. + fn ensure_bootstrap_declarations(&mut self) { + use std::collections::hash_map::Entry; - let other_ids = self.prepare_units(); + if let Entry::Vacant(e) = self.graph.declarations_mut().entry(*OBJECT_ID) { + e.insert(Declaration::Namespace(Namespace::Class(Box::new( + ClassDeclaration::new("Object".to_string(), *OBJECT_ID), + )))); + } + if let Entry::Vacant(e) = self.graph.declarations_mut().entry(*MODULE_ID) { + e.insert(Declaration::Namespace(Namespace::Class(Box::new( + ClassDeclaration::new("Module".to_string(), *OBJECT_ID), + )))); + } + if let Entry::Vacant(e) = self.graph.declarations_mut().entry(*CLASS_ID) { + e.insert(Declaration::Namespace(Namespace::Class(Box::new( + ClassDeclaration::new("Class".to_string(), *OBJECT_ID), + )))); + } + } + /// Runs the core resolution loop, processing units until no more progress can be made. + fn run_resolution_loop(&mut self) { loop { - // Flag to ensure the end of the resolution loop. We go through all items in the queue based on its current - // length. If we made any progress in this pass of the queue, we can continue because we're unlocking more work - // to be done self.made_progress = false; - // Loop through the current length of the queue, which won't change during this pass. Retries pushed to the back - // are only processed in the next pass, so that we can assess whether we made any progress for _ in 0..self.unit_queue.len() { let Some(unit_id) = self.unit_queue.pop_front() else { break; @@ -153,8 +143,6 @@ impl<'a> Resolver<'a> { break; } } - - self.handle_remaining_definitions(other_ids); } /// Resolves a single constant against the graph. This method is not meant to be used by the resolution phase, but by @@ -1438,71 +1426,64 @@ impl<'a> Resolver<'a> { parent_depth + nesting_depth + 1 } - fn prepare_units(&mut self) -> Vec { - let estimated_length = self.graph.definitions().len() / 2; + fn prepare_units(&mut self, work: Vec) -> Vec { + // Partition work items by type + let mut pending_definitions: Vec = Vec::new(); + let mut pending_references: Vec = Vec::new(); + let mut pending_ancestors: Vec = Vec::new(); + + for unit in work { + match unit { + Unit::Definition(id) => pending_definitions.push(id), + Unit::ConstantRef(id) => pending_references.push(id), + Unit::Ancestors(id) => pending_ancestors.push(id), + } + } + + // Deduplicate: the same definition can appear multiple times (e.g., from both + // remove_declaration_tree and extend) + pending_definitions.sort_unstable_by_key(|id| **id); + pending_definitions.dedup(); + + let estimated_length = pending_definitions.len() / 2; let mut definitions = Vec::with_capacity(estimated_length); let mut others = Vec::with_capacity(estimated_length); let names = self.graph.names(); - for (id, definition) in self.graph.definitions() { - let uri = self.graph.documents().get(definition.uri_id()).unwrap().uri(); + for id in pending_definitions { + let Some(definition) = self.graph.definitions().get(&id) else { + continue; + }; - match definition { - Definition::Class(def) => { - definitions.push(( - Unit::Definition(*id), - (names.get(def.name_id()).unwrap(), uri, definition.offset()), - )); - } - Definition::Module(def) => { - definitions.push(( - Unit::Definition(*id), - (names.get(def.name_id()).unwrap(), uri, definition.offset()), - )); - } - Definition::Constant(def) => { - definitions.push(( - Unit::Definition(*id), - (names.get(def.name_id()).unwrap(), uri, definition.offset()), - )); - } - Definition::ConstantAlias(def) => { - definitions.push(( - Unit::Definition(*id), - (names.get(def.name_id()).unwrap(), uri, definition.offset()), - )); - } - Definition::SingletonClass(def) => { - definitions.push(( - Unit::Definition(*id), - (names.get(def.name_id()).unwrap(), uri, definition.offset()), - )); - } - _ => { - others.push(*id); - } + if let Some(name_id) = definition.name_id() { + let uri = self.graph.documents().get(definition.uri_id()).unwrap().uri(); + let name_ref = names.get(name_id).unwrap(); + definitions.push((Unit::Definition(id), (name_ref, uri, definition.offset()))); + } else { + others.push(id); } } // Sort namespaces based on their name complexity so that simpler names are always first - // When the depth is the same, sort by URI and offset to maintain determinism definitions.sort_by(|(_, (name_a, uri_a, offset_a)), (_, (name_b, uri_b, offset_b))| { (Self::name_depth(name_a, names), uri_a, offset_a).cmp(&(Self::name_depth(name_b, names), uri_b, offset_b)) }); - let mut const_refs = self - .graph - .constant_references() - .iter() - .map(|(id, constant_ref)| { - let uri = self.graph.documents().get(&constant_ref.uri_id()).unwrap().uri(); - - ( - Unit::ConstantRef(*id), - (names.get(constant_ref.name_id()).unwrap(), uri, constant_ref.offset()), - ) - }) - .collect::>(); + let mut const_refs = Vec::new(); + for id in pending_references { + let Some(constant_ref) = self.graph.constant_references().get(&id) else { + continue; + }; + let Some(name_ref) = names.get(constant_ref.name_id()) else { + continue; + }; + // Skip references whose names are already resolved + if matches!(name_ref, NameRef::Resolved(_)) { + continue; + } + let uri = self.graph.documents().get(&constant_ref.uri_id()).unwrap().uri(); + const_refs.push((Unit::ConstantRef(id), (name_ref, uri, constant_ref.offset()))); + } // Sort constant references based on their name complexity so that simpler names are always first const_refs.sort_by(|(_, (name_a, uri_a, offset_a)), (_, (name_b, uri_b, offset_b))| { @@ -1514,6 +1495,16 @@ impl<'a> Resolver<'a> { self.unit_queue .extend(const_refs.into_iter().map(|(id, _)| id).collect::>()); + // Queue ancestor re-linearization after definitions and references + for decl_id in pending_ancestors { + if let Some(decl) = self.graph.declarations().get(&decl_id) + && let Some(ns) = decl.as_namespace() + && !ns.has_complete_ancestors() + { + self.unit_queue.push_back(Unit::Ancestors(decl_id)); + } + } + others.shrink_to_fit(); others } diff --git a/rust/rubydex/src/test_utils/graph_test.rs b/rust/rubydex/src/test_utils/graph_test.rs index 534822cd..76886dc4 100644 --- a/rust/rubydex/src/test_utils/graph_test.rs +++ b/rust/rubydex/src/test_utils/graph_test.rs @@ -37,9 +37,10 @@ impl GraphTest { self.graph.delete_document(uri); } + /// Resolves pending work accumulated from update/delete operations. pub fn resolve(&mut self) { let mut resolver = Resolver::new(&mut self.graph); - resolver.resolve_all(); + resolver.resolve(); } /// # Panics @@ -319,8 +320,24 @@ macro_rules! assert_ancestors_eq { .join(", ") ); } - $crate::model::declaration::Ancestors::Partial(_) => { - panic!("Expected ancestors to be resolved for {}", declaration.name()); + $crate::model::declaration::Ancestors::Partial(ancestors) => { + let expected_ancestors: Vec<$crate::model::declaration::Ancestor> = $expected + .iter() + .map(|n| { + $crate::model::declaration::Ancestor::Complete($crate::model::ids::DeclarationId::from(*n)) + }) + .collect(); + + // Allow Partial ancestors when both expected and actual are empty (invalidation cleared them) + if expected_ancestors.is_empty() && ancestors.is_empty() { + // OK - both empty + } else { + panic!( + "Expected ancestors to be resolved for {}, got Partial({:?})", + declaration.name(), + ancestors + ); + } } } }; diff --git a/test/graph_test.rb b/test/graph_test.rb index 9570e7c2..0c140dc0 100644 --- a/test/graph_test.rb +++ b/test/graph_test.rb @@ -562,6 +562,49 @@ def test_workspace_paths private + def test_without_resolution_prevents_memory_growth + with_context do |context| + context.write!("foo.rb", <<~RUBY) + module Foo + CONST = 1 + end + RUBY + + context.write!("bar.rb", <<~RUBY) + class Bar + Foo::CONST + end + RUBY + + graph = Rubydex::Graph.new(without_resolution: true) + assert(graph.without_resolution?) + graph.index_all(context.glob("**/*.rb")) + + # Without resolution, declarations should still be created from definitions + refute_nil(graph["Foo"]) + refute_nil(graph["Bar"]) + + # Repeated re-indexing should not accumulate unbounded work + 10.times do + graph.index_source(context.uri_to("bar.rb"), context.absolute_path_to("bar.rb"), <<~RUBY) + class Bar + Foo::CONST + end + RUBY + end + + # Graph still works for definition lookups + refute_nil(graph["Foo"]) + refute_nil(graph["Bar"]) + end + end + + def test_without_resolution_defaults_to_false + graph = Rubydex::Graph.new + refute(graph.without_resolution?) + end + + def assert_diagnostics(expected, actual) assert_equal( expected,