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 9.1.0.0 define_method creates private methods in some cases where it should create public methods #3854

Closed
jeremyevans opened this Issue May 5, 2016 · 14 comments

Comments

Projects
None yet
2 participants
@jeremyevans
Contributor

jeremyevans commented May 5, 2016

Environment

Happens on both OpenBSD/amd64 and Windows, and I'm guessing affects all environments:

jruby 9.1.0.0 (2.3.0) 2016-05-02 a633c63 OpenJDK 64-Bit Server VM 25.72-b15 on 1.8.0_72-b15 +jit [OpenBSD-x86_64]
jruby 9.1.0.0 (2.3.0) 2016-05-02 a633c63 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b15 +jit [mswin32-x86_64]

Expected Behavior

$ jruby -e 'Object.send(:define_method, :a){1}; p Object.new.a'
1

Actual Behavior

$ jruby -e 'Object.send(:define_method, :a){1}; p Object.new.a'
NoMethodError: private method `a' called for #<Object:0x6e6c3152>
  <top> at -e:1

For reference:

$ ruby -ve 'Object.send(:define_method, :a){1}; p Object.new.a'
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
1

I believe ruby 2.3's define_method visibility is as follows:

  1. Private/Protected if define_method called inside a class/module definition after the private/protected keyword with the current class/module as the receiver .
  2. Public in all other cases.

This bug broke Sequel's connection pool specs.

If you want some useful test cases across different ruby versions, here you go:

$ for x in 18 19 20 21 22 23; do ruby$x -ve "class A; private; define_method(:a){1} end; p A.new.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
-e:1: private method `a' called for #<A:0x1d46bffebc58> (NoMethodError)
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x000f8255434d88> (NoMethodError)
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x0013bf54e5ea58> (NoMethodError)
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x000263d55ab1b8> (NoMethodError)

$ for x in 18 19 20 21 22 23; do ruby$x -ve "class A; protected; define_method(:a){1} end; p A.new.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
-e:1: protected method `a' called for #<A:0x1d15870b1c60> (NoMethodError)
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
-e:1:in `<main>': protected method `a' called for #<A:0x0019c33114f038> (NoMethodError)
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
-e:1:in `<main>': protected method `a' called for #<A:0x00095082494a40> (NoMethodError)
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
-e:1:in `<main>': protected method `a' called for #<A:0x0003f211243518> (NoMethodError)

$ for x in 18 19 20 21 22 23; do ruby$x -ve "class A; public; define_method(:a){1} end; p A.new.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
1
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
1
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
1
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
1

$ for x in 18 19 20 21 22 23; do ruby$x -ve "class A; private; A.send(:define_method, :a){1} end; p A.new.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
-e:1: private method `a' called for #<A:0x16d03fa44bd0> (NoMethodError)
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x00129e7b09e170> (NoMethodError)
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x000dad300e6c90> (NoMethodError)
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
-e:1:in `<main>': private method `a' called for #<A:0x000e91347b4b60> (NoMethodError)


$ for x in 18 19 20 21 22 23; do ruby$x -ve "Object.send(:define_method, :a){1}; p Object.new.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
-e:1: private method `a' called for #<Object:0x18f26eebce08> (NoMethodError)
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
1
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
1
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
1

$ for x in 18 19 20 21 22 23; do ruby$x -ve "a = Object.new; (class << a; self end).send(:define_method, :a){1}; p a.a"; done
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
1
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
1
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-openbsd]
1
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
1
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-openbsd]
1
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-openbsd]
1

@headius headius added this to the JRuby 9.1.1.0 milestone May 9, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

This is a regression and will be fixed in 9.1.1.0.

$ rvm jruby-9.0.5.0 do jruby -e 'Object.send(:define_method, :a){1}; p Object.new.a'
1
Member

headius commented May 9, 2016

This is a regression and will be fixed in 9.1.1.0.

$ rvm jruby-9.0.5.0 do jruby -e 'Object.send(:define_method, :a){1}; p Object.new.a'
1
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

Ok, here's the fix from 9.1 that broke your example: 6ed2276

It was intended to fix #3507 which pointed out other inconsistencies in how define_method sets visibility.

Member

headius commented May 9, 2016

Ok, here's the fix from 9.1 that broke your example: 6ed2276

It was intended to fix #3507 which pointed out other inconsistencies in how define_method sets visibility.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

I believe ruby 2.3's define_method visibility is as follows:

  1. Private/Protected if define_method called inside a class/module definition after the private/protected keyword with the current class/module as the receiver .
  2. Public in all other cases.

The tricky bit for us here is that in MRI they often treat send as though it were a direct method call rather than having a separate call inbetween. You are doing send from top-level to call define_method. This is the define_method that lives on the class object, and when calling define_method on a class object it uses the lexically-scoped visibility for method definitions, which is private at toplevel:

[] ~/projects/jruby $ jruby -e "def a; end; self.a"
NoMethodError: private method `a' called for main:Object
  <top> at -e:1

[] ~/projects/jruby $ ruby23 -e "def a; end; self.a"
-e:1:in `<main>': private method `a' called for main:Object (NoMethodError)

