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.

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

@jeremyevans
Copy link
Contributor

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member

headius commented May 9, 2016

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

@jeremyevans
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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

@headius
Copy link
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
Projects
None yet
Development

No branches or pull requests

2 participants