unknown enum constant ElementType.MODULE with Truth 1.4.3

[ejona86]

Starting with Truth 1.4.3, compiling on Java 11+ with --release 8 produces a warning:

> Task :grpc-interop-testing:compileJava FAILED
warning: unknown enum constant ElementType.MODULE
error: warnings found and -Werror specified
1 error
1 warning

That warning message is maddening, as it tells you no context to track down the source. It's also maddening that it works on Java 8 but not on Java 11+. I am assuming the problem here is triggered by JSpecify's @NullMarked because its the only reference to ElementType.MODULE I've found to this point. And it appears you've already suffered from it in jspecify/jspecify#302

I get the feeling that the only possible solution, other than "don't use MODULE as we wait for Java 8 to die", is a multi-release jar and tools get updated to support them. But I do wonder if Java 8's death beats that to the finish line (not that anyone can predict Java 8's death).

I don't know what you can do about this. But I'm filing this issue to say "this bit me" and for the moment I'll probably use 1.4.2 in gRPC instead of getting on 1.4.3. I was upgrading from 1.1.5 so that's still a net win for a while.

[cpovirk]

Thanks very much, as always. I had lost sight of this somewhere along the way, and I'm glad that the Truth change shook it out before Guava (and sorry that you yet again drew the short straw!).

It looks like I've sometimes been under the impression that javac 11 with --release 8 was safe (unlike javac 8) and sometimes been under the impression that it's not. I guess I should just run it myself soon to be 100% sure that I know what Gradle and friends are up to. (It's especially sad that, in order to bump --release high enough to pick up MODULE, you need to pick up the annoying ByteBuffer API changes.)

RE: multi-release jars: They did not go well for this for reasons that I think arguably make sense. But basically, what a mess.

If you remove -Werror, do you see any other problems? (I might be able to check this myself when I get a chance.)

I wonder if things would be better if we used the annotation at the class level instead of the package level. [edit: Err, did I get that backward?] I know that we saw some weird stuff at the method level or something....

I'll take parts of this to various JSpecify issues that I've just linked above.

[ejona86]

I saw this with Java 11 and Java 17, via Github Actions which we run with -Werror. You can see CI runs in the commit before "Slightly downgrade Truth" at grpc/grpc-java#11365 (the red X).

I think ByteBuffer we've dealt with by pre-casting to Buffer before the method call. Annoying, but not enough call sites to do something fancy. I'm very fuzzy on why we hit that, because our releases using Java 11 javac should have been using --release 8... maybe we weren't then. 🤷

For our release binaries, we use Java 11 on Kokoro. There's Android and such that needs the newer version. But we have a separate CI that runs Java 8/11/17 on Github Actions (minus the problematic parts of our build), mostly to run the tests with those versions.

If you remove -Werror, do you see any other problems?

The build worked fine without -Werror, at least on Java 11. We don't run any fancy tool that would notice the JSpecify annotations though... except ErrorProne 2.28.0 if it's doing that now.

Turning off -Werror for these some of these builds is a potential option for me. It'd also let me run Java 21 as the only reason I don't is we need to go through the warnings. But we also want to keep things warning-clean.

I wonder if things would be better if we used the annotation at the class level instead of the package level

I wouldn't expect so. I think the problem is the reference in @NullMarked itself surprises the compiler, even if it turns out not to be needed.

[cpovirk]

• I've added this to the release notes. We'll also document it for JSpecify, and we'll mention it in the release notes for Guava when we (soon?) adopt @NullMarked there.
• I haven't yet succeeded in reproducing this. I've cloned git@github.com:grpc/grpc-java.git, set up ( echo skipAndroid=true; echo skipCodegen=true ) > gradle.properties (maybe that's too much?), and tried both JAVA_HOME=$HOME/jdk-17.0.2 ./gradlew :grpc-gcp-csm-observability:assemble and JAVA_HOME=$HOME/jdk-17.0.2 ./gradlew build without reproducing the error. Should I be suspicious that Java 8 is sneaking into CI somehow, perhaps here? (I have no /usr/libexec/java_home locally, so that check wouldn't trigger for me.) I'll try again with JAVA_HOME=$HOME/openjdk-8u342 for comparison, but...
• I had been wondering if this was a typo, but I see from the earlier results that things really do work with Java 8 but not with Java 11 and 17. I would have assumed that Java 8 would the one that would fail. I wonder if some problematic step becomes a no-op under builds that are "fully" Java 8? Hmm.

[cpovirk]

It looks like I might just need -PfailOnWarnings=true. That gets me a failure with JAVA_HOME=$HOME/jdk-17.0.2. (I played with some other changes, too, so I'll try to revert all that to make sure that none of it was relevant.)

[cpovirk]

OK, I can reproduce it like this:

git clone git@github.com:grpc/grpc-java.git && cd grpc-java
git checkout 0ff3f8e4ac4c265e91b4a0379a32cf25a0a2b2f7
sed -i -e 's/com.google.truth:truth:1.1.5/com.google.truth:truth:1.4.3/' gradle/libs.versions.toml
( echo skipAndroid=true; echo skipCodegen=true ) > gradle.properties
JAVA_HOME=$HOME/jdk-17.0.2 ./gradlew clean :grpc-interop-testing:compileJava -PfailOnWarnings=true

[ejona86]

I'm sorry you spent the time on that. I could have gotten you a recipe. Glad you figured it out, though. (Note, you can also do -PskipAndroid=true -PskipCodegen=true; it doesn't have to be in gradle.properties, but it is also equally good either way)

[ejona86]

Should I be suspicious that Java 8 is sneaking into CI somehow, perhaps here?

That is interesting. From what I can tell, that seems to be an OS X-ism. Since we regularly build with other Java versions now, I guess I'll remove it. We don't want unix.sh to be choosing the version anyway; we want that set up in the environment beforehand and it just do the build. And I take pleasure in removing JAVA_HOME usages.

[cpovirk]

np, that has not been the painful part :)

