diff --git a/rust/rubydex-mcp/src/server.rs b/rust/rubydex-mcp/src/server.rs index 468a17327..89531c594 100644 --- a/rust/rubydex-mcp/src/server.rs +++ b/rust/rubydex-mcp/src/server.rs @@ -1119,12 +1119,15 @@ mod tests { let res = parse(&s.codebase_stats()); assert_eq!(res["files"], 3); - assert_json_int!(res, "declarations", 7); + // 7 declarations (Animal, Greetable, and 5 built-ins) plus a materialized rank-1 singleton + // class for each of them. Definitions do not include singletons. + assert_json_int!(res, "declarations", 14); assert_json_int!(res, "definitions", 7); let breakdown = &res["breakdown_by_kind"]; assert_json_int!(breakdown, "Class", 5); assert_json_int!(breakdown, "Module", 2); + assert_json_int!(breakdown, "SingletonClass", 7); } // -- error states -- diff --git a/rust/rubydex/src/query.rs b/rust/rubydex/src/query.rs index 83b7d5fc3..b5b5ab467 100644 --- a/rust/rubydex/src/query.rs +++ b/rust/rubydex/src/query.rs @@ -43,7 +43,10 @@ pub fn declaration_search(graph: &Graph, queries: &[&str], match_mode: &MatchMod // directly. Since an empty query matches all, there's no point in checking the other queries or pay the price of // spawning threads. if queries.iter().any(|q| q.is_empty()) { - return declarations.keys().copied().collect(); + return declarations + .iter() + .filter_map(|(id, declaration)| is_searchable_declaration(declaration).then_some(*id)) + .collect(); } let ids: Vec = declarations.keys().copied().collect(); @@ -61,7 +64,12 @@ pub fn declaration_search(graph: &Graph, queries: &[&str], match_mode: &MatchMod chunk .iter() .filter(|id| { - let name = declarations.get(id).unwrap().name(); + let declaration = declarations.get(id).unwrap(); + if !is_searchable_declaration(declaration) { + return false; + } + + let name = declaration.name(); queries.iter().any(|query| matches_query(query, name, match_mode)) }) .copied() @@ -74,6 +82,12 @@ pub fn declaration_search(graph: &Graph, queries: &[&str], match_mode: &MatchMod }) } +/// Returns whether a declaration should be surfaced by user-facing declaration search. +#[must_use] +fn is_searchable_declaration(declaration: &Declaration) -> bool { + !matches!(declaration, Declaration::Namespace(Namespace::SingletonClass(_))) +} + /// Returns whether a single `query` matches `name` under the given [`MatchMode`]. #[must_use] fn matches_query(query: &str, name: &str, match_mode: &MatchMode) -> bool { @@ -931,6 +945,31 @@ mod tests { assert_results_eq!(context, "Fo", ["Foo"]); } + #[test] + fn search_excludes_singleton_classes() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + end + " + }); + context.resolve(); + + // Every declared class has its singleton class automatically populated... + assert!( + context + .graph() + .declarations() + .contains_key(&DeclarationId::from("Foo::")), + "expected `Foo::` to be materialized" + ); + // ...but it is internal and must not surface in search results, even when the query + // matches the singleton's name directly. + assert_results_eq!(context, "", &MatchMode::Exact, Vec::<&str>::new()); + assert_results_eq!(context, "Foo", ["Foo"]); + } + #[test] fn exact_partial_match_search() { let mut context = GraphTest::new(); @@ -962,8 +1001,17 @@ mod tests { let exact_results = declaration_search(context.graph(), &[""], &MatchMode::Exact); let fuzzy_results = declaration_search(context.graph(), &[""], &MatchMode::Fuzzy); + // An empty query returns every searchable declaration. Materialized singleton classes are + // excluded from search, so the expected count is all declarations minus the singletons. + let searchable = context + .graph() + .declarations() + .values() + .filter(|d| !matches!(d, Declaration::Namespace(Namespace::SingletonClass(_)))) + .count(); + assert_eq!(exact_results.len(), fuzzy_results.len()); - assert_eq!(context.graph().declarations().len(), exact_results.len()); + assert_eq!(searchable, exact_results.len()); } #[test] diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index 99f7e11f3..f690e4250 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -1,4 +1,4 @@ -use std::collections::{HashSet, VecDeque, hash_map::Entry}; +use std::collections::{HashMap, HashSet, VecDeque, hash_map::Entry}; use crate::diagnostic::{Diagnostic, Rule}; use crate::model::{ @@ -145,11 +145,187 @@ impl<'a> Resolver<'a> { self.handle_remaining_definitions(other_ids); + // Materialize singleton classes for every class/module declaration. This runs after the + // main singleton creation paths (the convergence loop and `handle_remaining_definitions`) + // so it only fills gaps in existing singleton-class ancestor/descendant chains. + self.materialize_singleton_classes(); + // Descendants are derived from the finalized ancestor chains, so they must be updated after // all linearization has settled. self.update_descendants(); } + /// Materializes singleton classes needed for complete ancestor/descendant chains. + /// + /// During the convergence loop and [`Self::handle_remaining_definitions`], singleton classes are + /// only created on demand: when a self-method, an `extend`, a `class << self`, or a class-level + /// instance variable forces one into existence. That lazy behavior is correct for Ruby, but it + /// leaves holes in graph queries that expect singleton-class ancestors and descendants to be + /// explicit declarations. + /// + /// For example, if source creates `Foo::` and `Bar < Foo`, then `Bar::` must also + /// exist so `Foo::` can be recorded as an ancestor of `Bar::`. The same rule applies + /// to nested singleton classes: + /// + /// ```rb + /// class Foo + /// class << self # Foo::, the singleton class of Foo + /// class << self # Foo::::<>, the singleton class of Foo:: + /// end + /// end + /// end + /// + /// class Bar < Foo + /// end + /// ``` + /// + /// Because the source explicitly opens the singleton class of `Foo::`, this pass also + /// creates `Bar::::<>`, the corresponding singleton class of `Bar::`. + /// + /// The work is intentionally bounded. The first stage creates the singleton class of every class + /// and module. The second stage creates deeper singleton classes only when there is a + /// source-backed singleton class at that depth, then descends from that owner through the class + /// inheritance tree. It does not keep creating singleton classes for every possible singleton of + /// every class. + fn materialize_singleton_classes(&mut self) { + // Scan all declarations once to collect every input both stages need: + // + // - `attached_ids`: every class and module whose singleton class should exist. Ids are + // collected up front because `get_or_create_singleton_class` borrows the graph mutably. + // - `class_parents`: each class paired with its resolved direct superclass. We resolve it here + // while the declaration is in hand; Stage 2 translates these edges to singleton ids (which + // do not exist until Stage 1 runs) to seed the `children` map. + // - `seed_owners_by_depth`: the owners of every source-backed nested singleton class, grouped + // by the singleton depth of that owner. `Foo::::<>` is source-backed when it has + // its own definition (a nested `class << self`) or members (for example, a `def self.x` + // written inside `class << self`). Its owner, `Foo::`, seeds materialization of the + // corresponding singleton class for each descendant of `Foo::`. Singletons with no + // definition or members are pure linearization artifacts: linearizing an explicit singleton + // creates the singletons of its ancestors (for example, `Object::::<>`), and + // seeding on those would descend the whole class tree. + // + // The seed set is fixed before this pass: source-backed nested singletons are created + // organically during resolution, and Stage 1 only adds the first singleton layer, so it never + // adds or removes a seed. That is why everything can be collected in this single scan. + let mut attached_ids: Vec = Vec::new(); + let mut class_parents: Vec<(DeclarationId, DeclarationId)> = Vec::new(); + let mut seed_owners_by_depth: HashMap> = HashMap::new(); + let mut max_singleton_depth = 1; + for (declaration_id, declaration) in self.graph.declarations() { + match declaration.as_namespace() { + Some(Namespace::Module(_)) => attached_ids.push(*declaration_id), + Some(namespace @ Namespace::Class(_)) => { + attached_ids.push(*declaration_id); + // Only classes participate in the inheritance tree Stage 2 descends; a module's + // singleton parent is always `Module`, and modules cannot be subclassed. + let (parent_class, _) = self.get_parent_class(namespace.definitions()); + class_parents.push((*declaration_id, parent_class)); + } + Some(namespace @ Namespace::SingletonClass(_)) + if !namespace.definitions().is_empty() || !namespace.members().is_empty() => + { + let depth = self.singleton_depth(*declaration_id); + if depth >= 2 { + seed_owners_by_depth + .entry(depth - 1) + .or_default() + .insert(*namespace.owner_id()); + max_singleton_depth = max_singleton_depth.max(depth); + } + } + _ => {} + } + } + + // --- Stage 1: singleton class for every class and module --- + for attached_id in &attached_ids { + // Idempotent: returns the existing singleton when one was already created on demand. + let _ = self.get_or_create_singleton_class(*attached_id, SingletonAncestors::Eager); + } + + // --- Stage 2: deeper singleton classes for descendants of source-backed singletons --- + // + // `children` maps a singleton to the singletons of the *direct* subclasses of its attached + // object: for `class B < A`, `A::`'s children contain `B::`. It starts as the first + // layer (translating the `class_parents` edges collected above, now that Stage 1 has + // materialized every class singleton) and grows one singleton layer at a time: building + // `B::::<>` records it as a child of `A::::<>` for the next iteration. This is a + // local map; the global `descendants` relation is updated later by [`Self::update_descendants`]. + let mut children: IdentityHashMap> = IdentityHashMap::default(); + for (child_class, parent_class) in &class_parents { + if let (Some(child_singleton), Some(parent_singleton)) = + (self.singleton_id_of(*child_class), self.singleton_id_of(*parent_class)) + { + children.entry(parent_singleton).or_default().insert(child_singleton); + } + } + + // Climb one singleton layer at a time. At owner depth `D`, descend the inheritance tree from + // each seed owner, materialize the singleton class of every node reached, and record those new + // child edges so depth `D + 1` can use them. Nested singleton definitions are contiguous: to + // create the singleton class of `Foo::`, the graph must already contain `Foo::`. + for owner_depth in 1..max_singleton_depth { + let Some(seed_owners) = seed_owners_by_depth.get(&owner_depth) else { + continue; + }; + + let mut stack: Vec = seed_owners.iter().copied().collect(); + let mut visited: IdentityHashSet = IdentityHashSet::default(); + let mut next_edges: Vec<(DeclarationId, DeclarationId)> = Vec::new(); + + while let Some(parent) = stack.pop() { + if !visited.insert(parent) { + continue; + } + + let Some(parent_next) = self.get_or_create_singleton_class(parent, SingletonAncestors::Eager) else { + continue; + }; + + let Some(child_singletons) = children.get(&parent).map(|set| set.iter().copied().collect::>()) + else { + continue; + }; + + for child in child_singletons { + if let Some(child_next) = self.get_or_create_singleton_class(child, SingletonAncestors::Eager) { + next_edges.push((parent_next, child_next)); + } + stack.push(child); + } + } + + for (parent_next, child_next) in next_edges { + children.entry(parent_next).or_default().insert(child_next); + } + } + } + + /// Returns how many singleton-class owners separate this declaration from its attached class or + /// module. A class declaration has depth `0`, `Foo::` has depth `1`, and the singleton class + /// of `Foo::` has depth `2`. Computed by walking the owner chain so it does not depend on + /// how the name is spelled. + fn singleton_depth(&self, mut id: DeclarationId) -> usize { + let mut depth = 0; + while let Some(declaration) = self.graph.declarations().get(&id) { + let Some(namespace @ Namespace::SingletonClass(_)) = declaration.as_namespace() else { + break; + }; + depth += 1; + id = *namespace.owner_id(); + } + depth + } + + /// Returns the id of the singleton class of the given declaration, if one has been materialized. + fn singleton_id_of(&self, attached_id: DeclarationId) -> Option { + self.graph + .declarations() + .get(&attached_id) + .and_then(Declaration::as_namespace) + .and_then(|namespace| namespace.singleton_class().copied()) + } + /// Updates the `descendants` relation to match the finalized ancestor chains. /// /// `descendants(D)` must contain exactly the declarations `x` such that `Ancestor::Complete(D)` diff --git a/rust/rubydex/src/resolution_tests.rs b/rust/rubydex/src/resolution_tests.rs index f320eea4a..2285cf39b 100644 --- a/rust/rubydex/src/resolution_tests.rs +++ b/rust/rubydex/src/resolution_tests.rs @@ -2205,6 +2205,150 @@ mod singleton_ancestors_tests { ); } + #[test] + fn materializes_rank1_singletons_for_all_namespaces() { + let mut context = graph_test(); + context.index_uri("file:///foo.rb", { + r" + class Foo; end + class Bar < Foo; end + module Baz; end + " + }); + context.resolve(); + + assert_no_diagnostics!(&context); + + // Rank-1 singletons exist even without self-methods, extends, or `class << self`. + assert_declaration_exists!(context, "Foo::"); + assert_declaration_exists!(context, "Bar::"); + assert_declaration_exists!(context, "Baz::"); + // Built-in classes are materialized too. + assert_declaration_exists!(context, "Object::"); + + // The singleton ancestor chain reflects the class hierarchy (#Bar < #Foo). + assert_ancestors_eq!( + context, + "Bar::", + [ + "Bar::", + "Foo::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + + // Descendants are the inverse of the singleton ancestor chains and are reflexive. + assert_descendants!(context, "Foo::", ["Foo::", "Bar::"]); + assert_descendants!(context, "Object::", ["Foo::", "Bar::"]); + } + + #[test] + fn materializes_rank_n_singletons_for_descendants_of_existing_singletons() { + let mut context = graph_test(); + context.index_uri("file:///foo.rb", { + r" + class A + class << self + class << self + end + end + end + + class B < A; end + " + }); + context.resolve(); + + assert_no_diagnostics!(&context); + + // The nested `class << self` materializes a rank-2 singleton for `A`. + assert_declaration_exists!(context, "A::::<>"); + + // `B` has no rank-2 trigger of its own, but because it is a descendant of `A`, its rank-2 + // singleton must be materialized so the rank-2 hierarchy under `A::::<>` is complete. + assert_declaration_exists!(context, "B::::<>"); + + // The rank-2 ancestor chain follows the superclass chain of the rank-1 singletons. + assert_ancestors_eq!( + context, + "B::::<>", + [ + "B::::<>", + "A::::<>", + "Object::::<>", + "BasicObject::::<>", + "Class::", + "Module::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + + // Descendants are the inverse of the rank-2 ancestor chains and are reflexive. + assert_descendants!(context, "A::::<>", ["A::::<>", "B::::<>"]); + } + + #[test] + fn materializes_rank_n_singletons_when_explicit_singleton_only_has_members() { + let mut context = graph_test(); + context.index_uri("file:///foo.rb", { + r" + class A + class << self + # `def self.bar` inside `class << self` attaches `bar` to the rank-2 singleton, + # creating `A::::<>` on demand. It has no definition of its own, only a member. + def self.bar; end + end + end + + class B < A; end + " + }); + context.resolve(); + + assert_no_diagnostics!(&context); + + // The rank-2 singleton exists because of `def self.bar`, even without a nested `class << self`. + assert_declaration_exists!(context, "A::::<>"); + assert_declaration_exists!(context, "A::::<>#bar()"); + + // A member-only rank-2 singleton must still seed materialization for descendants. + assert_declaration_exists!(context, "B::::<>"); + + assert_ancestors_eq!( + context, + "B::::<>", + [ + "B::::<>", + "A::::<>", + "Object::::<>", + "BasicObject::::<>", + "Class::", + "Module::", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject" + ] + ); + + assert_descendants!(context, "A::::<>", ["A::::<>", "B::::<>"]); + } + #[test] fn singleton_ancestors_for_modules() { let mut context = graph_test(); diff --git a/rust/rubydex/tests/cli.rs b/rust/rubydex/tests/cli.rs index 94d12ae8e..48dd7ea13 100644 --- a/rust/rubydex/tests/cli.rs +++ b/rust/rubydex/tests/cli.rs @@ -128,7 +128,9 @@ fn prints_index_metrics() { .success() .stderr(predicate::str::is_empty()) .stdout(predicate::str::contains("Indexed 3 files")) - .stdout(predicate::str::contains("Found 7 names")) + // 7 declarations (FirstClass, SecondModule, and 5 built-ins) plus a materialized + // rank-1 singleton class for each of them. Definitions do not include singletons. + .stdout(predicate::str::contains("Found 14 names")) .stdout(predicate::str::contains("Found 7 definitions")); }); } diff --git a/test/declaration_test.rb b/test/declaration_test.rb index 704f929d8..e87347a5e 100644 --- a/test/declaration_test.rb +++ b/test/declaration_test.rb @@ -124,7 +124,7 @@ def something; end graph.index_all(context.glob("**/*.rb")) graph.resolve - assert_nil(graph["Foo"].singleton_class) + assert_equal("Foo::", graph["Foo"].singleton_class.name) bar = graph["Foo::Bar"] assert_equal("Foo::Bar::", bar.singleton_class.name) diff --git a/test/graph_test.rb b/test/graph_test.rb index c0879eda1..2562a0ad3 100644 --- a/test/graph_test.rb +++ b/test/graph_test.rb @@ -179,10 +179,10 @@ def test_list_all_declarations_enumerator enumerator = graph.declarations - # Object, Class, Module, BasicObject, Kernel + the indexed files - assert_equal(7, enumerator.size) - assert_equal(7, enumerator.count) - assert_equal(7, enumerator.to_a.size) + # Object, Class, Module, BasicObject, Kernel + the indexed files, plus their singleton classes. + assert_equal(14, enumerator.size) + assert_equal(14, enumerator.count) + assert_equal(14, enumerator.to_a.size) end end @@ -200,7 +200,7 @@ def test_list_all_declarations_with_block declarations << declaration end - assert_equal(7, declarations.size) + assert_equal(14, declarations.size) end end