From 19579d25b48373493786b44bc614d6827ec73c3a Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 5 Feb 2026 22:39:52 +0000 Subject: [PATCH] Introduce Receiver enum to distinguish self vs constant method receivers Replace `Option` on `MethodDefinition.receiver` with `Option` where `Receiver` is either `SelfReceiver(DefinitionId)` or `ConstantReceiver(NameId)`. For `def self.foo`, the indexer now stores the enclosing definition's `DefinitionId` directly instead of eagerly extracting its `NameId` via `current_owner_name_id()`. During resolution, `SelfReceiver` uses a direct `definition_id_to_declaration_id` lookup (single hashmap hit) rather than going through `resolve_lexical_owner`'s recursion and namespace checks. This is safe because `SelfReceiver` always points at the enclosing class/module by construction. `ConstantReceiver(NameId)` preserves the existing behavior for `def Foo.bar`, which still requires name resolution and can fail if the constant is undefined. --- rust/rubydex/src/indexing/ruby_indexer.rs | 79 +++++++++++++++-------- rust/rubydex/src/model/definitions.rs | 27 ++++++-- rust/rubydex/src/resolution.rs | 70 +++++++++++--------- 3 files changed, 111 insertions(+), 65 deletions(-) diff --git a/rust/rubydex/src/indexing/ruby_indexer.rs b/rust/rubydex/src/indexing/ruby_indexer.rs index 503d82e9..4bfe64e5 100644 --- a/rust/rubydex/src/indexing/ruby_indexer.rs +++ b/rust/rubydex/src/indexing/ruby_indexer.rs @@ -8,7 +8,7 @@ use crate::model::definitions::{ ConstantAliasDefinition, ConstantDefinition, Definition, DefinitionFlags, ExtendDefinition, GlobalVariableAliasDefinition, GlobalVariableDefinition, IncludeDefinition, InstanceVariableDefinition, MethodAliasDefinition, MethodDefinition, Mixin, ModuleDefinition, Parameter, ParameterStruct, PrependDefinition, - SingletonClassDefinition, + Receiver, SingletonClassDefinition, }; use crate::model::document::Document; use crate::model::ids::{DefinitionId, NameId, StringId, UriId}; @@ -1042,9 +1042,17 @@ impl<'a> RubyIndexer<'a> { unreachable!("method definition for nesting should exist") }; - if let Some(method_def_receiver) = definition.receiver() { + if let Some(receiver) = definition.receiver() { is_singleton_name = true; - Some(*method_def_receiver) + match receiver { + Receiver::SelfReceiver(def_id) => self + .local_graph + .definitions() + .get(def_id) + .and_then(Definition::name_id) + .copied(), + Receiver::ConstantReceiver(name_id) => Some(*name_id), + } } else { self.current_owner_name_id() } @@ -1416,12 +1424,12 @@ impl Visit<'_> for RubyIndexer<'_> { let receiver = if let Some(recv_node) = node.receiver() { match recv_node { - // def self.foo - receiver is the current owner's NameId (includes Class.new/Module.new) - ruby_prism::Node::SelfNode { .. } => self.current_owner_name_id(), + // def self.foo - receiver is the enclosing definition's DefinitionId + ruby_prism::Node::SelfNode { .. } => self.current_nesting_definition_id().map(Receiver::SelfReceiver), // def Foo.bar or def Foo::Bar.baz - receiver is the constant's NameId - ruby_prism::Node::ConstantPathNode { .. } | ruby_prism::Node::ConstantReadNode { .. } => { - self.index_constant_reference(&recv_node, true) - } + ruby_prism::Node::ConstantPathNode { .. } | ruby_prism::Node::ConstantReadNode { .. } => self + .index_constant_reference(&recv_node, true) + .map(Receiver::ConstantReceiver), // Dynamic receiver (def foo.bar) - visit and then skip // We still want to visit because it could be a variable reference _ => { @@ -1451,7 +1459,7 @@ impl Visit<'_> for RubyIndexer<'_> { parent_nesting_id, parameters.clone(), Visibility::Public, - self.current_owner_name_id(), + self.current_nesting_definition_id().map(Receiver::SelfReceiver), ))); let definition_id = self.local_graph.add_definition(method); @@ -2007,7 +2015,7 @@ mod tests { assert_def_name_eq, assert_def_name_offset_eq, assert_def_str_eq, assert_definition_at, assert_local_diagnostics_eq, assert_name_path_eq, assert_no_local_diagnostics, assert_string_eq, model::{ - definitions::{Definition, Mixin, Parameter}, + definitions::{Definition, Mixin, Parameter, Receiver}, ids::{StringId, UriId}, visibility::Visibility, }, @@ -2114,20 +2122,27 @@ mod tests { /// - `assert_method_has_receiver!(ctx, method, "")` macro_rules! assert_method_has_receiver { ($context:expr, $method:expr, $expected_receiver:expr) => {{ - if let Some(receiver_name_id) = $method.receiver() { - let name = $context.graph().names().get(receiver_name_id).unwrap(); - let actual_name = $context.graph().strings().get(name.str()).unwrap().as_str(); - assert_eq!( - $expected_receiver, actual_name, - "method receiver mismatch: expected `{}`, got `{}`", - $expected_receiver, actual_name - ); - } else { - panic!( - "Method receiver mismatch: expected `{}`, got `None`", - $expected_receiver - ); - } + let name_id = match $method.receiver() { + Some(Receiver::SelfReceiver(def_id)) => { + let def = $context.graph().definitions().get(def_id).unwrap(); + *def.name_id().expect("SelfReceiver definition should have a name_id") + } + Some(Receiver::ConstantReceiver(name_id)) => *name_id, + None => { + panic!( + "Method receiver mismatch: expected `{}`, got `None`", + $expected_receiver + ); + } + }; + + let name = $context.graph().names().get(&name_id).unwrap(); + let actual_name = $context.graph().strings().get(name.str()).unwrap().as_str(); + assert_eq!( + $expected_receiver, actual_name, + "method receiver mismatch: expected `{}`, got `{}`", + $expected_receiver, actual_name + ); }}; } @@ -5429,8 +5444,12 @@ mod tests { assert_no_local_diagnostics!(&context); assert_definition_at!(&context, "3:5-4:8", Method, |bar| { - let receiver = bar.receiver().unwrap(); - let name_ref = context.graph().names().get(&receiver).unwrap(); + let Receiver::SelfReceiver(def_id) = bar.receiver().as_ref().unwrap() else { + panic!("Expected SelfReceiver for def self.bar in Class.new"); + }; + let def = context.graph().definitions().get(def_id).unwrap(); + let name_id = def.name_id().expect("Owner definition should have a name_id"); + let name_ref = context.graph().names().get(name_id).unwrap(); assert_eq!(StringId::from("A"), *name_ref.str()); let nesting_name = context.graph().names().get(&name_ref.nesting().unwrap()).unwrap(); @@ -5475,8 +5494,12 @@ mod tests { assert_no_local_diagnostics!(&context); assert_definition_at!(&context, "3:5-4:8", Method, |bar| { - let receiver = bar.receiver().unwrap(); - let name_ref = context.graph().names().get(&receiver).unwrap(); + let Receiver::SelfReceiver(def_id) = bar.receiver().as_ref().unwrap() else { + panic!("Expected SelfReceiver for def self.bar in anonymous Class.new"); + }; + let def = context.graph().definitions().get(def_id).unwrap(); + let name_id = def.name_id().expect("Owner definition should have a name_id"); + let name_ref = context.graph().names().get(name_id).unwrap(); let uri_id = UriId::from("file:///foo.rb"); assert_eq!(StringId::from(&format!("{uri_id}:13")), *name_ref.str()); assert!(name_ref.nesting().is_none()); diff --git a/rust/rubydex/src/model/definitions.rs b/rust/rubydex/src/model/definitions.rs index 3f0d72c0..911c036c 100644 --- a/rust/rubydex/src/model/definitions.rs +++ b/rust/rubydex/src/model/definitions.rs @@ -712,10 +712,22 @@ pub struct MethodDefinition { lexical_nesting_id: Option, parameters: Vec, visibility: Visibility, - receiver: Option, + receiver: Option, } + assert_mem_size!(MethodDefinition, 112); +/// The receiver of a singleton method definition. +#[derive(Debug)] +pub enum Receiver { + /// `def self.foo` - receiver is the enclosing definition (class, module, singleton class or DSL) + SelfReceiver(DefinitionId), + /// `def Foo.bar` - receiver is an explicit constant that needs resolution + ConstantReceiver(NameId), +} + +assert_mem_size!(Receiver, 16); + impl MethodDefinition { #[allow(clippy::too_many_arguments)] #[must_use] @@ -728,7 +740,7 @@ impl MethodDefinition { lexical_nesting_id: Option, parameters: Vec, visibility: Visibility, - receiver: Option, + receiver: Option, ) -> Self { Self { str_id, @@ -747,8 +759,13 @@ impl MethodDefinition { pub fn id(&self) -> DefinitionId { let mut formatted_id = format!("{}{}{}", *self.uri_id, self.offset.start(), *self.str_id); - if let Some(receiver) = self.receiver { - formatted_id.push_str(&receiver.to_string()); + if let Some(receiver) = &self.receiver { + match receiver { + Receiver::SelfReceiver(def_id) => formatted_id.push_str(&def_id.to_string()), + Receiver::ConstantReceiver(name_id) => { + formatted_id.push_str(&name_id.to_string()); + } + } } DefinitionId::from(&formatted_id) @@ -785,7 +802,7 @@ impl MethodDefinition { } #[must_use] - pub fn receiver(&self) -> &Option { + pub fn receiver(&self) -> &Option { &self.receiver } diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index a9009ec2..4a391c63 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -9,7 +9,7 @@ use crate::model::{ Declaration, GlobalVariableDeclaration, InstanceVariableDeclaration, MethodDeclaration, ModuleDeclaration, Namespace, SingletonClassDeclaration, }, - definitions::{Definition, Mixin}, + definitions::{Definition, Mixin, Receiver}, graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID}, identity_maps::{IdentityHashMap, IdentityHashSet}, ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId}, @@ -272,23 +272,26 @@ impl<'a> Resolver<'a> { match self.graph.definitions().get(&id).unwrap() { Definition::Method(method_definition) => { let str_id = *method_definition.str_id(); - let owner_id = if let Some(receiver) = method_definition.receiver() { - let receiver_decl_id = match self.graph.names().get(receiver).unwrap() { - NameRef::Resolved(resolved) => *resolved.declaration_id(), - NameRef::Unresolved(_) => { - // Error diagnostic: if we couldn't resolve the constant being used, then we don't know - // where this method is being defined. For example: - // - // def Foo.bar; end - // - // where Foo is undefined + let owner_id = match method_definition.receiver() { + Some(Receiver::SelfReceiver(def_id)) => { + let Some(&owner_decl_id) = self.graph.definition_id_to_declaration_id(*def_id) else { + // The enclosing class/module couldn't be resolved (e.g., `class Foo::Bar` where + // `Foo` is undefined). The method is orphaned as a consequence. continue; - } - }; + }; + self.get_or_create_singleton_class(owner_decl_id) + } + Some(Receiver::ConstantReceiver(name_id)) => { + let receiver_decl_id = match self.graph.names().get(name_id).unwrap() { + NameRef::Resolved(resolved) => *resolved.declaration_id(), + NameRef::Unresolved(_) => { + continue; + } + }; - self.get_or_create_singleton_class(receiver_decl_id) - } else { - self.resolve_lexical_owner(*method_definition.lexical_nesting_id()) + self.get_or_create_singleton_class(receiver_decl_id) + } + None => self.resolve_lexical_owner(*method_definition.lexical_nesting_id()), }; self.create_declaration(str_id, id, owner_id, |name| { @@ -346,25 +349,28 @@ impl<'a> Resolver<'a> { match nesting_def { // When the instance variable is inside a method body, we determine the owner based on the method's receiver Definition::Method(method) => { - // Method has explicit receiver (def self.foo or def Foo.bar) - if let Some(receiver_name_id) = method.receiver() { - let Some(NameRef::Resolved(resolved)) = self.graph.names().get(receiver_name_id) else { - // TODO: add diagnostic for unresolved receiver - continue; + if let Some(receiver) = method.receiver() { + let receiver_decl_id = match receiver { + Receiver::SelfReceiver(def_id) => *self + .graph + .definition_id_to_declaration_id(*def_id) + .expect("SelfReceiver definition should have a declaration"), + Receiver::ConstantReceiver(name_id) => { + let Some(NameRef::Resolved(resolved)) = self.graph.names().get(name_id) else { + continue; + }; + *resolved.declaration_id() + } }; - let receiver_decl_id = *resolved.declaration_id(); - // Instance variable in singleton method - owned by the receiver's singleton class let owner_id = self.get_or_create_singleton_class(receiver_decl_id); - { - debug_assert!( - matches!( - self.graph.declarations().get(&owner_id), - Some(Declaration::Namespace(Namespace::SingletonClass(_))) - ), - "Instance variable in singleton method should be owned by a SingletonClass" - ); - } + debug_assert!( + matches!( + self.graph.declarations().get(&owner_id), + Some(Declaration::Namespace(Namespace::SingletonClass(_))) + ), + "Instance variable in singleton method should be owned by a SingletonClass" + ); self.create_declaration(str_id, id, owner_id, |name| { Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( name, owner_id,