Skip to content

Commit 123cf40

Browse files
marco-bearzi-sonarsourceandrea-guarino-sonarsource
marco-bearzi-sonarsource
authored and
andrea-guarino-sonarsource
committed
SONARPY-1041: Fix the quick-fix for S1940 (BooleanCheckNotInvertedCheck)
1 parent 8e87e1b commit 123cf40

File tree

3 files changed

+176
-17
lines changed

3 files changed

+176
-17
lines changed

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

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
*/
2020
package org.sonar.python.checks;
2121

22-
import java.util.List;
2322
import org.sonar.check.Rule;
2423
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2524
import org.sonar.plugins.python.api.SubscriptionContext;
@@ -35,6 +34,7 @@
3534
import org.sonar.python.quickfix.IssueWithQuickFix;
3635
import org.sonar.python.quickfix.PythonQuickFix;
3736
import org.sonar.python.quickfix.PythonTextEdit;
37+
import org.sonar.python.tree.TreeUtils;
3838

3939
@Rule(key = "S1940")
4040
public class BooleanCheckNotInvertedCheck extends PythonSubscriptionCheck {
@@ -58,14 +58,14 @@ private static void checkNotExpression(SubscriptionContext ctx, UnaryExpression
5858
String oppositeOperator = oppositeOperator(binaryExp.operator());
5959

6060
IssueWithQuickFix issue = ((IssueWithQuickFix) ctx.addIssue(original, String.format(MESSAGE, oppositeOperator)));
61-
createQuickFix(issue, oppositeOperator, binaryExp, negatedExpr);
61+
createQuickFix(issue, oppositeOperator, binaryExp, original);
6262
}
6363
} else if (negatedExpr.is(Kind.IN, Kind.IS)) {
6464
BinaryExpression isInExpr = (BinaryExpression) negatedExpr;
6565
String oppositeOperator = oppositeOperator(isInExpr.operator(), isInExpr);
6666

6767
IssueWithQuickFix issue = ((IssueWithQuickFix) ctx.addIssue(original, String.format(MESSAGE, oppositeOperator)));
68-
createQuickFix(issue, oppositeOperator, isInExpr, negatedExpr);
68+
createQuickFix(issue, oppositeOperator, isInExpr, original);
6969
}
7070
}
7171

@@ -110,18 +110,37 @@ static String oppositeOperatorString(String stringOperator) {
110110
}
111111
}
112112

