Support loops and conditionals

2022-01-28 21:39:07 +01:00
1 parent 31fdf9f8d2
commit 38c7ba2bf6
@@ -37,6 +37,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTBooleanLiteral;
+import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTCastExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
@@ -53,6 +54,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLabeledStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTList;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTLoopStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
@@ -60,6 +62,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTNumericLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
@@ -1003,4 +1006,13 @@ public final class JavaRuleUtil {
        }
        return false;
    }
+
+    /**
+     * @see {@link ASTBreakStatement#getTarget()}
+     */
+    public static boolean mayBeBreakTarget(JavaNode it) {
+        return it instanceof ASTLoopStatement
+            || it instanceof ASTSwitchStatement
+            || it instanceof ASTLabeledStatement;
+    }
 }
@@ -18,7 +18,6 @@ import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;

-import org.apache.commons.lang3.NotImplementedException;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.pcollections.PSet;
@@ -35,12 +34,14 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTCompactConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTForeachStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
 import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLabeledStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
@@ -59,10 +60,12 @@ import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
+import net.sourceforge.pmd.lang.java.ast.BinaryOp;
 import net.sourceforge.pmd.lang.java.ast.InternalApiBridge;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.ast.JavaVisitorBase;
 import net.sourceforge.pmd.lang.java.internal.JavaAstProcessor;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.symbols.table.JSymbolTable;
 import net.sourceforge.pmd.lang.java.symbols.table.internal.PatternBindingsUtil.BindSet;
 import net.sourceforge.pmd.lang.java.types.JClassType;
@@ -432,6 +435,40 @@ public final class SymbolTableResolver {
            return null;
        }

+        @Override
+        public Void visit(ASTInfixExpression node, @NonNull ReferenceCtx ctx) {
+            // need to account for pattern bindings.
+            // visit left operand first. Maybe it introduces bindings in the rigt operand.
+
+            node.getLeftOperand().acceptVisitor(this, ctx);
+
+            BinaryOp op = node.getOperator();
+            if (op == BinaryOp.CONDITIONAL_AND) {
+
+                PSet<ASTVariableDeclaratorId> trueBindings = bindersOfExpr(node.getLeftOperand()).getTrueBindings();
+                if (!trueBindings.isEmpty()) {
+                    int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), trueBindings));
+                    setTopSymbolTableAndRecurse(node.getRightOperand(), ctx);
+                    popStack(pushed);
+                    return null;
+                }
+
+            } else if (op == BinaryOp.CONDITIONAL_OR) {
+
+                PSet<ASTVariableDeclaratorId> falseBindings = bindersOfExpr(node.getLeftOperand()).getFalseBindings();
+                if (!falseBindings.isEmpty()) {
+                    int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), falseBindings));
+                    setTopSymbolTableAndRecurse(node.getRightOperand(), ctx);
+                    popStack(pushed);
+                    return null;
+                }
+
+            }
+
+            // not a special case, finish visiting right operand
+            return node.getRightOperand().acceptVisitor(this, ctx);
+        }
+
        // non-static
        // Every visit method returns the set of variables that are introduced by the statement
        // as defined in the JLS:
@@ -460,17 +497,24 @@ public final class SymbolTableResolver {
                ASTStatement thenBranch = node.getThenBranch();
                ASTStatement elseBranch = node.getElseBranch();

-                int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), NodeStream.fromIterable(bindSet.getTrueBindings())));
+                node.getCondition().acceptVisitor(MyVisitor.this, ctx);
+
+                // the true bindings of the condition are in scope in the then branch
+                int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), bindSet.getTrueBindings()));
                setTopSymbolTableAndRecurse(thenBranch, ctx);
                popStack(pushed);
+
                if (elseBranch != null) {
-                    pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), NodeStream.fromIterable(bindSet.getFalseBindings())));
+                    // if there is an else, the false bindings are in scope in the else branch
+                    pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), bindSet.getFalseBindings()));
                    setTopSymbolTableAndRecurse(elseBranch, ctx);
                    popStack(pushed);

                    boolean thenCanCompleteNormally = canCompleteNormally(thenBranch);
                    boolean elseCanCompleteNormally = canCompleteNormally(elseBranch);

