Crash during compilation on JDK 8 when using JSpecify 0.3.0-alpha-1

[msridhar]

NullAway branch is here:

https://github.com/uber/NullAway/tree/update-jspecify

This is the crash (see here), which comes when running ./gradlew :test-java-lib:compileJava on a JDK 8 JVM:

warning: unknown enum constant java.lang.annotation.ElementType.MODULE
warning: unknown enum constant java.lang.annotation.ElementType.MODULE
An exception has occurred in the compiler (1.8.0_345). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program and the following diagnostic in your report. Thank you.
java.lang.AssertionError: annotationType(): unrecognized Attribute name MODULE (class com.sun.tools.javac.util.UnsharedNameTable$NameImpl)
        at com.sun.tools.javac.util.Assert.error(Assert.java:133)
        at com.sun.tools.javac.code.TypeAnnotations.annotationType(TypeAnnotations.java:231)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.separateAnnotationsKinds(TypeAnnotations.java:294)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.visitMethodDef(TypeAnnotations.java:1066)
        at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:778)
        at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.scan(TypeAnnotations.java:275)
        at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:57)

From #34 I can't tell if this was intended or not. Do we not support using JSpecify annotations when building on JDK 8? Building on JDK 8 is not a requirement for NullAway (we're planning to remove JDK 8 support soon); I am just wondering if this was intentional.

[msridhar]

FWIW a possible difference between this and previous test cases is that the code being compiled uses @NullMarked and @NullUnmarked on methods:

https://github.com/uber/NullAway/blob/e1ea7f62e8c82fe578e1c18446827849dad76c39/test-java-lib/src/main/java/com/example/jspecify/unannotatedpackage/Methods.java

[msridhar]

After further thought I realized we probably want to have NullAway support JDK 8 for some time longer, to support more open-source projects that haven't updated. So having JSpecify support building with JDK 8 would be helpful to us.

[cpovirk]

Thanks very much for testing this and for the repro instructions.

This is new: We'd known that building with Java 8 could produce a warning (which becomes an error under -Werror), and we'd known that reflection under Java 8 could lead to exceptions at runtime. But I don't think we'd known of build-time crashes.

Brain dump follows.

Testing notes

It does appear that the code compiles fine if the usages of @NullMarked and @NullUnmarked appear only on classes, at least in the testing I've done so far.

(I would say "And the code compiles fine with the 0.2.0 release," but: In order to test with 0.2.0, I have to change the test code such that... the usages of @NullMarked and @NullUnmarked appear only on classes :) And we already know that that's enough to eliminate the error.)

One annotation of either type on a method is enough to produce the error. That's true even if the annotation is redundant on account of a matching annotation on the enclosing class.

Removing MODULE from the @Target of the all annotations that are used in the file being compiled eliminates the problem. Removing it from any annotations that aren't used doesn't, though it eliminates the relevant "unknown enum constant ElementType.MODULE" warning. (I didn't test this all exhaustively, but that appears to be what's going on.) Moving MODULE to come after PACKAGE in NullUnmarked.java (as it already was in NullMarked.java) does not help.

It also appears that the code compiles fine if I build with JDK11 but with --release 8 (options.compilerArgs += ["--release", "8"] in test-java-lib/build.gradle)... well, aside from the usual "unknown enum constant" warning, the one that becomes an error under -Werror until I turn that off in build.gradle.

Finally, it appears that NullAway is not necessary for the crash: A build of your Methods.java with plain javac8 likewise crashes.

Why this is happening

Part of the story here is almost certainly #245: We used to have the MODULE target on @NullMarked only in a Java-9+ copy in the jar. But that turned out to not always let people actually use the annotation on a module. So 0.3.0-alpha-1 switched us to having only a single copy of @NullMaked, which naturally includes the MODULE target.

It's still a bit of a mystery as to why this error appears only with certain test code. [edit: I believe msridhar suggested that it has something to do with which cases javac needs to ask "Is this a type-use annotation, an annotation applicable to this element, or both?] But it's not surprising in general that a "missing" internal class could cause problems for javac, and it's not surprising that that could cause crashes that appear and disappear for mysterious reasons.

How to avoid it

