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

Use all the tricks to properly eliminate illegal access warnings #6195

merged 5 commits into from Apr 28, 2020


Copy link

@headius headius commented Apr 27, 2020

This PR attempts to tweak all of our uses of setAccessible for accessing internal JDK classes to do the "right thing", be that using method handles, addOpens, or other tweaks to open up these classes if we legally can or fall back gracefully to degraded functionality without warnings.

The general strategy is described in the first commit to FilenoUtil, but I summarize it again here:

  • Reflecting against these classes should always succeed if they are present, so this does not change.
  • Instead of using the Field or Method directly, we unreflect it into a MethodHandle. This lets us know we have a fully-accessible way to deal with the reflected access, and since it raises errors instead of silently warns, we can fall back for inaccessible structures.
  • When we are dealing with an inaccessible structure, use Module.addOpens to make it really open, which then gives us a workable method handle and no warning output.

Much of this is to deal with the unfortunate fact that our current check, Module.isOpen, will always return true for the unnamed module but still warn when you set it accessible. This "kinda-sorta-open" state is a bug in my opinion, but at least explicitly using addOpens promotes it to a fully-open state with no warnings.

I will endeavor to eliminate all warnings in all modes with as little fallback as necessary.

Because all modules will report isOpen => true when called from
the unnamed module, we need a different way of accessing them.
This logic makes two changes:

* Use Lookup to acquire a method handle rather than using the
  reflected object. This allows to be sure we now have a fully
  accessible mechanism for accessing the related field or method,
  without triggering any warnings. Lookup.unreflect* will raise
  an exception if the reflected object is not already accessible,
  rather than a warning we cannot detect.
* Use addOpens to attempt to *really* make the pseudo-open
  packages be open. This allows us to proceed to setAccessible
  knowing the package has moved from "kinda-sorta-open" to really

The fallbacks here occur in the following cases:

* If we cannot acquire a reference to a non-standard JDK class,
  like those in This should not ever fail; reflecting
  against these classes is always permitted.
* If we cannot acquire the method or field in question because it
  is not there. Again this should not fail under normal
* If we cannot unreflect a handle and cannot addOpens for the
  package in question.
@headius headius added this to the JRuby milestone Apr 27, 2020
@headius headius linked an issue Apr 28, 2020 that may be closed by this pull request
@headius headius merged commit 17d7e92 into jruby:master Apr 28, 2020
1 check failed
@headius headius deleted the illegal_access_fixes branch Apr 28, 2020
Copy link

kares commented May 10, 2020

👍 very interesting, falsely assumed the runtime addOpens would still print warnings ...

Copy link
Member Author

headius commented May 11, 2020

@kares It's pretty silly if you ask me. If you're running from the classpath (not the module path) these core JDK classes will claim to be open, but then actually warn because you're not supposed to access them. Adding via addOpens in this case is safe, because they're "kinda sorta open", and then the warnings won't happen. I'm frustrated by this silly choice, but at least we have a path forward.

Copy link
Member Author

headius commented Jun 17, 2020

Retarget to 9.2.12 since this is largely zero-cost and cleans up most remaining warnings in JRuby proper. See #6285.

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

Successfully merging this pull request may close these issues.

"Illegal reflective access operation"
2 participants