False positive for ImmutableAnnotationChecker check on Java 11

[davido]

When switching to using Java 11, ImmutableAnnotationChecker check is starting to fail for AutoAnnotaion class. On Java 8 the check is passing, even when String[] attributes are used. Changing the attributes from String[] to ImmutableList[] as suggested in [2] doesn't fix it.

Reproducer is here: [1]:

  $ git clone https://github.com/davido/error_prone_auto_annotation_java11_breakage
  $ cd error_prone_auto_annotation_java11_breakage

  $ java -fullversion
  openjdk full version "11.0.4+11-suse-1.1-x8664"

  $ bazel version
  Build label: 1.0.0rc2

  $ bazel build cli
    $ bazel build cli
INFO: Writing tracer profile to '/home/davido/.cache/bazel/_bazel_davido/b78564ff4088c32bd1290c7e0b234fc2/command.profile.gz'
INFO: Analyzed target //:cli (1 packages loaded, 5 targets configured).
INFO: Found 1 target...
ERROR: /home/davido/projects/error_prone_auto_annotation_java11_breakage/BUILD:30:1: Building libcli.jar (1 source file) and running annotation processors (AutoAnnotationProcessor, AutoValueProcessor) failed (Exit 1)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:11: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'aliases' of type 'java.lang.String[]', arrays are mutable
  private final String[] aliases;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:25: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'depends' of type 'java.lang.String[]', arrays are mutable
  private final String[] depends;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:27: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'forbids' of type 'java.lang.String[]', arrays are mutable
  private final String[] forbids;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
Target //:cli failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.420s, Critical Path: 0.30s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

The generated code is here: [3]. As can be seen, the generated getter is:

@Override
  public String[] aliases() {
    return aliases.clone()
  ;
  }

When building on Java 8, all is fine, and that is why the problem wasn't detected for so long time:

  $ java -fullversion
  openjdk full version "1.8.0_222-b10"

  $ bazel build cli
INFO: Writing tracer profile to '/home/davido/.cache/bazel/_bazel_davido/b78564ff4088c32bd1290c7e0b234fc2/command.profile.gz'
INFO: Analyzed target //:cli (1 packages loaded, 199 targets configured).
INFO: Found 1 target...
Target //:cli up-to-date:
  bazel-bin/libcli.jar
INFO: Elapsed time: 2.894s, Critical Path: 2.69s
INFO: 2 processes: 1 linux-sandbox, 1 worker.
INFO: Build completed successfully, 3 total actions

I have also reported the problem on Bazel issue tracker, as it wasn't clear to me if this is a Bazel or EP issue: [4].

[1] https://github.com/davido/error_prone_auto_annotation_java11_breakage
[2] https://errorprone.info/bugpattern/ImmutableAnnotationChecker
[3] http://paste.openstack.org/show/775768
[4] bazelbuild/bazel#9378

[cushon]

Recent versions of Error Prone disable that check on AutoAnnotation-generated code: 11e28aa, and recent versions of AutoAnnotation should be using the correct @Generated annotation for Java > 9: google/auto@f04406c

Please re-open if this is still happening with recent versions of Error Prone / bazel AutoAnnotation.

[davido]

Recent versions of Error Prone disable that check on AutoAnnotation-generated code: 11e28aa,

Unfortunately this fix regresses for Java language source level 8, because it requires javax.annotation to be on the classpath, so that auto-value can generate the @generated annotation, so that EP can correctly detect that the code was in fact generated and skip the check.

This regression was also the root cause, why recent attempt to bump EP version in Bazel was reverted: [1],[2],[3] because both rules_closure and rules_kotlin were affected by very similar change: 5ad37b7.

This is still happening with EP 2.3.4 and Bazel@HEAD and custom release of java_tools that I conducted recently with this PR included:

WORKSPACE

