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
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).
85f652d to
b703780
Compare
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
getOldNameandgetSourceName. 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
sourceNamestored inDynamicMethodcan be final, I think. It is only initialized during method duping, which could call an internal constructor.
| 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.
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.
| 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.
There was a problem hiding this comment.
The only simple way I could make final work was by introducing RenamedDynamicMethod .
| 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.
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.
ce24e5a to
4ee8bcb
Compare
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.
4ee8bcb to
0d5e85e
Compare
|
Refactored: for |
Preserve a method's source name when methods share a single underlying wrapper, so
Method#original_nameand#inspectreport the right name instead of whichever name happened to live on the shared wrapper. The dup'd method is wrapped in a per-bindingRenamedDynamicMethod(aDelegatingDynamicMethodsubclass) holding the source name in a final field.Method#original_nameconsultsDynamicMethod#getOldName, which the wrapper overrides andAliasMethodwalks 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
@JRubyMethodmethods still leaks the underlying name (Class.new.method(:is_a?).inspectstill shows(kind_of?)). The specs in ruby/spec#1356 for the inspect case for likeClass.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.