SONARPY-1024: Add quick fixes for S5799 (ImplicitStringConcatenationCheck) (#1140)

Author: marco-bearzi-sonarsource

python-checks/pom.xml

@@ -53,9 +53,9 @@
       <scope>test</scope>
     </dependency>
     <dependency>
-        <groupId>org.sonarsource.analyzer-commons</groupId>
-        <artifactId>sonar-analyzer-commons</artifactId>
-        <scope>test</scope>
+      <groupId>org.sonarsource.analyzer-commons</groupId>
+      <artifactId>sonar-analyzer-commons</artifactId>
+      <scope>test</scope>
     </dependency>
     <dependency>
       <groupId>org.mockito</groupId>

python-checks/src/main/java/org/sonar/python/checks/ImplicitStringConcatenationCheck.java

@@ -27,6 +27,10 @@
 import org.sonar.plugins.python.api.tree.StringElement;
 import org.sonar.plugins.python.api.tree.StringLiteral;
 import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.python.quickfix.IssueWithQuickFix;
+import org.sonar.python.quickfix.PythonQuickFix;
+
+import static org.sonar.python.quickfix.PythonTextEdit.insertAfter;
 
 @Rule(key = "S5799")
 public class ImplicitStringConcatenationCheck extends PythonSubscriptionCheck {
@@ -58,17 +62,17 @@ private static void checkStringLiteral(StringLiteral stringLiteral, Subscription
     List<StringElement> stringElements = stringLiteral.stringElements();
     for (int i = 1; i < stringElements.size(); i++) {
       StringElement current = stringElements.get(i);
-      StringElement previous = stringElements.get(i-1);
+      StringElement previous = stringElements.get(i - 1);
       if (!current.prefix().equalsIgnoreCase(previous.prefix()) || !haveSameQuotes(current, previous)) {
         continue;
       }
       if (current.firstToken().line() == previous.firstToken().line()) {
-        ctx.addIssue(previous.firstToken(), MESSAGE_SINGLE_LINE).secondary(current.firstToken(), null);
+        createQuickFix(ctx.addIssue(previous.firstToken(), MESSAGE_SINGLE_LINE).secondary(current.firstToken(), null), previous);
         // Only raise 1 issue per string literal
         return;
       }
       if ((isWithinCollection(stringLiteral) && !isException(previous, current))) {
-        ctx.addIssue(previous.firstToken(), MESSAGE_MULTIPLE_LINES).secondary(current.firstToken(), null);
+        createQuickFix(ctx.addIssue(previous.firstToken(), MESSAGE_MULTIPLE_LINES).secondary(current.firstToken(), null), previous);
         return;
       }
     }
@@ -89,5 +93,28 @@ private static boolean haveSameQuotes(StringElement first, StringElement second)
     return first.isTripleQuoted() == second.isTripleQuoted() &&
       first.value().charAt(first.value().length() - 1) == second.value().charAt(second.value().length() - 1);
   }
-}
 
+  private static boolean isInFunctionOrArrayOrTupleOrExpression(StringElement token) {
+    Tree t = token;
+    while (t.parent().is(Tree.Kind.STRING_LITERAL)) {
+      t = t.parent();
+    }
+    return t.parent().is(Tree.Kind.ARG_LIST, Tree.Kind.EXPRESSION_LIST, Tree.Kind.PARAMETER_LIST, Tree.Kind.TUPLE);
+  }
+
+  private static void createQuickFix(PreciseIssue issueRaised, StringElement stringElement) {
+    IssueWithQuickFix issue = (IssueWithQuickFix) issueRaised;
+
+    if (isInFunctionOrArrayOrTupleOrExpression(stringElement)) {
+      PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Add the comma between string or byte tokens.")
+        .addTextEdit(insertAfter(stringElement, ","))
+        .build();
+      issue.addQuickFix(quickFix);
+    }
+
+    PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Make the addition sign between string or byte tokens explicit.")
+      .addTextEdit(insertAfter(stringElement, "+"))
+      .build();
+    issue.addQuickFix(quickFix);
+  }
+}

python-checks/src/test/java/org/sonar/python/checks/ImplicitStringConcatenationCheckTest.java

@@ -20,12 +20,40 @@
 package org.sonar.python.checks;
 
 import org.junit.Test;
