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.

Sign up for GitHub

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

Jump to bottom

JRuby 2.2 mode inconsistent with MRI 2.1+ #3507

Closed
envygeeks opened this issue Dec 1, 2015 · 8 comments
Closed

JRuby 2.2 mode inconsistent with MRI 2.1+ #3507

envygeeks opened this issue Dec 1, 2015 · 8 comments
Labels
ruby 2.0-2.2
Milestone
JRuby 9.1.0.0

Comments

@envygeeks
Copy link

As it stands JRuby 2.2 mode (on 9.0.3.0) is inconsistent with MRI 2.1+ when it comes to module_function and define_method behavior. Take the following sample:

module MyModule module_function
  methods = { :hello => :world }
  methods.each do |key, val|
    define_method key do
      val
    end
  end
end

In Ruby 2.0 module_function is not inherited, but in 2.1+ it is inherited, this leads to inconsistent behavior when moving between JRuby and MRI.
@enebo enebo added this to the JRuby 9.0.5.0 milestone Dec 1, 2015
@headius
Copy link
Member

headius commented Jan 19, 2016

Surprising. This should just be a frame field that propagates to the define_method.

@headius
Copy link
Member

headius commented Jan 19, 2016

Simpler repro:

module Foo
  module_function
  define_method(:foo) {}
end
Foo.foo # error on JRuby, not on MRI

@headius
Copy link
Member

headius commented Jan 19, 2016

I'm not sure this has worked for a long time. JRuby appears to always define_method as public as far back as 2011:

[] ~/projects/jruby $ rvm jruby-1.6 do ruby -e "module Foo; module_function; define_method :foo do; end; end; Foo.foo"
NoMethodError: undefined method `foo' for Foo:Module
  (root) at -e:1

[] ~/projects/jruby $ rvm jruby-1.7 do ruby -e "module Foo; module_function; define_method :foo do; end; end; Foo.foo"
NoMethodError: undefined method `foo' for Foo:Module
  (root) at -e:1

Going to look at improving this for 9.1, when we make a bigger move to 2.3 compatibility.

@headius headius modified the milestones: JRuby 9.1.0.0, JRuby 9.0.5.0 Jan 19, 2016
@envygeeks
Copy link
Author

Aww boo 😢 so no fix for 9.0.5.0 so we can stop doing extend self on modules?

@eregon
Copy link
Member

eregon commented Jan 23, 2016

I added a spec for the private case with an UnboundMethod in d6b97fb. It would be good to have new specs for blocks and private and module_function visibility.

@headius
Copy link
Member

headius commented Feb 14, 2016

@envygeeks You should be able to modify it to do module_function after the definition. It will work the same way as calling it once before a group of method definitions.

@headius
Copy link
Member

headius commented Feb 14, 2016

The trivial fix (grabbing current frame's visibility) appears to regress a couple specs:

1)
main#define_method creates a public method in TOPLEVEL_BINDING FAILED
Expected Object to have method 'boom' but it does not
/Users/headius/projects/jruby/spec/ruby/core/main/define_method_spec.rb:17:in `block in (root)'
org/jruby/RubyBasicObject.java:1641:in `instance_eval'
org/jruby/RubyEnumerable.java:1562:in `all?'
org/jruby/RubyFixnum.java:296:in `times'
org/jruby/RubyArray.java:1565:in `each'
/Users/headius/projects/jruby/spec/ruby/core/main/define_method_spec.rb:6:in `<top>'
org/jruby/RubyKernel.java:952:in `load'
org/jruby/RubyBasicObject.java:1641:in `instance_eval'
org/jruby/RubyArray.java:1565:in `each'

2)
main#define_method creates a public method in script binding FAILED
Expected Object to have method 'boom' but it does not
/Users/headius/projects/jruby/spec/ruby/core/main/define_method_spec.rb:22:in `block in (root)'
org/jruby/RubyBasicObject.java:1641:in `instance_eval'
org/jruby/RubyEnumerable.java:1562:in `all?'
org/jruby/RubyFixnum.java:296:in `times'
org/jruby/RubyArray.java:1565:in `each'
/Users/headius/projects/jruby/spec/ruby/core/main/define_method_spec.rb:6:in `<top>'
org/jruby/RubyKernel.java:952:in `load'
org/jruby/RubyBasicObject.java:1641:in `instance_eval'
org/jruby/RubyArray.java:1565:in `each'

I don't think we're properly managing visibility yet.

headius added a commit that referenced this issue Feb 14, 2016
@headius
Use frame visibility for Module#define_method. Fixes #3507.
6ed2276 
This also modifies main#define_method to always use public. I'm
not sure this is the correct behavior, but it passes specs.
@headius
Copy link
Member

headius commented Feb 14, 2016

With a tweak to the "top self" main I believe all specs pass again. I've pushed my fix and we'll see how the other suites like it.

@headius headius closed this as completed Feb 14, 2016
@headius headius mentioned this issue May 9, 2016
Closed
@envygeeks envygeeks mentioned this issue Jun 29, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ruby 2.0-2.2
Projects
None yet
Milestone
JRuby 9.1.0.0
Development

No branches or pull requests
4 participants
@headius @enebo @envygeeks @eregon