From 445b488bcdd30dc45b17420ceb888a15032ad5ef Mon Sep 17 00:00:00 2001 From: Nicolas PERU Date: Thu, 26 Nov 2015 16:58:43 +0100 Subject: [PATCH] SONARJAVA-1401 Correction of CFG for conditional OR/AND --- .../src/main/java/org/sonar/java/cfg/CFG.java | 33 ++++++++----------- java-squid/src/test/files/se/Reproducer.java | 4 +++ .../test/java/org/sonar/java/cfg/CFGTest.java | 24 ++++++++++++-- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/java-squid/src/main/java/org/sonar/java/cfg/CFG.java b/java-squid/src/main/java/org/sonar/java/cfg/CFG.java index 220b017c8e1..ac3cb44b2e8 100644 --- a/java-squid/src/main/java/org/sonar/java/cfg/CFG.java +++ b/java-squid/src/main/java/org/sonar/java/cfg/CFG.java @@ -149,17 +149,6 @@ public Block falseBlock() { return falseBlock; } - Block branchingBlock() { - if (elements.isEmpty() && terminator != null) { - if (terminator.is(Tree.Kind.CONDITIONAL_AND)) { - return falseBlock; - } else if (terminator.is(Tree.Kind.CONDITIONAL_OR)) { - return trueBlock; - } - } - return this; - } - void addSuccessor(Block successor) { successors.add(successor); } @@ -529,26 +518,30 @@ private void buildMemberSelect(MemberSelectExpressionTree mse) { } private void buildConditionalAnd(BinaryExpressionTree tree) { - // If the current block is itself a conditional expression, the false block should branch to the false block of it - final Block falseBlock = currentBlock; - // process RHS + Block falseBlock = currentBlock; currentBlock = createBlock(falseBlock); + // process RHS build(tree.rightOperand()); - final Block trueBlock = currentBlock; // process LHS - currentBlock = createBranch(tree, trueBlock, falseBlock.branchingBlock()); - build(tree.leftOperand()); + buildConditionalBinaryLHS(tree, currentBlock, falseBlock); } private void buildConditionalOr(BinaryExpressionTree tree) { - // process RHS Block trueBlock = currentBlock; currentBlock = createBlock(trueBlock); + // process RHS build(tree.rightOperand()); - Block falseBlock = currentBlock; // process LHS - currentBlock = createBranch(tree, trueBlock.branchingBlock(), falseBlock); + buildConditionalBinaryLHS(tree, trueBlock, currentBlock); + } + + private void buildConditionalBinaryLHS(BinaryExpressionTree tree, Block trueBlock, Block falseBlock) { + currentBlock = createBlock(); + Block toComplete = currentBlock; build(tree.leftOperand()); + toComplete.terminator = tree; + toComplete.addFalseSuccessor(falseBlock); + toComplete.addTrueSuccessor(trueBlock); } private void buildLabeledStatement(LabeledStatementTree labeledStatement) { diff --git a/java-squid/src/test/files/se/Reproducer.java b/java-squid/src/test/files/se/Reproducer.java index 7a450ed9cfe..e50ae7a9ce3 100644 --- a/java-squid/src/test/files/se/Reproducer.java +++ b/java-squid/src/test/files/se/Reproducer.java @@ -83,4 +83,8 @@ private void increment(int index, int index2) { } } + private boolean sizesDontMatch(boolean bool, boolean a, boolean b) { + return (!bool && a) || (bool && b); + } + } \ No newline at end of file diff --git a/java-squid/src/test/java/org/sonar/java/cfg/CFGTest.java b/java-squid/src/test/java/org/sonar/java/cfg/CFGTest.java index 319e2d25c4a..17414e59ec3 100644 --- a/java-squid/src/test/java/org/sonar/java/cfg/CFGTest.java +++ b/java-squid/src/test/java/org/sonar/java/cfg/CFGTest.java @@ -256,6 +256,7 @@ public ElementChecker(final Tree.Kind kind) { case PLUS_ASSIGNMENT: case ASSIGNMENT: case ARRAY_ACCESS_EXPRESSION: + case LOGICAL_COMPLEMENT: case PLUS: break; default: @@ -1141,7 +1142,7 @@ public void returnCascadedAnd() throws Exception { final CFG cfg = buildCFG( "void andAll(boolean a, boolean b, boolean c) { return a && b && c;}"); final CFGChecker cfgChecker = checker( - block(element(Kind.IDENTIFIER, "a")).terminator(Kind.CONDITIONAL_AND).ifTrue(4).ifFalse(1), + block(element(Kind.IDENTIFIER, "a")).terminator(Kind.CONDITIONAL_AND).ifTrue(4).ifFalse(3), block(element(Kind.IDENTIFIER, "b")).successors(3), terminator(Kind.CONDITIONAL_AND).ifTrue(2).ifFalse(1), block(element(Kind.IDENTIFIER, "c")).successors(1), @@ -1154,7 +1155,7 @@ public void returnCascadedOr() throws Exception { final CFG cfg = buildCFG( "void orAll(boolean a, boolean b, boolean c) { return a || b || c;}"); final CFGChecker cfgChecker = checker( - block(element(Kind.IDENTIFIER, "a")).terminator(Kind.CONDITIONAL_OR).ifTrue(1).ifFalse(4), + block(element(Kind.IDENTIFIER, "a")).terminator(Kind.CONDITIONAL_OR).ifTrue(3).ifFalse(4), block(element(Kind.IDENTIFIER, "b")).successors(3), terminator(Kind.CONDITIONAL_OR).ifTrue(1).ifFalse(2), block(element(Kind.IDENTIFIER, "c")).successors(1), @@ -1162,6 +1163,25 @@ public void returnCascadedOr() throws Exception { cfgChecker.check(cfg); } + @Test + public void complex_boolean_expression() throws Exception { + final CFG cfg = buildCFG(" private boolean fun(boolean bool, boolean a, boolean b) {\n" + + " return (!bool && a) || (bool && b);\n" + + " }"); + final CFGChecker cfgChecker = checker( + block( + element(Kind.IDENTIFIER, "bool"), + element(Kind.LOGICAL_COMPLEMENT) + ).terminator(Kind.CONDITIONAL_AND).ifTrue(5).ifFalse(4), + block(element(Kind.IDENTIFIER, "a")).successors(4), + terminator(Kind.CONDITIONAL_OR).ifTrue(1).ifFalse(3), + block(element(Kind.IDENTIFIER, "bool")).terminator(Kind.CONDITIONAL_AND).ifTrue(2).ifFalse(1), + block(element(Kind.IDENTIFIER, "b")).successors(1), + terminator(Kind.RETURN_STATEMENT).successors(0)); + cfgChecker.check(cfg); + + } + @Test public void method_reference() throws Exception { final CFG cfg = buildCFG("void fun() { foo(Object::toString); }");