Project

General

Profile

Actions

Bug #18960

closed

Module#using raises RuntimeError when called at toplevel from wrapped script

Added by shioyama (Chris Salzberg) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:109456]

Description

I noticed that this file works when loaded with load, but fails if you pass true (or a module) as the wrap argument.

# using.rb
using Module.new

This works:

load "./using.rb"

This doesn't:

load "./using.rb", true
# raises RuntimeError (main.using is permitted only at toplevel)

I believe the latter should work.

Actions #1

Updated by shioyama (Chris Salzberg) over 1 year ago

  • ruby -v set to 3.1.2p20

Updated by shioyama (Chris Salzberg) over 1 year ago

I'm new to contributing to Ruby, but I haven't gotten a response on this in a couple weeks so wondering if maybe this requires more context.

For reference, here are the docs on the wrap parameter to Kernel#load:

If the optional wrap parameter is true, the loaded script will be executed under an anonymous module, protecting the calling program's global namespace. If the optional wrap parameter is a module, the loaded script will be executed under the given module. In no circumstance will any local variables in the loaded file be propagated to the loading environment.

So in theory if I have a file like this:

# using.rb
using Module.new

then loading it with wrap in the following:

MyModule = Module.new

load "./using.rb", MyModule

should be the same as this:

module MyModule
  using Module.new
end

However, whereas the latter works fine, the former fails with the error reported in the issue.

Note that if I do the same inside of a block to Module.new, like this:

Module.new do
  using Module.new
end

or like this:

Module.new.module_eval do
  using Module.new
end

then I get a different error, RuntimeError: Module#using is not permitted in methods. So "the loaded script will be executed under an anonymous module" in the docs is somewhat misleading since it's not as simple as injecting the code in using.rb into the block to Module.new. In fact, AFAICT there is no way to achieve what load with a wrap option does using inline Ruby. (This I find very interesting actually.)

Regardless though I don't think the error I'm seeing (main.using is permitted only at toplevel) is correct, I think this should work.

@jeremyevans0 (Jeremy Evans) You most recently contributed to the wrap option, what do you think about this? Am I correct to consider this a bug, and this the correct fix?

Updated by jeremyevans0 (Jeremy Evans) over 1 year ago

shioyama (Chris Salzberg) wrote in #note-3:

@jeremyevans0 (Jeremy Evans) You most recently contributed to the wrap option, what do you think about this? Am I correct to consider this a bug, and this the correct fix?

I'm not sure whether this is a bug, but since using is permitted at top-;evel and also permitted in a module definition, it seems logical it should work at top-level with an implicit top-level module. The pull request looks correct to me. Not sure if the spec should use ruby_bug, I would just use ruby_version_is.

Updated by shugo (Shugo Maeda) over 1 year ago

shioyama (Chris Salzberg) wrote in #note-3:

@jeremyevans0 (Jeremy Evans) You most recently contributed to the wrap option, what do you think about this? Am I correct to consider this a bug, and this the correct fix?

main.using should not be called in a method definition even if it's a wrapped script.
With your fix, the following script causes BUG:

excelsior:ruby$ cat using_in_method.rb
# using.rb
def foo
  using Module.new
end

foo
excelsior:ruby$ cat t.rb
load "./using_in_method.rb", true
excelsior:ruby$ ./miniruby t.rb
/Users/shugo/src/ruby/using_in_method.rb:3: [BUG] vm_cref_dup: unreachable
ruby 3.2.0dev (2022-08-22T06:05:24Z shioyama-using_in_.. 08bdfcde78) [x86_64-darwin21]

I'm not sure whether this issue should be considered a bug, but it may be safe not to backport the fix to stable versions.

Updated by shioyama (Chris Salzberg) over 1 year ago

@jeremyevans0 (Jeremy Evans)

I'm not sure whether this issue should be considered a bug, but it may be safe not to backport the fix to stable versions.

Yes, that's what I had originally, in review I changed it to ruby_bug. Happy to put it back.

