Conversation
This is part of fixes for jruby#3869. Duping methods by reconstructing them causes them to have a new serial number, which is what we are using to determine method equality (and not much else). By changing dup to use clone here, Ruby methods transplanted from a module to the same module will still be == and eql?.
This is for jruby#3869 and relates to the module_function change from and redefine it with a new visibility and implementation class. However the impl class never passed through to the contained method, preventing it from being used in super. This affected, for example, module_fuction singleton methods that need to super or methods transplanted using defined_method with a Method instance. The new logic always tries to dup the target method so it can be truly populated with the altered fields. This change fixed The previous commit, using cloning instead of construction for IR methods, works around the fact that there's no semi-transparent WrapperMethod to delegate its serial number to the wrapped method. Since in that case and in this one, the method's serial number was expected to be the same after duplication, the clone technique seems acceptable.
Member
Author
|
@enebo Review please! |
Member
|
@headius sorry we went over this yesterday and there are no issues I can readily see. This seems much nicer fwiw too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These commits remove all remaining uses of WrapperMethod, which fixes issues like #3708 and #3869.
See the commits for the longer story, but basically WrapperMethod does not pass through the "new" impl class to the contained method, which results in it supering up the wrong hierarchy.
I have a few concerns, which is why I opened a PR for review:
==andeql?. The wrapper previously made them appear to be two methods with the same serial, so this may be ok...but if we depend on the serial number being unique after a method dup, that will no longer work (I don't know that we do).