In an ideal world, everyone would build with a newer JDK, even if they target Java 8, typically by using --release. (See, for example, #271 (comment).) But using --release can be kind of a pain. (See, for example, google/guava#3990.) And beyond that, upgrading javac is often easy but can sometimes require some work.

A theoretical way for a project to continue to be able to build with javac8 to use a @ModulelessNullMarked alias that @Implies(NullMarked.class) but does not include MODULE in its @Target. (My naive reading of javac is that it might be OK for dependencies of that project to continue using plain @NullMarked: The specific crash here is in code that I don't think runs on binary deps, only on source code. However, it's possible that there's similar code that does run when looking at binary deps. We'd have to test. If binary deps produce a similar issue, then all the JSpecify-using libraries that that project depends on would also need to use a @ModulelessNullMarked alias. And "unfortunately," those libraries' developers wouldn't notice that they need to use the special annotation that unless they built with javac8 themselves. If we did end up wanting to go this direction, then we could consider providing a standard "@ModulelessNullMarked" annotation.) Note that this alias might also help with runtime reflection, but I wouldn't say that confidently until we'd actually implemented it :) For that matter, I'm not even sure if the build-time crash would necessarily go away. Ah, we could probably make most (all?) of the problems go away if tools had a special case for this standard "@ModulelessNullMarked" annotation (as opposed to relying on normal @Implies logic). But that's a little gross.

Another spin on that possibility is for the main @NullMarked to be the one to not contain MODULE in its @Target. We'd then probably want to supply a standard alias that does target modules. (Or maybe the aliasing would run in the reverse direction, as in my previous option: Then the canonical annotation would be @NullMarked.NullMarkedModule, and @NullMarked would imply it. That way, we avoid having an annotation with @Target(MODULE) imply one without @Target(MODULE), which I think we've been assuming would be unrecognized (1, 2)? But maybe giving @NullMarked.NullMarkedModule @Target(MODULE) reintroduces many of the problems I was trying to avoid?) The hope then would be that eventually we could add MODULE to the @Target of @NullMarked once javac8 use is really dead. This has the advantage that the negative experience is mostly limited to people who try to put @NullMarked on a module, get a clear error, and hopefully read enough of our docs to see that @NullMarked.NullMarkedModule is available as a workaround.

A different approach is to omit @Target entirely. But that has the obvious downsides. Plus, it sounds like it still doesn't actually work how it's intended to: #34 (comment).

This continues to need more thought

Yuck. But thanks again.

[cpovirk]

Oh, this is a good place to recall that MODULE makes annotations usable elsewhere under some versions of javac. That is a small and gross but practical reason to avoid it on the main annotation. It would also be a reason for tools to more carefully validate the locations on which annotations appear... which in turn could make the no-@Target approach more appealing, if only that approach, you know, worked :)

[msridhar]

Thanks so much for investigating, @cpovirk. I have one question. Based on the stack trace and error message, I don't think this crash in JDK 8 javac is intended behavior. Also it looks like one can at least still backport fixes to JDK 8 for future patch releases. Here is the line of code throwing the assertion error:

https://github.com/openjdk/jdk8u-dev/blob/d285382278fcb76ceddfb61cf8bb047ff269de44/langtools/src/share/classes/com/sun/tools/javac/code/TypeAnnotations.java#L231

I wonder if we could submit a PR that changes this assertion to something less crash-y? Like maybe emit another warning instead for an unknown enum constant? Then at least this issue could be fixed in a future JDK 8 release, which gives us more options (like leaving things the way they are and telling users to update their JDK 8 release). Of course this doesn't help with working around the current crash.

It looks like someone submitted an issue on this before:

https://bugs.openjdk.org/browse/JDK-8279557

They didn't have a reproducer so it got closed, but we have one now. If we think it's worthwhile, I can try to re-open that issue or submit a new one, to see if we get feedback on the possibility of a change. Let me know if we think that's worth a try.

[cpovirk]

Thank you! That is a better thought than any of the thoughts in my post :)

It does seem likely that upstream would like the compiler to not crash, regardless of how strange the code we feed it is.

