3.4.x incompatible change: error-prone is stricter

[laszlocsomor]

Description of the problem / feature request:

Error-prone is stricter with Bazel 3.4.x than Bazel 3.3.x.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

mkdir /tmp/foo

cd /tmp/foo

wget https://repo1.maven.org/maven2/com/google/guava/guava/29.0-jre/guava-29.0-jre.jar

touch WORKSPACE

cat >BUILD <<eof
java_binary(
    name = "bin",
    srcs = ["Main.java"],
    main_class = "Main",
    deps = [":guava"],
)

java_import(
    name = "guava",
    jars = ["guava-29.0-jre.jar"],
)
eof

cat >Main.java <<eof
import com.google.common.base.Preconditions;

public class Main {
  public static void main(String[] args) {
    Preconditions.checkState(false, "%d", 1);
  }
}
eof

Then:

  $ bazel version
Bazelisk version: v1.4.0
Build label: 3.4.1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Jul 14 06:27:53 2020 (1594708073)
Build timestamp: 1594708073
Build timestamp as int: 1594708073

  $ bazel build //:bin
(...)
Execution platform: @local_config_platform//:host
Main.java:5: error: [PreconditionsInvalidPlaceholder] Preconditions only accepts the %s placeholder in error message strings
    Preconditions.checkState(false, "%d", 1);
                                    ^
    (see https://errorprone.info/bugpattern/PreconditionsInvalidPlaceholder)
  Did you mean 'Preconditions.checkState(false, "%s", 1);'?
Target //:bin failed to build
(19:58:49) INFO: Elapsed time: 6.016s, Critical Path: 1.38s
(19:58:49) INFO: 1 process: 1 remote cache hit.
(19:58:49) FAILED: Build did NOT complete successfully

Downgrading to 3.3.0 fixes it:

  $ echo '3.3.0' > .bazelversion

  $ bazel version
Bazelisk version: v1.4.0
Build label: 3.3.0
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Wed Jun 17 12:55:32 2020 (1592398532)
Build timestamp: 1592398532
Build timestamp as int: 1592398532

  $ bazel build //:bin
(...)
(19:59:28) INFO: Build completed successfully, 3 total actions

What operating system are you running Bazel on?

Ubuntu 20.04

What's the output of bazel info release?

See above.

[meisterT]

Cc @davido @meteorcloudy

[davido]

Cc @cushon .

Yeah, that is totally expected.

@laszlocsomor Have you checked the release notes, link, quoting:

Upgraded java_tools (to version 9.0) including a major Error Prone upgrade (to version 2.4.0).
This includes new checks and new diagnostics for existing checks.
As a result, you may see new compiler warnings or failures with this release (example fix).

Having said that, I have uploaded this incompatible change PR to avoid that breakage you are seeing.
So that future java_tools upgrades might be less disruptive.

You can fix your broken code (and be happy that Bazel detected it for you)
or you could disable that bug pattern using javacopt option.

Or as the last option, we could consider to revert the Error Prone upgrade again,
like it was done last time, when the EP upgrade was attempted.

[davido]

Also related here (or even duplicate?):

#2713
#11446

[laszlocsomor]

@davido , thanks for adding context. I missed the note, but I didn't anticipate a breaking minor release either.

[meisterT]

Sorry for the inconvenience, Laszlo.

Previously, we used an old snapshot version of Error Prone that was not compatible with newer Java versions, but the community wanted to start adding support for Java 14. So they went through weeks of preparing this in little steps, waiting for us to review and merge it, do a JavaTools release, add that into Bazel in time for Bazel 3.4.0 so that they have a stable release upon which they can build the Java 14 support.

We discovered that the new ErrorProne version broke a few downstream projects due to stricter checks, but we fixed the downstream projects with simple commits. We had a commit prepared that would have turned these specific errors into warnings, but decided to not merge it, because downstream was green by the time we needed to make the cut.

We missed that the GuardedBy check is not the only one that got stricter but there are others as well (as Laszlo demonstrates in his repro).

If you run into this issue, you have four options:

• If trivial, fix the code by following the URL and instructions from ErrorProne (https://errorprone.info/bugpattern/PreconditionsInvalidPlaceholder in the example above).
• Pin your Bazel version with .bazelversion, e.g. to 3.3.1 (see https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-which-bazel-version-to-run) until you are ready to upgrade.
• Demote the specific check to a warning by putting the following in your .bazelrc: build --javacopt=-Xep:<CHECK>:WARN (replace CHECK with the one that ErrorProne complains about; build --javacopt=-Xep:PreconditionsInvalidPlaceholder:WARN for the example above).
• Demote all ErrorProne checks to warnings by putting build --javacopt=-XepAllErrorsAsWarnings in your .bazelrc.

[laszlocsomor]

@meisterT , thanks for the details!

[davido]

@meisterT

Pin your Bazel version with .bazelversion, e.g. to 3.3.1 (see https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-which-bazel-version-to-run) until you are ready to upgrade.

Given the above option do we really need --incompatible_use_java_tools_beta_release option from my PR that would complicate the code and the upgrade process.

Bazel users could also miss to try the above option.

1. Say we would add java_tools-beta-realase with new Error Prone diagnostics and checks in Bazel 3.4.1 release.
2. In upcoming release 3.5.0 (or would it be 4.0.0) we would drop that option again and promote java_tools_beta to be the regular java_tools. Who guarantee, that Bazel users would actually use that option an fix their build issues with stricter EP (or different tool chain modification?).

What I am trying to say: can we define the java_tools standard upgrade process as non disruptive without having --incompatible_use_java_tools_beta_release in place? And close #2713 as resolved and abandon #11446?

Having said that, we do have this @brandjon comment: #2713 (comment), quoting:

There must be a way for users to migrate their code easily.
Our normal policy is that there must be at least one Bazel release that has a flag users
can toggle to control whether or not the new behavior is enabled. That's how it works
when a Starlark API is deleted, for instance, and @meisterT says we won't treat EP any different.

[meisterT]

@davido while this is a worthwhile conversation to have I think it's out of scope for this issue. @aiuto has some ideas how to improve the process for both users and developers and will share his thoughts soon.

[davido]

There is one more workaround, that should be mentioned for completeness, to specify javacopts in java_library rule. Example is in JGit project:

INSECURE_CIPHER_FACTORY = [
    "src/org/eclipse/jgit/transport/InsecureCipherFactory.java",
]

java_library(
    name = "insecure_cipher_factory",
    srcs = INSECURE_CIPHER_FACTORY,
    javacopts = ["-Xep:InsecureCryptoUsage:OFF"],
)

[meisterT]

I am closing this issue since we provided the workarounds above.