-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Comments
This is a regression and will be fixed in 9.1.1.0.
|
The tricky bit for us here is that in MRI they often treat
If you call define_method without a receiver, you call a separate 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. |
JRuby behaves like MRI 2.3.1 in all but the toplevel case in your examples. |
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. |
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:
We also could do the cref check. I'm not sure what the equivalent check would be in JRuby for the second one. |
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. |
@jeremyevans If you get a chance, try out the branch on that PR and confirm it runs your specs ok. |
@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. |
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. |
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) |
@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!) |
@headius It's the first google link for "jruby download snapshot" |
@jeremyevans Heh, of course it is! Well at least it points to the right place now. Guess we need to do some SEO. |
Environment
Happens on both OpenBSD/amd64 and Windows, and I'm guessing affects all environments:
Expected Behavior
Actual Behavior
For reference:
I believe ruby 2.3's define_method visibility is as follows:
This bug broke Sequel's connection pool specs.
If you want some useful test cases across different ruby versions, here you go:
The text was updated successfully, but these errors were encountered: