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

Method missing for prepended module #6445

Closed
donv opened this issue Oct 23, 2020 · 4 comments
Closed

Method missing for prepended module #6445

donv opened this issue Oct 23, 2020 · 4 comments
Milestone

Comments

@donv
Copy link
Member

donv commented Oct 23, 2020

Hi!

I have a mismatch in behaviour between JRuby and MRI, and a bonus question :-)

Given the following snippet:

module M
  def stuff
    puts 'Stuff M'
  end
end

class A
  include M
end

module X
  def stuff
    puts 'Stuff X'
    super
  end
end

M.prepend(X)

A.new.stuff

MRI outputs:

Stuff M

while JRuby fails with this output:

Stuff X
NoMethodError: super: no superclass method `stuff' for #<A:0x5d767218>
   stuff at scratch.rb:14
  <main> at scratch.rb:20

Unless this is illegal Ruby, I guess JRuby should behave the same as MRI.

Bonus question:

Why is the stuff method of module X not called when using MRI? If I move the definition of class A below the call to M.prepend it works.

Environment Information

jruby 9.2.13.0 (2.5.7) 2020-08-03 9a89c94 OpenJDK 64-Bit Server VM 11.0.7+10 on 11.0.7+10 +jit [darwin-x86_64]
Darwin 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64

Any help is appreciated :-)

@headius
Copy link
Member

headius commented Oct 26, 2020

Yup this is a bug for sure, but it may be the same cause as #4531, especially since moving things around fixes it. I guess this needs to be a higher priority and fixed in 9.3.

@enebo
Copy link
Member

enebo commented Aug 5, 2021

For @donv original snippet I notice that they seem to have fixed the oddity raised in the original report:

2.7 and lower:

Stuff M

3.0:

Stuff X
Stuff M

Based on this and how I would expect things to work I would expect the 3.0 behavior. So MRI must have fixed this lookup at some point. I would not use this pattern an expect 'Stuff M' since it will stop working.

With that said JRuby dispatches to X like we would expect but then the super does not find M. At this point I feel like for 9.2/3/4 we should make it behave like 3.0 but that obviously has a potential to be reported again. I will examine the other bugs to see how closely they seem to the same problem or not.

@enebo
Copy link
Member

enebo commented Aug 6, 2021

So in talking yesterday we figured out a few things.

  1. Ruby 3.0 will keep track of all included stuff somehow and the ancestors will reflect current state of the originally included/prepended thing (including other modules included/prepended into it). This is why in 3.0 it sees X even though X was prepended AFTER M was included into A. So while we have to support this and my original advice about behavior changing means not depending on behavior that has already changed...it will be a bigger change we will only do for 3.0.
  2. Ruby 2.7 and earlier will only reflect what the module has included/prepended into it at the time it is included/prepended into something else. So X is not seen because it has been prepended after it was included into A.
  3. JRuby does inclusion a bit differently. We include the same way as Ruby 2.7 except that we delegate method lookup (and some other things) to the original module definition. In MRI, they do not delegate back to original module but will copy the method pointer into the included copy of the module. Because we go back to ask original M for the stuff method it actually ends up finding X's stuff instead. If we had just copied the reference to the method table and used the included module as our method search module it would not find X.

So one idea we are considering (which will only be 9.3+ due to risk) is to change our IncludedModuleWrapper (PrependedModule) to just contain a reference to the original modules method map. This involves changing alot of code which tends to do something like: included_module.get_non_included_module.search_method. It will become included_module.search_method. Sounds simple? It is all over and there are likely other things we have not considered but it is the current thing to correct this behavior.

@enebo
Copy link
Member

enebo commented Aug 20, 2021

Fixed in PR #6777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants