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 index 29428ffd807..ee876be351f 100644 --- 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 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 index b37797633e5..440303b7b4c 100644 --- 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()