diff --git a/docs/pages/7_0_0_release_notes.md b/docs/pages/7_0_0_release_notes.md index 086da5af92..f3a86fd7e4 100644 --- a/docs/pages/7_0_0_release_notes.md +++ b/docs/pages/7_0_0_release_notes.md @@ -208,6 +208,7 @@ The following previously deprecated rules have been finally removed: * [#2182](https://github.com/pmd/pmd/issues/2182): \[java] LawOfDemeter: False positive with package-private access * [#2188](https://github.com/pmd/pmd/issues/2188): \[java] LawOfDemeter: False positive with fields assigned to local vars * [#2536](https://github.com/pmd/pmd/issues/2536): \[java] ClassWithOnlyPrivateConstructorsShouldBeFinal can't detect inner class + * [#3786](https://github.com/pmd/pmd/issues/3786): \[java] SimplifyBooleanReturns should consider operator precedence * java-errorprone * [#659](https://github.com/pmd/pmd/issues/659): \[java] MissingBreakInSwitch - last default case does not contain a break * [#1005](https://github.com/pmd/pmd/issues/1005): \[java] CloneMethodMustImplementCloneable triggers for interfaces diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/BinaryOp.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/BinaryOp.java index df8b9ecf42..549fb1955a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/BinaryOp.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/BinaryOp.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.java.ast; import java.util.Comparator; +import java.util.EnumSet; import java.util.Set; import org.checkerframework.checker.nullness.qual.NonNull; @@ -135,6 +136,17 @@ public enum BinaryOp implements InternalInterfaces.OperatorLike { return comparePrecedence(other) == 0; } + + /** + * Returns the ops with strictly greater precedence than the given op. + * This may return an empty set. + */ + public static Set opsWithGreaterPrecedence(BinaryOp op) { + Set range = EnumSet.range(op, MOD); + range.remove(op); + return range; + } + private int precedenceClass() { switch (this) { case CONDITIONAL_OR: diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SimplifyBooleanReturnsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SimplifyBooleanReturnsRule.java index 23992ac627..d4b9628987 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SimplifyBooleanReturnsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SimplifyBooleanReturnsRule.java @@ -4,21 +4,43 @@ package net.sourceforge.pmd.lang.java.rule.design; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.CONDITIONAL_AND; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.CONDITIONAL_OR; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.EQ; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.GE; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.GT; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.LE; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.LT; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.NE; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.isInfixExprWithOperator; +import static net.sourceforge.pmd.lang.java.ast.BinaryOp.opsWithGreaterPrecedence; import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.areComplements; import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isBooleanLiteral; +import java.util.EnumSet; +import java.util.Set; + import org.checkerframework.checker.nullness.qual.Nullable; +import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.java.ast.ASTBlock; +import net.sourceforge.pmd.lang.java.ast.ASTCastExpression; import net.sourceforge.pmd.lang.java.ast.ASTExpression; import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; +import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; +import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression; +import net.sourceforge.pmd.lang.java.ast.BinaryOp; import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; +import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil; import net.sourceforge.pmd.lang.java.types.JPrimitiveType.PrimitiveTypeKind; public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule { + private static final Set NEGATABLE_OPS = EnumSet.of(EQ, NE, GT, LT, GE, LE); + public SimplifyBooleanReturnsRule() { super(ASTReturnStatement.class); } @@ -31,7 +53,8 @@ public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule { || !isThenBranchOfSomeIf(node)) { return null; } - return checkIf(node.ancestors(ASTIfStatement.class).firstOrThrow(), data, expr); + checkIf(node.ancestors(ASTIfStatement.class).firstOrThrow(), asCtx(data), expr); + return null; } // Only explore the then branch. If we explore the else, then we'll report twice. @@ -46,21 +69,114 @@ public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule { && node.getParent().getIndexInParent() == 1; } - private Object checkIf(ASTIfStatement node, Object data, ASTExpression thenExpr) { + private void checkIf(ASTIfStatement node, RuleContext data, ASTExpression thenExpr) { // that's the case: if..then..return; return; ASTExpression elseExpr = getElseExpr(node); if (elseExpr == null) { - return data; + return; } if (isBooleanLiteral(thenExpr) || isBooleanLiteral(elseExpr)) { - addViolation(data, node); + String fix = needsToBeReportedWhenOneBranchIsBoolean(node.getCondition(), thenExpr, elseExpr); + if (fix != null) { + data.addViolation(node, fix); + } } else if (areComplements(thenExpr, elseExpr)) { // if (foo) return !a; // else return a; - addViolation(data, node); + data.addViolation(node, "return {condition};"); } - return data; + } + + /** + * Whether refactoring an if-then-else to use shortcut operators + * would require adding parentheses to respect operator precedence. + * 
{@code
+     * if (cond) true   else expr      ->  cond || expr
+     * if (cond) false  else expr      -> !cond && expr
+     * if (cond) expr   else true      -> !cond || expr
+     * if (cond) expr   else false     ->  cond && expr
+     * }
+ * Note that both the `expr` and the `condition` may require parentheses + * (if the cond has to be negated). + */ + private String needsToBeReportedWhenOneBranchIsBoolean(ASTExpression condition, + ASTExpression thenExpr, + ASTExpression elseExpr) { + // at least one of these is true + boolean thenFalse = isBooleanLiteral(thenExpr, false); + boolean thenTrue = isBooleanLiteral(thenExpr, true); + boolean elseTrue = isBooleanLiteral(elseExpr, true); + boolean elseFalse = isBooleanLiteral(elseExpr, false); + assert thenFalse || elseFalse || thenTrue || elseTrue + : "expected boolean branch"; + + if (isBooleanLiteral(thenExpr) && isBooleanLiteral(elseExpr)) { + // both are boolean + if (thenTrue && elseFalse) { + return "return {condition};"; + } else if (thenFalse && elseTrue) { + return "return !{condition};"; + } else if (thenTrue) { // both are true + return "return true;"; + } else { // both are false + return "return false;"; + } + } + + boolean conditionNegated = thenFalse || elseTrue; + if (conditionNegated && needsNewParensWhenNegating(condition)) { + return null; + } + + BinaryOp op = thenFalse || elseFalse ? CONDITIONAL_AND : CONDITIONAL_OR; + // the branch that is not a literal, if both are literals, prefers elseExpr + ASTExpression branch = thenFalse || thenTrue ? elseExpr : thenExpr; + + + if (doesNotNeedNewParensUnderInfix(condition, op) + && doesNotNeedNewParensUnderInfix(branch, op)) { + if (thenTrue) { + return "return {condition} || {elseBranch};"; + } else if (thenFalse) { + return "return !{condition} || {elseBranch};"; + } else if (elseTrue) { + return "return !{condition} && {thenBranch};"; + } else { + return "return {condition} && {thenBranch};"; + } + } + return null; + } + + private static boolean needsNewParensWhenNegating(ASTExpression e) { + if (e instanceof ASTPrimaryExpression + || e instanceof ASTCastExpression + // parenthesized expressions are primary + || e.isParenthesized() + // == -> != + || isInfixExprWithOperator(e, NEGATABLE_OPS) + // !! -> + || JavaRuleUtil.isBooleanNegation(e)) { + return false; + } else if (isInfixExprWithOperator(e, CONDITIONAL_OR) + || isInfixExprWithOperator(e, CONDITIONAL_AND)) { + // negating these ops can be replaced with complement op + // and the negation pushed down branches using De Morgan's laws + ASTInfixExpression infix = (ASTInfixExpression) e; + return needsNewParensWhenNegating(infix.getLeftOperand()) + || needsNewParensWhenNegating(infix.getRightOperand()); + } + return true; + } + + private static boolean doesNotNeedNewParensUnderInfix(ASTExpression e, BinaryOp op) { + // those nodes have greater precedence than infix + return e instanceof ASTPrimaryExpression + || e instanceof ASTCastExpression + || e instanceof ASTUnaryExpression + || e.isParenthesized() + || isInfixExprWithOperator(e, opsWithGreaterPrecedence(op)); } private @Nullable ASTExpression getReturnExpr(JavaNode node) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java index b0891dfbf5..9c89da1100 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java @@ -662,13 +662,14 @@ public final class JavaRuleUtil { return node instanceof ASTNullLiteral; } + /** Returns true if the node is a boolean literal with any value. */ - public static boolean isBooleanLiteral(ASTExpression e) { + public static boolean isBooleanLiteral(JavaNode e) { return e instanceof ASTBooleanLiteral; } /** Returns true if the node is a boolean literal with the given constant value. */ - public static boolean isBooleanLiteral(ASTExpression e, boolean value) { + public static boolean isBooleanLiteral(JavaNode e, boolean value) { return e instanceof ASTBooleanLiteral && ((ASTBooleanLiteral) e).isTrue() == value; } diff --git a/pmd-java/src/main/resources/category/java/design.xml b/pmd-java/src/main/resources/category/java/design.xml index e937d24f9d..8d0c144a78 100644 --- a/pmd-java/src/main/resources/category/java/design.xml +++ b/pmd-java/src/main/resources/category/java/design.xml @@ -1211,7 +1211,7 @@ public class Bar { diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SimplifyBooleanReturns.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SimplifyBooleanReturns.xml index 18d9cb33c9..9ef56b3eaa 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SimplifyBooleanReturns.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SimplifyBooleanReturns.xml @@ -140,4 +140,129 @@ public class SimplifyBooleanReturns { } ]]> + + + don't report if expr would need paren + 1 + 13 + + This if statement can be replaced by `return {condition} && {thenBranch};` + + + + + some ops can be negated ok + 2 + 3,8 + + This if statement can be replaced by `return !{condition} && {thenBranch};` + This if statement can be replaced by `return !{condition} && {thenBranch};` + + + + + expr already has parens + 1 + 3 + + This if statement can be replaced by `return {condition} && {thenBranch};` + + + + + both booleans + 1 + 3 + + This if statement can be replaced by `return {condition};` + + + + + De Morgan + 1 + 6 + + This if statement can be replaced by `return !{condition} && {thenBranch};` + + entry == null || entry == b || a + if (entry != null && entry != b) { + return a; + } else { + return true; + } + } + } + ]]> +