From c837e244e9106d07ddefba31a1fbf24d6509dd6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 22 Jun 2020 15:51:07 +0200 Subject: [PATCH] Handle shortcut boolean expressions in if/ternary --- .../java/ast/ASTConditionalExpression.java | 2 +- .../rule/errorprone/UnusedAssignmentRule.java | 98 +++++++++-- .../rule/errorprone/xml/UnusedAssignment.xml | 166 ++++++++++++++++++ 3 files changed, 249 insertions(+), 17 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConditionalExpression.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConditionalExpression.java index 521b5828f0..1d4a84d645 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConditionalExpression.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConditionalExpression.java @@ -74,7 +74,7 @@ public class ASTConditionalExpression extends AbstractJavaTypeNode { * Returns the node that represents the guard of this conditional. * That is the expression before the '?'. */ - public Node getCondition() { + public JavaNode getCondition() { return getChild(0); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnusedAssignmentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnusedAssignmentRule.java index 13f292fae2..5c48228fa4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnusedAssignmentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/UnusedAssignmentRule.java @@ -12,6 +12,7 @@ import java.util.Comparator; import java.util.Deque; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -28,6 +29,9 @@ import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement; import net.sourceforge.pmd.lang.java.ast.ASTCatchStatement; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody; import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; +import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression; +import net.sourceforge.pmd.lang.java.ast.ASTConditionalExpression; +import net.sourceforge.pmd.lang.java.ast.ASTConditionalOrExpression; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTContinueStatement; import net.sourceforge.pmd.lang.java.ast.ASTDoStatement; @@ -93,13 +97,15 @@ public class UnusedAssignmentRule extends AbstractJavaRule { could also use that to detect other kinds of bug, eg conditions that are always true, or dereferences that will always NPE. In the general case though, this is complicated and better left to - an off-the-shelf data flow analyser, eg google Z3. + a DFA library, eg google Z3. TODO * labels on arbitrary statements (currently only loops) * explicit ctor call (hard to impossible without type res, or at least proper graph algorithms like toposort) -> this is pretty invisible as it causes false negatives, not FPs + * shortcut conditionals have their own control-flow + * test ternary expr DONE * conditionals @@ -212,7 +218,9 @@ public class UnusedAssignmentRule extends AbstractJavaRule { Collections.sort(sorted, new Comparator() { @Override public int compare(AssignmentEntry o1, AssignmentEntry o2) { - return Integer.compare(o1.rhs.getBeginLine(), o2.rhs.getBeginLine()); + int lineRes = Integer.compare(o1.rhs.getBeginLine(), o2.rhs.getBeginLine()); + return lineRes != 0 ? lineRes + : Integer.compare(o1.rhs.getBeginColumn(), o2.rhs.getBeginColumn()); } }); @@ -321,15 +329,78 @@ public class UnusedAssignmentRule extends AbstractJavaRule { @Override public Object visit(ASTIfStatement node, Object data) { - AlgoState before = acceptOpt(node.getCondition(), (AlgoState) data); + AlgoState before = (AlgoState) data; + return makeConditional(before, node.getCondition(), node.getThenBranch(), node.getElseBranch()); + } - AlgoState thenState = acceptOpt(node.getThenBranch(), before.fork()); - AlgoState elseState = node.hasElse() ? acceptOpt(node.getElseBranch(), before) - : before; + @Override + public Object visit(ASTConditionalExpression node, Object data) { + AlgoState before = (AlgoState) data; + return makeConditional(before, node.getCondition(), node.getChild(1), node.getChild(2)); + } + + AlgoState makeConditional(AlgoState before, JavaNode condition, JavaNode thenBranch, JavaNode elseBranch) { + AlgoState thenState = before.fork(); + AlgoState elseState = elseBranch != null ? before.fork() : before; + + linkConditional(before, condition, thenState, elseState); + + thenState = acceptOpt(thenBranch, thenState); + elseState = acceptOpt(elseBranch, elseState); return elseState.absorb(thenState); } + private AlgoState linkConditional(AlgoState prev, JavaNode condition, AlgoState thenState, AlgoState elseState) { + if (condition instanceof ASTConditionalOrExpression) { + return visitShortcutOrExpr(condition, prev, thenState, elseState); + } else if (condition instanceof ASTConditionalAndExpression) { + // To mimic a shortcut AND expr, swap the thenState and the elseState + // See explanations in method + return visitShortcutOrExpr(condition, prev, elseState, thenState); + } else if (condition instanceof ASTExpression && condition.getNumChildren() == 1) { + return linkConditional(prev, condition.getChild(0), thenState, elseState); + } else { + return acceptOpt(condition, prev); + } + // TODO parenthesized expression + } + + AlgoState visitShortcutOrExpr(JavaNode orExpr, + AlgoState before, + AlgoState thenState, + AlgoState elseState) { + + // + // if ( || || ... || ) + // else + // + // in , we are sure that at least was evaluated, + // but really any prefix of ... is possible so they're all merged + + // in , we are sure that all of ... were evaluated (to false) + + // If you replace || with &&, then the above holds if you swap and + // So this method handles the OR expr, the caller can swap the arguments to make an AND + + // --- + // This method side effects on thenState and elseState + + Iterator iterator = orExpr.children().iterator(); + + AlgoState cur = before; + do { + JavaNode cond = iterator.next(); + cur = linkConditional(cur, cond, thenState, elseState); + thenState.absorb(cur); + } while (iterator.hasNext()); + + elseState.absorb(cur); + + return cur; + } + + @Override public Object visit(ASTTryStatement node, Object data) { final AlgoState before = (AlgoState) data; @@ -795,8 +866,8 @@ public class UnusedAssignmentRule extends AbstractJavaRule { private static void processInitializers(List declarations, - AlgoState beforeLocal, - ClassScope scope) { + AlgoState beforeLocal, + ClassScope scope) { ReachingDefsVisitor visitor = new ReachingDefsVisitor(scope); @@ -916,7 +987,7 @@ public class UnusedAssignmentRule extends AbstractJavaRule { void assign(VariableNameDeclaration var, JavaNode rhs) { AssignmentEntry entry = new AssignmentEntry(var, rhs); - Set killed = reachingDefs.put(var, newSet(entry)); + Set killed = reachingDefs.put(var, Collections.singleton(entry)); if (killed != null) { // those assignments were overwritten ("killed") for (AssignmentEntry k : killed) { @@ -932,12 +1003,6 @@ public class UnusedAssignmentRule extends AbstractJavaRule { global.allAssignments.add(entry); } - private Set newSet(AssignmentEntry member) { - HashSet set = new HashSet<>(1); - set.add(member); - return set; - } - void use(VariableNameDeclaration var) { Set reaching = reachingDefs.get(var); // may be null for implicit assignments, like method parameter @@ -950,8 +1015,9 @@ public class UnusedAssignmentRule extends AbstractJavaRule { reachingDefs.remove(var); } - // fork duplicates this context, to preserve the reaching defs + // Forks duplicate this context, to preserve the reaching defs // of the current context while analysing a sub-block + // Forks must be merged later if control flow merges again, see ::absorb AlgoState fork() { return doFork(this, new HashMap<>(this.reachingDefs)); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnusedAssignment.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnusedAssignment.xml index 066b1d78a9..ff36c5c1c9 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnusedAssignment.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/UnusedAssignment.xml @@ -1978,5 +1978,171 @@ public class Foo { ]]> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Test shortcut OR + 3 + 5,7,8 + + The initializer for variable 'i' is never used (overwritten on line 7) + The value assigned to variable 'j' is never used (overwritten on line 8) + The value assigned to variable 'j' is never used (goes out of scope) + + + + + + + Test shortcut OR 2 + 1 + + The initializer for variable 'i' is never used (overwritten on line 7) + + + + + + + Test shortcut AND + 2 + 5,7 + + The initializer for variable 'i' is never used (overwritten on line 7) + The value assigned to variable 'j' is never used (overwritten on line 8) + + + + + + Test shortcut AND 2 + 1 + + The initializer for variable 'i' is never used (overwritten on line 7) + + + +