No more wrapper #4102

Merged
merged 2 commits into from Aug 23, 2016

Conversation

Projects
None yet
2 participants
@headius
Member

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 #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.

headius added some commits Aug 22, 2016

Make all IR methods cloneable and eliminate trivial dup impls.
This is part of fixes for #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?.
Stop using WrapperMethod, since it messes up super logic.
This is for #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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 22, 2016

Member

@enebo Review please!

Member

headius commented Aug 22, 2016

@enebo Review please!

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 23, 2016

Member

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

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

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius deleted the headius:no_more_wrapper branch Aug 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment