Automatically promote constants to namespaces

Author: vinistock

rust/rubydex/src/indexing/ruby_indexer.rs

@@ -571,7 +571,24 @@ impl<'a> RubyIndexer<'a> {
         definition_id
     }
 
-    fn add_constant_definition(&mut self, node: &ruby_prism::Node, also_add_reference: bool) -> Option<DefinitionId> {
+    /// Returns whether a value node represents a method call that could produce a class or module.
+    /// Only regular method calls (bare calls or dot/safe-nav calls) are considered promotable.
+    /// Operator calls like `1 + 2` are `CallNode`s in Prism but should not be promotable.
+    fn is_promotable_value(value: &ruby_prism::Node) -> bool {
+        value.as_call_node().is_some_and(|call| {
+            // Bare calls (no receiver): `some_factory_call`
+            // Dot/safe-nav/scope calls: `Struct.new(...)`, `foo&.bar`, `Struct::new`
+            // Excluded: operator calls like `1 + 2` which have a receiver but no call operator
+            call.receiver().is_none() || call.call_operator_loc().is_some()
+        })
+    }
+
+    fn add_constant_definition(
+        &mut self,
+        node: &ruby_prism::Node,
+        also_add_reference: bool,
+        promotable: bool,
+    ) -> Option<DefinitionId> {
         let name_id = self.index_constant_reference(node, also_add_reference)?;
 
         // Get the location for the constant name/path only (not including the value)
@@ -583,7 +600,10 @@ impl<'a> RubyIndexer<'a> {
         };
 
         let offset = Offset::from_prism_location(&location);
-        let (comments, flags) = self.find_comments_for(offset.start());
+        let (comments, mut flags) = self.find_comments_for(offset.start());
+        if promotable {
+            flags |= DefinitionFlags::PROMOTABLE;
+        }
         let lexical_nesting_id = self.parent_lexical_scope_id();
 
         let definition = Definition::Constant(Box::new(ConstantDefinition::new(
@@ -1297,7 +1317,7 @@ impl Visit<'_> for RubyIndexer<'_> {
         if let Some(target_name_id) = self.index_constant_alias_target(&node.value()) {
             self.add_constant_alias_definition(&node.as_node(), target_name_id, true);
         } else {
-            self.add_constant_definition(&node.as_node(), true);
+            self.add_constant_definition(&node.as_node(), true, Self::is_promotable_value(&node.value()));
             self.visit(&node.value());
         }
     }
@@ -1311,7 +1331,7 @@ impl Visit<'_> for RubyIndexer<'_> {
         if let Some(target_name_id) = self.index_constant_alias_target(&value) {
             self.add_constant_alias_definition(&node.as_node(), target_name_id, false);
         } else {
-            self.add_constant_definition(&node.as_node(), false);
+            self.add_constant_definition(&node.as_node(), false, Self::is_promotable_value(&value));
             self.visit(&value);
         }
     }
@@ -1330,7 +1350,7 @@ impl Visit<'_> for RubyIndexer<'_> {
         if let Some(target_name_id) = self.index_constant_alias_target(&node.value()) {
             self.add_constant_alias_definition(&node.target().as_node(), target_name_id, true);
         } else {
-            self.add_constant_definition(&node.target().as_node(), true);
+            self.add_constant_definition(&node.target().as_node(), true, Self::is_promotable_value(&node.value()));
             self.visit(&node.value());
         }
     }
@@ -1344,7 +1364,7 @@ impl Visit<'_> for RubyIndexer<'_> {
         if let Some(target_name_id) = self.index_constant_alias_target(&value) {
             self.add_constant_alias_definition(&node.target().as_node(), target_name_id, false);
         } else {
-            self.add_constant_definition(&node.target().as_node(), false);
+            self.add_constant_definition(&node.target().as_node(), false, Self::is_promotable_value(&value));
             self.visit(&value);
         }
     }
@@ -1361,7 +1381,10 @@ impl Visit<'_> for RubyIndexer<'_> {
         for left in &node.lefts() {
             match left {
                 ruby_prism::Node::ConstantTargetNode { .. } | ruby_prism::Node::ConstantPathTargetNode { .. } => {
-                    self.add_constant_definition(&left, false);
+                    // Individual values aren't available in multi-write, so we default to
+                    // promotable because multi-assignment often comes from meta-programming
+                    // (e.g., `A, B = create_classes`).
+                    self.add_constant_definition(&left, false, true);
                 }
                 ruby_prism::Node::GlobalVariableTargetNode { .. } => {
                     self.add_definition_from_location(
@@ -5887,4 +5910,52 @@ mod tests {
         // We expect two constant references for `Foo` and `<Foo>` on each singleton method call
         assert_eq!(6, context.graph().constant_references().len());
     }
+
+    #[test]
+    fn constant_with_call_value_is_promotable() {
+        let context = index_source("Foo = some_call");
+
+        assert_definition_at!(&context, "1:1-1:4", Constant, |def| {
+            assert!(
+                def.flags().is_promotable(),
+                "Expected PROMOTABLE flag to be set for call value"
+            );
+        });
+    }
+
+    #[test]
+    fn constant_with_literal_value_is_not_promotable() {
+        let context = index_source("FOO = 42");
+
+        assert_definition_at!(&context, "1:1-1:4", Constant, |def| {
+            assert!(
+                !def.flags().is_promotable(),
+                "Expected PROMOTABLE flag to NOT be set for literal value"
+            );
+        });
+    }
+
+    #[test]
+    fn constant_with_operator_call_is_not_promotable() {
+        let context = index_source("FOO = 1 + 2");
+
+        assert_definition_at!(&context, "1:1-1:4", Constant, |def| {
+            assert!(
+                !def.flags().is_promotable(),
+                "Expected PROMOTABLE flag to NOT be set for operator call"
+            );
+        });
+    }
+
+    #[test]
+    fn constant_with_dot_call_is_promotable() {
+        let context = index_source("Foo = Bar.new");
+
+        assert_definition_at!(&context, "1:1-1:4", Constant, |def| {
+            assert!(
+                def.flags().is_promotable(),
+                "Expected PROMOTABLE flag to be set for dot call"
+            );
+        });
+    }
 }

rust/rubydex/src/model/declaration.rs

@@ -125,11 +125,14 @@ macro_rules! namespace_declaration {
                 }
             }
 
-            pub fn extend(&mut self, mut other: Namespace) {
+            pub fn extend(&mut self, mut other: Declaration) {
                 self.definition_ids.extend(other.definitions());
                 self.references.extend(other.references());
-                self.members.extend(other.members());
                 self.diagnostics.extend(other.take_diagnostics());
+
+                if let Declaration::Namespace(namespace) = other {
+                    self.members.extend(namespace.members());
+                }
             }
 
             pub fn set_singleton_class_id(&mut self, declaration_id: DeclarationId) {
@@ -412,6 +415,10 @@ impl Namespace {
         all_namespaces!(self, it => std::mem::take(&mut it.diagnostics))
     }
 
+    pub fn extend(&mut self, other: Declaration) {
+        all_namespaces!(self, it => it.extend(other));
+    }
+
     #[must_use]
     pub fn ancestors(&self) -> &Ancestors {
         all_namespaces!(self, it => it.ancestors())
@@ -484,6 +491,11 @@ impl Namespace {
     pub fn owner_id(&self) -> &DeclarationId {
         all_namespaces!(self, it => &it.owner_id)
     }
+
+    #[must_use]
+    pub fn name(&self) -> &str {
+        all_namespaces!(self, it => &it.name)
+    }
 }
 
 namespace_declaration!(Class, ClassDeclaration);

rust/rubydex/src/model/definitions.rs

@@ -39,6 +39,7 @@ bitflags! {
     #[derive(Debug, Clone)]
     pub struct DefinitionFlags: u8 {
         const DEPRECATED = 0b0001;
+        const PROMOTABLE = 0b0010;
     }
 }
 
@@ -47,6 +48,11 @@ impl DefinitionFlags {
     pub fn is_deprecated(&self) -> bool {
         self.contains(Self::DEPRECATED)
     }
+
+    #[must_use]
+    pub fn is_promotable(&self) -> bool {
+        self.contains(Self::PROMOTABLE)
+    }
 }
 
 #[derive(Debug)]

rust/rubydex/src/model/graph.rs

@@ -69,6 +69,9 @@ impl Graph {
         &mut self.declarations
     }
 
+    /// # Panics
+    ///
+    /// Will panic if the `definition_id` is not registered in the graph
     pub fn add_declaration<F>(
         &mut self,
         definition_id: DefinitionId,
@@ -80,24 +83,82 @@ impl Graph {
     {
         let declaration_id = DeclarationId::from(&fully_qualified_name);
 
+        let should_promote = self.declarations.get(&declaration_id).is_some_and(|existing| {
+            matches!(existing, Declaration::Constant(_))
+                && matches!(
+                    self.definitions.get(&definition_id),
+                    Some(Definition::Class(_) | Definition::Module(_) | Definition::SingletonClass(_))
+                )
+                && self.all_definitions_promotable(existing)
+        });
+
         match self.declarations.entry(declaration_id) {
-            Entry::Vacant(vacant_entry) => {
-                vacant_entry
-                    .insert(constructor(fully_qualified_name))
-                    .add_definition(definition_id);
-            }
             Entry::Occupied(mut occupied_entry) => {
                 debug_assert!(
                     occupied_entry.get().name() == fully_qualified_name,
                     "DeclarationId collision in global graph"
                 );
-                occupied_entry.get_mut().add_definition(definition_id);
+
+                if should_promote {
+                    let mut new_declaration = constructor(fully_qualified_name);
+                    let removed_declaration = occupied_entry.remove();
+                    new_declaration.as_namespace_mut().unwrap().extend(removed_declaration);
+                    new_declaration.add_definition(definition_id);
+                    self.declarations.insert(declaration_id, new_declaration);
+                } else {
+                    occupied_entry.get_mut().add_definition(definition_id);
+                }
+            }
+            Entry::Vacant(vacant_entry) => {
+                let mut declaration = constructor(fully_qualified_name);
+                declaration.add_definition(definition_id);
+                vacant_entry.insert(declaration);
             }
         }
 
         declaration_id
     }
 
+    /// Checks if all constant definitions for a declaration have the PROMOTABLE flag set.
+    /// Used to determine whether a constant can be promoted to a namespace.
+    #[must_use]
+    pub fn all_definitions_promotable(&self, declaration: &Declaration) -> bool {
+        declaration
+            .definitions()
+            .iter()
+            .all(|def_id| match self.definitions.get(def_id) {
+                Some(Definition::Constant(c)) => c.flags().is_promotable(),
+                _ => true,
+            })
+    }
+
+    /// Promotes a `Declaration::Constant` to a namespace using the provided constructor. Transfers all definitions,
+    /// references, and diagnostics from the old declaration.
+    ///
+    /// # Panics
+    ///
+    /// Will panic if the declaration ID doesn't exist
+    pub fn promote_constant_to_namespace<F>(&mut self, declaration_id: DeclarationId, constructor: F)
+    where
+        F: FnOnce(String, DeclarationId) -> Declaration,
+    {
+        let old_decl = self.declarations.remove(&declaration_id).unwrap();
+        let name = old_decl.name().to_string();
+        let owner_id = *old_decl.owner_id();
+
+        let mut new_decl = constructor(name, owner_id);
+        new_decl.as_namespace_mut().unwrap().extend(old_decl);
+
+        self.declarations.insert(declaration_id, new_decl);
+    }
+
+    #[must_use]
+    pub fn is_namespace(&self, declaration_id: &DeclarationId) -> bool {
+        self.declarations
+            .get(declaration_id)
+            .is_some_and(|decl| decl.as_namespace().is_some())
+    }
+
     pub fn clear_declarations(&mut self) {
         self.declarations.clear();
     }
@@ -502,9 +563,6 @@ impl Graph {
                 Declaration::Namespace(Namespace::SingletonClass(it)) => {
                     it.add_member(member_str_id, member_declaration_id);
                 }
-                Declaration::Constant(_) => {
-                    // TODO: temporary hack to avoid crashing on `Struct.new`, `Class.new` and `Module.new`
-                }
                 _ => panic!("Tried to add member to a declaration that isn't a namespace"),
             }
         }