It's possible that they could respond by changing the crash into a more normal but still fatal error, or it's possible that they don't deem this fix worth backporting. But maybe they'll see the value in making it easier for libraries to use the MODULE target. (As noted in #245, we tried solving this problem with multi-release jars instead. But it's not clear if multi-release jars are supposed to solve this problem, and regardless, they don't under some major tools today.)

Reproducing does look easy. If you're up for taking things to upstream, that would be terrific.

$ tail -n +1 *.java 
==> A.java <==
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.MODULE;

import java.lang.annotation.Target;

@Target({METHOD, MODULE})
@interface A {}

==> B.java <==
class B {
  @A void b() {}
}

$ $HOME/openjdk-11.0.16/bin/javac -source 8 -target 8 -sourcepath doesnotexist A.java && $HOME/openjdk-8u342/bin/javac -cp . -sourcepath doesnotexist B.java
warning: [options] bootstrap class path not set in conjunction with -source 8
1 warning
warning: unknown enum constant ElementType.MODULE
An exception has occurred in the compiler (1.8.0_342). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program and the following diagnostic in your report. Thank you.
java.lang.AssertionError: annotationType(): unrecognized Attribute name MODULE (class com.sun.tools.javac.util.SharedNameTable$NameImpl)
        at com.sun.tools.javac.util.Assert.error(Assert.java:133)
        at com.sun.tools.javac.code.TypeAnnotations.annotationType(TypeAnnotations.java:231)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.separateAnnotationsKinds(TypeAnnotations.java:294)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.visitMethodDef(TypeAnnotations.java:1066)
        at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:778)
        at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.scan(TypeAnnotations.java:275)
        at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:57)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.visitClassDef(TypeAnnotations.java:1042)
        at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:693)
        at com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.scan(TypeAnnotations.java:275)
        at com.sun.tools.javac.code.TypeAnnotations$1.run(TypeAnnotations.java:127)
        at com.sun.tools.javac.comp.Annotate.flush(Annotate.java:152)
        at com.sun.tools.javac.comp.Annotate.enterDone(Annotate.java:129)
        at com.sun.tools.javac.comp.Enter.complete(Enter.java:512)
        at com.sun.tools.javac.comp.Enter.main(Enter.java:471)
        at com.sun.tools.javac.main.JavaCompiler.enterTrees(JavaCompiler.java:982)
        at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:857)
        at com.sun.tools.javac.main.Main.compile(Main.java:523)
        at com.sun.tools.javac.main.Main.compile(Main.java:381)
        at com.sun.tools.javac.main.Main.compile(Main.java:370)
        at com.sun.tools.javac.main.Main.compile(Main.java:361)
        at com.sun.tools.javac.Main.compile(Main.java:56)
        at com.sun.tools.javac.Main.main(Main.java:42)

[msridhar]

I will report this upstream and then report back. Let's hope for a positive response :-)

[msridhar]

The bug report got created:

https://bugs.openjdk.org/browse/JDK-8295314

However, it got erroneously closed as "Cannot reproduce" because the repro instructions were not followed correctly (JDK 17 was used to compile B.java instead of JDK 8).

I will try to use a link they provided to "submit additional info" to clarify. But @cpovirk @kevinb9n if we know someone with a JDK bug system account who could comment directly, that might be more effective.

[cpovirk]

Thanks. I think @eamonnmcmanus said that he had The Power? Éamonn, are you up for it?

[eamonnmcmanus]

I do indeed have an openjdk.org account. Let me know if "submit additional info" doesn't produce the desired results and I can comment on the bug.

[msridhar]

Update: it looks like "submit additional info" worked as the bug has been re-opened. Not sure who would look at this / prioritize it. I'll wait a bit and see what happens.

[msridhar]

For those familiar with the JDK bug reporting process: is there any way to know if JDK-8295314 has or will be triaged? I am wondering how we can get the right eyes on it. I think we have a good case that it's worth fixing, but probably we need someone involved with JDK 8 maintenance to see it for it to get noticed, and I'm not sure if that will happen automatically.

We're about to add a one-off @NullUnmarked annotation to a project because of this issue 😐

[msridhar]

Update: thanks to help from @cushon and @cpovirk, we discovered this bug still exists in the latest version of javac if you create a .class file for an annotation with an invalid ElementType in its @Target meta-annotation. This has been filed as a new bug against JDK 19:

https://bugs.openjdk.org/browse/JDK-8296010

Our hope is that the new bug gets fixed in the latest Java version, and that eventually we can backport the fix all the way to JDK 8.

[cpovirk]

Trying to summarize where we are, which we discussed some in today's meeting:

• We'll of course want to document this incompatibility, along with the workaround of using a newer JDK to build with --release 8 or similar (though that's not always trivial).
  • NEW: Maybe we can even recommend continuing to build with JDK8 but switching to use the JDK8-compatible javac9 codebase, as we do in Guava for other reasons?
