diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index 2b6f592aa..ae84fe182 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/rust/rubydex/src/model/declaration.rs @@ -30,6 +30,18 @@ pub enum Ancestors { assert_mem_size!(Ancestors, 32); impl Ancestors { + #[must_use] + pub fn contains(&self, declaration_id: DeclarationId) -> bool { + match self { + Ancestors::Complete(ancestors) | Ancestors::Partial(ancestors) | Ancestors::Cyclic(ancestors) => { + ancestors.iter().any(|ancestor| match ancestor { + Ancestor::Complete(id) => *id == declaration_id, + Ancestor::Partial(_) => false, + }) + } + } + } + pub fn iter(&self) -> std::slice::Iter<'_, Ancestor> { match self { Ancestors::Complete(ancestors) | Ancestors::Partial(ancestors) | Ancestors::Cyclic(ancestors) => { diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index a414340f1..4515c44a7 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -1077,22 +1077,24 @@ impl<'a> Resolver<'a> { Declaration::Namespace(Namespace::Class(_)) => { let definition_ids = declaration.definitions().to_vec(); - Some(match self.linearize_parent_class(&definition_ids, context) { - Ancestors::Complete(ids) => ids, - Ancestors::Cyclic(ids) => { - context.cyclic = true; - ids - } - Ancestors::Partial(ids) => { - context.partial = true; - ids - } - }) + Some( + match self.linearize_parent_class(declaration_id, &definition_ids, context) { + Ancestors::Complete(ids) => ids, + Ancestors::Cyclic(ids) => { + context.cyclic = true; + ids + } + Ancestors::Partial(ids) => { + context.partial = true; + ids + } + }, + ) } Declaration::Namespace(Namespace::SingletonClass(_)) => { let owner_id = *declaration.owner_id(); - let (singleton_parent_id, partial_singleton) = self.singleton_parent_id(owner_id); + let (singleton_parent_id, partial_singleton) = self.singleton_parent_id(owner_id, 1); if partial_singleton { context.partial = true; } @@ -2019,7 +2021,7 @@ impl<'a> Resolver<'a> { /// - Module: parent is the `Module` class /// - Class: parent is the singleton class of the original parent class /// - Singleton class: recurse as many times as necessary to wrap the original attached object's parent class - fn singleton_parent_id(&mut self, attached_id: DeclarationId) -> (DeclarationId, bool) { + fn singleton_parent_id(&mut self, attached_id: DeclarationId, depth: u16) -> (DeclarationId, bool) { // Base case: if we reached `BasicObject`, then the parent is `Class` if attached_id == *BASIC_OBJECT_ID { return (*CLASS_ID, false); @@ -2034,7 +2036,7 @@ impl<'a> Resolver<'a> { // object let owner_id = *decl.owner_id(); - let (inner_parent, partial) = self.singleton_parent_id(owner_id); + let (inner_parent, partial) = self.singleton_parent_id(owner_id, depth + 1); ( self.get_or_create_singleton_class(inner_parent, SingletonAncestors::Deferred) .expect("singleton parent should always be a namespace"), @@ -2045,6 +2047,51 @@ impl<'a> Resolver<'a> { // For classes (the regular case), we need to return the singleton class of its parent let definition_ids = decl.definitions().to_vec(); + let class_ancestors = self + .graph + .declarations() + .get(&*CLASS_ID) + .unwrap() + .as_namespace() + .unwrap() + .ancestors(); + + // When creating a new singleton class, we need to ensure that all descendants also get a singleton + // class, otherwise we end up with broken chains. Because we also enqueue ancestor linearization for + // them, we land here again (making this process recursive). The stop condition is finding any ancestors + // of `Class`, which is the default parent for singletons. + // + // All singleton classes inherit from `Class` and its ancestors, so without this, the algorithm goes + // into infinite recursion: + // + // 1. Create a singleton class for a descendant + // 2. Enqueue ancestor linearization for the descendant's singleton class + // 3. Eventually, we reach this exact spot with `Class`. Since the singleton we just created is a + // descendant of `Class`, we create a new singleton one level deeper, taking us back to 1 + if !class_ancestors.contains(attached_id) { + let descendants = decl + .as_namespace() + .unwrap() + .descendants() + .iter() + .copied() + .collect::>(); + + for descendant in descendants { + if descendant == attached_id { + continue; + } + + let mut needs_singleton_id = descendant; + + for _ in 0..depth { + needs_singleton_id = self + .get_or_create_singleton_class(needs_singleton_id, SingletonAncestors::Enqueue) + .expect("descendants are always namespaces"); + } + } + } + let (picked_parent, unresolved_parent) = self.get_parent_class(&definition_ids); ( self.get_or_create_singleton_class(picked_parent, SingletonAncestors::Deferred) @@ -2096,12 +2143,15 @@ impl<'a> Resolver<'a> { fn linearize_parent_class( &mut self, + declaration_id: DeclarationId, definition_ids: &[DefinitionId], context: &mut LinearizationContext, ) -> Ancestors { let (picked_parent, unresolved_parent) = self.get_parent_class(definition_ids); let mut result = self.linearize_ancestors(picked_parent, context); + self.ensure_matching_singleton_class_depth(declaration_id, picked_parent); + if let Some(name_id) = unresolved_parent { context.partial = true; @@ -2118,6 +2168,37 @@ impl<'a> Resolver<'a> { } } + fn ensure_matching_singleton_class_depth(&mut self, declaration_id: DeclarationId, parent_id: DeclarationId) { + // Incremental resolution scenario: if a new class is created inheriting from a parent that already has a + // singleton class, we need to create its own singleton to avoid broken descendants + let mut parent_singleton = self + .graph + .declarations() + .get(&parent_id) + .unwrap() + .as_namespace() + .unwrap() + .singleton_class() + .copied(); + let mut attached = declaration_id; + + while let Some(parent_singleton_id) = parent_singleton { + attached = self + .get_or_create_singleton_class(attached, SingletonAncestors::Enqueue) + .expect("the declaration being linearized is always a namespace"); + + parent_singleton = self + .graph + .declarations() + .get(&parent_singleton_id) + .unwrap() + .as_namespace() + .unwrap() + .singleton_class() + .copied(); + } + } + fn mixins_of(&self, definition_id: DefinitionId) -> Option> { let definition = self.graph.definitions().get(&definition_id).unwrap(); diff --git a/rust/rubydex/src/resolution_tests.rs b/rust/rubydex/src/resolution_tests.rs index 3eb618172..93f1dc46c 100644 --- a/rust/rubydex/src/resolution_tests.rs +++ b/rust/rubydex/src/resolution_tests.rs @@ -1185,6 +1185,292 @@ mod superclass_tests { Ancestors::Partial(_) )); } + + #[test] + fn singleton_class_are_automatically_created_for_descendants() { + // We create singleton classes lazily, only when they are required by some usage, like defining a `def + // self.foo`. However, when we create one of these synthetic singletons, we must also create the singleton class + // for all descendants. Otherwise, the structure of the graph is inconsistent + let mut context = graph_test(); + context.index_uri( + "file:///foo.rb", + " + class Bar + def self.foo; end + end + + class Qux < Bar; end + + class Foo < Qux + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Qux::"); + assert_declaration_exists!(context, "Foo::"); + + assert_declaration_does_not_exist!(context, "Bar::::<>"); + assert_declaration_does_not_exist!(context, "Qux::::<>"); + assert_declaration_does_not_exist!(context, "Foo::::<>"); + + assert_descendants!(context, "Bar::", ["Qux::", "Foo::"]); + assert_descendants!(context, "Qux::", ["Foo::"]); + + assert_ancestors_eq!( + context, + "Foo::", + [ + "Foo::", + "Qux::", + "Bar::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + } + + #[test] + fn second_level_singleton_class_are_automatically_created_for_descendants() { + let mut context = graph_test(); + context.index_uri( + "file:///foo.rb", + " + class Bar + class << self + def self.foo; end + end + end + + class Qux < Bar; end + + class Foo < Qux + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Qux::"); + assert_declaration_exists!(context, "Foo::"); + + assert_declaration_exists!(context, "Bar::::<>"); + assert_declaration_exists!(context, "Qux::::<>"); + assert_declaration_exists!(context, "Foo::::<>"); + + assert_descendants!(context, "Bar::", ["Qux::", "Foo::"]); + assert_descendants!(context, "Qux::", ["Foo::"]); + + assert_descendants!( + context, + "Bar::::<>", + ["Qux::::<>", "Foo::::<>"] + ); + assert_descendants!(context, "Qux::::<>", ["Foo::::<>"]); + + assert_ancestors_eq!( + context, + "Foo::::<>", + [ + "Foo::::<>", + "Qux::::<>", + "Bar::::<>", + "Object::::<>", + "BasicObject::::<>", + "Class::", + "Module::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + } + + #[test] + fn descendant_singleton_created_for_new_subclass_added_after_base_singleton_exists() { + // If a new class inherits from a parent that already has its singleton class created with linearized ancestors, + // we need to create its singleton class too + let mut context = graph_test(); + context.index_uri( + "file:///base.rb", + " + class Bar + def self.foo; end + end + class Qux < Bar; end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Qux::"); + + context.index_uri("file:///sub.rb", "class NewSub < Bar; end"); + context.resolve(); + + assert_declaration_exists!(context, "NewSub::"); + assert_descendants!(context, "Bar::", ["NewSub::"]); + } + + #[test] + fn descendant_singletons_created_when_singleton_appears_on_reopened_base() { + let mut context = graph_test(); + context.index_uri( + "file:///hierarchy.rb", + " + class Bar; end + class Qux < Bar; end + class Foo < Qux; end + ", + ); + context.resolve(); + + assert_declaration_does_not_exist!(context, "Bar::"); + assert_declaration_does_not_exist!(context, "Qux::"); + assert_declaration_does_not_exist!(context, "Foo::"); + + context.index_uri( + "file:///singleton.rb", + " + class Bar + def self.foo; end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Qux::"); + assert_declaration_exists!(context, "Foo::"); + + assert_descendants!(context, "Bar::", ["Qux::", "Foo::"]); + assert_descendants!(context, "Qux::", ["Foo::"]); + } + + #[test] + fn descendant_singletons_created_when_singleton_class_block_appears_on_base() { + let mut context = graph_test(); + context.index_uri( + "file:///hierarchy.rb", + " + class Bar; end + class Qux < Bar; end + class Foo < Qux; end + ", + ); + context.resolve(); + + assert_declaration_does_not_exist!(context, "Bar::"); + + context.index_uri( + "file:///singleton.rb", + " + class << Bar + def foo; end + end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Qux::"); + assert_declaration_exists!(context, "Foo::"); + + assert_descendants!(context, "Bar::", ["Qux::", "Foo::"]); + assert_descendants!(context, "Qux::", ["Foo::"]); + } + + #[test] + fn class_with_approximated_object_parent_gets_automatic_singleton() { + let mut context = graph_test(); + context.index_uri( + "file:///foo.rb", + " + class A; def self.x; end; end + class B < Nonexistent; end + ", + ); + context.resolve(); + + // We don't know B's parent, so we approximate it to `Object`. When `A` is being linearized, it triggers the + // creation of singleton classes for all descendants of `Object::`, which includes `B` as well + assert_declaration_exists!(context, "A::"); + assert_declaration_exists!(context, "B::"); + } + + #[test] + fn second_level_descendant_singleton_created_for_incrementally_added_subclass() { + let mut context = graph_test(); + context.index_uri( + "file:///base.rb", + " + class Bar + class << self + def self.foo; end + end + end + class Qux < Bar; end + ", + ); + context.resolve(); + + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Bar::::<>"); + assert_declaration_exists!(context, "Qux::"); + assert_declaration_exists!(context, "Qux::::<>"); + + context.index_uri("file:///sub.rb", "class NewSub < Bar; end"); + context.resolve(); + + assert_declaration_exists!(context, "NewSub::"); + assert_declaration_exists!(context, "NewSub::::<>"); + + assert_ancestors_eq!( + context, + "NewSub::", + [ + "NewSub::", + "Bar::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + + assert_ancestors_eq!( + context, + "NewSub::::<>", + [ + "NewSub::::<>", + "Bar::::<>", + "Object::::<>", + "BasicObject::::<>", + "Class::", + "Module::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + } } mod include_tests { diff --git a/test/integration/mcp_server_test.rb b/test/integration/mcp_server_test.rb index a15e21dd1..65e891fa9 100644 --- a/test/integration/mcp_server_test.rb +++ b/test/integration/mcp_server_test.rb @@ -92,7 +92,7 @@ def test_mcp_server_e2e assert_operator(stats.fetch("declarations"), :>, 0) search_response = call_tool(stdin, stdout, request_id + 1, "search_declarations", { query: "Dog", match_mode: "exact" }) - assert_equal(["Dog"], search_response.fetch("results").map { |result| result.fetch("name") }) + assert_equal(["Dog", "Dog::"], search_response.fetch("results").map { |result| result.fetch("name") }) stdin.close Timeout.timeout(30) { wait_thr.value } diff --git a/test/mcp_server_tools_test.rb b/test/mcp_server_tools_test.rb index 739c23200..694dfc36c 100644 --- a/test/mcp_server_tools_test.rb +++ b/test/mcp_server_tools_test.rb @@ -17,7 +17,7 @@ def test_search_declarations_tool paginated = call_tool(graph, Rubydex::MCPServer::SearchDeclarationsTool, query: "Dog", match_mode: "exact", limit: 1) - assert_equal(3, paginated.fetch("total")) + assert_equal(4, paginated.fetch("total")) assert_equal(1, paginated.fetch("results").length) invalid = call_tool(graph, Rubydex::MCPServer::SearchDeclarationsTool, query: "Dog", match_mode: "contains")