Fix #3786

Node matchers

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,14 @@ public enum BinaryOp implements InternalInterfaces.OperatorLike {
         return comparePrecedence(other) == 0;
     }
 
+
+    /**
+     * Returns the ops with higher or equal precedence to the given op.
+     */
+    public static EnumSet<BinaryOp> opsWithGeqPrecedence(BinaryOp op) {
+        return EnumSet.range(op, MOD);
+    }
+
     private int precedenceClass() {
         switch (this) {
         case CONDITIONAL_OR:

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/JavaMetrics.java

@@ -34,7 +34,7 @@ import net.sourceforge.pmd.lang.java.metrics.internal.CognitiveComplexityVisitor
 import net.sourceforge.pmd.lang.java.metrics.internal.CycloVisitor;
 import net.sourceforge.pmd.lang.java.metrics.internal.NcssVisitor;
 import net.sourceforge.pmd.lang.java.metrics.internal.NpathBaseVisitor;
-import net.sourceforge.pmd.lang.java.rule.internal.JavaAstUtil;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.symbols.JClassSymbol;
 import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
 import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
@@ -428,7 +428,7 @@ public final class JavaMetrics {
     private static int computeNoam(ASTAnyTypeDeclaration node, MetricOptions ignored) {
         return node.getDeclarations()
                    .filterIs(ASTMethodDeclaration.class)
-                   .filter(JavaAstUtil::isGetterOrSetter)
+                   .filter(JavaRuleUtil::isGetterOrSetter)
                    .count();
     }
 
@@ -605,7 +605,7 @@ public final class JavaMetrics {
                 .filterIs(ASTMethodDeclaration.class)
                 .filter(it -> !it.isPrivate());
 
-        int notSetter = methods.filter(it -> !JavaAstUtil.isGetterOrSetter(it)).count();
+        int notSetter = methods.filter(it -> !JavaRuleUtil.isGetterOrSetter(it)).count();
         int total = methods.count();
         if (total == 0) {
             return 0;

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

@@ -4,6 +4,10 @@
 
 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.isInfixExprWithOperator;
+import static net.sourceforge.pmd.lang.java.ast.BinaryOp.opsWithGeqPrecedence;
 import static net.sourceforge.pmd.lang.java.rule.internal.JavaAstUtil.areComplements;
 import static net.sourceforge.pmd.lang.java.rule.internal.JavaAstUtil.isBooleanLiteral;
 
@@ -11,12 +15,15 @@ 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.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.JavaAstUtil;
 import net.sourceforge.pmd.lang.java.types.JPrimitiveType.PrimitiveTypeKind;
 
 public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule {
@@ -57,11 +64,14 @@ public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule {
         }
 
         if (isBooleanLiteral(thenExpr) || isBooleanLiteral(elseExpr)) {
-            data.addViolation(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;
-            data.addViolation(node);
+            data.addViolation(node, "return {condition};");
         }
     }
 
@@ -73,29 +83,59 @@ public class SimplifyBooleanReturnsRule extends AbstractJavaRulechainRule {
      * if (cond) false  else expr      -> !cond && expr
      * if (cond) expr   else true      -> !cond || expr
      * if (cond) expr   else false     ->  cond && expr
-     * if (!cond) false else expr      -> cond && expr
-     * if (!cond) expr  else true     ->  cond || expr
      * }</pre>
      * Note that both the `expr` and the `condition` may require parentheses
-     * (if the cond has to be negated). Note that the `expr` and `condition` may
-     * themselves be literals, or a negated expr.
+     * (if the cond has to be negated).
      */
-    private Result needsToBeReported(ASTExpression condition,
-                                     ASTExpression thenExpr,
-                                     ASTExpression elseExpr) {
-
-        if (JavaAstUtil.isBooleanLiteral(condition)
-        && ) {
+    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";
 
+        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;
 
-        boolean isAndOp = JavaAstUtil.isBooleanLiteral(thenExpr, true)
-            || JavaAstUtil.isBooleanLiteral(elseExpr, true);
 
+        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;
     }
-    enum Result {
-        LITERAL_CONDITION
+
+    private static boolean needsNewParensWhenNegating(ASTExpression e) {
+        return !(e instanceof ASTPrimaryExpression || e instanceof ASTCastExpression);
+    }
+
+    private static boolean doesNotNeedNewParensUnderInfix(ASTExpression e, BinaryOp op) {
+        if (e instanceof ASTPrimaryExpression
+            || e instanceof ASTCastExpression
+            || e instanceof ASTUnaryExpression) {
+            return true;
+        } else {
+            return isInfixExprWithOperator(e, opsWithGeqPrecedence(op))
+                && !isInfixExprWithOperator(e, op); // no need for parens here
+        }
     }
 
     private @Nullable ASTExpression getReturnExpr(JavaNode node) {

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/CommentRequiredRule.java

@@ -25,7 +25,6 @@ import net.sourceforge.pmd.lang.java.ast.AccessNode.Visibility;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.ast.JavadocCommentOwner;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
-import net.sourceforge.pmd.lang.java.rule.internal.JavaAstUtil;
 import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.properties.PropertyBuilder.GenericPropertyBuilder;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
@@ -162,7 +161,7 @@ public class CommentRequiredRule extends AbstractJavaRulechainRule {
     public Object visit(ASTMethodDeclaration decl, Object data) {
         if (decl.isOverridden()) {
             checkCommentMeetsRequirement(data, decl, OVERRIDE_CMT_DESCRIPTOR);
-        } else if (JavaAstUtil.isGetterOrSetter(decl)) {
+        } else if (JavaRuleUtil.isGetterOrSetter(decl)) {
             checkCommentMeetsRequirement(data, decl, ACCESSOR_CMT_DESCRIPTOR);
         } else {
             checkMethodOrConstructorComment(decl, data);

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/BeanMembersShouldSerializeRule.java

@@ -21,7 +21,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.ast.Annotatable;
 import net.sourceforge.pmd.lang.java.ast.JModifier;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
-import net.sourceforge.pmd.lang.java.rule.internal.JavaAstUtil;
 import net.sourceforge.pmd.lang.java.rule.internal.JavaPropertyUtil;
 import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
@@ -66,7 +65,7 @@ public class BeanMembersShouldSerializeRule extends AbstractJavaRulechainRule {
 
         Set<String> accessors =
             node.getDeclarations(ASTMethodDeclaration.class)
-                .filter(JavaAstUtil::isGetterOrSetter)
+                .filter(JavaRuleUtil::isGetterOrSetter)
                 .collect(Collectors.mapping(ASTMethodDeclaration::getName, Collectors.toSet()));
 
         NodeStream<ASTVariableDeclaratorId> fields =

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaAstUtil.java

@@ -164,9 +164,7 @@ public final class JavaAstUtil {
                 && ((ASTMethodDeclaration) node).isMainMethod();
     }
 
-    public static boolean isGetterOrSetter(ASTMethodDeclaration node) {
-        return JavaRuleUtil.isGetter(node) || JavaRuleUtil.isSetter(node);
-    }
+
 
     static boolean hasField(ASTAnyTypeDeclaration node, String name) {
         for (JFieldSymbol f : node.getSymbol().getDeclaredFields()) {
@@ -389,12 +387,12 @@ public final class JavaAstUtil {
     }
 
     /** 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;
     }
 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaNodeMatchers.java

@@ -0,0 +1,139 @@
+package net.sourceforge.pmd.lang.java.rule.internal;
+
+import java.util.Objects;
+import java.util.Set;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
+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.ast.TypeNode;
+import net.sourceforge.pmd.lang.java.types.JTypeMirror;
+
+/**
+ * A pattern to match over nodes.
+ *
+ * @author Clément Fournier
+ */
+public final class JavaNodeMatchers {
+
+    private JavaNodeMatchers() {
+        // utility class
+    }
+
+
+    /** Matches a boolean negation expr whose operand matches. */
+    public static <N extends JavaNode> NodeMatcher<N> neg(NodeMatcher<? super ASTExpression> operand) {
+        return n -> JavaAstUtil.isBooleanNegation(n) && operand.matches(((ASTUnaryExpression) n).getOperand());
+    }
+
+    /** Matches an infix expression whose expr whose operands and operator match. */
+    public static <N extends JavaNode> NodeMatcher<N> infix(NodeMatcher<? super ASTExpression> left,
+                                                            BinaryOp op,
+                                                            NodeMatcher<? super ASTExpression> right) {
+        return n -> {
+            if (BinaryOp.isInfixExprWithOperator(n, op)) {
+                ASTInfixExpression infix = (ASTInfixExpression) n;
+                return left.matches(infix.getLeftOperand())
+                    && right.matches(infix.getRightOperand());
+            }
+            return false;
+        };
+    }
+
+    /** Matches an infix expression whose expr whose operands and operator match. */
+    public static <N extends JavaNode> NodeMatcher<N> infix(NodeMatcher<? super ASTExpression> left,
+                                                            Set<BinaryOp> anyOp,
+                                                            NodeMatcher<? super ASTExpression> right) {
+        return n -> {
+            if (BinaryOp.isInfixExprWithOperator(n, anyOp)) {
+                ASTInfixExpression infix = (ASTInfixExpression) n;
+                return left.matches(infix.getLeftOperand())
+                    && right.matches(infix.getRightOperand());
+            }
+            return false;
+        };
+    }
+
+    /** Matches a literal boolean with the given value. */
+    public static <N extends JavaNode> NodeMatcher<N> bool(boolean b) {
+        return n -> JavaAstUtil.isBooleanLiteral(n, b);
+    }
+
+    /** Matches any literal boolean. */
+    public static <N extends JavaNode> NodeMatcher<N> bool() {
+        return JavaAstUtil::isBooleanLiteral;
+    }
+
+    private static <N extends JavaNode> NodeMatcher<N> literal() {
+        return is(ASTLiteral.class);
+    }
+
+
+    /**
+     * A pattern that matches a node whose static.
+     */
+    public static <N extends TypeNode> NodeMatcher<N> hasType(JTypeMirror type) {
+        Objects.requireNonNull(type);
+        return n -> n.getTypeMirror().equals(type);
+    }
+
+
+    /**
+     * A pattern that matches a node instance of the given class.
+     */
+    public static <T, N extends Node> NodeMatcher<N> is(Class<T> type) {
+        return type::isInstance;
+    }
+
+    /**
+     * Matches any node.
+     */
+    public static <N extends Node> NodeMatcher<N> any() {
+        return n -> true;
+    }
+
+    public static <N extends Node> NodeMatcher<N> capture(Capture<N> obj, NodeMatcher<N> pattern) {
+        return n -> {
+            if (pattern.matches(n)) {
+                obj.setValue(n);
+            }
+            return false;
+        };
+    }
+
+    public static <N extends Node> boolean capture(Capture<N> obj, N n) {
+        return obj.setValue(n);
+    }
+
+    public static <N extends Node> boolean match(N node, NodeMatcher<? super N> pattern) {
+        return pattern.matches(node);
+    }
+
+    public static final class Capture<N> {
+
+        N value;
+
+        private boolean setValue(N s) {
+            boolean isUnset = value == null;
+            value = s;
+            return isUnset;
+        }
+
+        public N get() {
+            if (value == null) {
+                throw new IllegalStateException("Pattern has not matched");
+            }
+            return value;
+        }
+
+        public static <N> Capture<N> uninit() {
+            return new Capture<>();
+        }
+
+    }
+
+}

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

@@ -206,6 +206,10 @@ public final class JavaRuleUtil {
         return index >= 0 && camelCaseString.length() == index + capitalizedWord.length();
     }
 
+    public static boolean isGetterOrSetter(ASTMethodDeclaration node) {
+        return JavaRuleUtil.isGetter(node) || JavaRuleUtil.isSetter(node);
+    }
+
     public static boolean isGetterOrSetterCall(ASTMethodCall call) {
         return isGetterCall(call) || isSetterCall(call);
     }

pmd-java/src/main/resources/category/java/design.xml

@@ -1181,7 +1181,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>

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

@@ -140,4 +140,44 @@ 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-data>