Skip to content

Commit 2e25ad5

Browse files
committed
Fix cleanup loop for singleton classes on undefined constants
When `class << Math` targets an undefined constant, the resolver creates a TODO for Math. Cleanup removes it (0 defs, no members), resolution re-creates it — looping forever. Two fixes: - Cycle detection: cleanup_empty_declarations tracks previous round's cleaned IDs; if the new candidates are a subset, stop the cycle. - Singleton emptiness: include references in has_no_backing_definitions for singletons, so Foo::<Foo> with callers (Foo.new) is never considered empty. Adds regression test: class << Math with def log2 (backports gem pattern).
1 parent 33be6a9 commit 2e25ad5

3 files changed

Lines changed: 58 additions & 9 deletions

File tree

rust/rubydex/src/model/declaration.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ impl Declaration {
373373
}
374374

375375
if let Some(singleton_class) = self.as_singleton_class() {
376-
return singleton_class.is_empty();
376+
return singleton_class.is_empty() && singleton_class.references().is_empty();
377377
}
378378

379379
self.has_no_definitions()

rust/rubydex/src/model/graph.rs

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -630,16 +630,26 @@ impl Graph {
630630
/// for compact-notation chains. Cascades to members, singletons, and
631631
/// descendants via `invalidate_graph`. Returns `true` if any declarations
632632
/// were cleaned up.
633-
pub(crate) fn cleanup_empty_declarations(&mut self) -> bool {
633+
/// Post-resolution cleanup: removes declarations that are still empty
634+
/// (no definitions). TODO declarations that have acquired members are
635+
/// preserved — they serve as namespace parents for compact-notation
636+
/// chains. Cascades to members, singletons, and descendants via
637+
/// `invalidate_graph`.
638+
///
639+
/// `previous_ids` contains declaration IDs cleaned in the prior round.
640+
/// If this round's candidates are a subset of `previous_ids`, the
641+
/// cleanup is cycling (definitions re-resolve into the same TODOs) and
642+
/// we stop. Returns the IDs cleaned this round for cycle detection.
643+
pub(crate) fn cleanup_empty_declarations(&mut self, previous_ids: &[DeclarationId]) -> Vec<DeclarationId> {
634644
let mut candidates = std::mem::take(&mut self.pending_declaration_cleanup);
635645

636646
if candidates.is_empty() {
637-
return false;
647+
return Vec::new();
638648
}
639649
candidates.sort_unstable();
640650
candidates.dedup();
641651

642-
let items: Vec<InvalidationItem> = candidates
652+
let cleaned_ids: Vec<DeclarationId> = candidates
643653
.into_iter()
644654
.filter(|decl_id| {
645655
self.declarations.get(decl_id).is_some_and(|decl| {
@@ -650,15 +660,25 @@ impl Graph {
650660
decl.has_no_definitions() && !is_todo_with_members
651661
})
652662
})
653-
.map(InvalidationItem::Declaration)
654663
.collect();
655664

656-
if items.is_empty() {
657-
return false;
665+
if cleaned_ids.is_empty() {
666+
return Vec::new();
658667
}
659668

669+
// If this round's candidates are a subset of the previous round's,
670+
// we're in a cycle: cleanup removes them, resolution re-creates them,
671+
// and cleanup finds them again. Stop the cycle — these TODOs are
672+
// permanent fixtures for undefined constants (e.g. `class << Math`
673+
// where Math has no definition in the indexed files).
674+
if !previous_ids.is_empty() && cleaned_ids.iter().all(|id| previous_ids.contains(id)) {
675+
return Vec::new();
676+
}
677+
678+
let items: Vec<InvalidationItem> = cleaned_ids.iter().copied().map(InvalidationItem::Declaration).collect();
679+
660680
self.invalidate_graph(items, IdentityHashMap::default());
661-
true
681+
cleaned_ids
662682
}
663683

664684
/// Removes `decl_id` from each of its ancestors' descendant sets. This
@@ -2440,6 +2460,32 @@ mod tests {
24402460
"Bar name should be removed from the names map"
24412461
);
24422462
}
2463+
2464+
/// Regression test for cleanup loop infinite cycle (backports gem).
2465+
///
2466+
/// `class << Math` on an undefined `Math` creates a singleton class
2467+
/// definition whose name has `Math` as parent scope. The resolver
2468+
/// creates a TODO for `Math` to serve as owner. Since the singleton
2469+
/// class is a special member (set via `set_singleton_class_id`, not
2470+
/// `add_member`), the `is_todo_with_members` check doesn't save it.
2471+
/// Cleanup removes the TODO, unresolves the singleton's name, and
2472+
/// re-resolution re-creates the TODO — looping forever.
2473+
#[test]
2474+
fn resolve_terminates_with_singleton_class_on_undefined_constant() {
2475+
let mut context = GraphTest::new();
2476+
context.index_uri(
2477+
"file:///a.rb",
2478+
"
2479+
class << Math
2480+
def log2(x)
2481+
x
2482+
end
2483+
end
2484+
",
2485+
);
2486+
// If the bug is present, resolve() panics with "cleanup loop did not converge".
2487+
context.resolve();
2488+
}
24432489
}
24442490

24452491
#[cfg(test)]

rust/rubydex/src/resolution.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ impl<'a> Resolver<'a> {
9393
/// Can panic if there's inconsistent data in the graph
9494
pub fn resolve(&mut self) {
9595
let mut other_ids = self.prepare_units();
96+
let mut previous_cleanup_ids = Vec::new();
9697

9798
loop {
9899
self.made_progress = false;
@@ -125,9 +126,11 @@ impl<'a> Resolver<'a> {
125126
self.graph.extend_work(std::mem::take(&mut self.unit_queue));
126127
self.handle_remaining_definitions(std::mem::take(&mut other_ids));
127128

128-
if !self.graph.cleanup_empty_declarations() {
129+
let cleaned = self.graph.cleanup_empty_declarations(&previous_cleanup_ids);
130+
if cleaned.is_empty() {
129131
break;
130132
}
133+
previous_cleanup_ids = cleaned;
131134

132135
// Cleanup cascaded — reclassify new work through prepare_units()
133136
// so regular methods/attrs/variables are routed correctly.

0 commit comments

Comments
 (0)