If you call define_method without a receiver, you call a separate define_method defined directly on the toplevel object, which always defaults to public.

So this is a weird middle ground. It's a call to Module#define_method from a scope in which the current method definition visibility is set to private, but defining a method from toplevel is supposed to be public.

Member

headius commented May 9, 2016

I believe ruby 2.3's define_method visibility is as follows:

  1. Private/Protected if define_method called inside a class/module definition after the private/protected keyword with the current class/module as the receiver .
  2. Public in all other cases.

The tricky bit for us here is that in MRI they often treat send as though it were a direct method call rather than having a separate call inbetween. You are doing send from top-level to call define_method. This is the define_method that lives on the class object, and when calling define_method on a class object it uses the lexically-scoped visibility for method definitions, which is private at toplevel:

[] ~/projects/jruby $ jruby -e "def a; end; self.a"
NoMethodError: private method `a' called for main:Object
  <top> at -e:1

[] ~/projects/jruby $ ruby23 -e "def a; end; self.a"
-e:1:in `<main>': private method `a' called for main:Object (NoMethodError)

If you call define_method without a receiver, you call a separate define_method defined directly on the toplevel object, which always defaults to public.

So this is a weird middle ground. It's a call to Module#define_method from a scope in which the current method definition visibility is set to private, but defining a method from toplevel is supposed to be public.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

JRuby behaves like MRI 2.3.1 in all but the toplevel case in your examples.

Member

headius commented May 9, 2016

JRuby behaves like MRI 2.3.1 in all but the toplevel case in your examples.

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans May 9, 2016

Contributor

That's good, sorry for not checking/including the JRuby output in the comparison. I included different versions because I know JRuby 1.7 needs to support ruby 1.8 and 1.9, 9.0 needs to support 2.2, and 9.1 needs to support 2.3.

Contributor

jeremyevans commented May 9, 2016

That's good, sorry for not checking/including the JRuby output in the comparison. I included different versions because I know JRuby 1.7 needs to support ruby 1.8 and 1.9, 9.0 needs to support 2.2, and 9.1 needs to support 2.3.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 9, 2016

Member