@shugo (Shugo Maeda)

Ah ok now I see the purpose of that error, thanks.

I'm not sure whether this issue should be considered a bug, but it may be safe not to backport the fix to stable versions.

Sure that makes sense.

It seems like we have two things (1- not allowing main.using to be called in a method definition, and 2- evaluating code in a wrapped script as if it were namespaced in a module) that are mutually incompatible in this case.

Updated by mame (Yusuke Endoh) over 1 year ago

We discussed this issue at the dev meeting. No one objected to correcting this behavior, but the proposed PR is unacceptable because it causes [BUG] as @shugo (Shugo Maeda) said. Could you investigate the issue and update the PR?

Updated by shioyama (Chris Salzberg) over 1 year ago

Thanks! I'll do that and update here once the PR is again ready for review.

Updated by shugo (Shugo Maeda) over 1 year ago

I guess SEGV is caused by using in a method body, which should be prohibited even in a wrapped script.

Updated by shioyama (Chris Salzberg) over 1 year ago

Hmm, ok this is where I originally encountered this problem:

https://github.com/rails/rails/blob/b9493a47e65287f3b135904dbdd511bbf5a5798c/activesupport/lib/active_support/core_ext/enumerable.rb#L285-L290

# Using Refinements here in order not to expose our internal method
using Module.new {
  refine Array do
    alias :orig_sum :sum
  end
}

Although this is somewhat contrived, you can reproduce the problem on the same file as follows:

mod = Module.new
mod::Enumerable = Enumerable

load $LOAD_PATH.resolve_feature_path("active_support/core_ext/enumerable")[1], mod
# main.using is permitted only at toplevel (RuntimeError)

Requiring the same file works as expected:

require "active_support/core_ext/enumerable"
# loads Enumerable extension

Wouldn't the method body issue also be present with require?

Updated by shioyama (Chris Salzberg) over 1 year ago

@shugo (Shugo Maeda)

Actually, it seems the same problem is there with require:

# foo.rb
def foo
  using Module.new
end

foo
require "foo"
# main.using is permitted only at toplevel (RuntimeError)

So I don't really understand why Ruby allows the usage of using at toplevel with require, but not with load. Isn't this a problem in both cases? In both cases you can call using from a method, but with require toplevel usage doesn't raise unless you call from within a method, whereas with load it raises regardless of whether you're calling from a method or not.

Updated by shioyama (Chris Salzberg) over 1 year ago

Oh never mind, I see. My PR results in the error with require but the unreachable BUG with load.

Ok I'll try to find another way to fix it.

Updated by shugo (Shugo Maeda) over 1 year ago

shioyama (Chris Salzberg) wrote in #note-12:

Oh never mind, I see. My PR results in the error with require but the unreachable BUG with load.

Ok I'll try to find another way to fix it.

Yes, RuntimeError should be raised in both cases.

Updated by shioyama (Chris Salzberg) over 1 year ago

@shugo (Shugo Maeda) I've updated my PR and added a spec calling a method which calls using, which fails with the previous code but passes with the updated code. Can you have a look?

Updated by shugo (Shugo Maeda) over 1 year ago

shioyama (Chris Salzberg) wrote in #note-14:

@shugo (Shugo Maeda) I've updated my PR and added a spec calling a method which calls using, which fails with the previous code but passes with the updated code. Can you have a look?

Thank you!

I found another corner case. The following code should be prohibited in a wrapped script:

MAIN = self

module X
  MAIN.send(:using, Module.new)
end

I suggested changes in the following comment:

https://github.com/ruby/ruby/pull/6226#pullrequestreview-1118228759

Actions #16

Updated by shioyama (Chris Salzberg) over 1 year ago

  • Status changed from Open to Closed

Applied in changeset git|82ac4a2399516a3ffda750b815c244aad6d38277.


Support using at toplevel in wrapped script

Allow refinements to be used at the toplevel within a script that is
loaded under a module.

Fixes [Bug #18960]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0