From 77e4f72a0e438cebf3c8a72fb7aa92117c8236e7 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 12 Jul 2024 09:42:18 -0700 Subject: [PATCH] In `NullArgumentForNonNullParameter`, don't trigger completion for `NullMarked`. Completion can fail under `--release 8`, leading to `warning: unknown enum constant ElementType.MODULE`. This CL is one of a variety of ways that I'll be addressing https://github.com/google/truth/issues/1320. It alone should be sufficient (unless there are other problems that I'm unaware of), but I'll do more, both for people who might not upgrade Error Prone immediately and for anyone else (NullAway?) who writes a check that calls `hasAnnotation(nullMarked)`. PiperOrigin-RevId: 651801963 --- .../NullArgumentForNonNullParameter.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java index 74fff8e8713..2ba93e8a04d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java @@ -28,7 +28,6 @@ import static com.google.errorprone.util.ASTHelpers.enclosingClass; import static com.google.errorprone.util.ASTHelpers.enclosingPackage; import static com.google.errorprone.util.ASTHelpers.getSymbol; -import static com.google.errorprone.util.ASTHelpers.hasAnnotation; import static java.util.Arrays.stream; import static javax.lang.model.type.TypeKind.TYPEVAR; @@ -70,6 +69,10 @@ public final class NullArgumentForNonNullParameter extends BugChecker memoize(state -> state.getName("com.google.common.collect.Immutable")); private static final Supplier GUAVA_GRAPH_IMMUTABLE_PREFIX = memoize(state -> state.getName("com.google.common.graph.Immutable")); + private static final Supplier PROTO_NONNULL_API_NAME = + memoize(state -> state.getName("com.google.protobuf.Internal$ProtoNonnullApi")); + private static final Supplier NULL_MARKED_NAME = + memoize(state -> state.getName("org.jspecify.annotations.NullMarked")); private static final Supplier> NULL_MARKED_PACKAGES_WE_TRUST = memoize( state -> @@ -245,10 +248,10 @@ private static boolean isParameterOfMethodOnTypeStartingWith( private boolean enclosingAnnotationDefaultsNonTypeVariablesToNonNull( Symbol sym, VisitorState state) { for (; sym != null; sym = sym.getEnclosingElement()) { - if (hasAnnotation(sym, "com.google.protobuf.Internal$ProtoNonnullApi", state)) { + if (hasDirectAnnotation(sym, PROTO_NONNULL_API_NAME.get(state))) { return true; } - if (hasAnnotation(sym, "org.jspecify.annotations.NullMarked", state) + if (hasDirectAnnotation(sym, NULL_MARKED_NAME.get(state)) && weTrustNullMarkedOn(sym, state)) { return true; } @@ -256,6 +259,18 @@ && weTrustNullMarkedOn(sym, state)) { return false; } + /* + * ASTHelpers has hasAnnotation and hasDirectAnnotationWithSimpleName but I think not this. + * + * We avoid hasAnnotation not just because it's unnecessary but also because it would cause issues + * under --release 8 on account of NullMarked's use of @Target(MODULE, ...). + */ + private static boolean hasDirectAnnotation(Symbol sym, Name name) { + return sym.getAnnotationMirrors().stream() + .anyMatch( + a -> ((Symbol) a.getAnnotationType().asElement()).getQualifiedName().equals(name)); + } + private boolean weTrustNullMarkedOn(Symbol sym, VisitorState state) { /* * Similar to @NonNull (discussed above), the "default to non-null" annotation @NullMarked is