Skip to content
This repository

call Kernel.require ruby method from autoload callback instead of callin... #281

Closed
wants to merge 1 commit into from

4 participants

dontfidget Don't Add Me To Your Organization a.k.a The Travis Bot Charles Oliver Nutter Hiroshi Nakamura
dontfidget

In jruby, the autoload mechanism doesn't actually call Kernel.require like the rubydocs claims it will. This change attempts to fix that.

From http://www.ruby-doc.org/core-1.9.3/Module.html#method-i-autoload:

autoload(module, filename) → nil click to toggle source

Registers filename to be loaded (using Kernel::require) the first time that module (which may be a String or a symbol) is accessed in the namespace of mod.

Note that this change uses the context when require happens rather than when the autoload method is called. I wasn't sure what the right thing to do is, or whether it really matters.

The purpose of this change is to allow the user to override Kernel.require even with autoloading. I assume it rubygems is the most well-known overrider of Kernel.require. It presumably ends up pulling in enough stuff into LOAD_PATH via direct requires that any autoloading it does still works, but I suspect it could be problematic if it tried to add paths for autoloaded files.

Thanks for your consideration,

Andrew

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged a311d53 into b37a0bd).

Charles Oliver Nutter
Owner

Ok, the change looks fine, but I want to make sure this is actually proper...i.e. that 1.9.3 does this too. I know there have been debates about MRI sometimes not actually dynamically dispatching to methods like require, and I think it should dynamically dispatch here, but doing so when MRI does not would be a significant behavioral difference.

Charles Oliver Nutter
Owner

Ok, my fears are confirmed...unfortunately.

blah.rb:

system ~/projects/jruby $ cat blah.rb
puts 'there'

Before your patch:


system ~/projects/jruby $ jruby -e "class Foo; autoload :Bar, './blah.rb'; end; def Kernel.require(a); puts a; end; Foo::Bar"
there
NameError: uninitialized constant Foo::Bar
  const_missing at org/jruby/RubyModule.java:2694
         (root) at -e:1

After your patch:

system ~/projects/jruby $ jruby -e "class Foo; autoload :Bar, './blah.rb'; end; def Kernel.require(a); puts a; end; Foo::Bar"
./blah.rb
NameError: uninitialized constant Foo::Bar
  const_missing at org/jruby/RubyModule.java:2694
         (root) at -e:1

Ruby 1.9.3:

system ~/projects/jruby $ ruby-1.9.3 -e "class Foo; autoload :Bar, './blah.rb'; end; def Kernel.require(a); puts a; end; Foo::Bar"
there
-e:1:in `<main>': uninitialized constant Foo::Bar (NameError)

So yeah, the bad news is that MRI does not exhibit this behavior, regardless of what the documentation claims, and therefore we can't accept this patch.

I would support you if you want to raise this as an issue with the MRI folks, since I agree it should be redispatching here to pick up require hooks like RubyGems installs. However, at this time I need to reject this pull request :(

Charles Oliver Nutter headius closed this September 03, 2012
dontfidget
Hiroshi Nakamura
Collaborator

We should solve that issue for 2.0. See http://bugs.ruby-lang.org/issues/5481, "Make autoload for stdlib gems work as long as autoload feature exists in 2.0."

Thanks @dontfidget, it would be nice to have test next time!

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

Showing 1 unique commit by 1 author.

Sep 03, 2012
dontfidget call Kernel.require ruby method from autoload callback instead of cal…
…ling internal require
a311d53
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 3 additions and 2 deletions. Show diff stats Hide diff stats

  1. 5  src/org/jruby/RubyKernel.java
5  src/org/jruby/RubyKernel.java
@@ -208,8 +208,9 @@ public String file() {
208 208
             }
209 209
 
210 210
             public void load(Ruby runtime) {
211  
-                if (runtime.getLoadService().autoloadRequire(file())) {
212  
-                    // Do not finish autoloading by cyclic autoload 
  211
+                final ThreadContext context = runtime.getCurrentContext();
  212
+                if (runtime.getKernel().callMethod(context, "require", file).isTrue()) {
  213
+                    // Do not finish autoloading by cyclic autoload
213 214
                     module.finishAutoload(baseName);
214 215
                 }
215 216
             }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.