• Even once the incompatibility is fixed, we'll want to document that people need to upgrade to <whichever JDK8 patch release> to pick up the fix, and we'll want to document that -Werror will still be a no-no unless you upgrade to JDK9+.
• We should write something about our the versions of JDK and Android that we support more generally (build and runtime, with whatever caveats we need to give).
• There aren't any wildly appealing workarounds available on the JSpecify side:
  • A no-@Target annotation is supposed to be usable on MODULE, but it currently isn't. (And of course it is also usable in other locations that we don't want it used on, even more so than MODULE itself sometimes accidentally permits.)
  • Multi-release jars don't currently help.
  • We could theoretically release two separate "versions" of 0.3.0, two separate "versions" of 1.0.0, etc. (continuing until we're willing to drop JDK8 support), similar to how Guava releases -android and -jre "versions." One "version" would give the annotations a MODULE target; the other would not. No one loves that approach, but we have evidence that it """works""" for this kind of thing.
  • For each affected annotation, we could provide two otherwise identical annotations with different @Target values (e.g., @NullMarked and @ModulelessNullMarked. That would be a permanent wart, but it seems like it would also work.
  • [later update] We can probably at least omit MODULE from @NullUnmarked to reduce the scope of the problem.

[msridhar]

• NEW: Maybe we can even recommend continuing to build with JDK8 but switching to use the JDK8-compatible javac9 codebase, as we do in Guava for other reasons?

I really like this as a temporary workaround! It's probably the least disruptive option for those who still need to build using JDK8. I just used it to unblock uber/NullAway#673 and get the build green on JDK 8 there. And Error Prone's javac9 is battle tested and reliable. Hopefully in the medium term, we will get a fix backported to JDK 8 so even this workaround is not needed. And then the only remaining issue is not being able to compile with -Werror on JDK 8, which I think we'll just have to live with.

[kevinb9n]

• We'll of course want to document

This oughtta be mediocre enough to snipe you into editing it:
../wiki/version-compatibility

[msridhar]

It looks like the MODULE target can also cause problems with javadoc:

https://github.com/wala/WALA/actions/runs/3634061250/jobs/6131766621#step:5:253

I see #351 actually removed @Target(MODULE) from @NullUnmarked. @cpovirk any chance we could get an 0.3.0-alpha-3 release that includes this change? It would save us from having to make a one-off @NullUnmarked annotation in WALA, where some configs still build on JDK 8. I don't know of a simple Error-Prone-javac-based workaround for the javadoc crash.

[cpovirk]

Oh, thanks for that report. It would be nice if we had a workaround like you describe, but I'm not excited about trying to figure one out right now... :)

As it happens, I did release 0.3.0-alpha-3 (because I missed a problem with our module-info). And while I forgot to mention it in the initial release notes, it does additionally contain the change you're looking for. I eventually noticed that and updated https://github.com/jspecify/jspecify/releases/tag/v0.3.0-alpha-3 to mention it.

[msridhar]

Thanks a lot @cpovirk I think the alpha 3 release should fix it!

[cpovirk]

Seen in the wild: https://stackoverflow.com/a/76254522/28465

I will continue feeling bad about this and eventually do something about it.

[msridhar]

Seen in the wild: https://stackoverflow.com/a/76254522/28465

If we ever get any further info, I'd be curious to know the use case for writing @NullMarked on a method (though of course it'd be better if we could just not cause javac to crash).

[SimY4]

Hello, everyone. Just stumbled upon this issue myself. Migrating my codebase away from JSR305 to jspecify and see that compiler raises a warning.

Not sure if that was discussed earlier, but have you considered using JDK 9 multi-release jars support? https://openjdk.org/jeps/238 TL;DR you can ship two versions of a class in a single jar. One for everything before Java 9 and then for every release of Java you can have it's own class file.

Essentially, for every release of jspecify you can ship two classfiles

1. One with Java 8 enum options for @Target
2. One with Java 9 enum options for @Target

Should mitigate the warning under Java 8 target and still support newer targets (even beyond Java 9).

[cpovirk]

Sorry, I lost track of this thread... among others recently... :(

We had tried a multi-release jar for this purpose a couple years ago, and it turns out that it doesn't achieve that purpose. The idea of a multi-release jar is that the API of the classes is supposed to match across versions. The definition of "API" was left a little fuzzy AFAICT, but the idea seems to (mostly?) be that a compiler should be able to look at whichever version it wants. That means that we can't count on it to look at the version that contains MODULE in its @Target, and indeed multiple tools currently don't do that.

Or maybe the tools are buggy. Even if so, we probably don't want to take an approach that will cause problems for those tools in order to work around a problem with a tool (javac) that is probably at least easier to migrate off of (by upgrading to javac 9+, even while still targeting older versions of Java).

[cpovirk]

(But thank you for raising that suggestion! I wish that it had worked out....)