For methods whose entire body is return null, begin adding `@Nullab…

…le` _if_ the method can't be overridden. Anyone who calls such a method can definitely see a null result. In contrast, if the method were overridable, it might be only a "stub" that is overridden by all implementations to return non-null values. (Also, update the comment about such "stub" methods to reflect that they are indeed common enough to be a problem, as discussed in #2910 (comment)) PiperOrigin-RevId: 427291930
google · Feb 8, 2022 · 5d9d989 · 5d9d989
1 parent f11e639
commit 5d9d989
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 6 deletions.
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
@@ -34,6 +34,7 @@
 import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
+import static com.google.errorprone.util.ASTHelpers.methodCanBeOverridden;
 import static com.sun.source.tree.Tree.Kind.NULL_LITERAL;
 import static java.lang.Boolean.FALSE;
 import static java.util.regex.Pattern.compile;
@@ -310,21 +311,23 @@ void doVisitReturn(ReturnTree returnTree) {
           return;
         }
 
+        MethodSymbol method = getSymbol(methodTree);
         List<? extends StatementTree> statements = methodTree.getBody().getStatements();
         if (beingConservative
             && statements.size() == 1
             && getOnlyElement(statements) == returnTree
-            && returnExpression.getKind() == NULL_LITERAL) {
+            && returnExpression.getKind() == NULL_LITERAL
+            && methodCanBeOverridden(method)) {
           /*
-           * When the entire method body is `return null`, I worry that this may be a stub
-           * implementation that all "real" implementations are meant to override. Ideally such
-           * stubs would use implementation like `throw new UnsupportedOperationException()`, but
-           * let's assume the worst.
+           * When the entire body of an overrideable method is `return null`, we are sometimes
+           * looking at a "stub" implementation that all "real" implementations are meant to
+           * override. Ideally such stubs would use implementation like `throw new
+           * UnsupportedOperationException()`, but we see `return null` often enough in practice
+           * that it warrants a special case when running in conservative mode.
            */
           return;
         }
 
-        MethodSymbol method = getSymbol(methodTree);
         Type returnType = method.getReturnType();
         if (beingConservative && isVoid(returnType, state)) {
           // `@Nullable Void` is accurate but noisy, so some users won't want it.

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
@@ -511,6 +511,21 @@ public void testOnlyIfAlreadyInScopeAndItIs() {
         .doTest();
   }
 
+  @Test
+  public void testOnlyStatementIsNullReturnButCannotBeOverridden() {
+    createCompilationTestHelper()
+        .addSourceLines(
+            "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
+            "package com.google.errorprone.bugpatterns.nullness;",
+            "public final class LiteralNullReturnTest {",
+            "  public String getMessage() {",
+            "    // BUG: Diagnostic contains: @Nullable",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testArrayDeclaration() {
     createRefactoringTestHelper()