From 99bfc0b8f8757889999c0065a1d116175f04218a Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 3 Nov 2023 15:18:24 -0700 Subject: [PATCH] Update `NullnessAnnotation`s API to use annotation mirrors and trees instead of strings PiperOrigin-RevId: 579319948 --- .../nullnesspropagation/MethodInfo.java | 5 +- .../NullnessAnnotations.java | 66 ++++++++++++------- .../NullnessPropagationTransfer.java | 25 ++++--- .../TrustingNullnessPropagation.java | 2 +- .../nullness/VoidMissingNullable.java | 13 ++-- 5 files changed, 62 insertions(+), 49 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/MethodInfo.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/MethodInfo.java index ca092da01e1..ee64578c991 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/MethodInfo.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/MethodInfo.java @@ -16,7 +16,8 @@ package com.google.errorprone.dataflow.nullnesspropagation; -import java.util.List; +import com.google.common.collect.ImmutableList; +import javax.lang.model.element.AnnotationMirror; /** Represents a Java method. Used for custom predicates to match non-null-returning methods. */ public interface MethodInfo { @@ -24,7 +25,7 @@ public interface MethodInfo { String method(); - List annotations(); + ImmutableList annotations(); boolean isStatic(); diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java index a194810d6f0..6a4bb884b38 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java @@ -19,17 +19,22 @@ import static javax.lang.model.element.ElementKind.TYPE_PARAMETER; import com.google.errorprone.util.MoreAnnotations; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; -import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Stream; import javax.annotation.Nullable; +import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Name; import javax.lang.model.element.Parameterizable; import javax.lang.model.element.TypeParameterElement; import javax.lang.model.type.IntersectionType; @@ -41,28 +46,44 @@ public class NullnessAnnotations { // TODO(kmb): Correctly handle JSR 305 @Nonnull(NEVER) etc. private static final Predicate ANNOTATION_RELEVANT_TO_NULLNESS = Pattern.compile( - ".*\\b(" - + "(Recently)?NonNull(Decl|Type)?|NotNull|Nonnull|" + "(Recently)?NonNull(Decl|Type)?|NotNull|Nonnull|" + "(Recently)?Nullable(Decl|Type)?|CheckForNull|PolyNull|MonotonicNonNull(Decl)?|" + "ProtoMethodMayReturnNull|ProtoMethodAcceptsNullParameter|" - + "ProtoPassThroughNullness" - + ")$") - .asPredicate(); + + "ProtoPassThroughNullness") + .asMatchPredicate(); private static final Predicate NULLABLE_ANNOTATION = Pattern.compile( - ".*\\b(" - + "(Recently)?Nullable(Decl|Type)?|CheckForNull|PolyNull|MonotonicNonNull(Decl)?|" + "(Recently)?Nullable(Decl|Type)?|CheckForNull|PolyNull|MonotonicNonNull(Decl)?|" + "ProtoMethodMayReturnNull|ProtoMethodAcceptsNullParameter|" - + "ProtoPassThroughNullness" - + ")$") - .asPredicate(); + + "ProtoPassThroughNullness") + .asMatchPredicate(); private NullnessAnnotations() {} // static methods only - public static Optional fromAnnotations(Collection annotations) { + public static Optional fromAnnotationTrees(List annotations) { + return fromAnnotationSimpleNames(annotations.stream().map(a -> simpleName(a))); + } + + public static Optional fromAnnotationMirrors( + List annotations) { return fromAnnotationStream(annotations.stream()); } + private static String simpleName(AnnotationTree annotation) { + Tree annotationType = annotation.getAnnotationType(); + if (annotationType instanceof IdentifierTree) { + return ((IdentifierTree) annotationType).getName().toString(); + } else if (annotationType instanceof MemberSelectTree) { + return ((MemberSelectTree) annotationType).getIdentifier().toString(); + } else { + throw new AssertionError(annotationType.getKind()); + } + } + + private static Name simpleName(AnnotationMirror annotation) { + return annotation.getAnnotationType().asElement().getSimpleName(); + } + public static Optional fromAnnotationsOn(@Nullable Symbol sym) { if (sym == null) { return Optional.empty(); @@ -97,13 +118,12 @@ public static Optional fromAnnotationsOn(@Nullable Symbol sym) { return fromElement; } - return fromAnnotationStream( - MoreAnnotations.getDeclarationAndTypeAttributes(sym).map(Object::toString)); + return fromAnnotationStream(MoreAnnotations.getDeclarationAndTypeAttributes(sym)); } public static Optional fromAnnotationsOn(@Nullable TypeMirror type) { if (type != null) { - return fromAnnotationList(type.getAnnotationMirrors()); + return fromAnnotationStream(type.getAnnotationMirrors().stream()); } return Optional.empty(); } @@ -119,8 +139,7 @@ public static Optional fromDefaultAnnotations(@Nullable Element sym) { // type annotations. For now we're just using a hard-coded simple name. // TODO(b/121272440): Look for existing default annotations if (sym.getAnnotationMirrors().stream() - .map(Object::toString) - .anyMatch(it -> it.endsWith(".DefaultNotNull"))) { + .anyMatch(a -> simpleName(a).contentEquals("DefaultNotNull"))) { return Optional.of(Nullness.NONNULL); } sym = sym.getEnclosingElement(); @@ -141,9 +160,7 @@ public static Optional getUpperBound(TypeVariable typeVar) { result = fromAnnotationStream( ((IntersectionType) typeVar.getUpperBound()) - .getBounds().stream() - .map(TypeMirror::getAnnotationMirrors) - .map(Object::toString)); + .getBounds().stream().flatMap(t -> t.getAnnotationMirrors().stream())); } else { result = fromAnnotationsOn(typeVar.getUpperBound()); } @@ -168,7 +185,7 @@ public static Optional getUpperBound(TypeVariable typeVar) { typeParam.getSimpleName().equals(typeVar.asElement().getSimpleName())) .findFirst() // Annotations at class/interface/method type variable declaration - .flatMap(decl -> fromAnnotationList(decl.getAnnotationMirrors())); + .flatMap(decl -> fromAnnotationStream(decl.getAnnotationMirrors().stream())); } } @@ -177,11 +194,12 @@ public static Optional getUpperBound(TypeVariable typeVar) { return result.isPresent() ? result : fromDefaultAnnotations(typeVar.asElement()); } - private static Optional fromAnnotationList(List annotations) { - return fromAnnotationStream(annotations.stream().map(Object::toString)); + private static Optional fromAnnotationStream( + Stream annotations) { + return fromAnnotationSimpleNames(annotations.map(a -> simpleName(a).toString())); } - private static Optional fromAnnotationStream(Stream annotations) { + private static Optional fromAnnotationSimpleNames(Stream annotations) { return annotations .filter(ANNOTATION_RELEVANT_TO_NULLNESS) .map(annot -> NULLABLE_ANNOTATION.test(annot) ? Nullness.NULLABLE : Nullness.NONNULL) diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java index 067ddbd61c8..b4699d5f11e 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java @@ -86,6 +86,7 @@ import java.util.Set; import java.util.function.Predicate; import javax.annotation.Nullable; +import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.VariableElement; import javax.lang.model.type.TypeVariable; import org.checkerframework.errorprone.dataflow.analysis.Analysis; @@ -203,7 +204,7 @@ private static class ReturnValueIsNonNull implements Predicate, Seri public boolean test(MethodInfo methodInfo) { // Any method explicitly annotated is trusted to behave as advertised. Optional fromAnnotations = - NullnessAnnotations.fromAnnotations(methodInfo.annotations()); + NullnessAnnotations.fromAnnotationMirrors(methodInfo.annotations()); if (fromAnnotations.isPresent()) { return fromAnnotations.get() == NONNULL; } @@ -406,11 +407,8 @@ Nullness visitInstanceOf( @Override Nullness visitTypeCast(TypeCastNode node, SubNodeValues inputs) { - ImmutableList annotations = - node.getType().getAnnotationMirrors().stream() - .map(Object::toString) - .collect(toImmutableList()); - return NullnessAnnotations.fromAnnotations(annotations) + List annotations = node.getType().getAnnotationMirrors(); + return NullnessAnnotations.fromAnnotationMirrors(annotations) .orElseGet( () -> hasPrimitiveType(node) ? NONNULL : inputs.valueOfSubNode(node.getOperand())); } @@ -782,7 +780,8 @@ private Nullness returnValueNullness(MethodInvocationNode node, @Nullable ClassA if (callee == null) { return defaultAssumption; } - Optional declaredNullness = NullnessAnnotations.fromAnnotations(callee.annotations); + Optional declaredNullness = + NullnessAnnotations.fromAnnotationMirrors(callee.annotations); if (declaredNullness.isPresent()) { return declaredNullness.get(); } @@ -988,7 +987,7 @@ public int hashCode() { static final class ClassAndMethod implements Member, MethodInfo { final String clazz; final String method; - final ImmutableList annotations; + final ImmutableList annotations; final boolean isStatic; final boolean isPrimitive; final boolean isBoolean; @@ -998,7 +997,7 @@ static final class ClassAndMethod implements Member, MethodInfo { private ClassAndMethod( String clazz, String method, - ImmutableList annotations, + ImmutableList annotations, boolean isStatic, boolean isPrimitive, boolean isBoolean, @@ -1016,10 +1015,8 @@ private ClassAndMethod( static ClassAndMethod make(MethodSymbol methodSymbol, @Nullable Types types) { // TODO(b/71812955): consider just wrapping methodSymbol instead of copying everything out. - ImmutableList annotations = - MoreAnnotations.getDeclarationAndTypeAttributes(methodSymbol) - .map(Object::toString) - .collect(toImmutableList()); + ImmutableList annotations = + MoreAnnotations.getDeclarationAndTypeAttributes(methodSymbol).collect(toImmutableList()); ClassSymbol clazzSymbol = (ClassSymbol) methodSymbol.owner; return new ClassAndMethod( @@ -1087,7 +1084,7 @@ public String method() { } @Override - public ImmutableList annotations() { + public ImmutableList annotations() { return annotations; } diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java index c8ab9c32f2d..4da8c70fdda 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java @@ -73,7 +73,7 @@ private enum TrustReturnAnnotation implements Predicate { @Override public boolean apply(MethodInfo input) { - return NullnessAnnotations.fromAnnotations(input.annotations()).orElse(Nullness.NONNULL) + return NullnessAnnotations.fromAnnotationMirrors(input.annotations()).orElse(Nullness.NONNULL) == Nullness.NONNULL; } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java index 116295bbae1..1c0d97fcb5e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java @@ -16,7 +16,6 @@ package com.google.errorprone.bugpatterns.nullness; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.fixByAddingNullableAnnotationToReturnType; import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.fixByAddingNullableAnnotationToType; @@ -44,8 +43,8 @@ import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotatedTypeTree; +import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ParameterizedTypeTree; @@ -56,6 +55,7 @@ import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; +import java.util.List; import javax.inject.Inject; /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ @@ -184,7 +184,7 @@ private void checkTree(Tree tree, VisitorState state) { * TODO(cpovirk): Provide this pair of checks as NullnessAnnotations.fromAnnotationsOn(Tree), * which might also be useful for a hypothetical future TypeArgumentMissingNullable? */ - if (NullnessAnnotations.fromAnnotations(annotationsIfAnnotatedTypeTree(tree)).orElse(null) + if (NullnessAnnotations.fromAnnotationTrees(annotationsIfAnnotatedTypeTree(tree)).orElse(null) == Nullness.NULLABLE) { return; } @@ -209,12 +209,9 @@ private boolean typeMatches(Type type, VisitorState state) { && NullnessAnnotations.fromAnnotationsOn(type).orElse(null) != Nullness.NULLABLE; } - private static ImmutableList annotationsIfAnnotatedTypeTree(Tree tree) { + private static List annotationsIfAnnotatedTypeTree(Tree tree) { if (tree instanceof AnnotatedTypeTree) { - AnnotatedTypeTree annotated = ((AnnotatedTypeTree) tree); - return annotated.getAnnotations().stream() - .map(ASTHelpers::getAnnotationName) - .collect(toImmutableList()); + return ((AnnotatedTypeTree) tree).getAnnotations(); } return ImmutableList.of(); }