diff --git a/WORKSPACE b/WORKSPACE
index 38cc59b..3341338 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1,4 +1,5 @@
 load("@bazel_tools//tools/build_defs/repo:maven_rules.bzl", "maven_jar")
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive", "http_file")
 
 maven_jar(
     name = "args4j",
@@ -25,3 +26,13 @@ maven_jar(
     artifact = "com.google.guava:guava:28.1-jre",
     sha1 = "b0e91dcb6a44ffb6221b5027e12a5cb34b841145",
 )
+
+http_archive(
+    name = "remote_java_tools_linux",
+    sha256 = "74d30ccf161c58bb69db9b2171c954a0563b2d1ff6f5831debbe71ced105c388",
+    urls = [
+        "https://github.com/davido/java_tools/releases/download/javac11-v11.0/java_tools_javac11_linux-v11.0.zip",
+    ],
+)

outcome:

$ bazel build --java_toolchain=@remote_java_tools_linux//:toolchain --host_java_toolchain=@remote_java_tools_linux//:toolchain cli
INFO: Analyzed target //:cli (21 packages loaded, 334 targets configured).
INFO: Found 1 target...
ERROR: /home/davido/projects/error_prone_auto_annotation_java11_breakage/BUILD:30:13: Building libcli.jar (1 source file) and running annotation processors (AutoAnnotationProcessor, AutoValueProcessor) failed (Exit 1)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:11: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'aliases' of type 'java.lang.String[]', arrays are mutable
  private final String[] aliases;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:25: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'depends' of type 'java.lang.String[]', arrays are mutable
  private final String[] depends;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:27: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'forbids' of type 'java.lang.String[]', arrays are mutable
  private final String[] forbids;
                         ^
    (see https://errorprone.info/bugpattern/ImmutableAnnotationChecker)
Target //:cli failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 26.378s, Critical Path: 5.35s
INFO: 5 processes: 5 linux-sandbox.
FAILED: Build did NOT complete successfully

Reason is, that javax.annotation is not on the classpath, "@generated" annotation is missing and the EP error reported:

import com.google.common.collect.ImmutableList;
import java.util.Arrays;
import org.kohsuke.args4j.Option;
import org.kohsuke.args4j.spi.OptionHandler;

// Generated by com.google.auto.value.processor.AutoAnnotationProcessor
final class AutoAnnotation_OptionUtil_newOption implements Option {

The fix is to add javax.annotation:javax.annotation-api:1.3.2 to the classpath:

WORKSPACE

+maven_jar(
+    name = "javax-annotation",
+    artifact = "javax.annotation:javax.annotation-api:1.3.2",
+    sha1 = "934c04d3cfef185a8008e7bf34331b79730a9d43",
+)

BUILD:

diff --git a/BUILD b/BUILD
index 0ff9ea5..e5b8f60 100644
--- a/BUILD
+++ b/BUILD
@@ -35,5 +35,6 @@ java_library(
         ":auto-value-annotations",
         "@args4j//jar",
         "@guava//jar",
+       "@javax-annotation//jar",
     ],
 )

Now, the annotation is generated:

import com.google.common.collect.ImmutableList;
import java.util.Arrays;
import javax.annotation.Generated;
import org.kohsuke.args4j.Option;
import org.kohsuke.args4j.spi.OptionHandler;

@Generated("com.google.auto.value.processor.AutoAnnotationProcessor")
final class AutoAnnotation_OptionUtil_newOption implements Option {

and the EP check is skipped:

$ bazel build --java_toolchain=@remote_java_tools_linux//:toolchain --host_java_toolchain=@remote_java_tools_linux//:toolchain cli
INFO: Analyzed target //:cli (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:cli up-to-date:
  bazel-bin/libcli.jar
INFO: Elapsed time: 0.083s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

[1] bazelbuild/bazel#9286
[2] bazelbuild/bazel#10023
[3] bazelbuild/bazel#11272

[davido]

@cushon Thanks for re-opening this issue again. After all the research that I did on that topic, including opening multiple issues across different projects: [1], [2], I think that 11e28aa, 5ad37b7 and friends were very intrusive and backwards incomptatible changes. It would break each and every client that rely on code generation and update EP to the new version, as have been seen by rules_kotlin and rules_closure.

I am not sure what is the right way to communicate the backwards incompatible changes for EP project, but may be we should close this issue and open a follow-up issue so that the potential breakages of clients as the consequence of 11e28aa, 5ad37b7 and friends should be properly announced?

[1] #1599
[2] google/dagger#1831

[cushon]

The failing compilation is using -source 8 -target 8 with a JDK 11 -bootclasspath (because it's targeting the local JDK 11). So AutoAnnotation is trying to use javax.annotation.Generated based on the source level, but it isn't available because the JDK 11 bootclasspath only contains javax.annotation.processing.Generated.

This issue wouldn't affect consistent cross-compilation configurations, i.e -source 8 -target 8 -bootclasspath 8, or -source 11 -target 11 -bootclasspath 11 / --release 11.

[davido]

This issue wouldn't affect consistent cross-compilation configurations, i.e -source 8 -target 8 -bootclasspath 8, or -source 11 -target 11 -bootclasspath 11 / --release 11.

The situation is even worse. For builder projects (both rules_kotlin and rules_closure are affected) it's usual to also build for host configuration. Given that Bazel is using remote JDK 11, host compilation has a mismatch between -bootclasspath 11 and -source 8, see: bazelbuild/rules_closure#483 (comment).

In short it doesn't work and false positive would be reported even with:

  $ java -fullversion
  openjdk full version "1.8.0_242-b08"

[cushon]

In short it doesn't work and false positive would be reported even with:

  $ java -fullversion
  openjdk full version "1.8.0_242-b08"

That's still an example of -source 8 -target 8 -bootclasspath 8, it's just in the host configuration. Targeting JDK 8 with -source 8 -target 8 does actually work:

$ $JAVA11_HOME/bin/java -fullversion
openjdk full version "11.0.7+10"
$ JAVA_HOME=$JAVA11_HOME bazel build cli
...
ERROR: ../error_prone_auto_annotation_java11_breakage/BUILD:30:1: Building libcli.jar (1 source file) and running annotation processors (AutoAnnotationProcessor, AutoValueProcessor) failed (Exit 1)
bazel-out/k8-fastbuild/bin/_javac/cli/libcli_sourcegenfiles/AutoAnnotation_OptionUtil_newOption.java:11: error: [ImmutableAnnotationChecker] annotations should be immutable: 'AutoAnnotation_OptionUtil_newOption' has field 'aliases' of type 'java.lang.String[]', arrays are mutable
  private final String[] aliases;
                         ^

$ $JAVA8_HOME/bin/java -fullversion
java full version "1.8.0_202-b08"
$ JAVA_HOME=$JAVA8_HOME bazel build cli
...
Target //:cli up-to-date:
  bazel-bin/libcli.jar

[cushon]

It would be nice if Bazel had a better way to handle bootclasspaths for cross compilation with -source 8 -target 8, but in the short-term we could just demote RefersToDaggerCodegen to a warning to avoid a breaking change.

Would that mitigate the issue you're seeing?

The repro uses ImmutableAnnotationChecker, but that check defaults to being a warnings, and the repro explicitly enables it as an error. Is leaving it as a warning an option? Was that repro based on a real build that had promoted it to an error, and then was broken by changes to @Generated handle in later versions of AutoAnnotation or something?

[davido]

but in the short-term we could just demote RefersToDaggerCodegen to a warning to avoid a breaking change.

Would that mitigate the issue you're seeing?

Yes. All checks, that are relying on availability of @Generated should be demoted to warning until Java 8 is still used, e.g.: ExtendsAutoValue as well, that was recently added. I just wrote this issue.

Was that repro based on a real build that had promoted it to an error, and then was broken by changes to @generated handle in later versions of AutoAnnotation or something?

Yes. The reproducer was created based on the real breakage of Gerrit Code Review build, after switching to local JDK 11: [1] and thus having exactly the described cross compilation Bazel problem. As you may know, Gerrit project promoted the severity levels for many EP checks in its build tool chain: [2] to error, and promoting the severity for ImmutableAnnotationChecker to error broke Gerrit build after switching to JDK 11.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/237183
[2] https://github.com/GerritCodeReview/gerrit/blob/master/tools/BUILD#L20-L97