SONARPY-145 Raise issues with precise location for SameBranchCheck

vilchik-elena · pynicolas · commit 0cdc63423b6e · 2016-06-30T14:38:59.000+02:00
diff --git a/its/ruling/src/test/resources/expected/python-S1871.json b/its/ruling/src/test/resources/expected/python-S1871.json
@@ -9,7 +9,7 @@
 664,
 ],
 'project:buildbot-0.8.6p1/contrib/bzr_buildbot.py':[
-221,
+229,
 ],
 'project:django-1.4/django/contrib/auth/tests/auth_backends.py':[
 120,
@@ -49,7 +49,7 @@
 115,
 ],
 'project:django-1.4/django/utils/regex_helper.py':[
-182,
+183,
 ],
 'project:django-1.4/django/views/generic/date_based.py':[
 160,
@@ -78,7 +78,7 @@
 295,
 ],
 'project:twisted-12.1.0/twisted/news/nntp.py':[
-904,
+905,
 ],
 'project:twisted-12.1.0/twisted/persisted/aot.py':[
 491,
@@ -94,7 +94,7 @@
 111,
 ],
 'project:twisted-12.1.0/twisted/test/test_doc.py':[
-67,
+69,
 ],
 'project:twisted-12.1.0/twisted/trial/runner.py':[
 626,
@@ -106,7 +106,7 @@
 419,
 ],
 'project:twisted-12.1.0/twisted/web/sux.py':[
-331,
+333,
 556,
 ],
 'project:twisted-12.1.0/twisted/words/protocols/oscar.py':[
diff --git a/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java b/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java
@@ -20,19 +20,21 @@
 package org.sonar.python.checks;
 
 import com.sonar.sslr.api.AstNode;
-import com.sonar.sslr.api.Grammar;
 import java.util.LinkedList;
 import java.util.List;
 import javax.annotation.Nullable;
 import org.sonar.check.Priority;
 import org.sonar.check.Rule;
+import org.sonar.python.PythonCheck;
 import org.sonar.python.api.PythonGrammar;
 import org.sonar.python.api.PythonKeyword;
+import org.sonar.python.api.PythonTokenType;
 import org.sonar.squidbridge.annotations.ActivatedByDefault;
 import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
-import org.sonar.squidbridge.checks.SquidCheck;
 import org.sonar.sslr.ast.AstSelect;
 
+import static org.sonar.python.api.PythonGrammar.STMT_LIST;
+
 @Rule(
     key = SameBranchCheck.CHECK_KEY,
     priority = Priority.MAJOR,
@@ -41,7 +43,7 @@
 )
 @SqaleConstantRemediation("10min")
 @ActivatedByDefault
-public class SameBranchCheck extends SquidCheck<Grammar> {
+public class SameBranchCheck extends PythonCheck {
   public static final String CHECK_KEY = "S1871";
   public static final String MESSAGE = "Either merge this branch with the identical one on line \"%s\" or change one of the implementations.";
 
@@ -91,15 +93,43 @@ private void findSameBranches(List<AstNode> branches) {
   }
 
   private void checkBranch(List<AstNode> branches, int index) {
+    AstNode duplicateBlock = branches.get(index);
     for (int j = 0; j < index; j++) {
-      if (CheckUtils.equalNodes(branches.get(j), branches.get(index))) {
-        String message = String.format(MESSAGE, branches.get(j).getToken().getLine() + 1);
-        getContext().createLineViolation(this, message, branches.get(index).getToken().getLine() + 1);
+      AstNode originalBlock = branches.get(j);
+      if (CheckUtils.equalNodes(originalBlock, duplicateBlock)) {
+        String message = String.format(MESSAGE, originalBlock.getToken().getLine() + 1);
+        addIssue(location(duplicateBlock, message))
+          .secondary(location(originalBlock, "Original"));
         return;
       }
     }
   }
 
+  private static IssueLocation location(AstNode suiteNode, String message) {
+    AstNode firstStatement = suiteNode.getFirstChild(PythonGrammar.STATEMENT);
+    if (firstStatement != null) {
+      return new IssueLocation(firstStatement, getLastNode(suiteNode), message);
+    } else {
+      return new IssueLocation(suiteNode.getFirstChild(STMT_LIST), message);
+    }
+  }
+
+  /**
+   * We need this method to avoid passing of new line or dedent as pointer to end of issue location
+   */
+  private static AstNode getLastNode(AstNode node) {
+    if (node.getNumberOfChildren() == 0) {
+      return node;
+    }
+
+    AstNode lastChild = node.getLastChild();
+    while (lastChild.is(PythonTokenType.NEWLINE, PythonTokenType.DEDENT, PythonTokenType.INDENT)) {
+      lastChild = lastChild.getPreviousSibling();
+    }
+
+    return getLastNode(lastChild);
+  }
+
   private static AstNode singleIfChild(AstNode suite) {
     List<AstNode> statements = suite.getChildren(PythonGrammar.STATEMENT);
     if (statements.size() == 1) {
diff --git a/python-checks/src/test/java/org/sonar/python/checks/SameBranchCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/SameBranchCheckTest.java
@@ -21,23 +21,14 @@
 
 import java.io.File;
 import org.junit.Test;
-import org.sonar.python.PythonAstScanner;
-import org.sonar.squidbridge.api.SourceFile;
-import org.sonar.squidbridge.checks.CheckMessagesVerifier;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
 
 public class SameBranchCheckTest {
 
   @Test
   public void test() {
     SameBranchCheck check = new SameBranchCheck();
-
-    SourceFile file = PythonAstScanner.scanSingleFile(new File("src/test/resources/checks/sameBranch.py"), check);
-    CheckMessagesVerifier.verify(file.getCheckMessages())
-        .next().atLine(9).withMessage(String.format(SameBranchCheck.MESSAGE, 5))
-        .next().atLine(17).withMessage(String.format(SameBranchCheck.MESSAGE, 14))
-        .next().atLine(26).withMessage(String.format(SameBranchCheck.MESSAGE, 21))
-        .next().atLine(32).withMessage(String.format(SameBranchCheck.MESSAGE, 30))
-        .noMore();
+    PythonCheckVerifier.verify(new File("src/test/resources/checks/sameBranch.py"), check);
   }
 
 }
diff --git a/python-checks/src/test/resources/checks/sameBranch.py b/python-checks/src/test/resources/checks/sameBranch.py
@@ -3,30 +3,39 @@
 
 if 0 <= a < 10:
     print(1)
+    foo()
 elif 10 <= a < 20:
     print(2)
 elif 20 <= a < 50:
-    print(1)
+    print(1) # Noncompliant [[sc=5;ec=10;el=+1;secondary=-5]]
+    foo()
 else:
     print(3)
 
 if param == 1:
     print(1)
 else:
     if param == 2:
-        print(1)
+        print(1) # Noncompliant [[secondary=-3]]
 
 
 if param == 1:
-    print(1)
+    if True:
+        pass
 else:
     if param == 2:
         print(2)
     else:
-        print(1)
+        if True: # Noncompliant [[secondary=-6]]
+            pass
 
 if True:
     if True:
         print(1)
     else:
-        print(1)
+        print(1)  # Noncompliant {{Either merge this branch with the identical one on line "34" or change one of the implementations.}}
+
+if 1: print("1"); foo()
+# Noncompliant@+1 [[secondary=-2;sc=9;ec=26;el=+0]]
+elif 2: print("1"); foo()
+else: print("2")
