diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PreferPreconditions.java b/core/src/main/java/com/google/errorprone/bugpatterns/PreferPreconditions.java new file mode 100644 index 00000000000..fc6bd334b4f --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PreferPreconditions.java @@ -0,0 +1,416 @@ +/* + * 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; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.constructor; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.containsComments; +import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.stripParentheses; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.ThrowTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.UnaryTree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeTag; +import java.util.List; +import javax.inject.Inject; +import javax.lang.model.element.ElementKind; +import org.jspecify.annotations.Nullable; + +/** + * A check that suggests using {@link com.google.common.base.Preconditions} instead of explicit + * if-throw checks on method parameters. + */ +@BugPattern( + summary = "Consider using Preconditions instead of explicit if-throw for parameter validation.", + severity = WARNING) +public final class PreferPreconditions extends BugChecker implements IfTreeMatcher { + + private final ConstantExpressions constantExpressions; + + @Inject + PreferPreconditions(ErrorProneFlags flags) { + this.constantExpressions = ConstantExpressions.fromFlags(flags); + } + + @Override + public Description matchIf(IfTree tree, VisitorState state) { + if (tree.getElseStatement() != null) { + return NO_MATCH; + } + Tree parent = state.getPath().getParentPath().getLeaf(); + if (parent instanceof IfTree) { + return NO_MATCH; + } + + ThrowTree throwTree = getSingleThrow(tree.getThenStatement()); + if (throwTree == null) { + return NO_MATCH; + } + + NewClassTree newClass = getThrownNewClass(throwTree.getExpression()); + if (newClass == null) { + return NO_MATCH; + } + + ExpressionTree condition = stripParentheses(tree.getCondition()); + if (!usesMethodParameter(condition)) { + return NO_MATCH; + } + + if (!areArgumentsSideEffectFree(newClass, state)) { + return NO_MATCH; + } + + // Don't refactor conditions with logical operators, as they can become quite complex and + // difficult to read when negated. + if (isHardToNegate(condition)) { + return NO_MATCH; + } + + // Keep this as late as possible to avoid scanning the tree for comments unnecessarily. + if (containsComments(tree, state)) { + return NO_MATCH; + } + + ExpressionTree checkedExpr = getNullCheckedExpression(condition); + if (NEW_NPE.matches(newClass, state) && checkedExpr != null) { + return suggest(tree, "checkNotNull", state.getSourceForNode(checkedExpr), newClass, state); + } + + if (NEW_IAE.matches(newClass, state)) { + return suggest(tree, "checkArgument", negate(condition, state), newClass, state); + } + + if (NEW_ISE.matches(newClass, state)) { + return suggest(tree, "checkState", negate(condition, state), newClass, state); + } + + return NO_MATCH; + } + + /** + * Returns the {@link ThrowTree} if {@code then} is a single throw statement, or a block + * containing a single throw statement; otherwise returns {@code null}. + */ + private static @Nullable ThrowTree getSingleThrow(StatementTree then) { + if (then instanceof ThrowTree throwTree) { + return throwTree; + } + if (then instanceof BlockTree block) { + List statements = block.getStatements(); + if (statements.size() == 1 && statements.getFirst() instanceof ThrowTree throwTree) { + return throwTree; + } + } + return null; + } + + /** + * Returns {@code expr} as a {@link NewClassTree} if it's a constructor call of the thrown + * exception; otherwise returns {@code null}. + */ + private static @Nullable NewClassTree getThrownNewClass(ExpressionTree expr) { + return stripParentheses(expr) instanceof NewClassTree newClassTree ? newClassTree : null; + } + + /** Returns true if {@code tree} contains any references to method parameters. */ + private static boolean usesMethodParameter(Tree tree) { + var scanner = + new TreeScanner() { + private boolean foundMethodParameter; + + @Override + public Void scan(Tree tree, Void unused) { + return foundMethodParameter ? null : super.scan(tree, null); + } + + @Override + public Void visitIdentifier(IdentifierTree node, Void unused) { + Symbol sym = getSymbol(node); + if (sym != null && sym.getKind() == ElementKind.PARAMETER) { + foundMethodParameter = true; + } + return super.visitIdentifier(node, null); + } + }; + scanner.scan(tree, null); + return scanner.foundMethodParameter; + } + + /** + * If {@code condition} is of the form {@code a == null} or {@code null == a}, returns the + * expression {@code a}; otherwise returns {@code null}. + */ + private static @Nullable ExpressionTree getNullCheckedExpression(ExpressionTree condition) { + if (condition instanceof BinaryTree binary && binary.getKind() == Tree.Kind.EQUAL_TO) { + if (binary.getLeftOperand().getKind() == Tree.Kind.NULL_LITERAL) { + return binary.getRightOperand(); + } + if (binary.getRightOperand().getKind() == Tree.Kind.NULL_LITERAL) { + return binary.getLeftOperand(); + } + } + return null; + } + + /** + * Returns a {@link Description} with a {@link SuggestedFix} to replace {@code ifTree} with a call + * to {@code Preconditions.method}. + */ + private Description suggest( + IfTree ifTree, + String method, + String conditionSource, + NewClassTree newClass, + VisitorState state) { + // Don't suggest refactoring to checkX if we are already inside a method named checkX, + // as this would introduce infinite recursion (e.g. if the user is implementing their + // own checkNotNull to avoid dependencies). + // This check relies purely on the method name. Since this checker specifically targets Guava's + // Preconditions, it's unlikely to cause issues, but a more robust check might also verify that + // the method belongs to a utility class or has a specific signature. Given this is a + // SUGGESTION, the current heuristic is acceptable. + MethodTree enclosingMethod = findEnclosingMethod(state); + if (enclosingMethod != null && enclosingMethod.getName().contentEquals(method)) { + return NO_MATCH; + } + SuggestedFix.Builder fix = SuggestedFix.builder(); + fix.addStaticImport("com.google.common.base.Preconditions." + method); + + StringBuilder replacement = new StringBuilder(method).append("(").append(conditionSource); + appendArguments(replacement, newClass, state); + replacement.append(");"); + + return describeMatch(ifTree, fix.replace(ifTree, replacement.toString()).build()); + } + + /** + * Returns true if all arguments to the exception constructor are side-effect-free. + * + *

This is to avoid hoisting side-effects, which could change behavior. For example, changing + * {@code if (foo == null) { throw new NPE(bar()); }} to {@code checkNotNull(foo, bar())} would + * cause {@code bar()} to be evaluated even if {@code foo} is not null. + */ + private boolean areArgumentsSideEffectFree(NewClassTree newClass, VisitorState state) { + List args = newClass.getArguments(); + if (args.isEmpty()) { + return true; + } + ExpressionTree firstArg = args.getFirst(); + List formatArgs = getSafeStringFormatArgs(firstArg, state); + if (formatArgs != null) { + return formatArgs.stream() + .allMatch(arg -> constantExpressions.constantExpression(arg, state).isPresent()); + } + return args.stream() + .allMatch(arg -> constantExpressions.constantExpression(arg, state).isPresent()); + } + + /** + * If {@code tree} is a call to {@code String.format} or {@code Strings.lenientFormat} with a + * literal format string that's safe for Preconditions, returns its arguments. Otherwise returns + * null. + */ + private static @Nullable List getSafeStringFormatArgs( + ExpressionTree tree, VisitorState state) { + if (STRING_FORMAT.matches(tree, state)) { + MethodInvocationTree formatCall = (MethodInvocationTree) tree; + ExpressionTree template = formatCall.getArguments().getFirst(); + if (template instanceof LiteralTree literal + && literal.getValue() instanceof String templateStr + && isSafeForPreconditions(templateStr)) { + return formatCall.getArguments(); + } + } + return null; + } + + /** + * Appends {@code newClass}'s arguments to {@code sb} as arguments to a Preconditions call. + * + *

If the only argument is a safe call to {@code String.format}, its arguments are unpacked to + * become arguments to the Preconditions method. + */ + private void appendArguments(StringBuilder sb, NewClassTree newClass, VisitorState state) { + List args = newClass.getArguments(); + if (args.isEmpty()) { + return; + } + List formatArgs = getSafeStringFormatArgs(args.getFirst(), state); + if (formatArgs != null) { + formatArgs.forEach(arg -> sb.append(", ").append(state.getSourceForNode(arg))); + return; + } + args.forEach(arg -> sb.append(", ").append(state.getSourceForNode(arg))); + } + + /** + * Returns true if the template string is safe to be passed to Preconditions varargs methods, + * which only support {@code %s} placeholders. + * + *

Preconditions' format methods (e.g., {@code checkArgument}) only support the {@code %s} + * format specifier. Other specifiers, including {@code %%} for a literal percent sign, are not + * supported and would lead to runtime errors if passed to Preconditions. + */ + private static boolean isSafeForPreconditions(String template) { + int index = 0; + while ((index = template.indexOf('%', index)) != -1) { + if (index + 1 >= template.length()) { + return false; + } + if (template.charAt(index + 1) != 's') { + return false; + } + index += 2; + } + return true; + } + + /** + * Returns a source representation of {@code condition}, heuristically negated. + * + *

If {@code condition} is a binary operator, we swap it for its negated counterpart (e.g., + * {@code ==} becomes {@code !=}). If it's a unary negation, we strip the negation. Otherwise, we + * prefix it with {@code !}. + */ + private static String negate(ExpressionTree condition, VisitorState state) { + condition = stripParentheses(condition); + if (condition instanceof BinaryTree binary) { + // Avoid swapping operators for floating point types because !(a < b) and a >= b are not + // equivalent when NaN is involved. The fallback to !(...) preserves behavior. + if (!isFloatingPoint(binary, state)) { + String op = + switch (condition.getKind()) { + case EQUAL_TO -> "!="; + case NOT_EQUAL_TO -> "=="; + case LESS_THAN -> ">="; + case LESS_THAN_EQUAL -> ">"; + case GREATER_THAN -> "<="; + case GREATER_THAN_EQUAL -> "<"; + default -> null; + }; + if (op != null) { + return String.format( + "%s %s %s", + state.getSourceForNode(binary.getLeftOperand()), + op, + state.getSourceForNode(binary.getRightOperand())); + } + } + } + return switch (condition.getKind()) { + case LOGICAL_COMPLEMENT -> + state.getSourceForNode(stripParentheses(((UnaryTree) condition).getExpression())); + case METHOD_INVOCATION, IDENTIFIER, MEMBER_SELECT -> "!" + state.getSourceForNode(condition); + default -> "!(" + state.getSourceForNode(condition) + ")"; + }; + } + + /** Returns true if either operand of {@code binary} is a float or double. */ + private static boolean isFloatingPoint(BinaryTree binary, VisitorState state) { + return isFloatingPoint(getType(binary.getLeftOperand()), state) + || isFloatingPoint(getType(binary.getRightOperand()), state); + } + + /** Returns true if {@code type} is a float or double. */ + private static boolean isFloatingPoint(@Nullable Type type, VisitorState state) { + if (type == null) { + return false; + } + Type unboxed = state.getTypes().unboxedTypeOrType(type); + return unboxed.hasTag(TypeTag.FLOAT) || unboxed.hasTag(TypeTag.DOUBLE); + } + + private static boolean isHardToNegate(ExpressionTree condition) { + var scanner = + new TreeScanner() { + private boolean foundComplex; + + @Override + public Void scan(Tree tree, Void unused) { + if (foundComplex) { + return null; + } + if (tree != null) { + switch (tree.getKind()) { + case CONDITIONAL_AND, + CONDITIONAL_OR, + XOR, + CONDITIONAL_EXPRESSION, + SWITCH_EXPRESSION -> + foundComplex = true; + default -> {} + } + } + return super.scan(tree, null); + } + }; + scanner.scan(condition, null); + return scanner.foundComplex; + } + + private static final Matcher STRING_FORMAT = + anyOf( + staticMethod().onClass("java.lang.String").named("format"), + staticMethod().onClass("com.google.common.base.Strings").named("lenientFormat")); + + private static final Matcher NEW_NPE = + anyOf( + constructor().forClass("java.lang.NullPointerException").withNoParameters(), + constructor() + .forClass("java.lang.NullPointerException") + .withParameters("java.lang.String")); + private static final Matcher NEW_IAE = + anyOf( + constructor().forClass("java.lang.IllegalArgumentException").withNoParameters(), + constructor() + .forClass("java.lang.IllegalArgumentException") + .withParameters("java.lang.String")); + private static final Matcher NEW_ISE = + anyOf( + constructor().forClass("java.lang.IllegalStateException").withNoParameters(), + constructor() + .forClass("java.lang.IllegalStateException") + .withParameters("java.lang.String")); +} 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..809fc65e29a 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -331,6 +331,7 @@ import com.google.errorprone.bugpatterns.PreconditionsInvalidPlaceholder; import com.google.errorprone.bugpatterns.PreferCharsetOverload; import com.google.errorprone.bugpatterns.PreferInstanceofOverGetKind; +import com.google.errorprone.bugpatterns.PreferPreconditions; import com.google.errorprone.bugpatterns.PreferTestParameter; import com.google.errorprone.bugpatterns.PreferredInterfaceType; import com.google.errorprone.bugpatterns.PrimitiveArrayPassedToVarargsMethod; @@ -1134,6 +1135,7 @@ public static ScannerSupplier warningChecks() { PreconditionsCheckNotNullRepeated.class, PreferCharsetOverload.class, PreferInstanceofOverGetKind.class, + PreferPreconditions.class, PreferTestParameter.class, PreferThrowsTag.class, PrimitiveAtomicReference.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/PreferPreconditionsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/PreferPreconditionsTest.java new file mode 100644 index 00000000000..213a08bef09 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/PreferPreconditionsTest.java @@ -0,0 +1,768 @@ +/* + * 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; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link PreferPreconditions}. */ +@RunWith(JUnit4.class) +public final class PreferPreconditionsTest { + + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(PreferPreconditions.class, getClass()); + + @Test + public void simpleIae() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new IllegalArgumentException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(String s) { + checkArgument(!s.isEmpty()); + } + } + """) + .doTest(); + } + + @Test + public void iaeWithNotNullSuggestion() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s == null) { + throw new IllegalArgumentException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(String s) { + checkArgument(s != null); + } + } + """) + .doTest(); + } + + @Test + public void npeWithNotNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s == null) { + throw new NullPointerException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkNotNull; + + class Test { + void m(String s) { + checkNotNull(s); + } + } + """) + .doTest(); + } + + @Test + public void iaeWithMessage() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i <= 0) { + throw new IllegalArgumentException("must be positive"); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(int i) { + checkArgument(i > 0, "must be positive"); + } + } + """) + .doTest(); + } + + @Test + public void iaeWithFormat() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i <= 0) { + throw new IllegalArgumentException(String.format("expected > 0, got %s", i)); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(int i) { + checkArgument(i > 0, "expected > 0, got %s", i); + } + } + """) + .doTest(); + } + + @Test + public void iseOnParam() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i != 42) { + throw new IllegalStateException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkState; + + class Test { + void m(int i) { + checkState(i == 42); + } + } + """) + .doTest(); + } + + @Test + public void notAParameter() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + private int field; + + void m() { + if (field == 0) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void hasElse() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i == 0) { + throw new IllegalArgumentException(); + } else { + System.out.println(i); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void negatedCondition() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int a, int b) { + if (!(a == b)) { + throw new IllegalArgumentException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(int a, int b) { + checkArgument(a == b); + } + } + """) + .doTest(); + } + + @Test + public void iseWithNotNullSuggestion() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s == null) { + throw new IllegalStateException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkState; + + class Test { + void m(String s) { + checkState(s != null); + } + } + """) + .doTest(); + } + + @Test + public void npeWithNonNotNullCheck() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new NullPointerException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void floatingPoint() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(double d) { + if (d < 0.0) { + throw new IllegalArgumentException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(double d) { + checkArgument(!(d < 0.0)); + } + } + """) + .doTest(); + } + + @Test + public void throwableCause() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(Throwable t) { + if (t == null) { + throw new IllegalArgumentException("throwable", t); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void partOfElseIf() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i, int j) { + if (i == 0) { + System.out.println(i); + } else if (j == 0) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void withComments() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i <= 0) { + // check if i is positive + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void withCommentsInside() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int i) { + if (i <= 0) { + throw new IllegalArgumentException(); // check if i is positive + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void messageWithMethodCall() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new IllegalArgumentException("got " + s.length()); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void formatWithConstantMethodCall() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new IllegalArgumentException(String.format("got %s", s.length())); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(String s) { + checkArgument(!s.isEmpty(), "got %s", s.length()); + } + } + """) + .doTest(); + } + + @Test + public void formatWithStaticMethodCall() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new IllegalArgumentException("got " + Math.random()); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void formatWithUnknownMethodCall() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s.isEmpty()) { + throw new IllegalArgumentException(String.format("got %s", random())); + } + } + + double random() { + return Math.random(); + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void withMemberSelect() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + static final String INVALID_ID = "invalid id"; + + void m(int i) { + if (i <= 0) { + throw new IllegalArgumentException(Test.INVALID_ID); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + static final String INVALID_ID = "invalid id"; + + void m(int i) { + checkArgument(i > 0, Test.INVALID_ID); + } + } + """) + .doTest(); + } + + @Test + public void simpleIf() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(boolean ok) { + if (!ok) { + throw new IllegalArgumentException(); + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.base.Preconditions.checkArgument; + + class Test { + void m(boolean ok) { + checkArgument(ok); + } + } + """) + .doTest(); + } + + @Test + public void conditionThatLooksBadNegated() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int c, int first, int last) { + if (c < first || c > last) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void conditionWithAndNegated() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int a, int b) { + if (a > 0 && b > 0) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void conditionWithXorNegated() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(boolean a, boolean b) { + if (a ^ b) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void conditionWithTernaryNegated() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(boolean a, boolean b, boolean c) { + if (a ? b : c) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void conditionWithSwitchExpressionNegated() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(int x) { + if (switch (x) { + case 1 -> true; + default -> false; + }) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void insideCheckNotNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public static T checkNotNull(T reference) { + if (reference == null) { + throw new NullPointerException(); + } + return reference; + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void insideCheckArgument() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public static void checkArgument(boolean expression) { + if (!expression) { + throw new IllegalArgumentException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void insideCheckState() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + public static void checkState(boolean expression) { + if (!expression) { + throw new IllegalStateException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void multipleStatementsInThen() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s == null) { + System.out.println("null"); + throw new NullPointerException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void exceptionNotNew() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + if (s == null) { + throw createNpe(); + } + } + NullPointerException createNpe() { + return new NullPointerException(); + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void localVariableInCondition() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + void m(String s) { + String local = s; + if (local == null) { + throw new NullPointerException(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } +} diff --git a/docs/bugpattern/PreferPreconditions.md b/docs/bugpattern/PreferPreconditions.md new file mode 100644 index 00000000000..0e0bec82069 --- /dev/null +++ b/docs/bugpattern/PreferPreconditions.md @@ -0,0 +1,58 @@ +Prefer Guava's [`Preconditions`](https://guava.dev/Preconditions) for checking +whether a constructor or method was invoked correctly (that is, whether its +*preconditions* were met). It is more concise than an explicit `if`-`throw` +block and clearly communicates the intent of the check. + +## Why use `Preconditions`? + +1. **Conciseness**: What takes several lines with an `if`-`throw` block can + often be done in a single line. +2. **Clearer Intent**: Methods like `checkArgument` (for + `IllegalArgumentException`) and `checkState` (for `IllegalStateException`) + explicitly state what kind of validation is being performed. +3. **Return Value**: `checkNotNull` returns the validated object, making it + convenient for assignment or usage in a constructor (e.g., `this.foo = + checkNotNull(foo);`) +4. **Message Formatting**: `Preconditions` support simple `%s` formatting + placeholders, avoiding the verbosity of `String.format` and the performance + pitfalls of manual string concatenation for simple failures. + +## Examples + +### `checkNotNull` + +```java +// Before +if (arg == null) { + throw new NullPointerException("arg must not be null"); +} + +// After +checkNotNull(arg, "arg must not be null"); +``` + +### `checkArgument` + +```java +// Before +if (index < 0) { + throw new IllegalArgumentException("index must be non-negative: " + index); +} + +// After +checkArgument(index >= 0, "index must be non-negative: %s", index); +``` + +## Caveats + +This check is conservative and will not suggest a refactoring if: + +* The failure message involves complex computation (like method calls) to + avoid eager evaluation of the message string. +* The `if` statement contains comments that might be lost during refactoring. +* The `if` statement has an `else` block or is part of an `if-else-if` chain. +* The condition contains logical operators (like `&&` or `||`) that would + require a complex/unreadable negation. +* The check is inside a method with the same name as the suggested + `Preconditions` method (e.g., inside `checkNotNull` itself) to avoid + infinite recursion.