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

Autoload fixes #5489

Merged
merged 4 commits into from Feb 11, 2019
Merged

Autoload fixes #5489

merged 4 commits into from Feb 11, 2019

Conversation

@headius
Copy link
Member

@headius headius commented Dec 2, 2018

The Kernel versions of autoload use the caller's frame to
determine the target module, where we were just detecting that
autoload was called against Kernel and then defining the
autoload on Object's singleton class.

test/jruby/test_autoload.rb has also been modified to match Ruby
2.5 behavior.

Fixes #5466.

@headius headius added this to the JRuby 9.2.5.0 milestone Dec 2, 2018
@headius
Copy link
Member Author

@headius headius commented Dec 2, 2018

This is a WIP. The original issue appears to be fixed but my changes regressed some tests. I will request reviews once I'm satisfied with it.

headius added 2 commits Dec 3, 2018
The Kernel versions of `autoload` use the caller's frame to
determine the target module, where we were just detecting that
`autoload` was called against Kernel and then defining the
autoload on Object's singleton class.

Other parts of autoload were not searching everything properly,
or were not being triggered in the right places.

* Fixes Kernel `autoload` and `autoload?` to use the caller's
  frame to determine the target module.
* Fixes `autoload?` to check the constant tables for UNDEF before
  checking the autoload map, properly returning nil for autoload
  constants defined elsewhere without triggering autoload.
* Fixes `const_defined?` to properly trigger autoload as in MRI's
  `rb_mod_const_defined`.
* Adds overrides for autoload tables to IncludedModuleWrapper to
  aid the above searches.
* Fixes an assertion in test/jruby/test_autoload that did not
  match MRI behavior.

Fixes jruby#5466.
@headius headius force-pushed the module_autoload branch from 49504d7 to 051ad43 Dec 3, 2018
@headius headius changed the title Separate Kernel#.autoload and Module#autoload for frame access. Autoload fixes Dec 3, 2018
@headius
Copy link
Member Author

@headius headius commented Dec 3, 2018

I think I've got everything in place. A number of spec tags and a few test excludes have been removed.

@headius headius requested review from kares and enebo Dec 3, 2018
static RubyModule getModuleForAutoload(ThreadContext context, IRubyObject recv) {
RubyModule target = context.getCurrentStaticScope().getModule();

while (target != null && (target.isSingleton() || target.isIncluded())) {
Copy link
Member

@enebo enebo Dec 3, 2018

@headius So if I autoload in a module which has been included then it migrates to where that was included? If that module is included in two places then does only the first one fire and get stuck somewhere else? This easily may be the semantics....

Copy link
Member Author

@headius headius Dec 3, 2018

Yes that appears to be the semantics. In MRI this logic is rb_class_real, our "getRealClass" but that method lives on RubyClass and not RubyModule.

kares
kares approved these changes Dec 4, 2018
Copy link
Member

@kares kares left a comment

looks good

@enebo enebo removed this from the JRuby 9.2.5.0 milestone Dec 6, 2018
@enebo enebo added this to the JRuby 9.2.6.0 milestone Dec 6, 2018
@headius
Copy link
Member Author

@headius headius commented Dec 10, 2018

This should also get fixed in this PR: https://bugs.ruby-lang.org/issues/14469

I'm tagging a couple MRI tests related to that fix.

headius added a commit to headius/jruby that referenced this issue Dec 10, 2018
headius added a commit to headius/jruby that referenced this issue Dec 12, 2018
@headius headius removed this from the JRuby 9.2.6.0 milestone Dec 18, 2018
@headius headius added this to the JRuby 9.2.7.0 milestone Dec 18, 2018
@enebo enebo merged commit 915e9db into jruby:master Feb 11, 2019
1 check failed
@headius headius mentioned this pull request Feb 12, 2019
headius added a commit to headius/jruby that referenced this issue Feb 12, 2019
@headius headius mentioned this pull request Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants