From 240e3803f96bd960098623e5d70cb8917d1136a4 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 11 Jun 2026 01:14:13 -0700 Subject: [PATCH] Suggest `Objects.requireNonNull` instead of `if`/`throw` PiperOrigin-RevId: 930358085 --- .../nullness/RequireNonNullRefactoring.java | 136 ++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../RequireNonNullRefactoringTest.java | 206 ++++++++++++++++++ .../nullness/RequireNonNullRefactoring.md | 15 ++ 4 files changed, 359 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoring.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoringTest.java create mode 100644 docs/bugpattern/nullness/RequireNonNullRefactoring.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoring.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoring.java new file mode 100644 index 00000000000..83e95ba9166 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoring.java @@ -0,0 +1,136 @@ +/* + * Copyright 2026 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 static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.suppliers.Suppliers.typeFromString; +import static com.google.errorprone.util.ASTHelpers.constValue; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static com.google.errorprone.util.ASTHelpers.stripParentheses; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.ThrowTree; +import com.sun.source.tree.Tree.Kind; +import com.sun.tools.javac.code.Type; +import java.util.List; +import org.jspecify.annotations.Nullable; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "Refactor explicit null checks to Objects.requireNonNull", + severity = SUGGESTION) +public final class RequireNonNullRefactoring extends BugChecker implements IfTreeMatcher { + + private static final Supplier NULL_POINTER_EXCEPTION = + typeFromString("java.lang.NullPointerException"); + + @Override + public Description matchIf(IfTree ifTree, VisitorState state) { + if (ifTree.getElseStatement() != null) { + return NO_MATCH; + } + ExpressionTree targetExpr = getNullCheckedExpression(ifTree.getCondition()); + if (targetExpr == null) { + return NO_MATCH; + } + ThrowTree throwTree = getThrowStatement(ifTree.getThenStatement()); + if (throwTree == null) { + return NO_MATCH; + } + ExpressionTree thrownExpr = stripParentheses(throwTree.getExpression()); + if (!(thrownExpr instanceof NewClassTree newClassTree)) { + return NO_MATCH; + } + if (!isSameType(getType(newClassTree), NULL_POINTER_EXCEPTION.get(state), state)) { + return NO_MATCH; + } + List arguments = newClassTree.getArguments(); + @Nullable ExpressionTree messageExpr; + if (arguments.isEmpty()) { + messageExpr = null; + } else if (arguments.size() == 1) { + messageExpr = arguments.get(0); + if (constValue(messageExpr, String.class) == null) { + return NO_MATCH; + } + } else { + return NO_MATCH; + } + SuggestedFix.Builder fix = SuggestedFix.builder(); + buildFix(fix, ifTree, targetExpr, messageExpr, state); + return describeMatch(ifTree, fix.build()); + } + + private static void buildFix( + SuggestedFix.Builder fix, + IfTree ifTree, + ExpressionTree targetExpr, + @Nullable ExpressionTree messageExpr, + VisitorState state) { + String requireNonNullQualified = + SuggestedFixes.qualifyStaticImport("java.util.Objects.requireNonNull", fix, state); + String targetExprSource = state.getSourceForNode(targetExpr); + String replacement = + String.format( + "%s(%s%s);", + requireNonNullQualified, + targetExprSource, + messageExpr == null ? "" : ", " + state.getSourceForNode(messageExpr)); + fix.replace(ifTree, replacement); + } + + private static @Nullable ExpressionTree getNullCheckedExpression(ExpressionTree condition) { + condition = stripParentheses(condition); + if (condition.getKind() != Kind.EQUAL_TO) { + return null; + } + BinaryTree binary = (BinaryTree) condition; + if (binary.getLeftOperand().getKind() == Kind.NULL_LITERAL) { + return binary.getRightOperand(); + } + if (binary.getRightOperand().getKind() == Kind.NULL_LITERAL) { + return binary.getLeftOperand(); + } + return null; + } + + private static @Nullable ThrowTree getThrowStatement(StatementTree then) { + while (then instanceof BlockTree block) { + var statements = block.getStatements(); + if (statements.size() != 1) { + return null; + } + then = statements.getFirst(); + } + return then instanceof ThrowTree throwTree ? throwTree : null; + } +} 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 522664a51f7..e38c951deac 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -611,6 +611,7 @@ import com.google.errorprone.bugpatterns.nullness.NullableWildcard; import com.google.errorprone.bugpatterns.nullness.ParameterMissingNullable; import com.google.errorprone.bugpatterns.nullness.RedundantNullCheck; +import com.google.errorprone.bugpatterns.nullness.RequireNonNullRefactoring; import com.google.errorprone.bugpatterns.nullness.ReturnMissingNullable; import com.google.errorprone.bugpatterns.nullness.UnnecessaryCheckNotNull; import com.google.errorprone.bugpatterns.nullness.UnsafeWildcard; @@ -1321,6 +1322,7 @@ public static ScannerSupplier warningChecks() { RedundantThrows.class, RefersToDaggerCodegen.class, RemoveUnusedImports.class, + RequireNonNullRefactoring.class, ReturnMissingNullable.class, ReturnsNullCollection.class, ScopeOnModule.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoringTest.java new file mode 100644 index 00000000000..71c177e9580 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RequireNonNullRefactoringTest.java @@ -0,0 +1,206 @@ +/* + * Copyright 2026 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 org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link RequireNonNullRefactoring}. */ +@RunWith(JUnit4.class) +public final class RequireNonNullRefactoringTest { + + private final BugCheckerRefactoringTestHelper testHelper = + BugCheckerRefactoringTestHelper.newInstance(RequireNonNullRefactoring.class, getClass()); + + @Test + public void refactoringNoMessage() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar == null) { + throw new NullPointerException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.util.Objects.requireNonNull; + + class Test { + void foo(Object bar) { + requireNonNull(bar); + } + } + """) + .doTest(); + } + + @Test + public void refactoringConstantMessage() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (null == bar) { + throw new NullPointerException("bar must not be null"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.util.Objects.requireNonNull; + + class Test { + void foo(Object bar) { + requireNonNull(bar, "bar must not be null"); + } + } + """) + .doTest(); + } + + @Test + public void noBraces() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar == null) throw new NullPointerException(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.util.Objects.requireNonNull; + + class Test { + void foo(Object bar) { + requireNonNull(bar); + } + } + """) + .doTest(); + } + + @Test + public void negativeWrongException() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar == null) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void negativeWrongCondition() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar != null) { + throw new NullPointerException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void negativeMultipleStatements() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar == null) { + System.out.println("bar is null!"); + throw new NullPointerException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void negativeNonConstantMessage() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + String msg = "bar is null"; + if (bar == null) { + throw new NullPointerException(msg); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void negativeHasElse() { + testHelper + .addInputLines( + "Test.java", + """ + class Test { + void foo(Object bar) { + if (bar == null) { + throw new NullPointerException(); + } else { + System.out.println("ok"); + } + } + } + """) + .expectUnchanged() + .doTest(); + } +} diff --git a/docs/bugpattern/nullness/RequireNonNullRefactoring.md b/docs/bugpattern/nullness/RequireNonNullRefactoring.md new file mode 100644 index 00000000000..b24b66931f1 --- /dev/null +++ b/docs/bugpattern/nullness/RequireNonNullRefactoring.md @@ -0,0 +1,15 @@ +Consider using `requireNonNull(value)` which is shorter and more readable than + +``` +if (value == null) { + throw new NullPointerException(...); +} +``` + +The main reason to prefer `requireNonNull` is for readability, but it has no +performance downsides and may have a small performance benefit. + +The method is annotated with `@ForceInline` and receives special treatment from +the JVM to ensure it is inlined into equivalent code to the `if`/`throw`. It +also results in slightly smaller Java bytecode, which can improve other JIT +inlining decisions.