Fix original_name when two methods share a wrapper (like is_a? kind_of?) - #9441
Fix original_name when two methods share a wrapper (like is_a? kind_of?)#9441sampokuokkanen wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Fix Method#original_name and #inspect when two methods share a wrapper (like is_a?/kind_of?) Introduce sourceName on DynamicMethod to preserve a method's source name when methods share a single underlying wrapper. Set it in RubyModule when binding, walk the alias chain in AliasMethod#getOldName to reach it, and consult it from Method#original_name and #inspect so both report the correct source name instead of whichever name happened to live on the wrapper. Covered by new specs in ruby/spec#1356 (ruby/spec#1356).
headius
left a comment
There was a problem hiding this comment.
I think we can distill this down a bit:
- We don't need both getOldName and getSourceName. We should either lift the former up or deprecate it and use the latter, I think.
- AliasMethod should override whichever method is the standard "get original method name" going forward, so we're not checking for AliasMethod and null values all over.
- The sourceName stored in DynamicMethod can be final, I think. It is only initialized during method duping, which could call an internal constructor.
Sorry, something went wrong.
| terminal = ((AliasMethod) terminal).entry.method; | ||
| } | ||
| String source = terminal.getSourceName(); | ||
| return source != null ? source : terminal.getName(); |
There was a problem hiding this comment.
Perhaps getSourceName should do this null check instead? We don't want to be null-checking everywhere.
Also wondering if we can avoid introducing a new method name and lift getOldName up to DynamicMethod doing basically what getSourceName does in your PR. I'm reluctant to add yet another way to get a method's name.
Sorry, something went wrong.
There was a problem hiding this comment.
I ended up dropping the null checks entirely and removing getSourceName along the way (had to introduce a RenamedDynamicMethod class for this special case). Lifted getOldName onto DynamicMethod as suggested and this way it's much cleaner.
Sorry, something went wrong.
| checkValidBindTargetFrom(context, (RubyModule) method.owner(context), false); | ||
|
|
||
| newMethod = method.getMethod().dup(); | ||
| newMethod.setSourceName(method.getMethodName()); |
There was a problem hiding this comment.
If we made the source name final we could modify this logic to dupWithSourceName or similar.
Sorry, something went wrong.
There was a problem hiding this comment.
The only simple way I could make final work was by introducing RenamedDynamicMethod .
Sorry, something went wrong.
| protected final String name; | ||
| /** The original source name when this method was bound under a different name | ||
| * via define_method(name, Method/UnboundMethod); null otherwise. */ | ||
| protected String sourceName; |
There was a problem hiding this comment.
Wondering if we can make sourceName final since it's only set for dup'ed methods passed to define_method.
Sorry, something went wrong.
There was a problem hiding this comment.
Had problems with Object.clone() if I tried making it final, so ended up doing it by introducing RenamedDynamicMethod for this case.
Sorry, something went wrong.
Replace the mutable sourceName field on DynamicMethod with RenamedDynamicMethod, a thin DelegatingDynamicMethod subclass that records the source name in a final field. Each define_method(name, Method) call now produces its own wrapper, so per-binding state is naturally separate without any mutation, and setSourceName drops off DynamicMethod's API.
|
Refactored: for define_method(name, Method/UnboundMethod), the dup'd method is now wrapped in a per-binding RenamedDynamicMethod that holds sourceName as a final field. No more mutable field or setter on DynamicMethod. |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Preserve a method's source name when methods share a single underlying wrapper, so Method#original_name and #inspect report the right name instead of whichever name happened to live on the shared wrapper. The dup'd method is wrapped in a per-binding RenamedDynamicMethod (a DelegatingDynamicMethod subclass) holding the source name in a final field. Method#original_name consults DynamicMethod#getOldName, which the wrapper overrides and AliasMethod walks through.
Covered by new specs in ruby/spec#1356 (ruby/spec#1356).
Sample (the case this PR fixes):
Known limitation: direct lookup of multi-name @JRubyMethod methods still leaks the underlying name (Class.new.method(:is_a?).inspect still shows (kind_of?)). The specs in ruby/spec#1356 for the inspect case for like Class.new.method(:id_a?) will fail, but I think that would be better tackled in a follow-up PR.
This PR fixes the first part of #9257.
@headius
Thanks for explaining to me how method lookup works in JRuby in #9420.