The following patch fixes the issue, by following some similar checks from MRI's "cref"-locating code:

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d5867237..e289f95 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1868,7 +1868,11 @@ public class RubyModule extends RubyObject {

     @JRubyMethod(name = "define_method", visibility = PRIVATE, reads = VISIBILITY)
     public IRubyObject define_method(ThreadContext context, IRubyObject arg0, Block block) {
-        Visibility visibility = context.getCurrentVisibility();
+        Visibility visibility = PUBLIC;
+
+        // These checks are similar to rb_vm_cref_in_context from MRI.
+        if (context.getCurrentFrame().getSelf() == this) visibility = context.getCurrentVisibility();
+
         return defineMethodFromBlock(context, arg0, block, visibility);
     }

The code from MRI is here:

static VALUE
rb_mod_define_method(int argc, VALUE *argv, VALUE mod)
{
    ID id;
    VALUE body;
    VALUE name;
    const rb_cref_t *cref = rb_vm_cref_in_context(mod, mod);
    const rb_scope_visibility_t default_scope_visi = {METHOD_VISI_PUBLIC, FALSE};
    const rb_scope_visibility_t *scope_visi = &default_scope_visi;
    int is_method = FALSE;

    if (cref) {
    scope_visi = CREF_SCOPE_VISI(cref);
    }

...

const rb_cref_t *
rb_vm_cref_in_context(VALUE self, VALUE cbase)
{
    rb_thread_t *th = GET_THREAD();
    const rb_control_frame_t *cfp = rb_vm_get_ruby_level_next_cfp(th, th->cfp);
    const rb_cref_t *cref;
    if (cfp->self != self) return NULL;
    if (!vm_env_cref_by_cref(cfp->ep)) return NULL;
    cref = rb_vm_get_cref(cfp->ep);
    if (CREF_CLASS(cref) != cbase) return NULL;
    return cref;
}

Basically, in order for the lexically-scoped visibility to override PUBLIC:

  • if (cfp->self != self) return NULL;
    The current frame's self must be the object we're calling define_method on (e.g. we need to be doing the call from either the module's open body or from one of its class methods).
  • if (!vm_env_cref_by_cref(cfp->ep)) return NULL;
    Not sure about the checks in this method, but they appear to be checking whether the current environment pointer is pointing at a cref (cref is roughly equivalent to our StaticScope, but unlike MRI we do not store visibility there).
  • if (CREF_CLASS(cref) != cbase) return NULL;
    This checks if the class that the cref points at is the same as the current module (again similar to the self check above.

We also could do the cref check. I'm not sure what the equivalent check would be in JRuby for the second one.

Member

headius commented May 9, 2016

The following patch fixes the issue, by following some similar checks from MRI's "cref"-locating code:

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d5867237..e289f95 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1868,7 +1868,11 @@ public class RubyModule extends RubyObject {

     @JRubyMethod(name = "define_method", visibility = PRIVATE, reads = VISIBILITY)
     public IRubyObject define_method(ThreadContext context, IRubyObject arg0, Block block) {
-        Visibility visibility = context.getCurrentVisibility();
+        Visibility visibility = PUBLIC;
+
+        // These checks are similar to rb_vm_cref_in_context from MRI.
+        if (context.getCurrentFrame().getSelf() == this) visibility = context.getCurrentVisibility();
+
         return defineMethodFromBlock(context, arg0, block, visibility);
     }

The code from MRI is here:

static VALUE
rb_mod_define_method(int argc, VALUE *argv, VALUE mod)
{
    ID id;
    VALUE body;
    VALUE name;
    const rb_cref_t *cref = rb_vm_cref_in_context(mod, mod);
    const rb_scope_visibility_t default_scope_visi = {METHOD_VISI_PUBLIC, FALSE};
    const rb_scope_visibility_t *scope_visi = &default_scope_visi;
    int is_method = FALSE;

    if (cref) {
    scope_visi = CREF_SCOPE_VISI(cref);
    }

...

const rb_cref_t *
rb_vm_cref_in_context(VALUE self, VALUE cbase)
{
    rb_thread_t *th = GET_THREAD();
    const rb_control_frame_t *cfp = rb_vm_get_ruby_level_next_cfp(th, th->cfp);
    const rb_cref_t *cref;
    if (cfp->self != self) return NULL;
    if (!vm_env_cref_by_cref(cfp->ep)) return NULL;
    cref = rb_vm_get_cref(cfp->ep);
    if (CREF_CLASS(cref) != cbase) return NULL;
    return cref;
}

Basically, in order for the lexically-scoped visibility to override PUBLIC:

  • if (cfp->self != self) return NULL;
    The current frame's self must be the object we're calling define_method on (e.g. we need to be doing the call from either the module's open body or from one of its class methods).
  • if (!vm_env_cref_by_cref(cfp->ep)) return NULL;
    Not sure about the checks in this method, but they appear to be checking whether the current environment pointer is pointing at a cref (cref is roughly equivalent to our StaticScope, but unlike MRI we do not store visibility there).
  • if (CREF_CLASS(cref) != cbase) return NULL;
    This checks if the class that the cref points at is the same as the current module (again similar to the self check above.

We also could do the cref check. I'm not sure what the equivalent check would be in JRuby for the second one.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 10, 2016

Member

I've got a few specs for this behavior and they show I haven't got logic quite right. Will try to revisit today so we can put this to rest once and for all.

Member

headius commented May 10, 2016

I've got a few specs for this behavior and they show I haven't got logic quite right. Will try to revisit today so we can put this to rest once and for all.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 11, 2016

Member

@jeremyevans If you get a chance, try out the branch on that PR and confirm it runs your specs ok.

Member

headius commented May 11, 2016

@jeremyevans If you get a chance, try out the branch on that PR and confirm it runs your specs ok.

@headius headius self-assigned this May 11, 2016

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans May 11, 2016

Contributor

@headius I'll see if I can do that tomorrow. I usually don't build JRuby from source (since OpenBSD switched to just using the binary distribution), but hopefully I can figure out how to do that.

Contributor

jeremyevans commented May 11, 2016

@headius I'll see if I can do that tomorrow. I usually don't build JRuby from source (since OpenBSD switched to just using the binary distribution), but hopefully I can figure out how to do that.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 11, 2016

Member

Well if it passes all tests in the PR I'll merge it to master and it will be in the next 9.1.1.0 snapshot build.

Member

headius commented May 11, 2016

Well if it passes all tests in the PR I'll merge it to master and it will be in the next 9.1.1.0 snapshot build.

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans May 11, 2016

Contributor

OK. I'll try the snapshot build. BTW, it looks like the link for downloading snapshots is broken: http://jruby.org/files/downloads/snapshots/master/index.html (linked from http://jruby.org/files/downloads/snapshots/index.html)

Contributor

jeremyevans commented May 11, 2016

OK. I'll try the snapshot build. BTW, it looks like the link for downloading snapshots is broken: http://jruby.org/files/downloads/snapshots/master/index.html (linked from http://jruby.org/files/downloads/snapshots/index.html)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 11, 2016

Member

@jeremyevans Thanks! How did you even get to the snapshots/index.html page? I could not find a path to it (and did not know it still existed!)

Member

headius commented May 11, 2016

@jeremyevans Thanks! How did you even get to the snapshots/index.html page? I could not find a path to it (and did not know it still existed!)

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans May 11, 2016

Contributor

@headius It's the first google link for "jruby download snapshot"

Contributor

jeremyevans commented May 11, 2016

@headius It's the first google link for "jruby download snapshot"

@headius headius closed this in 9255a39 May 11, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 11, 2016

Member

@jeremyevans Heh, of course it is! Well at least it points to the right place now. Guess we need to do some SEO.

Member

headius commented May 11, 2016

@jeremyevans Heh, of course it is! Well at least it points to the right place now. Guess we need to do some SEO.

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