Error Prone: ReturnValueIgnored bug pattern is flagged with Java 11 toolchain but not with Java 17 toolchain

[davido]

On Bazel@HEAD (38117d4) I try to build this code with different toolchains:

javatests/com/google/gerrit/common/data/GroupReferenceTest.java

  [...]
  @Test
  public void testHashcode() {
    AccountGroup.UUID uuid1 = AccountGroup.uuid("uuid1");
    assertThat(GroupReference.create(uuid1, "foo").hashCode())
        .isEqualTo(GroupReference.create(uuid1, "bar").hashCode());

    // Check that the following calls don't fail with an exception.
    GroupReference.create("bar").hashCode();
  }

With Java 11 the issue is reported as expected:

  $ bazeldev build --config java11 \
     //javatests/com/google/gerrit/common/data:data_tests
[...]
    javatests/com/google/gerrit/common/data/GroupReferenceTest.java:132: error: [ReturnValueIgnored] Return value of 'hashCode' must be used
    GroupReference.create("bar").hashCode();
                                         ^
    (see https://errorprone.info/bugpattern/ReturnValueIgnored)
  Did you mean to remove this line?
Target //javatests/com/google/gerrit/common/data:data_tests failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 217.789s, Critical Path: 182.46s
INFO: 131 processes: 1 disk cache hit, 14 internal, 63 linux-sandbox, 53 worker.
FAILED: Build did NOT complete successfully

On Java 17 (default configuration), the problem is unexpectedly not flagged:

  $ bazeld build \
    //javatests/com/google/gerrit/common/data:data_tests
  INFO: Invocation ID: 7b2d7505-d0f2-41b3-bd9a-967294e673f8
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=142
INFO: Reading rc options for 'build' from /home/davido/projects/gerrit/.bazelrc:
  'build' options: --workspace_status_command=python3 ./tools/workspace_status.py --repository_cache=~/.gerritcodereview/bazel-cache/repository --action_env=PATH --disk_cache=~/.gerritcodereview/bazel-cache/cas --java_language_version=17 --java_runtime_version=remotejdk_17 --tool_java_language_version=17 --tool_java_runtime_version=remotejdk_17 --incompatible_strict_action_env --announce_rc
INFO: Reading rc options for 'build' from /home/davido/.bazelrc:
  'build' options: --experimental_generate_json_trace_profile --experimental_profile_cpu_usage
[...]
  java/com/google/gerrit/server/notedb/CommitRewriter.java:585: warning: [JavaUtilDate] Date has a bad API that leads to bugs; prefer java.time.Instant or LocalDate.
        && newIdent.getWhen().getTime() == originalIdent.getWhen().getTime()
                                                                          ^
    (see https://errorprone.info/bugpattern/JavaUtilDate)
Target //javatests/com/google/gerrit/common/data:data_tests up-to-date:
  bazel-bin/javatests/com/google/gerrit/common/data/data_tests.jar
  bazel-bin/javatests/com/google/gerrit/common/data/data_tests
INFO: Elapsed time: 5.692s, Critical Path: 4.27s
INFO: 94 processes: 92 disk cache hit, 1 internal, 1 linux-sandbox.
INFO: Build completed successfully, 94 total actions

To reproduce, fetch this change: [1] and run the above mentioned commands.

Also note, that "-Xep:FutureReturnValueIgnored:ERROR" was enabled in custom toolchain (defined in tools/BUILD) in both cases (and this custom toolchain definition is used: If I promote "-Xep:JavaUtilDate:WARN" to -Xep:JavaUtilDate:ERROR" the build would fail as expected).

//CC @cushon @comius

[1] https://gerrit-review.googlesource.com/c/gerrit/+/327739

[cushon]

That ReturnValueIgnored behaviour is relatively new, and was disabled in java_tools v11.4 to avoid a breaking change (bazelbuild/java_tools#51 (comment), cf57345).

Passing -XepOpt:ReturnValueIgnored:ObjectMethods=true to re-enable it shows the expected behaviour:

buildozer 'add javacopts -XepOpt:ReturnValueIgnored:ObjectMethods=true' //javatests/com/google/gerrit/common/data:data_tests
bazel build //javatests/com/google/gerrit/common/data:data_tests
...
javatests/com/google/gerrit/common/data/GroupReferenceTest.java:132: error: [ReturnValueIgnored] Return value of 'hashCode' must be used
    GroupReference.create("bar").hashCode();
                                         ^

[cushon]

I'm not entirely sure why you're seeing this for 11 and not 17. Are you using versions of those toolchains that use the same version of Error Prone, and are configured the same way?

[davido]

Are you using versions of those toolchains that use the same version of Error Prone, and are configured the same way?

Yes. Everything is the same. I am seeing the same behavior with Bazel version: 5.0.0rc4, see .bazelversion and using Bazelisk and with Bazel@HEAD. The difference is only the Java version used and source and target java versin 11 vs. 17.

I can also confirm that with this diff:

diff --git a/tools/BUILD b/tools/BUILD
index 18b667f490..bb5586b6dc 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -388,6 +388,7 @@ java_package_configuration(
         "-Xep:RethrowReflectiveOperationExceptionAsLinkageError:ERROR",
         "-Xep:ReturnFromVoid:ERROR",
         "-Xep:ReturnValueIgnored:ERROR",
+        "-XepOpt:ReturnValueIgnored:ObjectMethods=true",
         "-Xep:RxReturnValueIgnored:ERROR",
         "-Xep:SameNameButDifferent:ERROR",
         "-Xep:SelfAssignment:ERROR",

The build is failing as expected also on Java 17:

  $ bazelisk build \
    //javatests/com/google/gerrit/common/data:data_tests
[...]
javatests/com/google/gerrit/common/data/GroupReferenceTest.java:132: error: [ReturnValueIgnored] Return value of 'hashCode' must be used
    GroupReference.create("bar").hashCode();
                                         ^
    (see https://errorprone.info/bugpattern/ReturnValueIgnored)
  Did you mean to remove this line?
Target //javatests/com/google/gerrit/common/data:data_tests failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 60.911s, Critical Path: 59.74s
INFO: 111 processes: 17 disk cache hit, 4 internal, 46 linux-sandbox, 44 worker.
FAILED: Build did NOT complete successfully

So that the question remains: why -XepOpt:ReturnValueIgnored:ObjectMethods=true is needed only on Java 17 to trigger expected behaviour of: -Xep:ReturnValueIgnored:ERROR?

[cushon]

The commit I linked to adds -XepOpt:ReturnValueIgnored:ObjectMethods=false to the default value of default_java_toolchain.misc

Gerrit's error_prone_warnings_toolchain_java11 overrides default_java_toolchain.misc to exclude that flag.

The error_prone_warnings_toolchain_java17 does not override default_java_toolchain.misc, so it gets the default values that include-XepOpt:ReturnValueIgnored:ObjectMethods=false.

[davido]

The error_prone_warnings_toolchain_java17 does not override default_java_toolchain.misc, so it gets the default values that include -XepOpt:ReturnValueIgnored:ObjectMethods=false.

I will overwrite default_java_toolchain.misc in error_prone_warnings_toolchain_java17 then:

default_java_toolchain(
    name = "error_prone_warnings_toolchain_java17",
    configuration = dict(),
    java_runtime = "@bazel_tools//tools/jdk:remotejdk_17",
    misc = [
        "-XDskipDuplicateBridges=true",
        "-XDcompilePolicy=simple",
        "-g",
        "-parameters",
# https://github.com/bazelbuild/java_tools/issues/51#issuecomment-927940699
#    "-XepOpt:ReturnValueIgnored:ObjectMethods=false",
    ],
    package_configuration = [
        ":error_prone",
    ],
    source_version = "17",
    target_version = "17",
    visibility = ["//visibility:public"],
)

Thanks for the quick help!

Should we file a follow-up issue to remove this exclusion in Bazel:

bazel@HEAD

  $ git grep XepOpt:ReturnValueIgnored:ObjectMethods
tools/jdk/default_java_toolchain.bzl:    "-XepOpt:ReturnValueIgnored:ObjectMethods=false",

We should fix Bazel code or add @SuppressArnings("ReturnValueIgnored"), right?