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

JRuby 2.2 mode inconsistent with MRI 2.1+ #3507

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

Comments

Projects
None yet
4 participants
@envygeeks

envygeeks commented Dec 1, 2015

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 the ruby 2.0-2.2 label Dec 1, 2015

@enebo enebo added this to the JRuby 9.0.5.0 milestone Dec 1, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 19, 2016

Member

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

Member

headius commented Jan 19, 2016

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 19, 2016

Member

Simpler repro:

module Foo
  module_function
  define_method(:foo) {}
end
Foo.foo # error on JRuby, not on MRI
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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 19, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 19, 2016

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

envygeeks commented Jan 19, 2016

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

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jan 23, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2016

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2016

Member

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.

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

Use frame visibility for Module#define_method. Fixes #3507.
This also modifies main#define_method to always use public. I'm
not sure this is the correct behavior, but it passes specs.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2016

Member

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.

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.

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