From 76224ea4111dfe359afcf9739df8dcf0d013bb3b Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 16 Apr 2026 22:28:53 +0100 Subject: [PATCH 1/2] Defer cleanup of emptied and synthetic declarations during invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After incremental invalidation, declarations can lose all their definitions. Previously these were removed immediately, destroying accumulated state (singleton links, member lists) the resolver needs. This defers TODO cleanup to after resolution via `cleanup_stale_todos()`. TODOs created speculatively by `create_todo_for_parent` are marked via `defer_todo_cleanup` and evaluated post-resolution — promoted TODOs and those with members are kept, empty stubs are removed. Singleton classes use references and members as "still in use" signals in `has_no_backing_definitions`, preventing premature removal when callers (Foo.new) or class-level state (@x = 1) still exist. Other empty declarations (from file deletion or member removal) cascade immediately via the invalidation worklist. --- rust/rubydex/src/model/declaration.rs | 22 ++ rust/rubydex/src/model/graph.rs | 538 +++++++++++++++++++++++++- rust/rubydex/src/resolution.rs | 26 +- 3 files changed, 556 insertions(+), 30 deletions(-) diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index d821c7d62..96137cc11 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/rust/rubydex/src/model/declaration.rs @@ -331,6 +331,14 @@ impl Declaration { } } + #[must_use] + pub fn as_singleton_class(&self) -> Option<&Namespace> { + match self { + Declaration::Namespace(ns @ Namespace::SingletonClass(_)) => Some(ns), + _ => None, + } + } + #[must_use] pub fn as_namespace_mut(&mut self) -> Option<&mut Namespace> { match self { @@ -349,6 +357,20 @@ impl Declaration { all_declarations!(self, it => it.definition_ids.is_empty()) } + /// Returns true if this declaration is no longer anchored to anything + /// and can be removed. For most declarations, that means having no + /// definitions. Singleton classes are an exception: they're also kept + /// alive by their members (e.g. `@x`, `def self.bar`) and by inbound + /// constant references (e.g. `Foo.new`). + #[must_use] + pub fn is_removable(&self) -> bool { + if let Some(ns) = self.as_singleton_class() { + return ns.definitions().is_empty() && ns.members().is_empty() && ns.references().is_empty(); + } + + self.has_no_definitions() + } + pub fn add_definition(&mut self, definition_id: DefinitionId) { all_declarations!(self, it => { debug_assert!( diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index d866fa107..a70a5d80c 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -83,6 +83,29 @@ pub struct Graph { /// Paths to exclude from file discovery during indexing. excluded_paths: HashSet, + + /// Watch list of TODOs created by `create_todo_for_parent` during + /// resolution. Every new TODO is recorded here via `track_todo`. + /// + /// Whether a tracked TODO actually gets removed is decided later, in + /// one of two ways: + /// + /// 1. Cascaded removal — invalidation removes the TODO's last child. + /// Example: with just `class A::B; end` in the codebase, deleting + /// the file removes `A::B` and cascades to the TODO `A`. + /// Handled inline in `invalidate_declaration`. + /// + /// 2. Post-resolution cleanup — some TODOs stay empty forever. + /// Example: `class << Math; def log2(x); x; end; end` with no + /// `module Math` anywhere. The resolver creates a TODO `Math` as + /// the singleton's parent, but the singleton attaches via + /// `set_singleton_class_id`, not `add_member`. So TODO `Math` + /// never gains a member; nothing is ever removed, so (1) never + /// fires. We can't decide this at creation time — a later file + /// could still add `module Math` and promote it. `cleanup_stale_todos` + /// sweeps this list after resolution and removes entries that are + /// still empty. + tracked_todos: IdentityHashSet, } impl Graph { @@ -100,6 +123,7 @@ impl Graph { name_dependents: IdentityHashMap::default(), pending_work: Vec::default(), excluded_paths: HashSet::new(), + tracked_todos: IdentityHashSet::default(), }; add_built_in_data(&mut graph); @@ -663,6 +687,10 @@ impl Graph { std::mem::take(&mut self.pending_work) } + pub(crate) fn track_todo(&mut self, decl_id: DeclarationId) { + self.tracked_todos.insert(decl_id); + } + pub(crate) fn push_work(&mut self, unit: Unit) { self.pending_work.push(unit); } @@ -671,6 +699,34 @@ impl Graph { self.pending_work.extend(units); } + /// Drains `tracked_todos` and removes orphan TODOs via + /// `invalidate_graph`. See `tracked_todos` for the full rationale. + pub(crate) fn cleanup_stale_todos(&mut self) { + let candidates = std::mem::take(&mut self.tracked_todos); + + if candidates.is_empty() { + return; + } + + let items: Vec = candidates + .into_iter() + .filter(|decl_id| match self.declarations.get(decl_id) { + // TODOs that acquired members are actively serving as namespace + // parents (e.g. compact notation `class A::B::C`). Keep them. + Some(Declaration::Namespace(ns @ Namespace::Todo(_))) => { + ns.definitions().is_empty() && ns.members().is_empty() + } + Some(decl) => decl.has_no_definitions(), + None => false, + }) + .map(InvalidationItem::Declaration) + .collect(); + + if !items.is_empty() { + self.invalidate_graph(items, IdentityHashMap::default()); + } + } + /// 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 { @@ -1047,7 +1103,7 @@ impl Graph { if let Some(constant_ref) = self.constant_references.remove(ref_id) { // Detach from target declaration. References unresolved during invalidation // were already detached; this catches the rest. - if let NameRef::Resolved(resolved) = self.names.get(constant_ref.name_id()).unwrap() + 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_constant_reference(ref_id); @@ -1074,10 +1130,18 @@ impl Graph { .collect(); if !missed_def_ids.is_empty() { - for declaration in self.declarations.values_mut() { + let mut emptied: Vec = Vec::new(); + for (decl_id, declaration) in &mut self.declarations { + let had_definitions = !declaration.definitions().is_empty(); for def_id in &missed_def_ids { declaration.remove_definition(def_id); } + if had_definitions && declaration.is_removable() { + emptied.push(InvalidationItem::Declaration(*decl_id)); + } + } + if !emptied.is_empty() { + self.invalidate_graph(emptied, IdentityHashMap::default()); } } @@ -1092,8 +1156,11 @@ impl Graph { } } - /// Unified invalidation worklist. Processes declaration and name items in a single loop, + /// Invalidation worklist. Processes declaration, name, and reference items in a loop, /// where processing one item can push new items back onto the queue. + /// + /// `pending_detachments` maps declarations to definitions that need to be detached + /// before deciding whether to remove or update each declaration. fn invalidate_graph( &mut self, items: Vec, @@ -1158,13 +1225,18 @@ impl Graph { } } + // The same declaration can be queued twice (e.g. from `tracked_todos` + // plus a cascade from an owner). Re-visits are idempotent: once removed, the + // lookup below returns None and we short-circuit. let Some(decl) = self.declarations.get(&decl_id) else { return; }; - let should_remove = decl.has_no_definitions() || !self.declarations.contains_key(decl.owner_id()); + let should_remove = decl.is_removable() || !self.declarations.contains_key(decl.owner_id()); if should_remove { - // Queue members + singleton for removal + // Remove path: definitions came from a deleted/changed file and won't + // re-resolve to this declaration. Removal must be immediate so child + // declarations see a missing owner and cascade correctly. if let Some(ns) = decl.as_namespace() { if let Some(singleton_id) = ns.singleton_class() { queue.push(InvalidationItem::Declaration(*singleton_id)); @@ -1193,7 +1265,11 @@ impl Graph { } } - // Clean up owner membership and queue remaining definitions for re-resolution + // Detach from the owner and re-queue any surviving definitions. + // The def_ids loop only does work when we're here because the owner + // was deleted (orphan path) — definitions still exist and need to + // be re-resolved against a new owner. On the is_removable + // path, `decl.definitions()` is already empty. if let Some(decl) = self.declarations.get(&decl_id) { let def_ids: Vec = decl.definitions().to_vec(); let unqualified_str_id = StringId::from(&decl.unqualified_name()); @@ -1213,6 +1289,12 @@ impl Graph { ns.remove_member(&unqualified_str_id); } } + + // The owner may now be eligible for removal (e.g. a TODO parent + // that lost its last child). Cascade inline via the worklist. + if self.declarations.get(&owner_id).is_some_and(Declaration::is_removable) { + queue.push(InvalidationItem::Declaration(owner_id)); + } } self.declarations.remove(&decl_id); @@ -1277,10 +1359,14 @@ impl Graph { decl.remove_definition(def_id); } + // This definition is from a surviving file — it was re-queued + // and will likely re-resolve to the same declaration. If it + // resolves elsewhere, the old declaration is left empty. Queue + // it for removal so it doesn't linger. if self .declarations .get(&old_decl_id) - .is_some_and(Declaration::has_no_definitions) + .is_some_and(Declaration::is_removable) { queue.push(InvalidationItem::Declaration(old_decl_id)); } @@ -2221,6 +2307,36 @@ mod tests { assert!(context.graph().get("Foo::::<>").is_none()); } + /// Regression test: `class << Math` with no `module Math` in the codebase + /// used to send `resolve()` into an infinite loop. A singleton attaches via + /// `set_singleton_class_id`, not `add_member`, so a TODO receiver would never + /// gain a member and would be removed on the next cleanup round — which + /// unresolved the singleton's name and re-ran resolution, re-creating the + /// TODO forever. The resolver now skips TODO creation when the owner being + /// placed is a singleton, so the receiver is never materialized and neither + /// is the singleton. + #[test] + fn resolve_terminates_with_singleton_class_on_undefined_constant() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///a.rb", + " + class << Math + def log2(x) + x + end + end + ", + ); + // If the bug returns, this call hangs. + context.resolve(); + + // No `Math` TODO is synthesized and no singleton is attached — the + // receiver is genuinely missing and the singleton can't be placed. + assert_declaration_does_not_exist!(context, "Math"); + assert_declaration_does_not_exist!(context, "Math::"); + } + #[test] fn indexing_the_same_document_twice() { let mut context = GraphTest::new(); @@ -3773,6 +3889,85 @@ mod incremental_resolution_tests { assert_no_dangling_definitions(context.graph()); } + /// Deleting a bare reopener (`class Foo; end`) must not cascade-remove + /// `Foo::` when another file still backs it with singleton members. + #[test] + fn singleton_class_survives_when_reopener_is_deleted() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", "class Foo; def self.bar; end; end"); + context.index_uri("file:///reopener.rb", "class Foo; end"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); + + context.delete_uri("file:///reopener.rb"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); + } + + #[test] + fn singleton_survives_when_singleton_definition_deleted_but_caller_remains() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", "class Foo; end"); + context.index_uri("file:///foo_singleton.rb", "class Foo; class << self; end; end"); + context.index_uri("file:///whatever.rb", "Foo.new"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); + + // Remove the file with the explicit `class << self`. Foo:: should + // survive because Foo.new in whatever.rb created an Attached reference + // that also triggers singleton creation. + context.delete_uri("file:///foo_singleton.rb"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); + } + + /// A class nested inside a `class << self` scope (`Outer::::Inner`) + /// has a singleton-class ancestor in its qualified name. Deleting an + /// unrelated reopener of the outer module must not cascade through the + /// singleton path and remove the nested class. + #[test] + fn nested_class_inside_singleton_scope_survives_reopener_deletion() { + let mut context = GraphTest::new(); + + context.index_uri( + "file:///main.rb", + r" + module Outer + class << self + class Inner + def initialize; end + end + + def run + Inner.new + end + end + end + ", + ); + context.index_uri("file:///reopener.rb", "module Outer; end"); + context.resolve(); + + assert_declaration_exists!(context, "Outer"); + assert_declaration_exists!(context, "Outer::::Inner"); + assert_declaration_exists!(context, "Outer::::Inner::"); + + context.delete_uri("file:///reopener.rb"); + context.resolve(); + + assert_declaration_exists!(context, "Outer"); + assert_declaration_exists!(context, "Outer::::Inner"); + assert_declaration_exists!(context, "Outer::::Inner::"); + } + #[test] fn singleton_class_preserved_after_delete_and_reindex() { let mut context = GraphTest::new(); @@ -3794,28 +3989,62 @@ mod incremental_resolution_tests { assert_declaration_exists!(context, "Foo::"); } + /// Deleting the file that defines singleton-class members (`@x`, + /// `self.bar`) must remove those member declarations, not just the + /// singleton itself. `Foo` survives because a bare reopener in another + /// file still defines it. #[test] - fn singleton_recreated_when_reference_nested_in_compact_class() { + fn singleton_members_cleaned_up_after_file_deletion() { let mut context = GraphTest::new(); + context.index_uri( + "file:///a.rb", + " + class Foo + @x = 1 + def self.bar; end + end + ", + ); + context.index_uri("file:///b.rb", "class Foo; end"); + context.resolve(); - context.index_uri("file:///parent.rb", "module Parent; end"); - context.index_uri("file:///target.rb", "class Parent::Target; end"); - context.index_uri("file:///caller.rb", "class Parent::Caller; Parent::Target.new; end"); + assert_declaration_exists!(context, "Foo::#@x"); + assert_declaration_exists!(context, "Foo::#bar()"); + + context.delete_uri("file:///a.rb"); context.resolve(); - assert_declaration_exists!(context, "Parent::Target"); - assert_declaration_exists!(context, "Parent::Target::"); + assert_declaration_exists!(context, "Foo"); + assert_declaration_does_not_exist!(context, "Foo::#@x"); + assert_declaration_does_not_exist!(context, "Foo::#bar()"); + } - context.delete_uri("file:///parent.rb"); - context.delete_uri("file:///target.rb"); + /// Mirror of `singleton_class_survives_when_reopener_is_deleted`: once + /// the file defining the singleton members is gone, the singleton must + /// be collected (it is no longer backed by anything). Also verifies the + /// singleton is recreated if the file comes back. + #[test] + fn empty_singleton_removed_after_its_members_are_deleted() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; def self.bar; end; end"); + context.index_uri("file:///reopener.rb", "class Foo; end"); context.resolve(); - context.index_uri("file:///parent.rb", "module Parent; end"); - context.index_uri("file:///target.rb", "class Parent::Target; end"); + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); + + context.delete_uri("file:///foo.rb"); context.resolve(); - assert_declaration_exists!(context, "Parent::Target"); - assert_declaration_exists!(context, "Parent::Target::"); + assert_declaration_exists!(context, "Foo"); + assert_declaration_does_not_exist!(context, "Foo::"); + + context.index_uri("file:///foo.rb", "class Foo; def self.bar; end; end"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::"); } #[test] @@ -4020,4 +4249,275 @@ mod incremental_resolution_tests { assert_declaration_exists!(context, "Foo::"); assert_declaration_exists!(context, "Bar::"); } + + /// Deleting the file that defines an ancestor module (`A`) makes + /// compact nested classes (`class A::B::C`) briefly unresolvable, so + /// `create_todo_for_parent` synthesizes a TODO for `A`. Re-adding the + /// ancestor file must not leave that orphan TODO behind — the final + /// state must match a fresh index. + #[test] + fn no_orphan_todo_after_ancestor_definer_readded() { + let index_all = |g: &mut GraphTest| { + g.index_uri("file:///a.rb", "module A; end"); + g.index_uri("file:///b.rb", "class A::B::C; end"); + }; + + let mut incremental = GraphTest::new(); + index_all(&mut incremental); + incremental.resolve(); + + incremental.delete_uri("file:///a.rb"); + incremental.resolve(); + + incremental.index_uri("file:///a.rb", "module A; end"); + incremental.resolve(); + + let mut fresh = GraphTest::new(); + index_all(&mut fresh); + fresh.resolve(); + + assert_declaration_ids_match(&incremental, &fresh); + } + + /// After deleting and re-adding the ancestor module, the `A::C::D` TODO + /// chain must be restored so `class A::C::D::E` still resolves. Checks + /// the restoration side of the delete+readd cycle (complements + /// `no_orphan_todo_after_ancestor_definer_readded` which checks + /// non-leakage). + #[test] + fn compact_class_todo_chain_restored_after_ancestor_delete_readd() { + let a = "module A; end; class A::B; end"; + let b = "class A::C::D::E < A::B; end"; + + let mut context = GraphTest::new(); + context.index_uri("file:///a.rb", a); + context.index_uri("file:///b.rb", b); + context.resolve(); + + assert_declaration_exists!(context, "A::C::D::E"); + + context.delete_uri("file:///a.rb"); + context.resolve(); + + context.index_uri("file:///a.rb", a); + context.resolve(); + + assert_declaration_exists!(context, "A"); + assert_declaration_exists!(context, "A::B"); + assert_declaration_exists!(context, "A::C::D::E"); + } + + /// `class A::B::C` with `class << self` creates a singleton on a TODO + /// declaration. The `include D::E` in `module A` changes resolution + /// ordering so the singleton is attached before the TODO is promoted. + /// Calling `resolve()` twice must produce the same state as once — no + /// leaked singletons from the promotion, no lost declarations from + /// over-eager cleanup. + #[test] + fn no_leaked_singleton_on_todo_after_double_resolve() { + let index_all = |g: &mut GraphTest| { + g.index_uri("file:///a.rb", "class A::B::C; class << self; def run; end; end; end"); + g.index_uri( + "file:///b.rb", + "module A; include D::E; class F < StandardError; end; end", + ); + g.index_uri("file:///c.rb", "module D; module E; end; end"); + }; + + let mut incremental = GraphTest::new(); + index_all(&mut incremental); + incremental.resolve(); + incremental.resolve(); + + let mut fresh = GraphTest::new(); + index_all(&mut fresh); + fresh.resolve(); + + assert_declaration_ids_match(&incremental, &fresh); + } + + /// Minimal cascade: deleting the only file that defines `A::B` + /// (compact form) must remove both `A::B` and the synthesized TODO + /// parent `A`. + #[test] + fn todo_parent_removed_after_compact_child_deleted() { + let mut context = GraphTest::new(); + + context.index_uri("file:///a.rb", "class A::B; end"); + context.resolve(); + + assert_declaration_exists!(context, "A"); + assert_declaration_exists!(context, "A::B"); + + context.delete_uri("file:///a.rb"); + context.resolve(); + + assert_declaration_does_not_exist!(context, "A"); + assert_declaration_does_not_exist!(context, "A::B"); + } + + /// Extends `todo_parent_removed_after_compact_child_deleted` with a + /// singleton on the compact child. Verifies the cascade reaches the + /// singleton through the owner-deleted path. + #[test] + fn orphan_singleton_cleaned_up_when_owner_deleted() { + let mut context = GraphTest::new(); + context.index_uri("file:///a.rb", "class A::B; class << self; end; end"); + context.resolve(); + + assert_declaration_exists!(context, "A"); + assert_declaration_exists!(context, "A::B"); + assert_declaration_exists!(context, "A::B::"); + + // Deleting the only file removes B (and its singleton) AND the TODO + // parent A. + context.delete_uri("file:///a.rb"); + context.resolve(); + + assert_declaration_does_not_exist!(context, "A::B"); + assert_declaration_does_not_exist!(context, "A::B::"); + assert_declaration_does_not_exist!(context, "A"); + assert_no_dangling_definitions(context.graph()); + } + + /// Cross-file superclass singleton cascade: deleting both a base class + /// and its subclass (where each has its own singleton) must remove + /// every declaration in the chain. + #[test] + fn ancestor_singletons_cleaned_up_after_class_chain_deleted() { + let mut context = GraphTest::new(); + context.index_uri("file:///a.rb", "class A; end"); + context.index_uri("file:///b.rb", "class B < A; class << self; end; end"); + context.resolve(); + + assert_declaration_exists!(context, "A::"); + assert_declaration_exists!(context, "B::"); + + // Delete both files — A:: should cascade-clean because A is gone. + context.delete_uri("file:///a.rb"); + context.delete_uri("file:///b.rb"); + context.resolve(); + + assert_declaration_does_not_exist!(context, "A"); + assert_declaration_does_not_exist!(context, "B"); + assert_declaration_does_not_exist!(context, "A::"); + assert_declaration_does_not_exist!(context, "B::"); + assert_no_dangling_definitions(context.graph()); + } + + /// A singleton materialized via a `.new` reference inside a compact-class + /// body (`class Parent::Caller; Parent::Target.new; end`) must be restored + /// after the parent TODO is deleted and re-indexed. Protects the + /// reference-keeps-singleton-alive path for compact nesting. + #[test] + fn singleton_recreated_when_reference_nested_in_compact_class() { + let mut context = GraphTest::new(); + + context.index_uri("file:///parent.rb", "module Parent; end"); + context.index_uri("file:///target.rb", "class Parent::Target; end"); + context.index_uri("file:///caller.rb", "class Parent::Caller; Parent::Target.new; end"); + context.resolve(); + + assert_declaration_exists!(context, "Parent::Target"); + assert_declaration_exists!(context, "Parent::Target::"); + + context.delete_uri("file:///parent.rb"); + context.delete_uri("file:///target.rb"); + context.resolve(); + + context.index_uri("file:///parent.rb", "module Parent; end"); + context.index_uri("file:///target.rb", "class Parent::Target; end"); + context.resolve(); + + assert_declaration_exists!(context, "Parent::Target"); + assert_declaration_exists!(context, "Parent::Target::"); + } + + /// A TODO left with only an external reference (no members, no + /// definitions) is still removed. `class Baz < Foo` registers a + /// reference against the TODO `Foo` while `class Foo::Bar` holds `Foo` + /// alive via the `Bar` member. Deleting `Foo::Bar` leaves `Foo` with + /// just the inbound reference — which must not keep it alive. + #[test] + fn todo_with_only_references_is_removed_after_member_deleted() { + let mut context = GraphTest::new(); + context.index_uri("file:///a.rb", "class Foo::Bar; end"); + context.index_uri("file:///b.rb", "class Baz < Foo; end"); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::Bar"); + assert_declaration_exists!(context, "Baz"); + + context.delete_uri("file:///a.rb"); + context.resolve(); + + assert_declaration_does_not_exist!(context, "Foo::Bar"); + assert_declaration_does_not_exist!(context, "Foo"); + assert_declaration_exists!(context, "Baz"); + } + + /// `class << Foo::Bar::Baz` on a compact chain where no ancestor is defined. + /// A sibling nested class keeps the chain alive after the initial resolve + /// so we can observe the TODOs, then deletion cascades the whole chain + /// plus the singleton. + #[test] + fn compact_chain_singleton_on_undefined_parents_cleaned_up_after_delete() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///a.rb", + " + class << Foo::Bar::Baz + def qux; end + end + class Foo::Bar::Baz::Keep; end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Foo"); + assert_declaration_exists!(context, "Foo::Bar"); + assert_declaration_exists!(context, "Foo::Bar::Baz"); + assert_declaration_exists!(context, "Foo::Bar::Baz::"); + assert_declaration_exists!(context, "Foo::Bar::Baz::Keep"); + + context.delete_uri("file:///a.rb"); + context.resolve(); + + assert_declaration_does_not_exist!(context, "Foo"); + assert_declaration_does_not_exist!(context, "Foo::Bar"); + assert_declaration_does_not_exist!(context, "Foo::Bar::Baz"); + assert_declaration_does_not_exist!(context, "Foo::Bar::Baz::"); + assert_declaration_does_not_exist!(context, "Foo::Bar::Baz::Keep"); + assert_no_dangling_definitions(context.graph()); + } + + /// Churn across multiple `resolve()` calls: delete one file, resolve, delete + /// another, resolve. End state must equal a fresh index of the final + /// sources. Catches bugs where `tracked_todos` leaks across resolves + /// or cascaded cleanup misses cross-resolve-round dependencies. + #[test] + fn multi_round_deletion_matches_fresh_index() { + let index_initial = |g: &mut GraphTest| { + g.index_uri("file:///a.rb", "class A; def self.x; end; end"); + g.index_uri("file:///b.rb", "class B < A; class << self; end; end"); + g.index_uri("file:///c.rb", "class A::C; end"); + }; + + let mut incremental = GraphTest::new(); + index_initial(&mut incremental); + incremental.resolve(); + + incremental.delete_uri("file:///b.rb"); + incremental.resolve(); + + incremental.delete_uri("file:///c.rb"); + incremental.resolve(); + + let mut fresh = GraphTest::new(); + fresh.index_uri("file:///a.rb", "class A; def self.x; end; end"); + fresh.resolve(); + + assert_declaration_ids_match(&incremental, &fresh); + } } // mod incremental_resolution_tests diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index 3fc8ec4aa..eb6d260ee 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -82,12 +82,12 @@ impl<'a> Resolver<'a> { } } - /// 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 + /// Resolves all pending work and cleans up synthetic declarations. + /// `prepare_units()` classifies pending work into the namespace/reference + /// fixpoint (`unit_queue`) and non-namespace definitions (`other_ids`). + /// After the fixpoint stalls, any leftovers spill back to `pending_work` + /// for the next `resolve()` call, `other_ids` are handled, and finally + /// `cleanup_stale_todos` sweeps orphan TODOs. /// /// # Panics /// @@ -134,6 +134,9 @@ impl<'a> Resolver<'a> { self.graph.extend_work(std::mem::take(&mut self.unit_queue)); self.handle_remaining_definitions(other_ids); + + // See `Graph::tracked_todos` for why this runs here. + self.graph.cleanup_stale_todos(); } /// Resolves a single constant against the graph. This method is not meant to be used by the resolution phase, but by @@ -1226,17 +1229,17 @@ impl<'a> Resolver<'a> { let str_id = *name_ref.str(); let outcome = match self.name_owner_id(name_id, singleton) { - // name_owner_id returns Unresolved(None) only when the parent scope is genuinely unknown - // (e.g., `class A::B::C` where `A` doesn't exist). This definition needs an owner, so - // create Todo placeholders for the missing parent chain. Todos get promoted when real - // definitions appear later. + // name_owner_id returns Unresolved(None) only when the parent scope is genuinely + // unknown (e.g., `class A::B::C` where `A` doesn't exist). For regular declarations + // we create Todo placeholders for the missing parent chain so `B::C` can still be + // placed; Todos get promoted when real definitions appear later. // // Singleton classes are the exception: `class << UndefinedReceiver` attaches via // `set_singleton_class_id`, not `add_member`, so a TODO receiver would never gain a // member. Emit Retry so the unit is preserved for a later resolve where the receiver // may exist. Outcome::Unresolved(None) if singleton => Outcome::Retry(None), - Outcome::Unresolved(None) => Outcome::Resolved(self.create_todo_for_parent(name_id), None), + Outcome::Unresolved(None) if !singleton => Outcome::Resolved(self.create_todo_for_parent(name_id), None), other => other, }; @@ -1395,6 +1398,7 @@ impl<'a> Resolver<'a> { parent_owner_id, ))))); self.graph.add_member(&parent_owner_id, declaration_id, parent_str_id); + self.graph.track_todo(declaration_id); } declaration_id From dc37cbf667e1e7c9b9aff3835eeeb466a2b2d89e Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Fri, 1 May 2026 11:14:47 +0800 Subject: [PATCH 2/2] Collect singleton classes after reference deletion --- rust/rubydex/src/model/graph.rs | 54 ++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index a70a5d80c..50f05e8d4 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -1099,14 +1099,23 @@ impl Graph { } } + // Detach references and queue any declaration that becomes removable as a + // result. `invalidate()` only seeds from removed definitions, so a singleton + // kept alive purely by a reference (e.g. `Foo::` materialized by + // `Foo.new`) would otherwise linger after the reference's file is deleted. + let mut emptied_by_ref_removal: Vec = Vec::new(); for ref_id in document.constant_references() { if let Some(constant_ref) = self.constant_references.remove(ref_id) { // Detach from target declaration. References unresolved during invalidation // were already detached; 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_constant_reference(ref_id); + if let Some(NameRef::Resolved(resolved)) = self.names.get(constant_ref.name_id()) { + let target_id = *resolved.declaration_id(); + if let Some(declaration) = self.declarations.get_mut(&target_id) { + declaration.remove_constant_reference(ref_id); + if declaration.is_removable() { + emptied_by_ref_removal.push(InvalidationItem::Declaration(target_id)); + } + } } self.remove_name_dependent(*constant_ref.name_id(), NameDependent::Reference(*ref_id)); @@ -1114,6 +1123,10 @@ impl Graph { } } + if !emptied_by_ref_removal.is_empty() { + self.invalidate_graph(emptied_by_ref_removal, IdentityHashMap::default()); + } + // Detach removed definitions from their declarations. // Most definitions were already detached by invalidate_declaration via // pending_detachments. Definitions not handled by pending_detachments are @@ -4520,4 +4533,37 @@ mod incremental_resolution_tests { assert_declaration_ids_match(&incremental, &fresh); } + + /// A singleton class materialized solely by a constant reference (e.g. + /// `Foo.new` creating `Foo::` even though no `class << Foo` block + /// exists) must be collected when its sole supporting reference is + /// removed. `invalidate()` only seeds from removed definitions, so the + /// reference detachment in `remove_document_data` queues now-removable + /// declarations for cleanup. + #[test] + fn singleton_kept_only_by_reference_collected_on_ref_delete() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", "class Foo; end"); + context.index_uri("file:///user.rb", "Foo.new"); + context.resolve(); + assert_declaration_exists!(context, "Foo::"); + + context.delete_uri("file:///user.rb"); + context.resolve(); + + // The reference that materialized the singleton is gone, so the + // singleton itself must not linger and `Foo.singleton_class_id` must + // be cleared. + assert_declaration_does_not_exist!(context, "Foo::"); + let foo = context + .graph() + .declarations() + .get(&crate::model::ids::DeclarationId::from("Foo")) + .expect("Foo should still exist"); + let foo_ns = foo.as_namespace().expect("Foo is a namespace"); + assert!( + foo_ns.singleton_class().is_none(), + "Foo.singleton_class_id should be cleared after the singleton is removed" + ); + } } // mod incremental_resolution_tests