SONARPY-1027: Add quick fix for S1940 (BooleanCheckNotInvertedCheck) (#1143)

Author: marco-bearzi-sonarsource

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

@@ -19,6 +19,7 @@
  */
 package org.sonar.python.checks;
 
+import java.util.List;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
@@ -28,8 +29,12 @@
 import org.sonar.plugins.python.api.tree.IsExpression;
 import org.sonar.plugins.python.api.tree.ParenthesizedExpression;
 import org.sonar.plugins.python.api.tree.Token;
+import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.tree.Tree.Kind;
 import org.sonar.plugins.python.api.tree.UnaryExpression;
+import org.sonar.python.quickfix.IssueWithQuickFix;
+import org.sonar.python.quickfix.PythonQuickFix;
+import org.sonar.python.quickfix.PythonTextEdit;
 
 @Rule(key = "S1940")
 public class BooleanCheckNotInvertedCheck extends PythonSubscriptionCheck {
@@ -46,56 +51,77 @@ private static void checkNotExpression(SubscriptionContext ctx, UnaryExpression
     while (negatedExpr.is(Kind.PARENTHESIZED)) {
       negatedExpr = ((ParenthesizedExpression) negatedExpr).expression();
     }
-    if(negatedExpr.is(Kind.COMPARISON)) {
+    if (negatedExpr.is(Kind.COMPARISON)) {
       BinaryExpression binaryExp = (BinaryExpression) negatedExpr;
       // Don't raise warning with "not a == b == c" because a == b != c is not equivalent
-      if(!binaryExp.leftOperand().is(Kind.COMPARISON)) {
-        ctx.addIssue(original, String.format(MESSAGE, oppositeOperator(binaryExp.operator())));
+      if (!binaryExp.leftOperand().is(Kind.COMPARISON)) {
+        String oppositeOperator = oppositeOperator(binaryExp.operator());
+
+        IssueWithQuickFix issue = ((IssueWithQuickFix) ctx.addIssue(original, String.format(MESSAGE, oppositeOperator)));
+        createQuickFix(issue, oppositeOperator, binaryExp, negatedExpr);
       }
-    } else if(negatedExpr.is(Kind.IN, Kind.IS) ) {
+    } else if (negatedExpr.is(Kind.IN, Kind.IS)) {
       BinaryExpression isInExpr = (BinaryExpression) negatedExpr;
-      ctx.addIssue(original, String.format(MESSAGE, oppositeOperator(isInExpr.operator(), isInExpr)));
+      String oppositeOperator = oppositeOperator(isInExpr.operator(), isInExpr);
+
+      IssueWithQuickFix issue = ((IssueWithQuickFix) ctx.addIssue(original, String.format(MESSAGE, oppositeOperator)));
+      createQuickFix(issue, oppositeOperator, isInExpr, negatedExpr);
     }
   }
 
-  private static String oppositeOperator(Token operator){
+  private static String oppositeOperator(Token operator) {
     return oppositeOperatorString(operator.value());
   }
 
-  private static String oppositeOperator(Token operator, Expression expr){
+  private static String oppositeOperator(Token operator, Expression expr) {
     String s = operator.value();
-    if(expr.is(Kind.IS) && ((IsExpression) expr).notToken() != null){
+    if (expr.is(Kind.IS) && ((IsExpression) expr).notToken() != null) {
       s = s + " not";
-    } else if(expr.is(Kind.IN) && ((InExpression) expr).notToken() != null){
+    } else if (expr.is(Kind.IN) && ((InExpression) expr).notToken() != null) {
       s = "not " + s;
     }
     return oppositeOperatorString(s);
   }
 
-  static String oppositeOperatorString(String stringOperator){
-    switch (stringOperator){
-      case ">"  :
+  static String oppositeOperatorString(String stringOperator) {
+    switch (stringOperator) {
+      case ">":
         return "<=";
-      case ">=" :
+      case ">=":
         return "<";
-      case "<"  :
+      case "<":
         return ">=";
-      case "<=" :
+      case "<=":
         return ">";
-      case "==" :
+      case "==":
         return "!=";
-      case "!=" :
+      case "!=":
         return "==";
-      case "is" :
+      case "is":
         return "is not";
       case "is not":
         return "is";
-      case "in" :
+      case "in":
         return "not in";
       case "not in":
         return "in";
-      default   :
+      default:
         throw new IllegalArgumentException("Unknown comparison operator : " + stringOperator);
     }
   }
+
+  private static void createQuickFix(IssueWithQuickFix issue, String oppositeOperator, BinaryExpression toUse, Expression toReplace) {
+    PythonTextEdit replaceEdit = getReplaceEdit(toUse, toReplace, oppositeOperator);
+
+    PythonQuickFix quickFix = PythonQuickFix.newQuickFix(String.format("Use %s instead", oppositeOperator))
+      .addTextEdit(replaceEdit)
+      .build();
+    issue.addQuickFix(quickFix);
+  }
+
+  private static PythonTextEdit getReplaceEdit(BinaryExpression toUse, Expression toReplace, String oppositeOperator) {
+    List<Tree> children = toReplace.parent().parent().children();
+    return PythonTextEdit.replaceRange(children.get(0), children.get(children.size() - 1),
+      toUse.leftOperand().firstToken().value() + " " + oppositeOperator + " " + toUse.rightOperand().firstToken().value());
+  }
 }

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

@@ -21,6 +21,7 @@
 
 import org.assertj.core.api.Assertions;
 import org.junit.Test;
+import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -48,4 +49,35 @@ public void operatorStringTest() {
     Assertions.assertThatThrownBy(() -> BooleanCheckNotInvertedCheck.oppositeOperatorString("-")).isInstanceOf(IllegalArgumentException.class);
     Assertions.assertThatThrownBy(() -> BooleanCheckNotInvertedCheck.oppositeOperatorString("*")).isInstanceOf(IllegalArgumentException.class);
   }
+
+  @Test
+  public void test_quickfix() {
+    String codeWithIssue = "a = not(b == c)";
+    String codeFixed = "a = b != c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b != c)";
+    codeFixed = "a = b == c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b < c)";
+    codeFixed = "a = b >= c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b is c)";
+    codeFixed = "a = b is not c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b is not c)";
+    codeFixed = "a = b is c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b in c)";
+    codeFixed = "a = b not in c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+
+    codeWithIssue = "a = not (b not in c)";
+    codeFixed = "a = b in c";
+    PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
+  }
 }

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

@@ -22,7 +22,9 @@
 import com.sonar.sslr.api.AstNode;
 import java.net.URI;
 import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.stream.Collectors;
 import org.sonar.plugins.python.api.PythonCheck;
@@ -61,13 +63,13 @@ public static void verify(PythonCheck check, String codeWithIssue, String... cod
       .hasSize(codesFixed.length);
 
     List<String> appliedQuickFix = issue.getQuickFixes().stream()
-      .map(quickFix ->  applyQuickFix(codeWithIssue, quickFix))
+      .map(quickFix -> applyQuickFix(codeWithIssue, quickFix))
       .collect(Collectors.toList());
 
     assertThat(appliedQuickFix)
       .as("Application of the quickfix")
       .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)
+        "Applied QuickFixes are:\n%s\nExpected result:\n%s", appliedQuickFix, Arrays.asList(codesFixed))
       .isEqualTo(Arrays.asList(codesFixed));
 
   }
@@ -94,13 +96,96 @@ private static List<PreciseIssue> getIssuesWithQuickFix(PythonCheck check, Strin
   }
 
   private static String applyQuickFix(String codeWithIssue, PythonQuickFix quickFix) {
-    PythonTextEdit textEdit = quickFix.getTextEdits().get(0);
+    List<PythonTextEdit> sortedEdits = sortTextEdits(quickFix.getTextEdits());
+    String codeBeingFixed = codeWithIssue;
+    for (PythonTextEdit edit : sortedEdits) {
+      codeBeingFixed = applyTextEdit(codeBeingFixed, edit);
+    }
+    return codeBeingFixed;
+  }
+
+  private static String applyTextEdit(String codeWithIssue, PythonTextEdit textEdit) {
     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);
   }
 
+  private static List<PythonTextEdit> sortTextEdits(List<PythonTextEdit> pythonTextEdits) {
+    checkNoCollision(pythonTextEdits);
+    ArrayList<PythonTextEdit> list = new ArrayList<>(pythonTextEdits);
+    list.sort(Comparator.comparingInt(PythonTextEdit::startLine).thenComparing(PythonTextEdit::startLineOffset));
+    Collections.reverse(list);
+    return Collections.unmodifiableList(list);
+  }
+
+  private static void checkNoCollision(List<PythonTextEdit> pythonTextEdits) throws IllegalArgumentException {
+    for (int i = 0; i < pythonTextEdits.size(); i++) {
+      PythonTextEdit edit = pythonTextEdits.get(i);
+      for (int j = i + 1; j < pythonTextEdits.size(); j++) {
+        PythonTextEdit edit2 = pythonTextEdits.get(j);
+        if (oneEnclosedByTheOther(edit2, edit)) {
+          throw new IllegalArgumentException("There is a collision between the range of the quickfixes.");
+        }
+      }
+    }
+  }
+
+  private static boolean oneEnclosedByTheOther(PythonTextEdit toCheck, PythonTextEdit reference) {
+    if (onSameLine(toCheck, reference)) {
+      // If on same line, we need to check that the bounds of toCheck are not contained in reference bounds
+      return !(toCheck.endLineOffset() < reference.startLineOffset() || toCheck.startLineOffset() > reference.endLineOffset());
+    } else {
+      if (compactOnDifferentLines(toCheck, reference)) {
+        return false;
+      } else if (isCompact(toCheck)) {
+        return isSecondInFirst(toCheck, reference);
+      } else if (isCompact(reference)) {
+        return isSecondInFirst(reference, toCheck);
+      } else {
+        // Both edits exploded on different lines
+        if (noLineIntersection(toCheck, reference)) {
+          return false;
+        } else {
+          // There is an intersection between edits, only need to check valid case
+          if (reference.startLine() == toCheck.endLine()) {
+            return !(toCheck.endLineOffset() < reference.startLineOffset());
+          } else if (reference.endLine() == toCheck.startLine()) {
+            return !(reference.endLineOffset() < toCheck.startLineOffset());
+          }
+        }
+      }
+    }
+    // All other cases are invalid and will cause an intersection
+    return true;
+  }
+
+  private static boolean onSameLine(PythonTextEdit check, PythonTextEdit ref) {
+    return ref.startLine() == ref.endLine() && ref.endLine() == check.startLine() && check.startLine() == check.endLine();
+  }
+
+  private static boolean compactOnDifferentLines(PythonTextEdit check, PythonTextEdit ref) {
+    return ref.startLine() == ref.endLine() && check.startLine() == check.endLine() && ref.endLine() != check.startLine();
+  }
+
+  private static boolean isCompact(PythonTextEdit check) {
+    return check.startLine() == check.endLine();
+  }
+
+  private static boolean isSecondInFirst(PythonTextEdit second, PythonTextEdit first) {
+    if (first.startLine() == second.startLine()) {
+      return first.startLineOffset() <= second.endLineOffset();
+    } else if (first.endLine() == second.startLine()) {
+      return second.startLineOffset() < first.endLineOffset();
+    }
+    // No intersection
+    return false;
+  }
+
+  private static boolean noLineIntersection(PythonTextEdit check, PythonTextEdit ref) {
+    return check.endLine() < ref.startLine() || check.startLine() > ref.endLine();
+  }
+
   private static int convertPositionToIndex(String fileContent, int line, int lineOffset) {
     int currentLine = 1;
     int currentIndex = 0;

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

@@ -64,9 +64,9 @@ public void one_issue_one_qf_wrong_fix() {
       .isInstanceOf(AssertionError.class)
       .hasMessageContaining("[Application of the quickfix] The code with the quickfix applied is not the expected result.\n" +
         "Applied QuickFixes are:\n" +
-        "[a==10]\n" +
+        "[a!=10]\n" +
         "Expected result:\n" +
-        "[a!=10]");
+        "[a==10]");
   }
 
   @Test

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

@@ -49,6 +49,22 @@ public static PythonTextEdit insertAfter(Tree tree, String textToInsert) {
     return new PythonTextEdit(textToInsert, token.line(), token.column() + lengthToken, token.line(), token.column() + lengthToken);
   }
 
+  public static PythonTextEdit replace(Tree toReplace, String replacementText) {
+    Token token = toReplace.firstToken();
+    Token last = token.lastToken();
+    return new PythonTextEdit(replacementText, token.line(), token.column(), last.line(), last.column() + last.value().length());
+  }
+
+  public static PythonTextEdit replaceRange(Tree start, Tree end, String replacementText) {
+    Token first =start.firstToken();
+    Token last = end.lastToken();
+    return new PythonTextEdit(replacementText, first.line(), first.column(), last.line(), last.column() + last.value().length());
+  }
+
+  public static PythonTextEdit remove(Tree toRemove) {
+    return replace(toRemove, "");
+  }
+
   public String replacementText() {
     return message;
   }

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

@@ -68,4 +68,76 @@ public void insert_after() {
     assertThat(textEdit.endLine()).isEqualTo(1);
     assertThat(textEdit.endLineOffset()).isEqualTo(12);
   }
+
+  @Test
+  public void replace() {
+    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.replace(token, replacementText);
+
+    assertThat(textEdit.replacementText()).isEqualTo(replacementText);
+    assertThat(textEdit.startLine()).isEqualTo(1);
+    assertThat(textEdit.startLineOffset()).isEqualTo(7);
+    assertThat(textEdit.endLine()).isEqualTo(1);
+    assertThat(textEdit.endLineOffset()).isEqualTo(12);
+  }
+
+  @Test
+  public void remove() {
+    String tokenValue = "token";
+
+    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.remove(token);
+
+    assertThat(textEdit.replacementText()).isEmpty();
+    assertThat(textEdit.startLine()).isEqualTo(1);
+    assertThat(textEdit.startLineOffset()).isEqualTo(7);
+    assertThat(textEdit.endLine()).isEqualTo(1);
+    assertThat(textEdit.endLineOffset()).isEqualTo(12);
+  }
+
+  @Test
+  public void replaceChildren() {
+    // Parsing 'a = (b and c)'
+    String tokenValue1 = "(";
+    String tokenValue2 = ")";
+
+    Token token1 = Mockito.mock(Token.class);
+    Token token2 = Mockito.mock(Token.class);
+
+    when(token1.line()).thenReturn(1);
+    when(token1.column()).thenReturn(4);
+    when(token2.line()).thenReturn(1);
+    when(token2.column()).thenReturn(12);
+
+    when(token1.firstToken()).thenReturn(token1);
+    when(token2.lastToken()).thenReturn(token2);
+
+    when(token1.value()).thenReturn(tokenValue1);
+    when(token2.value()).thenReturn(tokenValue2);
+
+    PythonTextEdit textEdit = PythonTextEdit.replaceRange(token1, token2, "b and c");
+
+    assertThat(textEdit.replacementText()).isEqualTo("b and c");
+    assertThat(textEdit.startLine()).isEqualTo(1);
+    assertThat(textEdit.startLineOffset()).isEqualTo(4);
+    assertThat(textEdit.endLine()).isEqualTo(1);
+    assertThat(textEdit.endLineOffset()).isEqualTo(13);
+  }
 }