phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #3795 from

Browse Source 
oowekyala:issue3786-simplify-boolean-returns

[java] Fix #3786 - make SimplifyBooleanReturns consider parentheses.
#3795
This commit is contained in:
Andreas Dangel
2022-02-25 12:37:48 +01:00
parent f21008dce6 01fa91a540
commit 347a91a57e
6 changed files with 264 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
docs/pages/7_0_0_release_notes.md
View File

@ -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

12
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/BinaryOp.java
View File

@ -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<BinaryOp> opsWithGreaterPrecedence(BinaryOp op) {
Set<BinaryOp> range = EnumSet.range(op, MOD);
range.remove(op);
return range;
}
private int precedenceClass() {
switch (this) {
case CONDITIONAL_OR:

128
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SimplifyBooleanReturnsRule.java
View File

@ -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<BinaryOp> 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.
* <pre>{@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
* }</pre>
* 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) {

5
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java
View File

@ -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;
}

2
pmd-java/src/main/resources/category/java/design.xml
View File

@ -1211,7 +1211,7 @@ public class Bar {
<rule name="SimplifyBooleanReturns"
language="java"
since="0.9"
message="Avoid unnecessary if..then..else statements when returning booleans"
message="This if statement can be replaced by `{0}`"
class="net.sourceforge.pmd.lang.java.rule.design.SimplifyBooleanReturnsRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_design.html#simplifybooleanreturns">
<description>

125
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SimplifyBooleanReturns.xml
View File

@ -140,4 +140,129 @@ public class SimplifyBooleanReturns {
}
]]></code>
</test-code>
<test-code>
<description>don't report if expr would need paren</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>13</expected-linenumbers>
<expected-messages>
<message>This if statement can be replaced by `return {condition} &amp;&amp; {thenBranch};`</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public boolean foo(Object foo, boolean a, boolean b) {
if (foo instanceof Foo) { // foo instanceof Foo && (a || b)
return a || b;
} else {
return false;
}
if (foo instanceof Foo) { // !(foo instanceof Foo) || a
return a;
} else {
return true;
}
if (foo instanceof Foo) { // foo instanceof Foo && a
return a;
} else {
return false;
}
if (a || b) { // (a || b) && a
return a;
} else {
return false;
}
if (foo instanceof Foo) { // !(foo instanceof Foo) && a;
return a;
} else {
return true;
}
}
}
]]></code>
</test-code>
<test-code>
<description>some ops can be negated ok</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>3,8</expected-linenumbers>
<expected-messages>
<message>This if statement can be replaced by `return !{condition} &amp;&amp; {thenBranch};`</message>
<message>This if statement can be replaced by `return !{condition} &amp;&amp; {thenBranch};`</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public boolean foo(Object foo, boolean a, boolean b) {
if (foo == null) { // foo != null || a
return a;
} else {
return true;
}
if (!(foo instanceof String)) { // foo instanceof String || a
return a;
} else {
return true;
}
}
}
]]></code>
</test-code>
<test-code>
<description>expr already has parens</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<expected-messages>
<message>This if statement can be replaced by `return {condition} &amp;&amp; {thenBranch};`</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public boolean foo(Object foo, boolean a, boolean b) {
if (foo instanceof Foo) { // foo instanceof Foo && (a || b)
return (a || b);
} else {
return false;
}
}
}
]]></code>
</test-code>
<test-code>
<description>both booleans</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<expected-messages>
<message>This if statement can be replaced by `return {condition};`</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public boolean foo(Object entry, boolean a, boolean b) {
if ((entry != null) && (entry instanceof String)) {
return true;
} else {
return false;
}
}
}
]]></code>
</test-code>
<test-code>
<description>De Morgan</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>6</expected-linenumbers>
<expected-messages>
<message>This if statement can be replaced by `return !{condition} &amp;&amp; {thenBranch};`</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public boolean foo(Object entry, boolean a, boolean b) {
// !(entry != null && entry != b) || a
// -> entry == null || entry == b || a
if (entry != null && entry != b) {
return a;
} else {
return true;
}
}
}
]]></code>
</test-code>
</test-data>