Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No more wrapper #4102

Merged
merged 2 commits into from
Aug 23, 2016
Merged

No more wrapper #4102

merged 2 commits into from
Aug 23, 2016

Conversation

headius
Copy link
Member

@headius headius commented Aug 22, 2016

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:

  • Methods that were wrapped before will now be their own true instances of the original method.
  • Because we now attempt to dup rather than wrap, I had to modify duping of IR-based methods to always use clone. This makes them have the same serial number, so that transplanted methods (e.g. those in Unable to use UnboundMethod referencing super with define_singleton_method #3869) will still be == and eql?. 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).
  • This is a relatively deep change right before 9.1.3.0, but it fixes known bugs with super that have dogged us for a long time.

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.
@headius
Copy link
Member Author

headius commented Aug 22, 2016

@enebo Review please!

@enebo
Copy link
Member

enebo commented Aug 23, 2016

@headius sorry we went over this yesterday and there are no issues I can readily see. This seems much nicer fwiw too.

@headius headius merged commit 823f97c into jruby:master Aug 23, 2016
@headius headius deleted the no_more_wrapper branch August 23, 2016 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants