From e366103ab298bb07b277c5a03c404977930358d1 Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Mon, 27 Jul 2026 13:50:57 -0700 Subject: [PATCH] Make ReturnsNullCollection and ProvidesNull use hasDefinitelyNullBranch. Update ReturnsNullCollection and ProvidesNull to use NullnessUtils.hasDefinitelyNullBranch instead of looking only for null literals, allowing them to flag conditional null expressions (e.g., return foo ? bar : null). Also make NullnessUtils and hasDefinitelyNullBranch public to support these checks. PiperOrigin-RevId: 954810744 --- .../errorprone/bugpatterns/ReturnsNullCollection.java | 10 ++++++++-- .../bugpatterns/inject/dagger/ProvidesNull.java | 10 ++++++++-- .../errorprone/bugpatterns/nullness/NullnessUtils.java | 4 ++-- .../bugpatterns/ReturnsNullCollectionTest.java | 4 ++-- .../bugpatterns/inject/dagger/ProvidesNullTest.java | 4 ++-- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java index ae2bf85af51..93cf0750e7c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java @@ -17,6 +17,7 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasDefinitelyNullBranch; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; @@ -24,8 +25,8 @@ import static com.google.errorprone.matchers.Matchers.methodReturns; import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod; import static com.google.errorprone.util.ASTHelpers.getSymbol; -import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; @@ -62,7 +63,12 @@ private static boolean methodWithoutNullable(MethodTree tree, VisitorState state @Override public final Description matchReturn(ReturnTree tree, VisitorState state) { - if (tree.getExpression() == null || tree.getExpression().getKind() != NULL_LITERAL) { + if (tree.getExpression() == null + || !hasDefinitelyNullBranch( + tree.getExpression(), + /* definitelyNullVars= */ ImmutableSet.of(), + /* varsProvenNullByParentIf= */ ImmutableSet.of(), + state)) { return NO_MATCH; } MethodTree methodTree = findEnclosingMethod(state); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java index d65917aa7c9..3a5e27e26fb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java @@ -17,8 +17,10 @@ package com.google.errorprone.bugpatterns.inject.dagger; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasDefinitelyNullBranch; import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -31,7 +33,6 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; -import com.sun.source.tree.Tree.Kind; import com.sun.tools.javac.code.Symbol.MethodSymbol; /** @@ -53,7 +54,12 @@ public class ProvidesNull extends BugChecker implements ReturnTreeMatcher { @Override public Description matchReturn(ReturnTree returnTree, VisitorState state) { ExpressionTree returnExpression = returnTree.getExpression(); - if (returnExpression == null || returnExpression.getKind() != Kind.NULL_LITERAL) { + if (returnExpression == null + || !hasDefinitelyNullBranch( + returnExpression, + /* definitelyNullVars= */ ImmutableSet.of(), + /* varsProvenNullByParentIf= */ ImmutableSet.of(), + state)) { return Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java index 37b0158639f..beb1c030721 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java @@ -82,7 +82,7 @@ * * @author awturner@google.com (Andy Turner) */ -final class NullnessUtils { +public final class NullnessUtils { private NullnessUtils() {} private static final Matcher OPTIONAL_OR_NULL = @@ -472,7 +472,7 @@ enum Polarity { } } - static boolean hasDefinitelyNullBranch( + public static boolean hasDefinitelyNullBranch( ExpressionTree tree, Set definitelyNullVars, /* diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnsNullCollectionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnsNullCollectionTest.java index 834cb8448c2..7179feb6204 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnsNullCollectionTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnsNullCollectionTest.java @@ -135,12 +135,12 @@ public void ternary_b536946282_shouldBeFlagged() { class Test { List methodReturnsNullTernary(boolean foo, List bar) { - // TODO(b/536946282): should be flagged by ReturnsNullCollection + // BUG: Diagnostic contains: ReturnsNullCollection return foo ? bar : null; } List methodReturnsNullTernaryReversed(boolean foo, List bar) { - // TODO(b/536946282): should be flagged by ReturnsNullCollection + // BUG: Diagnostic contains: ReturnsNullCollection return foo ? null : bar; } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNullTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNullTest.java index 19d67c19699..4c727c7c897 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNullTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNullTest.java @@ -253,13 +253,13 @@ public void ternary_b536946282_shouldBeFlagged() { public class Test { @Provides public Object providesObject(boolean foo, Object bar) { - // TODO(b/536946282): should be flagged by ProvidesNull + // BUG: Diagnostic contains: Did you mean '@Nullable' or 'throw new RuntimeException();' return foo ? bar : null; } @Provides public Object providesObjectReversed(boolean foo, Object bar) { - // TODO(b/536946282): should be flagged by ProvidesNull + // BUG: Diagnostic contains: Did you mean '@Nullable' or 'throw new RuntimeException();' return foo ? null : bar; } }