+                    // the bindings are visible in the statements following this if/else
+                    // if one of those conditions match
                    if (thenCanCompleteNormally && !elseCanCompleteNormally) {
                        return bindSet.getTrueBindings();
                    } else if (!thenCanCompleteNormally && elseCanCompleteNormally) {
@@ -489,6 +533,8 @@ public final class SymbolTableResolver {
                    return super.visit(node, ctx);
                }

+                node.getCondition().acceptVisitor(MyVisitor.this, ctx);
+
                int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), NodeStream.fromIterable(bindSet.getTrueBindings())));
                setTopSymbolTableAndRecurse(node.getBody(), ctx);
                popStack(pushed);
@@ -505,15 +551,36 @@ public final class SymbolTableResolver {

            @Override
            public PSet<ASTVariableDeclaratorId> visit(ASTForStatement node, @NonNull ReferenceCtx ctx) {
-                BindSet bindSet = bindersOfExpr(node.getCondition());
-                if (bindSet.isEmpty()) {
-                    int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), varsOfInit(node)));
-                    setTopSymbolTableAndRecurse(node, ctx);
-                    popStack(pushed);
-                    return BindSet.noBindings();
-                }
+                int pushed = pushOnStack(f.localVarSymTable(top(), enclosing(), varsOfInit(node)));

-                throw new NotImplementedException("TODO - for with pattern bindings in condition");
+                ASTExpression condition = node.getCondition();
+                setTopSymbolTableAndRecurse(condition, ctx);
+
+                BindSet bindSet = bindersOfExpr(condition);
+                pushed += pushOnStack(f.localVarSymTable(top(), enclosing(), bindSet.getTrueBindings()));
+                setTopSymbolTableAndRecurse(node.getUpdate(), ctx);
+                setTopSymbolTableAndRecurse(node.getBody(), ctx);
+                popStack(pushed);
+
+                if (bindSet.getFalseBindings().isEmpty()) {
+                    return BindSet.noBindings();
+                } else {
+                    // A pattern variable is introduced by a basic for statement iff
+                    // (i) it is introduced by the condition expression when false and
+                    // (ii) the contained statement, S, does not contain a reachable
+                    // break statement whose break target contains S (§14.15).
+                    Set<JavaNode> containingStatements = node.ancestorsOrSelf()
+                                                             .filter(JavaRuleUtil::mayBeBreakTarget)
+                                                             .collect(Collectors.toSet());
+                    boolean hasNoBreaks = node.getBody()
+                                              .descendants(ASTBreakStatement.class)
+                                              .none(it -> containingStatements.contains(it.getTarget()));
+                    if (hasNoBreaks) {
+                        return bindSet.getFalseBindings();
+                    } else {
+                        return BindSet.noBindings();
+                    }
+                }
            }

            @Override
@@ -6,6 +6,7 @@ package net.sourceforge.pmd.lang.java.symbols.table.internal

 import io.kotest.assertions.withClue
 import net.sourceforge.pmd.lang.ast.test.shouldBe
