SONARPY-1191 The quick fix of S1854 (DeadStoreCheck) should remove indent before removed line (#1279)

Author: nils-werner-sonarsource

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

@@ -19,8 +19,6 @@
  */
 package org.sonar.python.checks;
 
-import java.util.List;
-import java.util.Optional;
 import java.util.Set;
 import javax.annotation.Nullable;
 import org.sonar.check.Rule;
@@ -83,15 +81,25 @@ private static void verifyBlock(SubscriptionContext ctx, CfgBlock block, LiveVar
       // symbols should have at least one read usage (otherwise will be reported by S1481)
       .filter(unnecessaryAssignment -> readSymbols.contains(unnecessaryAssignment.symbol))
       .filter((unnecessaryAssignment -> !isException(unnecessaryAssignment.symbol, unnecessaryAssignment.element, functionDef)))
-      .forEach(unnecessaryAssignment -> {
-        Tree element = unnecessaryAssignment.element;
-        String message = String.format(MESSAGE_TEMPLATE, unnecessaryAssignment.symbol.name());
-        IssueWithQuickFix issue = (IssueWithQuickFix) separatorTokenForIssue(element)
-          .map(separator -> ctx.addIssue(element.firstToken(), separator, message))
-          .orElseGet(() -> ctx.addIssue(element, message));
+      .forEach(unnecessaryAssignment -> raiseIssue(ctx, unnecessaryAssignment));
+  }
+
+  private static void raiseIssue(SubscriptionContext ctx, DeadStoreUtils.UnnecessaryAssignment unnecessaryAssignment) {
+    Tree element = unnecessaryAssignment.element;
+    String message = String.format(MESSAGE_TEMPLATE, unnecessaryAssignment.symbol.name());
+    Token lastRelevantToken = TreeUtils.getTreeSeparatorOrLastToken(element);
+    PreciseIssue issue;
+    if ("\n".equals(lastRelevantToken.value())) {
+      issue = ctx.addIssue(element, message);
+    } else {
+      issue = ctx.addIssue(element.firstToken(), lastRelevantToken, message);
+    }
+
+    if (element instanceof Statement && !isExceptionForQuickFix((Statement) element)) {
+      ((IssueWithQuickFix) issue).addQuickFix(PythonQuickFix.newQuickFix("Remove the unused statement",
+        PythonTextEdit.removeStatement((Statement) element)));
+    }
 
-        createQuickFix(issue, element);
-      });
   }
 
   private static boolean isMultipleAssignement(Tree element) {
@@ -157,60 +165,21 @@ private static boolean isFunctionDeclarationSymbol(Symbol symbol) {
     return symbol.usages().stream().anyMatch(u -> u.kind() == Usage.Kind.FUNC_DECLARATION);
   }
 
-  private static Optional<Token> separatorTokenForIssue(Tree element) {
-    return separatorTokenOfElement(element)
-      .filter(sep -> !"\n".equals(sep.value()));
-  }
-
-  private static Optional<Token> separatorTokenOfElement(Tree element) {
-    if (element.is(Tree.Kind.ASSIGNMENT_STMT, Tree.Kind.EXPRESSION_STMT)) {
-      Token separator = ((Statement) element).separator();
-      return Optional.ofNullable(separator);
-    }
-    return Optional.empty();
-  }
-
-  private static PythonTextEdit removeDeadStore(Tree currentTree) {
-    Optional<Token> tokenSeparator = separatorTokenOfElement(currentTree);
-    List<Tree> childrenOfParent = currentTree.parent().children();
-
-    if (childrenOfParent.size() == 1) {
-      return PythonTextEdit.replace(currentTree, "pass");
-    }
 
-    Token currentFirstToken = currentTree.firstToken();
-    int indexOfCurrentTree = childrenOfParent.indexOf(currentTree);
-
-    // If the tree to remove is the last one, we remove from the end of the previous tree until the end of the current one
-    if (indexOfCurrentTree == childrenOfParent.size() - 1) {
-      Tree previousTree = childrenOfParent.get(indexOfCurrentTree - 1);
-      Optional<Token> previousSep = separatorTokenOfElement(previousTree);
-      Token previous = previousSep.orElse(previousTree.lastToken());
-      currentFirstToken = tokenSeparator.orElse(currentTree.lastToken());
-      return removeFromEndOfTillEndOf(previous, currentFirstToken);
-    }
-
-    // If the next tree is more than 1 line after the currentFirstToken tree, we only remove the separator
-    Token next;
-    int currentLine = currentTree.lastToken().line();
-    int nextLine = childrenOfParent.get(indexOfCurrentTree + 1).firstToken().line();
-    if (nextLine > currentLine + 1) {
-      next = tokenSeparator.orElse(currentTree.lastToken());
-      // Remove from the start of the currentFirstToken token until after the separator
-      return new PythonTextEdit("", currentFirstToken.line(), currentFirstToken.column(), next.line(), next.column() + next.value().length());
-    } else {
-      // Remove from the start of the currentFirstToken token until the next token
-      next = childrenOfParent.get(indexOfCurrentTree + 1).firstToken();
-      return new PythonTextEdit("", currentFirstToken.line(), currentFirstToken.column(), next.line(), next.column());
-    }
-  }
-
-  private static boolean hasPotentialSideEffect(Tree tree) {
+  private static boolean isExceptionForQuickFix(Statement tree) {
     switch (tree.getKind()) {
+      // foo:str = bar
       case ANNOTATED_ASSIGNMENT:
         return SideEffectDetector.hasSideEffect(((AnnotatedAssignment) tree).assignedValue());
+      // foo = bar or foo = bar = 1
       case ASSIGNMENT_STMT:
+        // TODO: SONARPY-1192 Provide quick fix for chained assignment
+        AssignmentStatement assignmentStatement = (AssignmentStatement) tree;
+        if(assignmentStatement.lhsExpressions().size() > 1) {
+          return true;
+        }
         return SideEffectDetector.hasSideEffect(((AssignmentStatement) tree).assignedValue());
+      // foo(bar:=3)
       case EXPRESSION_STMT:
         ExpressionStatement expressionStatement = (ExpressionStatement) tree;
         return expressionStatement.expressions().stream().anyMatch(SideEffectDetector::hasSideEffect);
@@ -219,22 +188,6 @@ private static boolean hasPotentialSideEffect(Tree tree) {
     }
   }
 
-  private static void createQuickFix(IssueWithQuickFix issue, Tree unnecessaryAssignment) {
-    if (hasPotentialSideEffect(unnecessaryAssignment)) {
-      return;
-    }
-    PythonTextEdit edit = removeDeadStore(unnecessaryAssignment);
-    PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Remove the unused statement")
-      .addTextEdit(edit)
-      .build();
-    issue.addQuickFix(quickFix);
-  }
-
-  private static PythonTextEdit removeFromEndOfTillEndOf(Token first, Token last) {
-    return new PythonTextEdit("", first.line(), first.column() + first.value().length(),
-      last.line(), last.column() + last.value().length());
-  }
-
   private static class SideEffectDetector extends BaseTreeVisitor {
 
     private boolean sideEffect = false;

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

@@ -36,7 +36,7 @@ public void test() {
   }
 
   @Test
-  public void quickfix() {
+  public void assignment_on_single_line() {
     String codeWithIssue = code(
       "def foo():",
       "    x = 42",
@@ -50,7 +50,7 @@ public void quickfix() {
   }
 
   @Test
-  public void semicolon() {
+  public void assignment_with_semicolon_on_single_line() {
     String codeWithIssue = code(
       "def foo():",
       "    x = 42 ;",
@@ -64,7 +64,35 @@ public void semicolon() {
   }
 
   @Test
-  public void space() {
+  public void assignment_between_two_siblings() {
+    String codeWithIssue = code(
+      "def foo():",
+      "    y = 0; x = 42; x = 0",
+      "    print(x)");
+    String codeFixed = code(
+      "def foo():",
+      "    y = 0; x = 0",
+      "    print(x)");
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
+  }
+
+  @Test
+  public void assignment_with_previous_element_on_line() {
+    String codeWithIssue = code(
+      "def foo():",
+      "    y = 0; x = 42",
+      "    x = 0",
+      "    print(x)");
+    String codeFixed = code(
+      "def foo():",
+      "    y = 0;",
+      "    x = 0",
+      "    print(x)");
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
+  }
+
+  @Test
+  public void assignment_with_trailing_whitespace() {
     String codeWithIssue = code(
       "def foo():",
       "    x = 42 ",
@@ -78,11 +106,11 @@ public void space() {
   }
 
   @Test
-  public void quickfix_after_non_issue() {
+  public void assignment_wrapped_by_elements_on_different_lines() {
     String codeWithIssue = code(
       "def foo():",
       "    a = 1",
-      "    x = 10 ;",
+      "    x = 10",
       "    x = 0",
       "    print(x)");
     String codeFixed = code(
@@ -94,14 +122,14 @@ public void quickfix_after_non_issue() {
   }
 
   @Test
-  public void quickfix_oneline() {
+  public void all_statements_on_single_line() {
     String codeWithIssue = "def dead_store(): unused = 24; unused = 42; print(unused)";
     String codeFixed = "def dead_store(): unused = 42; print(unused)";
     PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
   }
 
   @Test
-  public void quickfix_in_condition() {
+  public void assignment_before_condition() {
     String codeWithIssue = code(
       "def simple_conditional():",
       "    x = 10",
@@ -138,39 +166,7 @@ public void assignment_expression_in_function_call() {
     PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
   }
 
-  @Test
-  public void no_separator_found(){
-    String codeWithIssue = code(
-      "def ab():",
-      "    a = foo()",
-      "    print(a)",
-      "    a = foo()",
-      "");
-    PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
-  }
 
-  @Test
-  public void one_separator_found(){
-    String codeWithIssue = code(
-      "def ab():",
-      "    a = foo()",
-      "    print(a)",
-      "    a = foo();");
-    PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
-  }
-
-  @Test
-  public void two_separators_found(){
-    String codeWithIssue = code(
-      "def ab():",
-      "    a = foo()",
-      "    print(a)",
-      "    a = foo();",
-      "");
-    PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
-  }
-
-  // TODO: The quick fix should remove indent before removed line (SONARPY-1191)
   @Test
   public void comment_after_should_not_be_removed(){
     String codeWithIssue = code(
@@ -181,35 +177,34 @@ public void comment_after_should_not_be_removed(){
       "    print(a)");
     String codeFixed = code(
       "def ab():",
-      "        # This is an important comment",
+      "    # This is an important comment",
       "    a = 43",
       "    print(a)");
     PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
   }
 
-  // TODO: The quick fix should remove indent before removed line (SONARPY-1191)
   @Test
-  public void space_comment_after_should_not_be_removed(){
+  public void new_line_after_assignment_should_not_be_removed(){
     String codeWithIssue = code(
       "def ab():",
       "    b = 1",
       "    a = 42",
-      "\n"+
+      "",
       "    # This is an important comment",
       "    a = 43",
       "    print(a)");
     String codeFixed = code(
       "def ab():",
       "    b = 1",
-      "    \n"+
+      "",
       "    # This is an important comment",
       "    a = 43",
       "    print(a)");
     PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
   }
 
   @Test
-  public void deadstore_one_branch(){
+  public void assignment_on_branch(){
     String codeWithIssue = code(
       "def a():",
       "    x = 42",
@@ -225,6 +220,41 @@ public void deadstore_one_branch(){
     PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
   }
 
+  @Test
+  public void assignment_on_branch_with_multiple_elements(){
+    String codeWithIssue = code(
+      "def a():",
+      "    x = 42",
+      "    if x:",
+      "        x = 43",
+      "        a = 4",
+      "    print(a)");
+    String codeFixed = code(
+      "def a():",
+      "    x = 42",
+      "    if x:",
+      "        a = 4",
+      "    print(a)");
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
+  }
+
+  @Test
+  public void assignment_on_branch_with_multiple_elements_on_same_line(){
+    String codeWithIssue = code(
+      "def a():",
+      "    x = 42",
+      "    if x:",
+      "        a = 4; x = 43",
+      "    print(a)");
+    String codeFixed = code(
+      "def a():",
+      "    x = 42",
+      "    if x:",
+      "        a = 4;",
+      "    print(a)");
+    PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
+  }
+
   @Test
   public void side_effect_in_binary_op(){
     String codeWithIssue = code(
@@ -234,4 +264,15 @@ public void side_effect_in_binary_op(){
       "    print(a)");
     PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
   }
+
+  @Test
+  public void chain_assignment() {
+    String codeWithIssue = code(
+      "def chain_assign():",
+        "    a = b = 42",
+        "    a = foo()",
+        "    print(a, b)"
+    );
+    PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
+  }
 }