error_prone_core 2.0.6 adds Objects.requireNonNull() to source (which can crash Android)

[dlew]

I've been using the error-prone compiler for my Android application. java.util.Objects was only added recently to Android (API 19). The most recent version of error_prone_core (2.0.6) can add Objects.requireNonNull() to the source, which causes the Android app to crash if run on an older device.

I haven't been able to track down exactly how this happens; I'm not super familiar with error-prone's internals.

I've been using error-prone via the gradle-errorprone-plugin for Android builds. By default it pulls in the latest error_prone_core for use. As a workaround you can force it to use 2.0.5.

---

As an aside, the stack traces that result are quite mystifying since the source will contain no references to java.util.Objects. For example:

Fatal Exception: java.lang.NoClassDefFoundError: java.util.Objects
       at com.trello.syncadapter.StarredBoardsSync.execute(StarredBoardsSync.java:66)
       at com.trello.syncadapter.StarredBoardsSync.performSync(StarredBoardsSync.java:58)
       at com.trello.syncadapter.TSyncAdapter.onPerformSync(TSyncAdapter.java:49)
       at android.content.AbstractThreadedSyncAdapter$SyncThread.run(AbstractThreadedSyncAdapter.java:254)

...Where StarredBoardsSync didn't even import java.util.Objects in the original source.

[cushon]

Are you compiling with -source 7 -target 7?

Older versions of javac generate code for null-checks using Object.getClass, but the version used by Error Prone has started to use Objects.requireNonNull for target >= 7. (see JDK-8074306)

[dlew]

Technically Android runs on 6, but we're actually compiling java8 with retrolambda for backporting.

It looks like we'll have to stick with the older javac for the time being.

[cushon]

I think this issue prevents anyone from compiling against JDK9 and using retrolambda to run on JDK < 7, so I filed a bug: luontola/retrolambda#75

[dlew]

Cool, thanks.

I'm going to close this in the meantime, since it's not directly an error-prone issue.

[kageiit]

This happens even when not using retrolambda. Specifically this line

https://github.com/google/error-prone-javac/blob/a68a002c508b9b862ab4b966c70aad0a1686d328/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Target.java#L126

Will make it so that Objects.requireNonNull() is always used when source and target levels are set to 7. This is not a problem when using oracle's javac because it does not have this check. This makes it so that when using error prone with any android codeabse, if you do not use retrolambda, the app will always crash on api levels below 19.

cc @cushon Can we get this fixed in error-prone-javac or consider making this an option?

[kageiit]

This is happening on error prone 2.0.15

[cushon]

This happens even when not using retrolambda.

This is only an issue when not using retrolambda, right?

This is not a problem when using oracle's javac

This change was made to OpenJDK 9, it is not specific to Error Prone: http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/804b6a348702/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Target.java#l124

[kageiit]

Yes, but when using jdk8 from oracle, its not a problem. JDK 9 is not available from oracle yet. Yes it is an issue when not using retrolamdba. Which means we cannot use error prone on android without using retrolambda, which is not ideal

[kageiit]

We had to patch error-prone-javac-9-dev-r3297-1 with a locally compiled Target.class which set both invokeDynamic and hasObjects to false. Target level 7 does not necessarily mean those classes and capabilities will always be available when running on mobile.

I think it would be great if we could fix it in error-prone's javac fork so android projects can continue using error prone.

cc @tbroyer

[cushon]

I'll add a flag to turn it off. I don't want to disable it by default, because using requireNonNull fixes a bug. And do note that this is a stop-gap; if you want to use -source 7 -target 7 with JDK 9 someday you'll need a different fix.

Re: invokeDynamic, are you sure that's necessary?

[kageiit]

InvokeDynamic is not necessary

[justinmuller]

What version of error-prone-javac will this flag be added to?

Echoing what @kageitt mentioned, when we were investigating this issue we chose to patch on top of error-prone-javac-9-dev-r3297-1 as it picked up JDK-8059976 resolving corrupted zip file/bad class file errors we were having with ZipFileIndexCache.

[cushon]

google/error-prone-javac@9a2de2a

[kageiit]

@cushon great! thanks for adding the option. How can one use this option in the javac invocation?

[cushon]

-XDallowBetterNullChecks=false

[kageiit]

got it. thanks!

We tried out the snapshot and it works as intended

[cushon]

The fix has been released in 2.0.16.