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

Constant access via . does not work when using Java 11 #5730

Closed
jeremyevans opened this issue May 14, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@jeremyevans
Copy link
Contributor

commented May 14, 2019

On Java 8 both A.B and A::B will work for referencing a Java constant. On Java 11, only A::B works, A.B raises NoMethodError. This could be considered a bug fix an not a regression, considering that for Ruby constants, A.B will raise NoMethodError if B is a constant and not a method.

Environment

jruby -v:

With Java 8: jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 OpenJDK 64-Bit Server VM 25.202-b08 on 1.8.0_202-b08 +jit [OpenBSD-x86_64]

With Java 11: jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 OpenJDK 64-Bit Server VM 11.0.3+7-1 on 11.0.3+7-1 +jit [OpenBSD-x86_64]

lopex on IRC confirmed this issue occurs on Linux, so it is not specific to OpenBSD.

Expected Behavior

Behavior should be the same for all supported Java versions, not depend on the Java version in use. Either raise an NoMethodError for A.B if B is a Java constant and not a method (similar to Ruby constants), or allow it to work with Java constants (for backwards compatibility with earlier JRuby versions).

Actual Behavior

$ jruby -ve "p java.sql.Statement::RETURN_GENERATED_KEYS; p java.sql.Statement.RETURN_GENERATED_KEYS"
jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 OpenJDK 64-Bit Server VM 25.202-b08 on 1.8.0_202-b08 +jit [OpenBSD-x86_64]
1
1

$ jruby -ve "p java.sql.Statement::RETURN_GENERATED_KEYS; p java.sql.Statement.RETURN_GENERATED_KEYS"
jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 OpenJDK 64-Bit Server VM 11.0.3+7-1 on 11.0.3+7-1 +jit [OpenBSD-x86_64]
1
NoMethodError: undefined method `RETURN_GENERATED_KEYS' for Java::JavaSql::Statement:Module
  <main> at -e:1
@headius

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I'm very surprised A.B ever worked.

@headius

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Ok this looks like a bug to begin with. The correct behavior is that constant values on Java classes are bound only as constants (A::B) and not as accessors (A.B).

However when this code was last updated in 2015, it appears that constants on interfaces do both. That's a bug; at the very last it doesn't match class logic.

This appears to have been here for a very long time, though: beaaeb8

@headius

This comment has been minimized.

Copy link
Member

commented May 15, 2019

It looks like this was an oversight by @bdortch when this bit rework of Java integration landed. The same logic for classes does skip the accessors, and I guess we just never noticed until now that the behavior differs for interfaces.

As for why it differs on Java 9+, here's the logic used by the static getter/setter installers:

    @Override void install(final RubyModule proxy) {
        if (isAccessible()) {
            proxy.getSingletonClass().addMethod(name, new StaticFieldGetter(name, proxy, field));
        }
    }

The isAccessible check is perhaps too extreme here; we don't need it to be accessible unless it's not a public field (and we have been trending away from binding such fields automatically anyway). On Java 9+, because we can't set the field accessible without module warnings, the accessors end up not getting bound.

So there's a couple questions going forward:

  • This behavior has been here a long time. Do we need to keep supporting it? Obviously @jeremyevans is using it in Sequel.
  • Should we make classes do the same thing for consistency?

And any fix going forward should clean up how these accessors are bound; if the field is already public, we don't need "accessible" to read it; if it's public and writable, we don't need it to be "accessible" to write it. cc @kares

@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Note that Sequel is no longer using it, I fixed that version in jeremyevans/sequel@3b7c590. It's still in the latest release until I push out a new version early next month.

I think making it like Ruby is best, and do not allow A.B to access constants. I'm not sure how widely that syntax is used to access constants. If fixing the behavior to be like Ruby would break too much currently, you could always deprecate it and remove it in 9.3+.

@headius

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@jeremyevans I wish I knew how much it affected. Maybe it's not much, since constants on interfaces are not super common, and anyone working with constants probably uses :: anyway. I'm guessing you just accidentally fell into this behavior rather than actively made the change expected the accessors to work.

Short term I'm going to fix the accessibility check and we can debate whether the behavior should be altered.

@headius headius added this to the JRuby 9.2.8.0 milestone May 15, 2019

@headius

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Closing the loop on a few of these questions, after discussion on our Matrix channel...

  • The fix appears to be changing && to || here. I had a concern about public fields on non-public interfaces, but it appears we do fail to bind such fields much earlier:
$ (pickjdk 8 ; jruby -Xbacktrace.style=full -e 'p Java::java.net.SocksConsts.BIND')
New JDK: jdk-11.0.2+9-hotspot
NameError: missing class or uppercase package name (`java.net.SocksConsts'), caused by Unable to make field public static final int java.net.SocksConsts.DEFAULT_PORT accessible: module java.base does not "opens java.net" to module org.jruby.dist
                  getStackTrace at java/lang/Thread.java:1606
               getBacktraceData at org/jruby/runtime/backtrace/TraceType.java:237
                   getBacktrace at org/jruby/runtime/backtrace/TraceType.java:48
               captureBacktrace at org/jruby/RubyException.java:417
                       preRaise at org/jruby/exceptions/RaiseException.java:114
                       preRaise at org/jruby/exceptions/RaiseException.java:95
                         <init> at org/jruby/exceptions/RaiseException.java:61
                         <init> at org/jruby/exceptions/Exception.java:38
                         <init> at org/jruby/exceptions/StandardError.java:38
                         <init> at org/jruby/exceptions/NameError.java:38
             constructThrowable at org/jruby/RubyNameError.java:183
                    toThrowable at org/jruby/RubyException.java:395
                   newNameError at org/jruby/Ruby.java:4101
                   newNameError at org/jruby/Ruby.java:4130
                   newNameError at org/jruby/Ruby.java:4115
  getProxyOrPackageUnderPackage at org/jruby/javasupport/Java.java:922
  • Because this behavior has been around for so long, we will deprecate and warn about it until JRuby 9.3. I will submit a 9.3 PR to remove this behavior.

headius added a commit to headius/jruby that referenced this issue May 15, 2019

headius added a commit to headius/jruby that referenced this issue May 15, 2019

headius added a commit to headius/jruby that referenced this issue May 15, 2019

headius added a commit to headius/jruby that referenced this issue May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.