Skip to content

Commit 5b77f87

Browse files
marco-bearzi-sonarsourcemarco-bearzi-sonarsource
authored
SONARPY-1032: Add quick fix for S3923 (AllBranchesAreIdenticalCheck) (#1147)
1 parent 999302a commit 5b77f87

File tree

5 files changed

+237
-8
lines changed

5 files changed

+237
-8
lines changed

‎python-checks/pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
<dependency>
5656
<groupId>org.sonarsource.analyzer-commons</groupId>
5757
<artifactId>sonar-analyzer-commons</artifactId>
58-
<scope>test</scope>
58+
<scope>compile</scope>
5959
</dependency>
6060
<dependency>
6161
<groupId>org.mockito</groupId>

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

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,25 @@
2121

2222
import java.util.ArrayList;
2323
import java.util.List;
24+
import java.util.Optional;
2425
import org.sonar.check.Rule;
26+
import org.sonar.plugins.python.api.IssueLocation;
2527
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2628
import org.sonar.plugins.python.api.SubscriptionContext;
2729
import org.sonar.plugins.python.api.tree.ConditionalExpression;
30+
import org.sonar.plugins.python.api.tree.ElseClause;
2831
import org.sonar.plugins.python.api.tree.Expression;
2932
import org.sonar.plugins.python.api.tree.IfStatement;
33+
import org.sonar.plugins.python.api.tree.Statement;
3034
import org.sonar.plugins.python.api.tree.StatementList;
3135
import org.sonar.plugins.python.api.tree.Token;
3236
import org.sonar.plugins.python.api.tree.Tree;
33-
import org.sonar.plugins.python.api.IssueLocation;
37+
import org.sonar.python.quickfix.IssueWithQuickFix;
38+
import org.sonar.python.quickfix.PythonQuickFix;
39+
import org.sonar.python.quickfix.PythonTextEdit;
40+
import org.sonar.python.tree.IfStatementImpl;
3441
import org.sonar.python.tree.TreeUtils;
42+
import org.sonarsource.analyzer.commons.collections.ListUtils;
3543

3644
@Rule(key = "S3923")
3745
public class AllBranchesAreIdenticalCheck extends PythonSubscriptionCheck {
@@ -63,6 +71,7 @@ private static void handleIfStatement(IfStatement ifStmt, SubscriptionContext ct
6371
issue.secondary(issueLocation(ifStmt.body()));
6472
ifStmt.elifBranches().forEach(e -> issue.secondary(issueLocation(e.body())));
6573
issue.secondary(issueLocation(ifStmt.elseBranch().body()));
74+
createQuickFix((IssueWithQuickFix) issue, ifStmt, ifStmt.body().statements());
6675
}
6776

6877
private static IssueLocation issueLocation(StatementList body) {
@@ -78,6 +87,7 @@ private static void handleConditionalExpression(ConditionalExpression conditiona
7887
PreciseIssue issue = ctx.addIssue(conditionalExpression.ifKeyword(), "This conditional expression returns the same value whether the condition is \"true\" or \"false\".");
7988
addSecondaryLocations(issue, conditionalExpression.trueExpression());
8089
addSecondaryLocations(issue, conditionalExpression.falseExpression());
90+
createQuickFixConditional((IssueWithQuickFix) issue, conditionalExpression);
8191
}
8292
}
8393

@@ -111,4 +121,70 @@ private static Expression unwrapIdenticalExpressions(Expression expression) {
111121
}
112122
return unwrappedExpression;
113123
}
124+
125+
private static void createQuickFixConditional(IssueWithQuickFix issue, Tree tree) {
126+
List<Tree> children = tree.children();
127+
Token lastTokenOfFirst = children.get(0).lastToken();
128+
Token lastTokenOfConditional = children.get(children.size() - 1).lastToken();
129+
PythonTextEdit edit = new PythonTextEdit("", lastTokenOfFirst.line(), lastTokenOfFirst.column(),
130+
lastTokenOfConditional.line(), lastTokenOfConditional.column());
131+
132+
PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Remove the if statement")
133+
.addTextEdit(edit)
134+
.build();
135+
issue.addQuickFix(quickFix);
136+
}
137+
138+
private static void createQuickFix(IssueWithQuickFix issue, Tree tree, List<Statement> statements) {
139+
IfStatementImpl ifStatement = (IfStatementImpl) tree;
140+
141+
Token firstBodyToken = statements.get(0).firstToken();
142+
Token keyword = ifStatement.keyword();
143+
Statement lastStatement = ListUtils.getLast(statements);
144+
145+
int firstLine = firstBodyToken.line();
146+
int lastLine = lastStatement.lastToken().line();
147+
148+
Optional<ElseClause> elseBranch = Optional.ofNullable(ifStatement.elseBranch());
149+
Optional<Integer> lineElseBranch = elseBranch
150+
.map(Tree::firstToken)
151+
.map(Token::line);
152+
153+
// lastLine is one line further if there is another if block enclosed
154+
if (lastStatement.lastToken().column() == 0) {
155+
lastLine--;
156+
}
157+
158+
PythonQuickFix.Builder quickFixBuilder = PythonQuickFix.newQuickFix("Remove the if statement");
159+
160+
// Remove the if line
161+
quickFixBuilder.addTextEdit(new PythonTextEdit("", keyword.line(), keyword.column(), firstBodyToken.line(), firstBodyToken.column()));
162+
163+
// Remove indent from the second line until the last statement
164+
for (int line = firstLine + 1; line <= lastLine; line++) {
165+
quickFixBuilder.addTextEdit(editIndentAtLine(line));
166+
}
167+
168+
// Remove else branch
169+
elseBranch.ifPresent(branch -> quickFixBuilder.addTextEdit(PythonTextEdit.remove(branch)));
170+
171+
// Remove the indent on the else line
172+
if (ifStatement.elifBranches().isEmpty()) {
173+
lineElseBranch.ifPresent(lineElse -> quickFixBuilder.addTextEdit(editIndentAtLine(lineElse)));
174+
}
175+
176+
// Take care of the elif branches, the elif branch goes up to the next else or elif branch
177+
for (IfStatement branch : ifStatement.elifBranches()) {
178+
int lineElifBranch = branch.firstToken().line();
179+
quickFixBuilder.addTextEdit(PythonTextEdit.remove(branch))
180+
.addTextEdit(editIndentAtLine(lineElifBranch));
181+
}
182+
183+
issue.addQuickFix(quickFixBuilder.build());
184+
}
185+
186+
private static PythonTextEdit editIndentAtLine(int line) {
187+
return new PythonTextEdit("", line, 0, line, 4);
188+
}
189+
114190
}

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

