Suggestion: `BadImport` should ignore annotation parameters

[Stephan202]

Consider the following code:

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonSubTypes.Type;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeInfo.Id;

@JsonTypeInfo(use = Id.NAME)
@JsonSubTypes({
    @Type(value = VariantA.class, name = "variant_a"),
    @Type(value = VariantB.class, name = "variant_b")
})
interface Dummy {}

final class VariantA implements Dummy {}

final class VariantB implements Dummy {}

Applying the BadImport check to this code:

wget \
  https://repo1.maven.org/maven2/com/google/errorprone/error_prone_core/2.4.0/error_prone_core-2.4.0-with-dependencies.jar
wget \
  https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-annotations/2.11.0/jackson-annotations-2.11.0.jar
javac \
  -XDcompilePolicy=simple \
  -processorpath error_prone_core-2.4.0-with-dependencies.jar \
  '-Xplugin:ErrorProne -XepPatchChecks:BadImport -XepPatchLocation:/tmp' \
  -cp jackson-annotations-2.11.0.jar \
  Dummy.java

This yields:

--- /tmp/Dummy.java
+++ /tmp/Dummy.java
@@ -1,11 +1,9 @@
 import com.fasterxml.jackson.annotation.JsonSubTypes;
-import com.fasterxml.jackson.annotation.JsonSubTypes.Type;
 import com.fasterxml.jackson.annotation.JsonTypeInfo;
-import com.fasterxml.jackson.annotation.JsonTypeInfo.Id;

-@JsonTypeInfo(use = Id.NAME)
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME)
 @JsonSubTypes({
-    @Type(value = VariantA.class, name = "variant_a"),
-    @Type(value = VariantB.class, name = "variant_b")
+    @JsonSubTypes.Type(value = VariantA.class, name = "variant_a"),
+    @JsonSubTypes.Type(value = VariantB.class, name = "variant_b")
 })
 interface Dummy {}

In this case the @JsonTypeInfo and @JsonSubTypes annotations have elements that reference nested types with generic names. In the case of @JsonTypeInfo this is an enum (Id), while in the case of @JsonSubTypes it is another annotation type (@Type).

Since annotations can only reference primitive types, strings, enums or other annotations, no ambiguity arises in this case. As such it seems to me that BadImport should not flag these cases; the suggested changes make the code more verbose without improving readability.

[cushon]

I don't have strong opinions about this check--we landed on this as a recommended practice for our code, I think it's reasonable to have a different approach to imports or qualified names. But I think the check is doing what it's intended to here.

The bit in the docs about

Importing nested classes/static methods/static fields with commonly-used names can make code harder to read, because it may not be clear from the context exactly which type is being referred to.

is referring to the type being imported, not the type of the expression reference that imported type. i.e. in the case of Id.NAME the claim is that the name of the type Id is not very unique name so it could be clearer to use JsonTypeInfo.Id, it isn't referring to the fact that the type of the expression Id.NAME is an int. (And if anything the fact that it's an int makes this more valuable to me, since that isn't a very unique type either.)