I didn't dump one subsequent finding from earlier: I can disable the error with -PerrorProne=false. This fits with something I think I remember before. It would also make sense for the error to go away with JDK8, since Error Prone no longer supports that.

And then, much later (as in ~2 minutes ago): I've traced the problem to Error Prone's isInherited check. If I change that to return false, then the problem goes away. That dates back to cl/125219502, when the implementation was rather different (using enterClass). Maybe we can stop doing some of that?

[cpovirk]

It looks like the removing the call to complete() doesn't help.

So I am currently very tempted by (an optimized equivalent to) if (annotationName.contentEquals("org.jspecify.annotations.NullMarked")) return false....

[ejona86]

It would also make sense for the error to go away with JDK8, since Error Prone no longer supports that.

Oh, this is related to ErrorProne. In that case, you need to know we use two different ErrorProne versions. (Wow, you've got a nice perfect storm of your projects intersecting here. "So this is what bleeding edge feels like." --You, every day 2012-2024)

Errorprone 2.10.0 on Java 8.
https://github.com/grpc/grpc-java/blob/0ff3f8e4ac4c265e91b4a0379a32cf25a0a2b2f7/gradle/libs.versions.toml#L102

Errorprone 2.23.0 on Java 11+ (I have a local commit to upgrade to 2.28.0; I'm waiting for the first dependency PR to go in).
https://github.com/grpc/grpc-java/blob/0ff3f8e4ac4c265e91b4a0379a32cf25a0a2b2f7/gradle/libs.versions.toml#L30

[cpovirk]

Thanks, so quite presumably there was an EP check added/changed between those two versions that now happens to exercise this code path.

As best I can tell, we can't perform isInherited without calling at least getRawAttributes, which triggers "completion" in the case of a ClassSymbol (which I believe is what we're looking at here, since that's the scope at which we're using @NullMarked in Truth and that's where it makes sense to look for @Inherited). I suspect that the completion step is what leads to the warning that we're seeing. (It does not appear to produce a CompletionFailure, at least not what that propagates up to us.) I don't see an obvious way to prevent that warning from happening, aside from (a) at least short-circuiting for well-known non-@Inherited classes and (b) maybe messing with Completer instances, which sounds like a bad idea anyway.

(What I'm going to say in this parenthetical isn't particularly relevant to the main project here, but: I think that this check is redundant with this check, and I may try to remove the former. I also wonder if there's any bizarre situation in which a class from one module could fail to complete, leading us to return a class from another module. But hopefully that will never come up.)

(I also note that -XDdev is not helpful in producing debugging info for this kind of error. (That may be related to how we don't see a CompletionFailure.) It's also possible that Error Prone prevents -XDdev from producing output in some cases by swallowing CompletionFailure, but I haven't seen evidence of that. If we someday find that it is the case, we can check Options.instance(context).isSet("dev") ourselves and dump things to Log.instance(context).)

I see a few ways to make things at least somewhat better:

• Encourage users to drop -Werror, though I grant that that's sad. (Incidentally, prepare yourself for at least the few this-escape warnings that I encountered during my testing with JDK 22.) I believe that Bazel offers more fine-grained filtering of warnings and/or promotion of warnings to errors, so if your hybrid setup ever shifts further to Bazel, you may have more options at that point.
• Annotate Truth at the package level instead of the module level. I confirmed that this actually eliminates the problem. Now, we haven't annotated the Java 8 Subject classes yet, so simply slapping @NullMarked on the whole package wouldn't be accurate. But those remaining classes shouldn't be hard to annotate. And it looks like we could also leave any classes that we want as @NullUnmarked, which sidesteps this problem because we removed MODULE from its @Target to reduce chances of an even worse problem. That latter approach may be better for future work on Guava [edit: but we have seen a variety of problems with package-info annotations...], since I'm not enthusiastic about annotating some of its remaining classes, like LocalCache. (Now, package-info technically applies to the tests, too. The J2KT folks have annotated some of our tests but not all. But bad test annotations should mostly not matter except maybe in IntelliJ and some possible future Error Prone checks.)
• Hard-code into Error Prone that NullMarked is not @Inherited. (Analogous special handling for NullUnmarked shouldn't be necessary, since, as noted above, we removed MODULE from its @Target.)

I will probably attempt all 3.

[cpovirk]

@cushon, how many layers of paper bags would you need to put over your head before you could consider approving a CL that modifies ASTHelpers.isInherited to include a special case for if (annotationName.equals(NULL_MARKED)) return false;? It would save users from the mysterious warning reported in this bug by saving us from having to perform completion on the ClassSymbol for NullMarked, whose @Target(MODULE) causes a warning with --release 8 even under new versions of javac.

[cpovirk]

...and I am belatedly having the obvious realization: The EP checks that are causing this problem must be the ones I wrote that... look for @NullMarked. So that suggests yet another approach to solving this: Those checks can probably look for it directly instead of using the EP helper methods.

(More generally, I'll be there are other EP checks that could do the same, though there wouldn't be any need for that, since they don't have problems with MODULE. I suppose it's possible that such a change could help performance....)

[ejona86]

Thanks, so quite presumably there was an EP check added/changed between those two versions that now happens to exercise this code path.

It started on Errorprone 2.22.0. No MODULE warning for 2.21.1 (although it has other warnings. 2.21.0 was a clean build with gRPC.)

[cpovirk]

Thanks. I can reproduce the appearance at 2.22.0. I have also confirmed that the problem comes from NullArgumentForNonNullParameter (and so it goes away with -Xep:NullArgumentForNonNullParameter:OFF).

So: The trigger for the problem was the change in which JSpecify package that that check recognizes. (It looks like that change should have been part of google/error-prone@2fea215 but that Copybara picked up that part of the change (because it was the result of a change to a Copybara config) separately from the rest of the change? This sounds a bit like internal b/266369385, but I'm not going to worry about it at the moment.)

[cpovirk]

I've released 1.4.4 with the workaround. The next Error Prone release will also include changes of its own to avoid triggering the problem.

There may still be warnings for people who combine just the right set of other circumstances (using @NullMarked at the method level in their own code with --release 8), but I think the remaining situations that we know about are rare, and Truth should at least not be a contributor any longer.

Thanks again for reporting this, especially so that we could sort things out before the upcoming JSpecify 1.0.0 release!