Skip to content

Commit b5ddfad

Browse files
marco-bearzi-sonarsourcemarco-bearzi-sonarsource
authored
SONARPY-1043: Fix the quick-fix for S1854 (DeadStoreCheck) (#1154)
1 parent 123cf40 commit b5ddfad

File tree

3 files changed

+165
-21
lines changed

3 files changed

+165
-21
lines changed

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

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.sonar.plugins.python.api.tree.FunctionDef;
3636
import org.sonar.plugins.python.api.tree.Name;
3737
import org.sonar.plugins.python.api.tree.NumericLiteral;
38+
import org.sonar.plugins.python.api.tree.Statement;
3839
import org.sonar.plugins.python.api.tree.Token;
3940
import org.sonar.plugins.python.api.tree.Tree;
4041
import org.sonar.plugins.python.api.tree.UnaryExpression;
@@ -45,7 +46,6 @@
4546
import org.sonar.python.tree.TreeUtils;
4647

4748
import static org.sonar.python.checks.DeadStoreUtils.isUsedInSubFunction;
48-
import static org.sonar.python.quickfix.PythonTextEdit.remove;
4949

5050
@Rule(key = "S1854")
5151
public class DeadStoreCheck extends PythonSubscriptionCheck {
@@ -82,10 +82,11 @@ private static void verifyBlock(SubscriptionContext ctx, CfgBlock block, LiveVar
8282
.forEach(unnecessaryAssignment -> {
8383
Tree element = unnecessaryAssignment.element;
8484
String message = String.format(MESSAGE_TEMPLATE, unnecessaryAssignment.symbol.name());
85-
IssueWithQuickFix issue = (IssueWithQuickFix) separatorToken(element)
85+
IssueWithQuickFix issue = (IssueWithQuickFix) separatorTokenForIssue(element)
8686
.map(separator -> ctx.addIssue(element.firstToken(), separator, message))
8787
.orElseGet(() -> ctx.addIssue(element, message));
88-
createQuickFix(issue, unnecessaryAssignment.element);
88+
89+
createQuickFix(issue, element);
8990
});
9091
}
9192

@@ -152,37 +153,64 @@ private static boolean isFunctionDeclarationSymbol(Symbol symbol) {
152153
return symbol.usages().stream().anyMatch(u -> u.kind() == Usage.Kind.FUNC_DECLARATION);
153154
}
154155

155-
private static Optional<Token> separatorToken(Tree element) {
156-
if (element instanceof AssignmentStatement) {
157-
Token seperator = ((AssignmentStatement) element).separator();
158-
return "\n".equals(seperator.value()) ? Optional.empty() : Optional.of(seperator);
156+
private static Optional<Token> separatorTokenForIssue(Tree element) {
157+
return separatorTokenOfElement(element)
158+
.filter(sep -> !"\n".equals(sep.value()));
159+
}
160+
161+
private static Optional<Token> separatorTokenOfElement(Tree element) {
162+
if (element.is(Tree.Kind.ASSIGNMENT_STMT, Tree.Kind.EXPRESSION_STMT)) {
163+
Token separator = ((Statement) element).separator();
164+
return Optional.ofNullable(separator);
159165
}
160166
return Optional.empty();
161167
}
162168

163-
private static PythonTextEdit removeDeadStore(Tree tree) {
164-
List<Tree> childrenOfParent = tree.parent().children();
169+
private static PythonTextEdit removeDeadStore(Tree currentTree) {
170+
Optional<Token> tokenSeparator = separatorTokenOfElement(currentTree);
171+
List<Tree> childrenOfParent = currentTree.parent().children();
172+
165173
if (childrenOfParent.size() == 1) {
166-
return remove(tree);
174+
return PythonTextEdit.replace(currentTree, "pass");
175+
}
176+
177+
Token currentFirstToken = currentTree.firstToken();
178+
int indexOfCurrentTree = childrenOfParent.indexOf(currentTree);
179+
180+
// 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
181+
if (indexOfCurrentTree == childrenOfParent.size() - 1) {
182+
Tree previousTree = childrenOfParent.get(indexOfCurrentTree - 1);
183+
Optional<Token> previousSep = separatorTokenOfElement(previousTree);
184+
Token previous = previousSep.orElse(previousTree.lastToken());
185+
currentFirstToken = tokenSeparator.orElse(currentTree.lastToken());
186+
return removeFromEndOfTillEndOf(previous, currentFirstToken);
167187
}
168-
Token current = tree.firstToken();
169-
int i = childrenOfParent.indexOf(tree);
170-
if (i == childrenOfParent.size() - 1) {
171-
Token previous = childrenOfParent.get(i - 1).lastToken();
172-
current = tree.lastToken();
173-
// Replace from the end of the previous token (will also remove the separator and trailing whitespaces) until the end of the current token
174-
return new PythonTextEdit("", previous.line(), previous.column() + previous.value().length(), current.line(), current.column() + current.value().length());
188+
189+
// If the next tree is more than 1 line after the currentFirstToken tree, we only remove the separator
190+
Token next;
191+
int currentLine = currentTree.lastToken().line();
192+
int nextLine = childrenOfParent.get(indexOfCurrentTree + 1).firstToken().line();
193+
if (nextLine > currentLine + 1) {
194+
next = tokenSeparator.orElse(currentTree.lastToken());
195+
// Remove from the start of the currentFirstToken token until after the separator
196+
return new PythonTextEdit("", currentFirstToken.line(), currentFirstToken.column(), next.line(), next.column() + next.value().length());
197+
} else {
198+
// Remove from the start of the currentFirstToken token until the next token
199+
next = childrenOfParent.get(indexOfCurrentTree + 1).firstToken();
200+
return new PythonTextEdit("", currentFirstToken.line(), currentFirstToken.column(), next.line(), next.column());
175201
}
176-
Token next = childrenOfParent.get(i + 1).firstToken();
177-
// Remove from the start of the current tokenuntil the next token
178-
return new PythonTextEdit("", current.line(), current.column(), next.line(), next.column());
179202
}
180203

181204
private static void createQuickFix(IssueWithQuickFix issue, Tree unnecessaryAssignment) {
182205
PythonTextEdit edit = removeDeadStore(unnecessaryAssignment);
183-
PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Remove the line(s)")
206+
PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Remove the unused statement")
184207
.addTextEdit(edit)
185208
.build();
186209
issue.addQuickFix(quickFix);
187210
}
211+
212+
private static PythonTextEdit removeFromEndOfTillEndOf(Token first, Token last) {
213+
return new PythonTextEdit("", first.line(), first.column() + first.value().length(),
214+
last.line(), last.column() + last.value().length());
215+
}
188216
}

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

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,115 @@ public void unused_after_reassignment() {
115115
" print(c)\n";
116116
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
117117
}
118+
119+
@Test
120+
public void end_of_line_comments_should_be_removed() {
121+
String codeWithIssue = "" +
122+
"def assignment_expression():\n" +
123+
" foo(a:=3) # Comment 1\n" +
124+
"# Comment 2\n" +
125+
" a = 2\n" +
126+
" print(a)";
127+
String codeFixed = "" +
128+
"def assignment_expression():\n" +
129+
" # Comment 2\n" +
130+
" a = 2\n" +
131+
" print(a)";
132+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
133+
}
134+
135+
@Test
136+
public void no_separator_found(){
137+
String codeWithIssue = "" +
138+
"def ab():\n" +
139+
" a = foo()\n" +
140+
" print(a)\n" +
141+
" a = foo()";
142+
String codeFixed = "" +
143+
"def ab():\n" +
144+
" a = foo()\n" +
145+
" print(a)\n";
146+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
147+
}
148+
149+
@Test
150+
public void one_separator_found(){
151+
String codeWithIssue = "" +
152+
"def ab():\n" +
153+
" a = foo()\n" +
154+
" print(a)\n" +
155+
" a = foo();";
156+
String codeFixed = "" +
157+
"def ab():\n" +
158+
" a = foo()\n" +
159+
" print(a)\n";
160+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
161+
}
162+
163+
@Test
164+
public void two_separators_found(){
165+
String codeWithIssue = "" +
166+
"def ab():\n" +
167+
" a = foo()\n" +
168+
" print(a)\n" +
169+
" a = foo();\n";
170+
String codeFixed = "" +
171+
"def ab():\n" +
172+
" a = foo()\n" +
173+
" print(a)\n";
174+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
175+
}
176+
177+
@Test
178+
public void comment_after_should_not_be_removed(){
179+
String codeWithIssue = "" +
180+
"def ab():\n" +
181+
" a = 42\n" +
182+
" # This is an important comment\n" +
183+
" a = 43\n" +
184+
" print(a)";
185+
String codeFixed = "" +
186+
"def ab():\n" +
187+
" # This is an important comment\n" +
188+
" a = 43\n" +
189+
" print(a)";
190+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
191+
}
192+
193+
@Test
194+
public void space_comment_after_should_not_be_removed(){
195+
String codeWithIssue = "" +
196+
"def ab():\n" +
197+
" b = 1\n" +
198+
" a = 42\n" +
199+
"\n"+
200+
" # This is an important comment\n" +
201+
" a = 43\n" +
202+
" print(a)";
203+
String codeFixed = "" +
204+
"def ab():\n" +
205+
" b = 1\n" +
206+
" \n"+
207+
" # This is an important comment\n" +
208+
" a = 43\n" +
209+
" print(a)";
210+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
211+
}
212+
213+
@Test
214+
public void deadstore_one_branch(){
215+
String codeWithIssue = "" +
216+
"def a():\n" +
217+
" x = 42\n" +
218+
" if x:\n" +
219+
" x = 43\n" +
220+
" print(a)";
221+
String codeFixed = "" +
222+
"def a():\n" +
223+
" x = 42\n" +
224+
" if x:\n" +
225+
" pass\n" +
226+
" print(a)";
227+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
228+
}
118229
}

‎python-checks/src/test/resources/checks/deadStore.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,3 +322,8 @@ def match_statement_no_fp(value):
322322

323323
def dead_store(): unused = 24; unused = 42; print(unused); # NonCompliant
324324
# ^^^^^^^^^^^^
325+
326+
def no_separator_after_deadstore():
327+
a = foo()
328+
print(a)
329+
a = foo() # Noncompliant

0 commit comments

Comments
 (0)