Lines changed: 147 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,159 @@
2020
package org.sonar.python.checks;
2121

2222
import org.junit.Test;
23+
import org.sonar.plugins.python.api.PythonCheck;
24+
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
2325
import org.sonar.python.checks.utils.PythonCheckVerifier;
2426

2527
public class AllBranchesAreIdenticalCheckTest {
2628

29+
private PythonCheck check = new AllBranchesAreIdenticalCheck();
2730
@Test
2831
public void test() {
29-
PythonCheckVerifier.verify("src/test/resources/checks/allBranchesAreIdentical.py", new AllBranchesAreIdenticalCheck());
32+
PythonCheckVerifier.verify("src/test/resources/checks/allBranchesAreIdentical.py", check);
3033
}
3134

35+
@Test
36+
public void quickfix_one_statement() {
37+
String codeWithIssue =
38+
"def func():\n" +
39+
" if b == 0:\n" +
40+
" doSomething()\n" +
41+
" else:\n" +
42+
" doSomething()\n";
43+
String codeFixed =
44+
"def func():\n" +
45+
" doSomething()\n";
46+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
47+
}
48+
@Test
49+
public void quickfix_semicolons() {
50+
String codeWithIssue =
51+
"def func():\n" +
52+
" if b == 0:\n" +
53+
" doSomething(); doOneMoreThing()\n"+
54+
" else:\n" +
55+
" doSomething(); doOneMoreThing()\n";
56+
String codeFixed =
57+
"def func():\n" +
58+
" doSomething(); doOneMoreThing()\n";
59+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
60+
}
61+
62+
@Test
63+
public void if_enclosed() {
64+
String codeWithIssue =
65+
"def func():\n" +
66+
" if b == 0:\n" +
67+
" if a == 1:\n"+
68+
" doSomething()\n"+
69+
" else:\n" +
70+
" if a == 1:\n"+
71+
" doSomething()\n";
72+
String codeFixed =
73+
"def func():\n" +
74+
" if a == 1:\n"+
75+
" doSomething()\n";
76+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
77+
}
78+
79+
80+
@Test
81+
public void oneline(){
82+
String codeWithIssue = "a = 1 if x else 1";
83+
String codeFixed = "a = 1";
84+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
85+
}
86+
87+
@Test
88+
public void test_multiple_statement(){
89+
String codeWithIssue ="def func():\n" +
90+
" if b == 0:\n" +
91+
" doSomething()\n" +
92+
" doOneMoreThing()\n" +
93+
" else:\n" +
94+
" doSomething()\n" +
95+
" doOneMoreThing()\n";
96+
String codeFixed = "def func():\n" +
97+
" doSomething()\n" +
98+
" doOneMoreThing()\n";
99+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
100+
}
101+
102+
@Test
103+
public void lambda(){
104+
String codeWithIssue = "a = (lambda x: x+1\n" +
105+
" if x > 0 # Noncompliant\n" +
106+
" else x+1)";
107+
String codeFixed = "a = (lambda x: x+1)";
108+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
109+
}
110+
111+
@Test
112+
public void multiple_conditional_statements(){
113+
String codeWithIssue = "a = 1 if x else 1 if y else 1 if z else 1";
114+
String codeFixed = "a = 1";
115+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
116+
}
117+
118+
@Test
119+
public void test_elseif(){
120+
String codeWithIssue ="def func():\n" +
121+
" if b == 0:\n" +
122+
" doSomething()\n" +
123+
" elif b == 1:\n" +
124+
" doSomething()\n" +
125+
" else:\n" +
126+
" doSomething()\n";
127+
String codeFixed = "def func():\n" +
128+
" doSomething()\n";
129+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
130+
}
131+
132+
@Test
133+
public void test_elseif_multiple(){
134+
String codeWithIssue ="def func():\n" +
135+
" if b == 0:\n" +
136+
" doSomething()\n" +
137+
" doOneMoreThing()\n"+
138+
" elif b == 1:\n" +
139+
" doSomething()\n" +
140+
" doOneMoreThing()\n"+
141+
" else:\n" +
142+
" doSomething()\n"+
143+
" doOneMoreThing()\n";;
144+
String codeFixed = "def func():\n" +
145+
" doSomething()\n"+
146+
" doOneMoreThing()\n";;
147+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
148+
}
149+
150+
@Test
151+
public void test_elseif_more(){
152+
String codeWithIssue ="def func():\n" +
153+
" if b == 0:\n" +
154+
" doSomething()\n" +
155+
" doSomething()\n" +
156+
" doOneMoreThing()\n"+
157+
" elif b == 1:\n" +
158+
" doSomething()\n" +
159+
" doSomething()\n" +
160+
" doOneMoreThing()\n"+
161+
" else:\n" +
162+
" doSomething()\n"+
163+
" doSomething()\n" +
164+
" doOneMoreThing()\n";;
165+
String codeFixed = "def func():\n" +
166+
" doSomething()\n"+
167+
" doSomething()\n" +
168+
" doOneMoreThing()\n";;
169+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
170+
}
171+
172+
@Test
173+
public void test_complex_condition(){
174+
String codeWithIssue = "a = do_something(a, b, c, do_something_else(d)) if x else do_something(a, b, c, do_something_else(d))";
175+
String codeFixed = "a = do_something(a, b, c, do_something_else(d))";
176+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
177+
}
32178
}

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ private static void checkNoCollision(List<PythonTextEdit> pythonTextEdits) throw
131131
}
132132
}
133133

