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

Support JDK 11 #259

Open
msridhar opened this issue Dec 2, 2018 · 11 comments
Open

Support JDK 11 #259

msridhar opened this issue Dec 2, 2018 · 11 comments

Comments

@msridhar
Copy link
Collaborator

msridhar commented Dec 2, 2018

WIP at https://github.com/uber/NullAway/tree/ms/jdk-11. We should get the core NullAway functionality working first. Some notes:

  • I don't think coveralls works yet
  • Android-related sample app building will be a pain
  • This JarInfer-related code won't work on JDK 11; see here. We should probably disable this handler by default and enable via a flag anyway (along with a couple other rarely-used handlers).
@msridhar msridhar self-assigned this Dec 2, 2018
@msridhar
Copy link
Collaborator Author

msridhar commented Dec 2, 2018

/cc @lazaroclapp @subarnob

@msridhar
Copy link
Collaborator Author

msridhar commented Dec 3, 2018

Another issue here: WALA seems to fail on JDK 11:

> Task :jar-infer:test-java-lib-jarinfer:jar FAILED
Exception in thread "main" java.lang.IllegalStateException: could not find boot classpath
        at com.ibm.wala.util.PlatformUtil.getBootClassPathJars(PlatformUtil.java:62)
        at com.ibm.wala.properties.WalaProperties.getJ2SEJarFiles(WalaProperties.java:64)
        at com.ibm.wala.util.config.AnalysisScopeReader.processScopeDefLine(AnalysisScopeReader.java:177)
        at com.ibm.wala.util.config.AnalysisScopeReader.read(AnalysisScopeReader.java:80)
        at com.ibm.wala.util.config.AnalysisScopeReader.readJavaScope(AnalysisScopeReader.java:53)
        at com.ibm.wala.util.config.AnalysisScopeReader.makePrimordialScope(AnalysisScopeReader.java:192)
        at com.uber.nullaway.jarinfer.DefinitelyDerefedParamsDriver.run(DefinitelyDerefedParamsDriver.java:160)
        at com.uber.nullaway.jarinfer.JarInfer.main(JarInfer.java:91)

msridhar added a commit that referenced this issue Dec 3, 2018
This is mostly so that we can have a somewhat-functional NullAway on JDK 11 sooner (see #259).  But also, removing a handler may give a small performance boost.  With this change, you need to pass `-XepOpt:NullAway:JarInferEnabled=true` to enable JarInfer.

Also, we separate out the JarInfer tests into their own module, so that there is no direct dependence from the `nullaway` module to the `jarinfer` libraries.  This will enable us to get the `:nullaway:test` target passing on JDK 11 sooner.
msridhar added a commit that referenced this issue Dec 4, 2018
For now, only the `:nullaway:test` target is passing on CI.  We need to address issues outlined in #259 to get everything working, and it may be a while before the sample Android app can easily be built.  That said, I think this initial level of support is useful and worth landing.
@msridhar
Copy link
Collaborator Author

Did some digging here around what it would take to support running JarInfer on JDK 11. There are two issues:

  1. The WALA issue from this comment is due to the fact that since JDK 9 the standard libraries are no longer distributed as jar files. We can work around this by providing our own standard library jar (basically a stub jar for analysis); shouldn't be too hard.

  2. The code using URLClassLoader is trickier to work around. Here I think the best solution is to change JarInfer to rewrite library jars with nullability annotations, rather than generating stub files. A shorter term solution would be to pass in the stubs as a separate argument to NullAway, rather than just including in the class path. In either case, this will be more work to fix than 1.

FYI @lazaroclapp @kageiit

@kageiit
Copy link
Contributor

kageiit commented Jan 3, 2019

I think code using UrlClassLoader is fine as we can just ignore classloaders which do not typecast safely. That said, the eventual plan is to rewrite jarinfer to embed stubs in jars along with bytecode instead of producing stub jars. We can probably have a switch to jarinfer to support both output jar/aar modes.

@msridhar
Copy link
Collaborator Author

msridhar commented Jan 3, 2019

@lazaroclapp thoughts here?

@lazaroclapp
Copy link
Collaborator

Basically, I agree that the long term solution is bytecode rewriting to add the annotations, which further buys us Kotlin support automatically.

As for the short term solutions, I see two options: add a check around that cast that will make that handler basically a no-op for Java 9 and above, or make code specific to Java 9+ to get the url out of a BuiltinClassLoader. The later seems doable by using reflection to access private fields (e.g. LoadedModule.codeSourceURL), but might need to be specialized per Java version supported 🤷‍♂️ . I'd be fine with either as a stopgap solution and can make the either change.

@msridhar
Copy link
Collaborator Author

msridhar commented Jan 4, 2019

Ok that makes sense. It might be better to crash with a relevant message rather than no-op. The JarInfer handler is off by default now, so users would only see the crash if they opted into JarInfer. Even better would be the workaround via reflection. I’m fine with only supporting JDK 11 but not intermediate releases since it’s a temporary fix.

@ctcpip
Copy link

ctcpip commented Jun 18, 2019

current status?

@msridhar
Copy link
Collaborator Author

I think core NullAway should work fine on JDK 11. It's lesser-used features like JarInfer that are still being worked on. @lazaroclapp any other details?

@msridhar msridhar removed their assignment Aug 16, 2019
@pwittchen
Copy link

Any news related with Java 11 support? Recently, I tried to integrate NullAway with a small Java 11 project built with Gradle and it didn't work. I received errors during the build, but I don't have logs now. I can try to reproduce this situation later.

@lazaroclapp
Copy link
Collaborator

Basic NullAway on Java 11 should work, we'd be happy to look at an issue here with a repo case.

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

5 participants