+import org.sonar.plugins.python.api.PythonCheck;
+import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
 
 public class ImplicitStringConcatenationCheckTest {
 
+  private static final PythonCheck check = new ImplicitStringConcatenationCheck();
+
   @Test
   public void test() {
-    PythonCheckVerifier.verify("src/test/resources/checks/implicitStringConcatenation.py", new ImplicitStringConcatenationCheck());
+    PythonCheckVerifier.verify("src/test/resources/checks/implicitStringConcatenation.py", check);
+  }
+
+  @Test
+  public void simple_expression_quickfix() {
+    String codeWithIssue = "a = '1' '2'";
+    String codeFixed1 = "a = '1'+ '2'";
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed1);
+
+    codeWithIssue = "a = ['1' '2']";
+    codeFixed1 = "a = ['1', '2']";
+    String codeFixed2 = "a = ['1'+ '2']";
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed1, codeFixed2);
   }
+
+  @Test
+  public void complex_expression_quickfix() {
+    String codeWithIssue = "def a():\n" +
+      "    b = ['1' '2']\n";
+    String codeFixed1 = "def a():\n" +
+      "    b = ['1', '2']\n";
+    String codeFixed2 = "def a():\n" +
+      "    b = ['1'+ '2']\n";
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed1, codeFixed2);
+  }
+
 }

python-checks/src/test/java/org/sonar/python/checks/quickfix/PythonQuickFixVerifier.java

@@ -21,8 +21,10 @@
 
 import com.sonar.sslr.api.AstNode;
 import java.net.URI;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.stream.Collectors;
 import org.sonar.plugins.python.api.PythonCheck;
 import org.sonar.plugins.python.api.PythonCheck.PreciseIssue;
 import org.sonar.plugins.python.api.PythonFile;
@@ -32,6 +34,7 @@
 import org.sonar.python.SubscriptionVisitor;
 import org.sonar.python.parser.PythonParser;
 import org.sonar.python.quickfix.IssueWithQuickFix;
+import org.sonar.python.quickfix.PythonQuickFix;
 import org.sonar.python.quickfix.PythonTextEdit;
 import org.sonar.python.semantic.ProjectLevelSymbolTable;
 import org.sonar.python.tree.PythonTreeMaker;
