Demoting severity level for PreferredInterfaceType bug pattern (aka MutableConstantField) doesn't have any effect

[davido]

After bumping java_tools in Bazel to the latest released version v12.1: [1],
that was updated to the latest EP release 2.18 in: [2],
the configuration of error prone bug patterns using CLI option,
e.g.: -Xep:PreferredInterfaceType:WARN stopped working:

  $ bazeldev build //java/com/google/gerrit/extensions:api
ERROR: /home/davido/projects/gerrit/java/com/google/gerrit/extensions/BUILD:29:13: Building java/com/google/gerrit/extensions/libapi.jar (359 source files) and running annotation processors (AutoAnnotationProcessor, AutoValueProcessor, AutoOneOfProcessor) failed: (Exit 1): java failed: error executing command (from target //java/com/google/gerrit/extensions:api) external/remotejdk11_linux/bin/java -XX:-CompactStrings '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 16 arguments skipped)
java/com/google/gerrit/extensions/conditions/BooleanCondition.java:273: error: [PreferredInterfaceType] Method return type can use a more specific type to convey more information to callers. Note that it is possible to return a more specific type even when overriding a method.
    public <T> Iterable<T> children(Class<T> type) {
                       ^
    (see https://errorprone.info/bugpattern/PreferredInterfaceType)
  Did you mean 'public <T> List<T> children(Class<T> type) {'?
Target //java/com/google/gerrit/extensions:api failed to build

^^^ What is unclear is as why the above bug pattern is activated in the first place,
because looking at the source code: [3] it is included in DISABLED_CHECKS section.

It turns out, that the above bug pattern references alternate names:

https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/PreferredInterfaceType.java#L79-L83

/** Tightens types which refer to an Iterable, Map, Multimap, etc. */
@BugPattern(
    altNames = {"MutableConstantField", "MutableMethodReturnType"},
    summary = "This type can be more specific.",
    severity = WARNING)

So that actually to demote the severity to the level WARN, a different bug pattern has to be demoted: from -Xep:MutableConstantField:ERROR to -Xep:MutableConstantField:WARN.

Also the documentation doesn't mention, that those two bug patterns are related: [4].

Reproducer is here: [5].

[1] https://github.com/bazelbuild/java_tools/releases/tag/java_v12.1
[2] bazelbuild/bazel#17470
[3] https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java#L1116
[4] https://github.com/google/error-prone/blob/master/docs/bugpattern/PreferredInterfaceType.md
[5] https://gerrit-review.googlesource.com/c/gerrit/+/370334

[cushon]

I may not have absorbed all of the details here, but this sounds like the expected result of 48bbef0, the -Xep:CheckName:OFF flags now affect checks based on altNames in addition to name.

So if you had -Xep:PrimaryName:WARN -Xep:AltNameOfSameCheck:ERROR, the second flag was previously ignored, and now it has an effect.

[davido]

So if you had -Xep:PrimaryName:WARN -Xep:AltNameOfSameCheck:ERROR, the second flag was previously ignored, and now it has an effect.

No alternative names for PreferredInterfaceType are documented: [1].

Are you suggesting that users should dive into EP code to find out why demoting using the primary name -Xep:PrimaryName:WARN (that was reported in the breakage) doesn't work?

[1] https://github.com/google/error-prone/blob/master/docs/bugpattern/PreferredInterfaceType.md

[cushon]

The alt names are documented in https://errorprone.info/bugpattern/PreferredInterfaceType

[davido]

You are right, I missed this line:

PreferredInterfaceType
This type can be more specific.

Severity
    WARNING

Alternate names: MutableConstantField, MutableMethodReturnType

Thanks for clarifying!