From e5dfb88050741936c3e0121ab902d1541f17dd62 Mon Sep 17 00:00:00 2001 From: cushon Date: Sat, 21 Mar 2020 13:19:30 -0700 Subject: [PATCH 1/5] Turn down ReturnMissingNullable it is disabled by default, and has bugs. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=302219979 --- .../nullness/ReturnMissingNullable.java | 120 ---- .../scanner/BuiltInCheckerSuppliers.java | 2 - .../nullness/ReturnMissingNullableTest.java | 672 ------------------ 3 files changed, 794 deletions(-) delete mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java delete mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java deleted file mode 100644 index 6fd04d46ab5..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java +++ /dev/null @@ -1,120 +0,0 @@ -/* - * Copyright 2016 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.SUGGESTION; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.BugPattern.ProvidesFix; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; -import com.google.errorprone.dataflow.nullnesspropagation.Nullness; -import com.google.errorprone.dataflow.nullnesspropagation.TrustingNullnessAnalysis; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.ReturnTree; -import com.sun.source.tree.Tree; -import com.sun.source.tree.Tree.Kind; -import com.sun.source.util.TreePath; -import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.tree.JCTree.JCMethodDecl; -import javax.annotation.Nullable; - -/** @author kmb@google.com (Kevin Bierhoff) */ -@BugPattern( - name = "ReturnMissingNullable", - summary = "Methods that can return null should be annotated @Nullable", - severity = SUGGESTION, - providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION) -public class ReturnMissingNullable extends BugChecker implements ReturnTreeMatcher { - - @Override - public Description matchReturn(ReturnTree tree, VisitorState state) { - ExpressionTree returnExpression = tree.getExpression(); - if (returnExpression == null) { - return Description.NO_MATCH; - } - - // Best-effort try to avoid running the dataflow analysis - // TODO(kmb): bail on more non-null expressions, such as "this", arithmethic, logical, and &&/|| - if (ASTHelpers.constValue(returnExpression) != null) { - // This should include literals such as "true" or a string - return Description.NO_MATCH; - } - - JCMethodDecl method = findSurroundingMethod(state.getPath()); - if (method == null || isIgnoredReturnType(method, state)) { - return Description.NO_MATCH; - } - if (TrustingNullnessAnalysis.hasNullableAnnotation(method.sym)) { - return Description.NO_MATCH; - } - - // Don't need dataflow to tell us that null is nullable - if (returnExpression.getKind() == ExpressionTree.Kind.NULL_LITERAL) { - return makeFix(state, method, tree, "Returning null literal"); - } - - // OK let's see what dataflow says - Nullness nullness = - TrustingNullnessAnalysis.instance(state.context) - .getNullness(new TreePath(state.getPath(), returnExpression), state.context); - switch (nullness) { - case BOTTOM: - case NONNULL: - return Description.NO_MATCH; - case NULL: - return makeFix(state, method, tree, "Definitely returning null"); - case NULLABLE: - return makeFix(state, method, tree, "May return null"); - default: - throw new AssertionError("Impossible: " + nullness); - } - } - - private boolean isIgnoredReturnType(JCMethodDecl method, VisitorState state) { - Type returnType = method.sym.getReturnType(); - // Methods returning a primitive cannot return null. Also ignore Void-returning methods as - // the only valid Void value is null, so it's implied. - // TODO(kmb): Technically we should assume NULL when we see a call to a method that returns Void - return returnType.isPrimitiveOrVoid() - || state.getTypes().isSameType(returnType, state.getTypeFromString("java.lang.Void")); - } - - private Description makeFix( - VisitorState state, Tree declaration, Tree matchedTree, String message) { - return buildDescription(matchedTree) - .setMessage(message) - .addFix(NullnessFixes.makeFix(state, declaration)) - .build(); - } - - @Nullable - private static JCMethodDecl findSurroundingMethod(TreePath path) { - while (path.getLeaf().getKind() != Kind.METHOD) { - if (path.getLeaf().getKind() == Kind.LAMBDA_EXPRESSION) { - // Ignore return statements in lambda expressions. There's no method declaration to suggest - // annotations for anyway. - return null; - } - path = path.getParentPath(); - } - return (JCMethodDecl) path.getLeaf(); - } -} 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 867b880adde..519946735d4 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -403,7 +403,6 @@ import com.google.errorprone.bugpatterns.nullness.FieldMissingNullable; import com.google.errorprone.bugpatterns.nullness.NullableDereference; import com.google.errorprone.bugpatterns.nullness.ParameterNotNullable; -import com.google.errorprone.bugpatterns.nullness.ReturnMissingNullable; import com.google.errorprone.bugpatterns.nullness.UnnecessaryCheckNotNull; import com.google.errorprone.bugpatterns.overloading.InconsistentOverloads; import com.google.errorprone.bugpatterns.threadsafety.DoubleCheckedLocking; @@ -899,7 +898,6 @@ public static ScannerSupplier errorChecks() { RedundantThrows.class, RemoveUnusedImports.class, ReturnFromVoid.class, - ReturnMissingNullable.class, ScopeAnnotationOnInterfaceOrAbstractClass.class, ScopeOnModule.class, ScopeOrQualifierAnnotationRetention.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java deleted file mode 100644 index 6513ce89dcb..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java +++ /dev/null @@ -1,672 +0,0 @@ -/* - * Copyright 2016 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.BugCheckerRefactoringTestHelper; -import com.google.errorprone.CompilationTestHelper; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** @author kmb@google.com (Kevin Bierhoff) */ -@RunWith(JUnit4.class) -public class ReturnMissingNullableTest { - - @Test - public void testLiteralNullReturn() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class LiteralNullReturnTest {", - " public String getMessage() {", - " // BUG: Diagnostic contains: @Nullable", - " return null;", - " }", - "}") - .doTest(); - } - - @Test - public void testDefiniteNullReturn() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class LiteralNullReturnTest {", - " public String getMessage(String message) {", - " // BUG: Diagnostic contains: @Nullable", - " return message != null ? null : message;", - " }", - "}") - .doTest(); - } - - @Test - public void testMaybeNullReturn() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class LiteralNullReturnTest {", - " public String getMessage(int x) {", - " // BUG: Diagnostic contains: @Nullable", - " return x >= 0 ? null : \"negative\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableMethodCall() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableMethodCallTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class NullableMethodCallTest {", - " public String getMessage(int x) {", - " // BUG: Diagnostic contains: @Nullable", - " return toSignString(x);", - " }", - "", - " @Nullable", - " private String toSignString(int x) {", - " return x < 0 ? \"negative\" : \"positive\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableDeclMethodCall() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/NullableDecl.java", - "package com.google.anno.my;", - "public @interface NullableDecl {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableDeclMethodCallTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.NullableDecl;", - "public class NullableDeclMethodCallTest {", - " public String getMessage(int x) {", - " // BUG: Diagnostic contains: @Nullable", - " return toSignString(x);", - " }", - "", - " @NullableDecl", - " private String toSignString(int x) {", - " return x < 0 ? \"negative\" : \"positive\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableMethodCall_alternativeAnnotation() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableMethodCallTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NullableMethodCallTest {", - " public String getMessage(int x) {", - " // BUG: Diagnostic contains: @Nullable", - " return toSignString(x);", - " }", - "", - " @com.google.anno.my.Nullable", - " private String toSignString(int x) {", - " return x < 0 ? \"negative\" : \"positive\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableMethodCall_typeAnnotation() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableMethodCallTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NullableMethodCallTest {", - " public String getMessage(int x) {", - " // BUG: Diagnostic contains: @Nullable", - " return toSignString(x);", - " }", - "", - " @com.google.anno.my.Nullable", - " private String toSignString(int x) {", - " return x < 0 ? \"negative\" : \"positive\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableField() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableFieldTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class NullableFieldTest {", - " @Nullable private String message;", - " public String getMessage() {", - " // BUG: Diagnostic contains: @Nullable", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableField_typeAnnotation() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableFieldTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.Nullable;", - "public class NullableFieldTest {", - " @Nullable private String message;", - " public String getMessage() {", - " // BUG: Diagnostic contains: @Nullable", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableParameter_typeAnnotation() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NullableParameterTest {", - " public String apply(@com.google.anno.my.Nullable String message) {", - " // BUG: Diagnostic contains: @Nullable", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableParameter() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class NullableParameterTest {", - " public String apply(@Nullable String message) {", - " // BUG: Diagnostic contains: @Nullable", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNullableArrayParameter_typeAnnotation() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.Nullable;", - "public class NullableParameterTest {", - " public String[] apply(String @Nullable [] message) {", - " // BUG: Diagnostic contains: @Nullable", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testGenericTypeParameter() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/GenericTypeParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.Nullable;", - "public class GenericTypeParameterTest {", - " public String first(java.util.List<@Nullable String> names) {", - " // BUG: Diagnostic contains: @Nullable", - " return names.get(0);", - " }", - " public String first(java.util.Set<@Nullable String> names) {", - " // BUG: Diagnostic contains: @Nullable", - " return names.iterator().next();", // go through method returning generic type - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_alreadyAnnotated() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class LiteralNullReturnTest {", - " @Nullable", - " public String getMessage() {", - " return null;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_alreadyAnnotatedNullableDecl() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/NullableDecl.java", - "package com.google.anno.my;", - "public @interface NullableDecl {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.NullableDecl;", - "public class LiteralNullReturnTest {", - " @NullableDecl", - " public String getMessage() {", - " return null;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_alreadyTypeAnnotated() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/TypeAnnoReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class TypeAnnoReturnTest {", - " public @com.google.anno.my.Nullable String getMessage() {", - " return null;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_checkNotNullNullableInput() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class NullReturnTest {", - " @Nullable String message;", - " public String getMessage() {", - " return checkNotNull(message);", - " }", - // One style of "check not null" method, whose type argument is unannotated, and accepts - // a @Nullable input. - " private static T checkNotNull(@Nullable T obj) {", - " if (obj==null) throw new NullPointerException();", - " return obj;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nonNullArrayWithNullableElements() { - createCompilationTestHelper() - .addSourceLines( - "com/google/anno/my/Nullable.java", - "package com.google.anno.my;", - "import java.lang.annotation.ElementType;", - "import java.lang.annotation.Target;", - "@Target({ElementType.TYPE_USE})", - "public @interface Nullable {}") - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NullableParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import com.google.anno.my.Nullable;", - "public class NullableParameterTest {", - " public String[] apply(@Nullable String[] message) {", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nonNullLiteral() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class LiteralNullReturnTest {", - " public String getMessage() {", - " return \"hello\";", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nonNullMethod() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NonNullMethodTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NonNullMethodTest {", - " public String getMessage(int x) {", - " return String.valueOf(x);", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nonNullField() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NonNullFieldTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NonNullFieldTest {", - " private String message;", - " public String getMessage() {", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nonNullParameter() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/NonNullParameterTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class NonNullParameterTest {", - " public String apply(String message) {", - " return message;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_this() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/ThisTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class ThisTest {", - " private String message;", - " public ThisTest setMessage(String message) {", - " this.message = message;", - " return this;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_capturedLocal() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/CapturedLocalTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public abstract class CapturedLocalTest {", - " public abstract String getMessage();", - " public CapturedLocalTest withMessage(final String message) {", - " return new CapturedLocalTest() {", - " public String getMessage() {", - " return message;", - " }", - " };", - " }", - "}") - .doTest(); - } - - /** - * Makes sure the check never flags methods returning a primitive. Returning null from them is a - * bug, of course, but we're not trying to find those bugs in this check. - */ - @Test - public void testNegativeCases_primitiveReturnType() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/PrimitiveReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class PrimitiveReturnTest {", - " public int getCount() {", - " return (Integer) null;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_voidMethod() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/VoidMethodTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class VoidMethodTest {", - " public void run(int iterations) {", - " if (iterations <= 0) { return; }", - " run(iterations - 1);", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_voidTypedMethod() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/VoidTypeTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "public class VoidTypeTest {", - " public Void run(int iterations) {", - " if (iterations <= 0) { return null; }", - " run(iterations - 1);", - " return null;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_nullableReturnInLambda() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/MissingNullableReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class MissingNullableReturnTest {", - " public static final java.util.function.Function IDENTITY =", - " (s -> { return s != null ? s : null; });", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_returnLambda() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/MissingNullableReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class MissingNullableReturnTest {", - " public static java.util.function.Function identity() {", - " return s -> s;", - " }", - "}") - .doTest(); - } - - @Test - public void testNegativeCases_returnParenthesizedLambda() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/MissingNullableReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "public class MissingNullableReturnTest {", - " public static java.util.function.Function identity() {", - " return (s -> s);", - " }", - "}") - .doTest(); - } - - // Regression test for b/110812469; verifies that untracked access paths that mix field access - // and method invocation are "trusted" to yield nonNull values - @Test - public void testNegativeCases_mixedMethodFieldAccessPath() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/MissingNullableReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nonnull;", - "public class MissingNullableReturnTest {", - " public @Nonnull MyClass test() {", - " return ((MyClass) null).myMethod().myField;", - " }", - " abstract class MyClass {", - " abstract MyClass myMethod();", - " MyClass myField;", - " }", - "}") - .doTest(); - } - - // Regression test for b/113123074 - @Test - public void testNegativeCases_delegate() { - createCompilationTestHelper() - .addSourceLines( - "com/google/errorprone/bugpatterns/nullness/MissingNullableReturnTest.java", - "package com.google.errorprone.bugpatterns.nullness;", - "import javax.annotation.Nullable;", - "import java.util.Optional;", - "public class MissingNullableReturnTest {", - " public String get() {", - " return getInternal(true, null);", - " }", - " private String getInternal(boolean flag, @Nullable Integer i) {", - " return \"hello\";", - " }", - "}") - .doTest(); - } - - @Test - public void testSuggestNonJsr305Nullable() { - createRefactoringTestHelper() - .addInputLines( - "in/Test.java", - "class T {", - " @Nullable private final Object obj1 = null;", - " private final Object method() { return null; }", - " @interface Nullable {}", - "}") - .addOutputLines( - "out/Test.java", - "class T {", - " @Nullable private final Object obj1 = null;", - " @Nullable private final Object method() { return null; }", - " @interface Nullable {}", - "}") - .doTest(); - } - - @Test - public void testNonAnnotationNullable() { - createRefactoringTestHelper() - .addInputLines( - "in/Test.java", - "class T {", - " private final Object method() { return null; }", - " class Nullable {}", - "}") - .addOutputLines( - "out/Test.java", - "class T {", - " @javax.annotation.Nullable private final Object method() { return null; }", - " class Nullable {}", - "}") - .doTest(); - } - - private CompilationTestHelper createCompilationTestHelper() { - return CompilationTestHelper.newInstance(ReturnMissingNullable.class, getClass()); - } - - private BugCheckerRefactoringTestHelper createRefactoringTestHelper() { - return BugCheckerRefactoringTestHelper.newInstance(new ReturnMissingNullable(), getClass()); - } -} From f838f788703c7ec1fcdac4ac593d0f059682087d Mon Sep 17 00:00:00 2001 From: Vincent Privat Date: Mon, 23 Mar 2020 03:19:00 -0700 Subject: [PATCH 2/5] Finally: Replace `.label` by `.getLabel()` See #1106 - field removed in JDK12 as per Fixes #1107 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=302397069 --- .../main/java/com/google/errorprone/bugpatterns/Finally.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java b/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java index 8d65ede89da..2ff0a5de383 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java @@ -163,12 +163,12 @@ private enum JumpType { } public FinallyJumpMatcher(JCContinue jcContinue) { - this.label = jcContinue.label; + this.label = jcContinue.getLabel(); this.jumpType = JumpType.CONTINUE; } public FinallyJumpMatcher(JCBreak jcBreak) { - this.label = jcBreak.label; + this.label = jcBreak.getLabel(); this.jumpType = JumpType.BREAK; } From 1477732a8535eee2492a55ec35826696061f49a9 Mon Sep 17 00:00:00 2001 From: ghm Date: Mon, 23 Mar 2020 05:07:25 -0700 Subject: [PATCH 3/5] Obtain Descriptions for testing via BugCheckers, rather than via Description's constructor. To fulfil a TODO, and allow tidying up the API. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=302410595 --- .../errorprone/matchers/Description.java | 17 +-- .../bugpatterns/ProvidesFixCheckerTest.java | 29 ----- .../refaster/DescriptionBasedDiffTest.java | 121 ++++++++---------- 3 files changed, 57 insertions(+), 110 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Description.java b/check_api/src/main/java/com/google/errorprone/matchers/Description.java index 05571bf95ea..e593cf02708 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Description.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Description.java @@ -45,8 +45,6 @@ public class Description { new Description( null, "", "", "", ImmutableList.of(), SUGGESTION); - private static final String UNDEFINED_CHECK_NAME = "Undefined"; - /** The position of the match. */ public final DiagnosticPosition position; @@ -94,24 +92,11 @@ public String getMessageWithoutCheckName() { : String.format("%s", rawMessage); } - /** TODO(cushon): Remove this constructor and ensure that there's always a check name. */ - @Deprecated - public Description( - Tree node, String message, Fix suggestedFix, BugPattern.SeverityLevel severity) { - this( - (DiagnosticPosition) node, - UNDEFINED_CHECK_NAME, - message, - message, - ImmutableList.of(suggestedFix), - severity); - } - private Description( DiagnosticPosition position, String checkName, String rawMessage, - String linkUrl, + @Nullable String linkUrl, List fixes, SeverityLevel severity) { this.position = position; diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java index cc88a00b0d0..e74eeeb11c0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java @@ -220,33 +220,4 @@ public void erroneousFixTag() { "}") .doTest(); } - - @Test - public void nestedNewClass() { - testHelper - .addSourceLines( - "ExampleChecker.java", - "import com.google.errorprone.BugPattern;", - "import com.google.errorprone.BugPattern.SeverityLevel;", - "import com.google.errorprone.VisitorState;", - "import com.google.errorprone.bugpatterns.BugChecker;", - "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", - "import com.google.errorprone.fixes.SuggestedFix;", - "import com.google.errorprone.matchers.Description;", - "import com.sun.source.tree.ClassTree;", - "// BUG: Diagnostic contains: ProvidesFix", - "@BugPattern(name = \"Example\", summary = \"\", severity = SeverityLevel.ERROR)", - "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", - " @Override public Description matchClass(ClassTree tree, VisitorState s) {", - " Helper wrapper = new Helper(new Description(", - " tree, \"\", SuggestedFix.builder().build(), SeverityLevel.ERROR));", - " return wrapper.d;", - " }", - " static class Helper {", - " public Description d;", - " public Helper(Description d) { this.d = d; }", - " }", - "}") - .doTest(); - } } diff --git a/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java b/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java index 748649bb0a2..f386f792fa9 100644 --- a/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java @@ -18,12 +18,17 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.apply.DescriptionBasedDiff; import com.google.errorprone.apply.ImportOrganizer; +import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.sun.tools.javac.tree.EndPosTable; +import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -70,9 +75,7 @@ public void noDiffs() { @Test public void oneDiff() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - diff.onDescribed( - new Description( - null, "message", SuggestedFix.replace(137, 140, "bar"), SeverityLevel.SUGGESTION)); + diff.onDescribed(dummyDescription(SuggestedFix.replace(137, 140, "bar"))); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -91,9 +94,7 @@ public void oneDiff() { @Test public void prefixDiff() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - diff.onDescribed( - new Description( - null, "message", SuggestedFix.replace(140, 140, "bar"), SeverityLevel.SUGGESTION)); + diff.onDescribed(dummyDescription(SuggestedFix.replace(140, 140, "bar"))); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -113,11 +114,8 @@ public void prefixDiff() { public void twoDiffs() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); diff.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().replace(124, 127, "longer").replace(137, 140, "bar").build(), - SeverityLevel.SUGGESTION)); + dummyDescription( + SuggestedFix.builder().replace(124, 127, "longer").replace(137, 140, "bar").build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -140,49 +138,27 @@ public void overlappingDiffs_throws() { IllegalArgumentException.class, () -> diff.onDescribed( - new Description( - null, - "message", + dummyDescription( SuggestedFix.builder() .replace(137, 140, "baz") .replace(137, 140, "bar") - .build(), - SeverityLevel.SUGGESTION))); + .build()))); DescriptionBasedDiff diff2 = createDescriptionBasedDiff(); - diff2.onDescribed( - new Description( - null, - "bah", - SuggestedFix.builder().replace(137, 140, "baz").build(), - SeverityLevel.SUGGESTION)); + diff2.onDescribed(dummyDescription(SuggestedFix.builder().replace(137, 140, "baz").build())); assertThrows( IllegalArgumentException.class, () -> diff2.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().replace(137, 140, "bar").build(), - SeverityLevel.SUGGESTION))); + dummyDescription(SuggestedFix.builder().replace(137, 140, "bar").build()))); DescriptionBasedDiff diff3 = DescriptionBasedDiff.createIgnoringOverlaps( compilationUnit, ImportOrganizer.STATIC_FIRST_ORGANIZER); - diff3.onDescribed( - new Description( - null, - "bah", - SuggestedFix.builder().replace(137, 140, "baz").build(), - SeverityLevel.SUGGESTION)); + diff3.onDescribed(dummyDescription(SuggestedFix.builder().replace(137, 140, "baz").build())); // No throw, since it's lenient. Refactors to the first "baz" replacement and ignores this. - diff3.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().replace(137, 140, "bar").build(), - SeverityLevel.SUGGESTION)); + diff3.onDescribed(dummyDescription(SuggestedFix.builder().replace(137, 140, "bar").build())); diff3.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) @@ -203,11 +179,7 @@ public void overlappingDiffs_throws() { public void applyDifferences_addsImportAndSorts_whenAddingNewImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); diff.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().addImport("com.google.foo.Bar").build(), - SeverityLevel.SUGGESTION)); + dummyDescription(SuggestedFix.builder().addImport("com.google.foo.Bar").build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -227,12 +199,7 @@ public void applyDifferences_addsImportAndSorts_whenAddingNewImport() { @Test public void applyDifferences_preservesImportOrder_whenAddingExistingImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - diff.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().addImport("com.foo.Bar").build(), - SeverityLevel.SUGGESTION)); + diff.onDescribed(dummyDescription(SuggestedFix.builder().addImport("com.foo.Bar").build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -252,11 +219,11 @@ public void applyDifferences_preservesImportOrder_whenAddingExistingImport() { public void removeImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); diff.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().removeImport("com.foo.Bar").removeImport("org.bar.Baz").build(), - SeverityLevel.SUGGESTION)); + dummyDescription( + SuggestedFix.builder() + .removeImport("com.foo.Bar") + .removeImport("org.bar.Baz") + .build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -275,11 +242,7 @@ public void removeImport() { public void applyDifferences_preservesOrder_whenRemovingNonExistentImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); diff.onDescribed( - new Description( - null, - "message", - SuggestedFix.builder().removeImport("com.google.foo.Bar").build(), - SeverityLevel.SUGGESTION)); + dummyDescription(SuggestedFix.builder().removeImport("com.google.foo.Bar").build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -299,15 +262,12 @@ public void applyDifferences_preservesOrder_whenRemovingNonExistentImport() { public void twoDiffsWithImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); diff.onDescribed( - new Description( - null, - "message", + dummyDescription( SuggestedFix.builder() .replace(124, 127, "longer") .replace(137, 140, "bar") .addImport("com.google.foo.Bar") - .build(), - SeverityLevel.SUGGESTION)); + .build())); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -323,4 +283,35 @@ public void twoDiffsWithImport() { "}") .inOrder(); } + + @BugPattern(name = "Test", summary = "", severity = SeverityLevel.WARNING) + static final class DummyChecker extends BugChecker {} + + private static Description dummyDescription(SuggestedFix fix) { + return BugChecker.buildDescriptionFromChecker( + new DiagnosticPosition() { + @Override + public JCTree getTree() { + return null; + } + + @Override + public int getStartPosition() { + return 0; + } + + @Override + public int getPreferredPosition() { + return 0; + } + + @Override + public int getEndPosition(EndPosTable endPosTable) { + return 0; + } + }, + new DummyChecker()) + .addFix(fix) + .build(); + } } From 6744ce2558418a2e2fa2235e1ba5271fae8df6f2 Mon Sep 17 00:00:00 2001 From: ghm Date: Mon, 23 Mar 2020 06:39:36 -0700 Subject: [PATCH 4/5] Fix two typos (for the price of one) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=302422163 --- .../src/main/java/com/google/errorprone/util/ASTHelpers.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 37d6718c2d7..35ece979440 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java @@ -1284,9 +1284,9 @@ public static MethodSymbol resolveExistingMethod( } /** - * Returns the value of the {@code @Generated} annotation on encosing classes, if present. + * Returns the value of the {@code @Generated} annotation on enclosing classes, if present. * - *

Although {@code @Generated} can be applied to non-class progam elements, there are no known + *

Although {@code @Generated} can be applied to non-class program elements, there are no known * cases of that happening, so it isn't supported here. */ public static ImmutableSet getGeneratedBy(VisitorState state) { From 363cca174dfccd8c73692d477f6b79dd2ad599a9 Mon Sep 17 00:00:00 2001 From: ghm Date: Mon, 23 Mar 2020 06:56:09 -0700 Subject: [PATCH 5/5] Enable Javadoc-related checkers by default ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=302424445 --- .../scanner/BuiltInCheckerSuppliers.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) 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 519946735d4..1af6a205353 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -638,6 +638,7 @@ public static ScannerSupplier errorChecks() { /** A list of all checks with severity WARNING that are on by default. */ public static final ImmutableSet ENABLED_WARNINGS = getSuppliers( + AlmostJavadoc.class, AmbiguousMethodReference.class, AnnotateFormatMethod.class, ArrayAsKeyOfSetOrMap.class, @@ -675,10 +676,12 @@ public static ScannerSupplier errorChecks() { DefaultCharset.class, DoubleBraceInitialization.class, DoubleCheckedLocking.class, + EmptyBlockTag.class, EqualsGetClass.class, EqualsIncompatibleType.class, EqualsUnsafeCast.class, EqualsUsingHashCode.class, + EscapedEntity.class, ExtendingJUnitAssert.class, FallThrough.class, Finally.class, @@ -695,10 +698,16 @@ public static ScannerSupplier errorChecks() { InconsistentCapitalization.class, InconsistentHashCode.class, IncrementInForLoopAndHeader.class, + InheritDoc.class, InjectOnConstructorOfAbstractClass.class, InputStreamSlowMultibyteRead.class, InstanceOfAndCastMatchWrongType.class, IntLongMath.class, + InvalidBlockTag.class, + InvalidInlineTag.class, + InvalidLink.class, + InvalidParam.class, + InvalidThrows.class, IterableAndIterator.class, JavaDurationGetSecondsGetNano.class, JavaDurationWithNanos.class, @@ -729,6 +738,7 @@ public static ScannerSupplier errorChecks() { MissingCasesInEnumSwitch.class, MissingFail.class, MissingOverride.class, + MissingSummary.class, MixedDescriptors.class, MixedMutabilityReturnType.class, ModifiedButNotUsed.class, @@ -768,6 +778,7 @@ public static ScannerSupplier errorChecks() { ReachabilityFenceUsage.class, ReferenceEquality.class, RequiredModifiersChecker.class, + ReturnFromVoid.class, RxReturnValueIgnored.class, SameNameButDifferent.class, ShortCircuitBoolean.class, @@ -792,6 +803,7 @@ public static ScannerSupplier errorChecks() { TypeParameterShadowing.class, TypeParameterUnusedInFormals.class, UndefinedEquals.class, + UnescapedEntity.class, UnnecessaryAnonymousClass.class, UnnecessaryLambda.class, UnnecessaryMethodInvocationMatcher.class, @@ -812,7 +824,6 @@ public static ScannerSupplier errorChecks() { /** A list of all checks that are off by default. */ public static final ImmutableSet DISABLED_CHECKS = getSuppliers( - AlmostJavadoc.class, AndroidJdkLibsChecker.class, AnnotationPosition.class, AssertFalse.class, @@ -833,12 +844,10 @@ public static ScannerSupplier errorChecks() { DepAnn.class, DescribeMatch.class, DivZero.class, - EmptyBlockTag.class, EmptyIfStatement.class, EmptySetMultibindingContributions.class, EmptyTopLevelDeclaration.class, EqualsBrokenForNull.class, - EscapedEntity.class, ExpectedExceptionChecker.class, ExpectedExceptionRefactoring.class, FieldCanBeFinal.class, @@ -851,17 +860,11 @@ public static ScannerSupplier errorChecks() { ImmutableRefactoring.class, ImplementAssertionWithChaining.class, InconsistentOverloads.class, - InheritDoc.class, InjectedConstructorAnnotations.class, InsecureCipherMode.class, InterfaceWithOnlyStatics.class, InterruptedExceptionSwallowed.class, - InvalidBlockTag.class, - InvalidInlineTag.class, - InvalidLink.class, - InvalidParam.class, InvalidTargetingOnScopingAnnotation.class, - InvalidThrows.class, IterablePathParameter.class, JMockTestWithoutRunWithOrRuleAnnotation.class, Java7ApiChecker.class, @@ -871,7 +874,6 @@ public static ScannerSupplier errorChecks() { LongLiteralLowerCaseSuffix.class, MethodCanBeStatic.class, MissingDefault.class, - MissingSummary.class, MixedArrayDimensions.class, MoreThanOneQualifier.class, MultiVariableDeclaration.class, @@ -897,7 +899,6 @@ public static ScannerSupplier errorChecks() { RedundantOverride.class, RedundantThrows.class, RemoveUnusedImports.class, - ReturnFromVoid.class, ScopeAnnotationOnInterfaceOrAbstractClass.class, ScopeOnModule.class, ScopeOrQualifierAnnotationRetention.class, @@ -915,7 +916,6 @@ public static ScannerSupplier errorChecks() { TransientMisuse.class, TryFailRefactoring.class, TypeParameterNaming.class, - UnescapedEntity.class, UngroupedOverloads.class, UnlockMethodChecker.class, UnnecessaryBoxedAssignment.class,