From dfba048f6da0d5d1581c870e952d0363d0dd8a66 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 7 Nov 2023 15:13:57 -0800 Subject: [PATCH] Discourage multiple nullness annotations PiperOrigin-RevId: 580315454 --- .../NullnessAnnotations.java | 12 ++ .../nullness/MultipleNullnessAnnotations.java | 77 +++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../MultipleNullnessAnnotationsTest.java | 109 ++++++++++++++++++ 4 files changed, 200 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotations.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java 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 96229c09dac..0eedff067ba 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 @@ -26,6 +26,7 @@ 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; @@ -71,6 +72,17 @@ public static Optional fromAnnotationMirrors( return fromAnnotationStream(annotations.stream()); } + public static boolean annotationsAreAmbiguous( + Collection annotations) { + return annotations.stream() + .map(a -> simpleName(a).toString()) + .filter(ANNOTATION_RELEVANT_TO_NULLNESS) + .map(NULLABLE_ANNOTATION::test) + .distinct() + .count() + == 2; + } + public static ImmutableList annotationsRelevantToNullness( List annotations) { return annotations.stream() diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotations.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotations.java new file mode 100644 index 00000000000..552c6f92fa4 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotations.java @@ -0,0 +1,77 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.nullness; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.AnnotatedTypeTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotatedTypeTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; +import java.util.Collection; +import javax.lang.model.element.AnnotationMirror; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "This type use has conflicting nullness annotations", severity = WARNING) +public class MultipleNullnessAnnotations extends BugChecker + implements AnnotatedTypeTreeMatcher, MethodTreeMatcher, VariableTreeMatcher { + @Override + public Description matchAnnotatedType(AnnotatedTypeTree tree, VisitorState state) { + return match(tree, ASTHelpers.getType(tree).getAnnotationMirrors()); + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + return match(tree, ASTHelpers.getSymbol(tree), tree.getReturnType()); + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + return match(tree, ASTHelpers.getSymbol(tree), tree.getType()); + } + + private Description match(Tree tree, Symbol symbol, Tree type) { + if (type == null) { + return NO_MATCH; + } + return match( + tree, + ImmutableSet.builder() + .addAll(symbol.getAnnotationMirrors()) + .addAll(ASTHelpers.getType(type).getAnnotationMirrors()) + .build()); + } + + private Description match(Tree tree, Collection annotations) { + if (NullnessAnnotations.annotationsAreAmbiguous(annotations)) { + return describeMatch(tree); + } + return NO_MATCH; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index e7d44a184af..a1eb6c17095 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -536,6 +536,7 @@ import com.google.errorprone.bugpatterns.nullness.EqualsMissingNullable; import com.google.errorprone.bugpatterns.nullness.ExtendsObject; import com.google.errorprone.bugpatterns.nullness.FieldMissingNullable; +import com.google.errorprone.bugpatterns.nullness.MultipleNullnessAnnotations; import com.google.errorprone.bugpatterns.nullness.NullArgumentForNonNullParameter; import com.google.errorprone.bugpatterns.nullness.NullablePrimitive; import com.google.errorprone.bugpatterns.nullness.NullablePrimitiveArray; @@ -981,6 +982,7 @@ public static ScannerSupplier warningChecks() { ModifyCollectionInEnhancedForLoop.class, ModifySourceCollectionInStream.class, MultimapKeys.class, + MultipleNullnessAnnotations.class, MultipleParallelOrSequentialCalls.class, MultipleUnaryOperatorsInMethodCall.class, MutablePublicArray.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java new file mode 100644 index 00000000000..b4268e45c6b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java @@ -0,0 +1,109 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.nullness; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class MultipleNullnessAnnotationsTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(MultipleNullnessAnnotations.class, getClass()); + + @Test + public void positive() { + testHelper + .addSourceLines( + "Test.java", + "import org.checkerframework.checker.nullness.compatqual.NonNullDecl;", + "import org.checkerframework.checker.nullness.compatqual.NullableDecl;", + "import org.checkerframework.checker.nullness.qual.NonNull;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "import java.util.List;", + "abstract class Test {", + " // BUG: Diagnostic contains:", + " @Nullable @NonNull Object x;", + " // BUG: Diagnostic contains:", + " @NullableDecl static @NonNull Object y;", + " // BUG: Diagnostic contains:", + " List<@Nullable @NonNull String> z;", + " // BUG: Diagnostic contains:", + " @NullableDecl abstract @NonNull Object f();", + " // BUG: Diagnostic contains:", + " abstract void f(@NullableDecl Object @NonNull[] x);", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "import org.checkerframework.checker.nullness.compatqual.NonNullDecl;", + "import org.checkerframework.checker.nullness.compatqual.NullableDecl;", + "import org.checkerframework.checker.nullness.qual.NonNull;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "import java.util.List;", + "abstract class Test {", + " @NonNullDecl @NonNull Object x;", + " @NullableDecl static @Nullable Object y;", + "}") + .doTest(); + } + + @Test + public void disambiguation() { + testHelper + .addSourceLines( + "Test.java", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "import java.util.List;", + "abstract class Test {", + " @Nullable Object @Nullable [] x;", + " abstract void f(@Nullable Object @Nullable ... x);", + "}") + .doTest(); + } + + @Test + public void declarationAndType() { + testHelper + .addSourceLines( + "Nullable.java", + "import java.lang.annotation.Target;", + "import java.lang.annotation.ElementType;", + "@Target({", + " ElementType.METHOD,", + " ElementType.FIELD,", + " ElementType.PARAMETER,", + " ElementType.LOCAL_VARIABLE,", + " ElementType.TYPE_USE", + "})", + "public @interface Nullable {}") + .addSourceLines( + "Test.java", + "abstract class Test {", + " abstract void f(@Nullable Object x);", + " abstract @Nullable Object g();", + " @Nullable Object f;", + "}") + .doTest(); + } +}