SONARJAVA-1401 Correction of CFG for conditional OR/AND

SonarSource · Nov 26, 2015 · 445b488 · 445b488
1 parent 6e95154
commit 445b488
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 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
@@ -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
@@ -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);
+  }
+
 }
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
@@ -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,14 +1155,33 @@ 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),
       terminator(Kind.RETURN_STATEMENT).successors(0));
     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); }");