+import net.sourceforge.pmd.lang.ast.test.shouldBeA
 import net.sourceforge.pmd.lang.java.ast.*

 /**
@@ -110,4 +111,169 @@ class PatternVarTests : ProcessorTestSpec({
        }
    }

+    parserTest("Bindings within condition", javaVersion = JavaVersion.J17) {
+
+        fun checkVars(firstIsPattern: Boolean, secondIsPattern: Boolean, code: () -> String) {
+            val exprCode = code().trimIndent()
+            val sourceCode = """
+                class Foo {
+                    int var; // a field
+                    {
+                        someFun( $exprCode );
+                    }
+                }
+            """.trimIndent()
+            val acu = parser.parse(sourceCode)
+
+
+            val expr = acu.descendants(ASTArgumentList::class.java)[0]!!
+            val (var1, var2) = expr.descendants(ASTMethodCall::class.java).crossFindBoundaries()
+                .map { it.qualifier as ASTVariableAccess }.toList()
+            withClue("First var in\n$exprCode") {
+                var1.referencedSym!!.tryGetNode()!!::isPatternBinding shouldBe firstIsPattern
+            }
+            withClue("Second var in\n$exprCode") {
+                var2.referencedSym!!.tryGetNode()!!::isPatternBinding shouldBe secondIsPattern
+            }
+        }
+
+        doTest("Condition with and") {
+
+            checkVars(firstIsPattern = true, secondIsPattern = false) {
+                """
+                a -> {
+                    if (a instanceof String var && var.isEmpty()) { // the binding
+                    }
+                    var.toString(); // the field
+                }
+                """
+            }
+        }
+
+        doTest("Condition with or (negated)") {
+
+            checkVars(firstIsPattern = true, secondIsPattern = false) {
+                """
+                a -> {
+                    if (!(a instanceof String var) || var.isEmpty()) { // the binding
+                    }
+                    var.toString(); // the field
+                }
+                """
+            }
+        }
+        doTest("Condition with or") {
+
+            checkVars(firstIsPattern = false, secondIsPattern = false) {
+                """
+                a -> {
+                    if (a instanceof String var || var.isEmpty()) { // the field
+                    }
+                    var.toString(); // the field
+                }
+                """
+            }
+        }
+    }
+
+
+    parserTest("Bindings within for loop", javaVersion = JavaVersion.J17) {
+
+        fun checkVars(firstIsPattern: Boolean, secondIsPattern: Boolean, code: () -> String) {
+            val exprCode = code().trimIndent()
+            val sourceCode = """
+                class Foo {
+                    int var; // a field
+                    {
+                        someFun( $exprCode );
+                    }
+                }
+            """.trimIndent()
+            val acu = parser.parse(sourceCode)
+
+
+            val expr = acu.descendants(ASTArgumentList::class.java)[0]!!
+            val (var1, var2) = expr.descendants(ASTMethodCall::class.java).crossFindBoundaries()
+                .map { it.qualifier as ASTVariableAccess }.toList()
+            withClue("First var in\n$exprCode") {
+                var1.referencedSym!!.tryGetNode()!!::isPatternBinding shouldBe firstIsPattern
+            }
+            withClue("Second var in\n$exprCode") {
+                var2.referencedSym!!.tryGetNode()!!::isPatternBinding shouldBe secondIsPattern
+            }
+        }
+
+        doTest("Positive cond") {
+
+            checkVars(firstIsPattern = true, secondIsPattern = false) {
+                """
+                a -> {
+                    for (; a instanceof String var; var = var.substring(1)) { // the binding
+                        
+                    }
+                    var.toString(); // the field
+                }
+                """
+            }
+        }
+        doTest("Negated cond, body does nothing") {
+
+            checkVars(firstIsPattern = false, secondIsPattern = true) {
+                """
+                a -> {
+                    for (; !(a instanceof String var); var = var.substring(1)) { // the field
+                        
+                    }
+                    var.toString(); // the binding though it is unreachable
+                }
+                """
+            }
+        }
+        doTest("Negated cond, body doesn't break") {
+
+            checkVars(firstIsPattern = false, secondIsPattern = true) {
+                """
+                a -> {
+                    for (; !(a instanceof String var); var = var.substring(1)) { // the field
+                        while (true) {
+                            break;
+                        }
+                    }
+                    var.toString(); // the binding
+                }
+                """
+            }
+        }
+        doTest("Negated cond, body does break") {
+
+            checkVars(firstIsPattern = false, secondIsPattern = false) {
+                """
+                a -> {
+                    for (; !(a instanceof String var); var = var.substring(1)) { // the field
+                        break;
+                    }
+                    var.toString(); // the field
+                }
+                """
+            }
+        }
+        doTest("Both bindings and init vars are in scope") {
+            inContext(StatementParsingCtx) {
+                val loop = doParse(
+                    """
+                 for (String x=""; a instanceof String v; v = x.substring(1)) {
+                        break;
+                 }
+                """
+                ).shouldBeA<ASTForStatement>()
+
+                val (x, v) = loop.descendants(ASTVariableDeclaratorId::class.java).toList()
+
+                val (_, vref, xref) = loop.descendants(ASTVariableAccess::class.java).toList()
+                vref.shouldResolveToLocal(v)
+                xref.shouldResolveToLocal(x)
+            }
+        }
+    }
+
 })