Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
97823c7 to
1315d61
Compare
| let receiver = receiver_node.as_ref().and_then(|node| match node { | ||
| ruby_prism::Node::ConstantPathNode { .. } | ruby_prism::Node::ConstantReadNode { .. } => self | ||
| .index_constant_reference(node, true) | ||
| .map(Receiver::ConstantReceiver), | ||
| _ => None, | ||
| }); | ||
|
|
||
| // alias_method references instance methods, so we use the class NameId directly | ||
| let method_ref_receiver = match &receiver { | ||
| Some(Receiver::ConstantReceiver(name_id)) => Some(*name_id), | ||
| Some(Receiver::SelfReceiver(_)) | None => { | ||
| self.method_receiver(receiver_node.as_ref(), node.location()) | ||
| } | ||
| }; |
There was a problem hiding this comment.
Why do we need these two separate paths for the receiver? It seems you're handling constant receiver in one and self in the other. Doesn't method_receiver already handle both?
There was a problem hiding this comment.
The old method_receiver() wrapped constant receivers into a singleton class, so we couldn't reuse it directly for alias_method.
Taking a deeper look at the problem, I split the responsibilities: method_receiver() now only classifies what kind of receiver it is (returning a Receiver enum), and extracted the rest into index_method_receiver(), which creates the singleton class name and indexes the constant reference.
I wasn't happy with how the code was turning out and noticed there were a few concepts tied together, which led me to refactor the Receiver enum to better handle the different scenarios.
Since this is an unexpected refactor, it's in a different commit for an easier review.
Let me know if you have any questions.
|
|
||
| /// The receiver of a singleton method definition. | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
There was a problem hiding this comment.
Why do we need to clone and copy receivers?
There was a problem hiding this comment.
Before this PR, the receiver handling was inline within each match arm, so there was no borrow conflict.
Extracting resolve_singleton_owner as a shared helper to remove duplication means we now need to pass the receiver to a &mut self method while self.graph.definitions() is still borrowed.
Copy lets us dereference the receiver out of the borrow (let receiver = *method_definition.receiver()), which I find cleaner than .clone()
e2e0984 to
3ef8701
Compare
| Constant(NameId), | ||
| /// `singleton_class` call or method nested inside a singleton method. | ||
| SingletonClass(NameId), | ||
| } |
There was a problem hiding this comment.
We already have Receiver enum in definitions.rs to represent receiver types. Why can't we use them here?
There was a problem hiding this comment.
Stan and I pair-reviewed this PR and revisited my original choice to create a separate enum. Thanks to his feedback, we're now reusing the existing Receiver enum directly. Simpler and avoids the extra enum.
@st0012, let me know what you think of the changes.
alias_method can be called on a specific class, like Foo.alias_method(:new, :old), where the alias belongs to Foo rather than the enclosing scope. To handle this, we now store the receiver on MethodAliasDefinition so the resolver knows which class owns the alias. Since alias_method always creates instance aliases (unlike def Foo.bar which targets the singleton class), constant receivers resolve to the class itself.
Change SelfReceiver from DefinitionId to NameId — every caller already resolved through the name system, making the indirection unnecessary. Add SingletonClassReceiver variant for singleton method nesting and singleton_class calls. Split the old method_receiver() into two functions: - method_receiver() returns a Receiver (classification only) - index_method_receiver() wraps in singleton name and registers constant references, returning NameId This lets the alias_method handler call method_receiver() directly to classify the receiver without singleton wrapping, while method definitions and references continue using index_method_receiver()
3ef8701 to
4ebcb18
Compare
|
|
||
| let name_id = match receiver { | ||
| /// Classifies a method receiver AST node into a [`Receiver`] without applying | ||
| /// singleton wrapping or registering constant references. |
There was a problem hiding this comment.
What does the singleton wrapping mean?
| Some(ruby_prism::Node::CallNode { .. }) => { | ||
| // Check if the receiver is `singleton_class` | ||
| // For `Foo.singleton_class.bar`, we need the inner receiver (`Foo`) with | ||
| // singleton wrapping and constant reference registration. This calls |
There was a problem hiding this comment.
This seems to contradict what the method comment says?
| SelfReceiver(DefinitionId), | ||
| /// `def Foo.bar` - receiver is an explicit constant that needs resolution | ||
| /// `self` or no receiver — resolved from the lexical nesting. | ||
| SelfReceiver(NameId), |
There was a problem hiding this comment.
Why are we changing this? We use DefinitionId because when the receiver relies on lexical scope, we can't immediately know the name when indexing. It could also have no name at all, for example:
foo do
def self.bar; end
endWe will need to preserve these definitions too for DSL handling.
| /// `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 | ||
| /// `self` or no receiver — resolved from the lexical nesting. |
There was a problem hiding this comment.
When there's no receiver, it's reflected by having None in the receiver attribute.

This PR resolves a gap identified in #631 (comment)
alias_methodcan be called on a specific class, likeFoo.alias_method(:new, :old). When that happens, the alias belongs toFoorather than whatever class happens to be the enclosing scope. Adding areceiverfield toMethodAliasDefinitionlets the resolver figure out the right owner.Unlike
def Foo.barwhich defines a singleton method,Foo.alias_methodalways creates instance aliases. So we resolve constant receivers to the class itself, not its singleton class.This PR also extracts
resolve_singleton_owner,create_singleton_name_id, andReceiver::append_to_idto reduce duplication, and moves the receiver guard out ofeach_string_or_symbol_argintoindex_attrsincealias_methodlegitimately accepts constant receivers now.