134+
// Returns true if the range of one edit crosses the range of the other. If one end of both edits is the same point,
135+
// we should return false
134136
private static boolean oneEnclosedByTheOther(PythonTextEdit toCheck, PythonTextEdit reference) {
135137
if (onSameLine(toCheck, reference)) {
136138
// If on same line, we need to check that the bounds of toCheck are not contained in reference bounds
@@ -149,9 +151,9 @@ private static boolean oneEnclosedByTheOther(PythonTextEdit toCheck, PythonTextE
149151
} else {
150152
// There is an intersection between edits, only need to check valid case
151153
if (reference.startLine() == toCheck.endLine()) {
152-
return !(toCheck.endLineOffset() < reference.startLineOffset());
154+
return !(toCheck.endLineOffset() <= reference.startLineOffset());
153155
} else if (reference.endLine() == toCheck.startLine()) {
154-
return !(reference.endLineOffset() < toCheck.startLineOffset());
156+
return !(reference.endLineOffset() <= toCheck.startLineOffset());
155157
}
156158
}
157159
}
@@ -172,11 +174,13 @@ private static boolean isCompact(PythonTextEdit check) {
172174
return check.startLine() == check.endLine();
173175
}
174176

175-
private static boolean isSecondInFirst(PythonTextEdit second, PythonTextEdit first) {
177+
// Returns true if there is an intersection between the ranges
178+
// The first parameter is a compact edit, i.e., the edit is only one one line
179+
private static boolean isSecondInFirst(PythonTextEdit first, PythonTextEdit second) {
176180
if (first.startLine() == second.startLine()) {
177-
return first.startLineOffset() <= second.endLineOffset();
178-
} else if (first.endLine() == second.startLine()) {
179181
return second.startLineOffset() < first.endLineOffset();
182+
} else if (first.endLine() == second.endLine()) {
183+
return first.startLineOffset() < second.endLineOffset();
180184
}
181185
// No intersection
182186
return false;

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,3 +75,6 @@ def func():
7575

7676
a = ((1 if x else 1) if cond else (1 if x else 1)) if other else (1 if x else (1 if y else 1 if z else 1)) # Noncompliant
7777
# ^> ^> ^> ^> ^^ ^< ^< ^< ^<
78+
79+
a = 1 if (x and y) else 1 # Noncompliant
80+

0 commit comments

Comments
 (0)