113-
private static void createQuickFix(IssueWithQuickFix issue, String oppositeOperator, BinaryExpression toUse, Expression toReplace) {
114-
PythonTextEdit replaceEdit = getReplaceEdit(toUse, toReplace, oppositeOperator);
113+
private static void createQuickFix(IssueWithQuickFix issue, String oppositeOperator, BinaryExpression toUse, UnaryExpression notAncestor) {
114+
PythonTextEdit replaceEdit = getReplaceEdit(toUse, oppositeOperator, notAncestor);
115115

116116
PythonQuickFix quickFix = PythonQuickFix.newQuickFix(String.format("Use %s instead", oppositeOperator))
117117
.addTextEdit(replaceEdit)
118118
.build();
119119
issue.addQuickFix(quickFix);
120120
}
121121

122-
private static PythonTextEdit getReplaceEdit(BinaryExpression toUse, Expression toReplace, String oppositeOperator) {
123-
List<Tree> children = toReplace.parent().parent().children();
124-
return PythonTextEdit.replaceRange(children.get(0), children.get(children.size() - 1),
125-
toUse.leftOperand().firstToken().value() + " " + oppositeOperator + " " + toUse.rightOperand().firstToken().value());
122+
private static PythonTextEdit getReplaceEdit(BinaryExpression toUse, String oppositeOperator, UnaryExpression notAncestor) {
123+
return PythonTextEdit.replace(notAncestor, getNewExpression(toUse, oppositeOperator));
124+
}
125+
126+
private static String getNewExpression(BinaryExpression toUse, String oppositeOperator) {
127+
return getText(toUse.leftOperand()) + " " + oppositeOperator + " " + getText(toUse.rightOperand());
128+
}
129+
130+
private static String getText(Tree tree) {
131+
return TreeUtils.tokens(tree).stream()
132+
.map(Token::value)
133+
.reduce("", (acc, currentChar) -> isSpaceNotNeeded(acc, currentChar) ? (acc + currentChar) : (acc + " " + currentChar));
134+
}
135+
136+
@SuppressWarnings("java:S125")
137+
private static boolean isSpaceNotNeeded(String acc, String toAdd) {
138+
// Heuristic telling when a space is not needed: 1) after a space, after opening parenthesis or bracket;
139+
// 2) if the accumulator is ending with anything other than a comma and that an opening parenthesis is to be added;
140+
// 3) if the accumulator is ending with anything other than a space and that an opening bracket is to be added
141+
// 4) if a specific character is added : such as closing parenthesis or bracket, or a comma.
142+
return acc.isBlank() || acc.endsWith("(") || acc.endsWith("[")
143+
|| (!acc.endsWith(",") && "(".equals(toAdd)) || (!acc.endsWith(" ") && "[".equals(toAdd))
144+
|| "]".equals(toAdd) || ")".equals(toAdd) || ",".equals(toAdd);
126145
}
127146
}

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

Lines changed: 130 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,19 @@
2121

2222
import org.assertj.core.api.Assertions;
2323
import org.junit.Test;
24+
import org.sonar.plugins.python.api.PythonCheck;
2425
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
2526
import org.sonar.python.checks.utils.PythonCheckVerifier;
2627

2728
import static org.assertj.core.api.Assertions.assertThat;
2829

2930
public class BooleanCheckNotInvertedCheckTest {
3031

32+
private final PythonCheck check = new BooleanCheckNotInvertedCheck();
33+
3134
@Test
3235
public void test() {
33-
PythonCheckVerifier.verify("src/test/resources/checks/booleanCheckNotInverted.py", new BooleanCheckNotInvertedCheck());
36+
PythonCheckVerifier.verify("src/test/resources/checks/booleanCheckNotInverted.py", check);
3437
}
3538

3639
@Test
@@ -54,30 +57,149 @@ public void operatorStringTest() {
5457
public void test_quickfix() {
5558
String codeWithIssue = "a = not(b == c)";
5659
String codeFixed = "a = b != c";
57-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
60+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
61+
62+
codeWithIssue = "a = not b != c";
63+
codeFixed = "a = b == c";
64+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
5865

5966
codeWithIssue = "a = not (b != c)";
6067
codeFixed = "a = b == c";
61-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
68+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
6269

6370
codeWithIssue = "a = not (b < c)";
6471
codeFixed = "a = b >= c";
65-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
72+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
6673

6774
codeWithIssue = "a = not (b is c)";
6875
codeFixed = "a = b is not c";
69-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
76+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
7077

7178
codeWithIssue = "a = not (b is not c)";
7279
codeFixed = "a = b is c";
73-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
80+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
7481

7582
codeWithIssue = "a = not (b in c)";
7683
codeFixed = "a = b not in c";
77-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
84+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
7885

7986
codeWithIssue = "a = not (b not in c)";
8087
codeFixed = "a = b in c";
81-
PythonQuickFixVerifier.verify(new BooleanCheckNotInvertedCheck(), codeWithIssue, codeFixed);
88+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
89+
}
90+
91+
@Test
92+
public void not_in_if() {
93+
String codeWithIssue = "" +
94+
"def func():\n" +
95+
" if not a == 2:\n" +
96+
" b = 10\n" +
97+
" return \"item1\" \"item2\"";
98+
String codeFixed = "" +
99+
"def func():\n" +
100+
" if a != 2:\n" +
101+
" b = 10\n" +
102+
" return \"item1\" \"item2\"";
103+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
104+
105+
codeWithIssue = "" +
106+
"def func():\n" +
107+
" if not a == 2 and b == 9:\n" +
108+
" b = 10\n" +
109+
" return \"item1\" \"item2\"";
110+
codeFixed = "" +
111+
"def func():\n" +
112+
" if a != 2 and b == 9:\n" +
113+
" b = 10\n" +
114+
" return \"item1\" \"item2\"";
115+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
116+
117+
codeWithIssue = "" +
118+
"def func():\n" +
119+
" if a != 2 and not b == 9:\n" +
120+
" b = 10\n" +
121+
" return \"item1\" \"item2\"";
122+
codeFixed = "" +
123+
"def func():\n" +
124+
" if a != 2 and b != 9:\n" +
125+
" b = 10\n" +
126+
" return \"item1\" \"item2\"";
127+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
128+
}
129+
130+
@Test
131+
public void not_parentheses() {
132+
String codeWithIssue = "a = not ((((((b > c))))))";
133+
String codeFixed = "a = b <= c";
134+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
135+
136+
codeWithIssue = "a = not (not (b == c))";
137+
codeFixed = "a = not (b != c)";
138+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
139+
140+
codeWithIssue = "a = not (a is (not b))";
141+
codeFixed = "a = a is not (not b)";
142+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
143+
144+
codeWithIssue = "a = not (1 == b == c) == 2";
145+
codeFixed = "a = (1 == b == c) != 2";
146+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
147+
148+
codeWithIssue = "a = not (1 == b == c) == (d == 2)";
149+
codeFixed = "a = (1 == b == c) != (d == 2)";
150+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
151+
152+
codeWithIssue = "a = not ((1 == b == c) and a) == (d == 2)";
153+
codeFixed = "a = ((1 == b == c) and a) != (d == 2)";
154+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
155+
156+
codeWithIssue = "a = not (b < foo())";
157+
codeFixed = "a = b >= foo()";
158+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
159+
}
160+
161+
@Test
162+
public void expression_list() {
163+
String codeWithIssue = "a = not [] < (c,d)";
164+
String codeFixed = "a = [] >= (c, d)";
165+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
166+
167+
codeWithIssue = "a = not [a, (a,b)] is c";
168+
codeFixed = "a = [a, (a, b)] is not c";
169+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
170+
171+
codeWithIssue = "a = not (foo((1)) < c)";
172+
codeFixed = "a = foo((1)) >= c";
173+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
174+
175+
codeWithIssue = "a = not (foo(()) < c)";
176+
codeFixed = "a = foo(()) >= c";
177+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
178+
179+
codeWithIssue = "a = not (() in c)";
180+
codeFixed = "a = () not in c";
181+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
182+
183+
codeWithIssue = "a = not ((1,) in c)";
184+
codeFixed = "a = (1,) not in c";
185+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
186+
187+
codeWithIssue = "a = not ((1,2,3,4) in c)";
188+
codeFixed = "a = (1, 2, 3, 4) not in c";
189+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
190+
}
191+
192+
@Test
193+
public void fstring() {
194+
String codeWithIssue = "x = not (a == f'foo${b}')";
195+
String codeFixed = "x = a != f'foo${b}'";
196+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
197+
198+
}
199+
@Test
200+
public void brackets(){
201+
String codeWithIssue = "x = not ( ham[1] in a)";
202+
String codeFixed = "x = ham[1] not in a";
203+
PythonQuickFixVerifier.verify(check, codeWithIssue, codeFixed);
82204
}
83205
}

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,21 @@
5050
# Both cases below are handled by RSPEC-2761
5151
## x = not(not 1) # Noncompliant
5252
## t = a is not(not b) # Noncompliant
53+
54+
def func():
55+
if not a == 2: #Noncompliant
56+
# ^^^^^^^^^^
57+
b = 10
58+
return "item1" "item2"
59+
60+
def func1():
61+
if not a == 2 and b == 9: # Noncompliant
62+
# ^^^^^^^^^^
63+
b = 10
64+
return "item1" "item2"
65+
66+
def func2():
67+
if a != 2 and not b == 9: # Noncompliant
68+
# ^^^^^^^^^^
69+
b = 10
70+
return "item1" "item2"

0 commit comments

Comments
 (0)