@@ -42,7 +45,7 @@ public class PythonQuickFixVerifier {
   private PythonQuickFixVerifier() {
   }
 
-  public static void verify(PythonCheck check, String codeWithIssue, String codeFixed) {
+  public static void verify(PythonCheck check, String codeWithIssue, String... codesFixed) {
     List<PythonCheck.PreciseIssue> issues = PythonQuickFixVerifier
       .getIssuesWithQuickFix(check, codeWithIssue);
 
@@ -54,14 +57,19 @@ public static void verify(PythonCheck check, String codeWithIssue, String codeFi
 
     assertThat(issue.getQuickFixes())
       .as("Number of quickfixes")
-      .overridingErrorMessage("Expected 1 quickfix but found %d", issue.getQuickFixes().size())
-      .hasSize(1);
+      .overridingErrorMessage("Expected %d quickfix but found %d", codesFixed.length, issue.getQuickFixes().size())
+      .hasSize(codesFixed.length);
+
+    List<String> appliedQuickFix = issue.getQuickFixes().stream()
+      .map(quickFix ->  applyQuickFix(codeWithIssue, quickFix))
+      .collect(Collectors.toList());
 
-    String codeQFApplied = PythonQuickFixVerifier.applyQuickFix(codeWithIssue, issue);
-    assertThat(codeQFApplied)
+    assertThat(appliedQuickFix)
       .as("Application of the quickfix")
-      .overridingErrorMessage("The code with the quickfix applied is not the expected result : %s instead of %s", codeQFApplied, codeFixed)
-      .isEqualTo(codeFixed);
+      .overridingErrorMessage("The code with the quickfix applied is not the expected result.\n" +
+        "Applied QuickFixes are:\n%s\nExpected result:\n%s", Arrays.asList(codesFixed), appliedQuickFix)
+      .isEqualTo(Arrays.asList(codesFixed));
+
   }
 
   private static List<PreciseIssue> scanFileForIssues(PythonCheck check, PythonVisitorContext context) {
@@ -85,12 +93,11 @@ private static List<PreciseIssue> getIssuesWithQuickFix(PythonCheck check, Strin
     return scanFileForIssues(check, visitorContext);
   }
 
-  private static String applyQuickFix(String codeWithIssue, IssueWithQuickFix issueWithQuickFix) {
-    assertThat(issueWithQuickFix.getQuickFixes()).hasSize(1);
-    PythonTextEdit loc = issueWithQuickFix.getQuickFixes().get(0).getTextEdits().get(0);
-    String replacement = loc.replacementText();
-    int start = convertPositionToIndex(codeWithIssue, loc.startLine(), loc.startLineOffset());
-    int end = convertPositionToIndex(codeWithIssue, loc.endLine(), loc.endLineOffset());
+  private static String applyQuickFix(String codeWithIssue, PythonQuickFix quickFix) {
+    PythonTextEdit textEdit = quickFix.getTextEdits().get(0);
+    String replacement = textEdit.replacementText();
+    int start = convertPositionToIndex(codeWithIssue, textEdit.startLine(), textEdit.startLineOffset());
+    int end = convertPositionToIndex(codeWithIssue, textEdit.endLine(), textEdit.endLineOffset());
     return codeWithIssue.substring(0, start) + replacement + codeWithIssue.substring(end);
   }
 

python-checks/src/test/java/org/sonar/python/checks/quickfix/PythonQuickFixVerifierTest.java

@@ -62,7 +62,11 @@ public void one_issue_one_qf_wrong_fix() {
     SimpleCheck simpleCheck = new SimpleCheck();
     assertThatThrownBy(() -> PythonQuickFixVerifier.verify(simpleCheck, "a=10", "a==10"))
       .isInstanceOf(AssertionError.class)
-      .hasMessageContaining("[Application of the quickfix] The code with the quickfix applied is not the expected result : a!=10 instead of a==10");
+      .hasMessageContaining("[Application of the quickfix] The code with the quickfix applied is not the expected result.\n" +
+        "Applied QuickFixes are:\n" +
+        "[a==10]\n" +
+        "Expected result:\n" +
+        "[a!=10]");
   }
 
   @Test

python-checks/src/test/resources/checks/implicitStringConcatenation.py

@@ -61,6 +61,7 @@ def prefixes():
 
 def different_quotes():
   ["""1""" """2"""]  # Noncompliant
+#  ^^^^^^^ ^^^^^^^<
   "1" '2' # Ok
   "1" """
   2""" # Ok. Even if strange

python-frontend/src/main/java/org/sonar/python/quickfix/PythonTextEdit.java

@@ -43,6 +43,12 @@ public static PythonTextEdit insertBefore(Tree tree, String textToInsert) {
     return new PythonTextEdit(textToInsert, token.line(), token.column(), token.line(), token.column());
   }
 
+  public static PythonTextEdit insertAfter(Tree tree, String textToInsert) {
+    Token token = tree.firstToken();
+    int lengthToken = token.value().length();
+    return new PythonTextEdit(textToInsert, token.line(), token.column() + lengthToken, token.line(), token.column() + lengthToken);
+  }
+
   public String replacementText() {
     return message;
   }

python-frontend/src/test/java/org/sonar/python/quickfix/PythonTextEditTest.java

@@ -46,4 +46,26 @@ public void test() {
     assertThat(textEdit.endLine()).isEqualTo(1);
     assertThat(textEdit.endLineOffset()).isEqualTo(7);
   }
+
+  @Test
+  public void insert_after() {
+    String tokenValue = "token";
+    String replacementText = "This is a replacement text";
+
+    Token token = Mockito.mock(Token.class);
+    when(token.line()).thenReturn(1);
+    when(token.column()).thenReturn(7);
+    when(token.firstToken()).thenReturn(token);
+    when(token.lastToken()).thenReturn(token);
+
+    when(token.value()).thenReturn(tokenValue);
+
+    PythonTextEdit textEdit = PythonTextEdit.insertAfter(token, replacementText);
+
+    assertThat(textEdit.replacementText()).isEqualTo(replacementText);
+    assertThat(textEdit.startLine()).isEqualTo(1);
+    assertThat(textEdit.startLineOffset()).isEqualTo(12);
+    assertThat(textEdit.endLine()).isEqualTo(1);
+    assertThat(textEdit.endLineOffset()).isEqualTo(12);
+  }
 }