Skip to content

Commit fe4d9d2

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARPY-543 Rule S5685: Walrus operator should not make code confusing (SonarSource#633)
1 parent e568a9f commit fe4d9d2

File tree

7 files changed

+249
-0
lines changed

7 files changed

+249
-0
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ public static Iterable<Class> getChecks() {
7676
CommentedCodeCheck.class,
7777
CommentRegularExpressionCheck.class,
7878
ComparisonToNoneCheck.class,
79+
ConfusingWalrusCheck.class,
7980
CorsCheck.class,
8081
DataEncryptionCheck.class,
8182
DbNoPasswordCheck.class,

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

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2020 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks;
21+
22+
import java.util.ArrayList;
23+
import java.util.List;
24+
import java.util.Optional;
25+
import org.sonar.check.Rule;
26+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
27+
import org.sonar.plugins.python.api.SubscriptionContext;
28+
import org.sonar.plugins.python.api.tree.ArgList;
29+
import org.sonar.plugins.python.api.tree.Argument;
30+
import org.sonar.plugins.python.api.tree.AssignmentExpression;
31+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
32+
import org.sonar.plugins.python.api.tree.Expression;
33+
import org.sonar.plugins.python.api.tree.Parameter;
34+
import org.sonar.plugins.python.api.tree.RegularArgument;
35+
import org.sonar.plugins.python.api.tree.StringElement;
36+
import org.sonar.plugins.python.api.tree.Tree;
37+
import org.sonar.python.tree.TreeUtils;
38+
39+
@Rule(key = "S5685")
40+
public class ConfusingWalrusCheck extends PythonSubscriptionCheck {
41+
42+
private static final String MESSAGE = "Use an assignment statement (\"=\") instead; \":=\" operator is confusing in this context.";
43+
private static final String MOVE_MESSAGE = "Move this assignment out of the %s; \":=\" operator is confusing in this context.";
44+
45+
@Override
46+
public void initialize(Context context) {
47+
context.registerSyntaxNodeConsumer(Tree.Kind.ASSIGNMENT_EXPRESSION, this::checkAssignmentExpression);
48+
49+
context.registerSyntaxNodeConsumer(Tree.Kind.STRING_ELEMENT, ctx -> {
50+
StringElement stringElement = (StringElement) ctx.syntaxNode();
51+
for (Expression expression : stringElement.interpolatedExpressions()) {
52+
checkNestedWalrus(ctx, expression, String.format(MOVE_MESSAGE, "interpolated expression"));
53+
}
54+
});
55+
56+
context.registerSyntaxNodeConsumer(Tree.Kind.PARAMETER, ctx -> {
57+
Parameter parameter = (Parameter) ctx.syntaxNode();
58+
checkNestedWalrus(ctx, parameter, String.format(MOVE_MESSAGE, "function definition"));
59+
});
60+
61+
context.registerSyntaxNodeConsumer(Tree.Kind.ARG_LIST, ctx -> {
62+
ArgList argList = (ArgList) ctx.syntaxNode();
63+
if (hasKeywordArguments(argList)) {
64+
checkNestedWalrus(ctx, argList, String.format(MOVE_MESSAGE, "argument list"));
65+
}
66+
});
67+
}
68+
69+
private static void checkNestedWalrus(SubscriptionContext ctx, Tree tree, String message) {
70+
WalrusVisitor walrusVisitor = new WalrusVisitor();
71+
tree.accept(walrusVisitor);
72+
if (!walrusVisitor.assignmentExpressions.isEmpty()) {
73+
PreciseIssue issue = ctx.addIssue(walrusVisitor.assignmentExpressions.get(0), message);
74+
walrusVisitor.assignmentExpressions.stream().skip(1).forEach(a -> issue.secondary(a, null));
75+
}
76+
}
77+
78+
private void checkAssignmentExpression(SubscriptionContext ctx) {
79+
AssignmentExpression assignmentExpression = (AssignmentExpression) ctx.syntaxNode();
80+
Optional<Tree> parentTree = Optional.ofNullable(TreeUtils.firstAncestor(assignmentExpression, a -> !a.is(Tree.Kind.PARENTHESIZED)));
81+
parentTree.ifPresent(parent -> {
82+
if (parent.is(Tree.Kind.ASSIGNMENT_STMT)) {
83+
ctx.addIssue(assignmentExpression, MESSAGE);
84+
}
85+
if (parent.is(Tree.Kind.EXPRESSION_STMT)) {
86+
ctx.addIssue(assignmentExpression, MESSAGE);
87+
}
88+
});
89+
}
90+
91+
private static boolean hasKeywordArguments(ArgList argList) {
92+
for (Argument argument : argList.arguments()) {
93+
if (argument.is(Tree.Kind.REGULAR_ARGUMENT) && ((RegularArgument) argument).keywordArgument() != null) {
94+
return true;
95+
}
96+
}
97+
return false;
98+
}
99+
100+
private static class WalrusVisitor extends BaseTreeVisitor {
101+
List<Tree> assignmentExpressions = new ArrayList<>();
102+
103+
@Override
104+
public void visitAssignmentExpression(AssignmentExpression assignmentExpression) {
105+
assignmentExpressions.add(assignmentExpression);
106+
}
107+
}
108+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5685.html‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<p>The <a href="https://www.python.org/dev/peps/pep-0572">walrus operator</a> <code>:=</code> (also known as "assignment expression") should be used
2+
with caution as it can easily make code more difficult to understand and thus maintain. In such case it is advised to refactor the code and use an
3+
assignment statement (i.e. <code>=</code>) instead.</p>
4+
<p>This rule raises an issue raises an issue when the walrus operator is used in a way which makes the code confusing.</p>
5+
<h2>Noncompliant Code Example</h2>
6+
<pre>
7+
# using an assignment expression (:=) as an assignment statement (=) is more explicit
8+
(v := f(p)) # Noncompliant
9+
v0 = (v1 := f(p)) # Noncompliant
10+
11+
# using an assignment expression in a function call when keyword arguments are also used.
12+
func(a=(b := f(p))) # Noncompliant
13+
func(a := f(p), b=2) # Noncompliant
14+
def func(param=(p := 21)): # Noncompliant
15+
pass
16+
17+
# using an assignment expression in an annotation
18+
def func(param: (p := 21) = 3): # Noncompliant
19+
pass
20+
21+
# using assignment expression in an f-string. Character ":" is also used as a formatting marker in f-strings.
22+
f'{(x:=10)}' # Noncompliant
23+
f'{x:=10}' # No issue raised but still not recommended. This is not an assignment expression. '=10' is passed to the f-string formatter.
24+
</pre>
25+
<h2>Compliant Solution</h2>
26+
<pre>
27+
v = f(p)
28+
v0 = v1 = f(p)
29+
30+
value = f(p)
31+
func(a=value)
32+
func(value, b=2)
33+
def func(param=21):
34+
p = 21
35+
36+
p = 21
37+
def func(param: p = 3):
38+
pass
39+
40+
x = 10
41+
f'{x}'
42+
</pre>
43+
<h2>See</h2>
44+
<ul>
45+
<li> <a href="https://www.python.org/dev/peps/pep-0572/#exceptional-cases">PEP 572 <del></del> Assignment Expressions</a> </li>
46+
</ul>
47+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5685.json‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"title": "Walrus operator should not make code confusing",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"tags": [
6+
7+
],
8+
"defaultSeverity": "Minor",
9+
"ruleSpecification": "RSPEC-5685",
10+
"sqKey": "S5685",
11+
"scope": "All"
12+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
"S5527",
8383
"S5603",
8484
"S5632",
85+
"S5685",
8586
"S5704",
8687
"S5706",
8788
"S5707",

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2020 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks;
21+
22+
import org.junit.Test;
23+
import org.sonar.python.checks.utils.PythonCheckVerifier;
24+
25+
public class ConfusingWalrusCheckTest {
26+
27+
@Test
28+
public void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/confusingWalrus.py", new ConfusingWalrusCheck());
30+
}
31+
}

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# using an assignment expression (:=) as an assignment statement (=) is more explicit
2+
(v := f(p)) # Noncompliant {{Use an assignment statement ("=") instead; ":=" operator is confusing in this context.}}
3+
#^^^^^^^^^
4+
v0 = (v1 := f(p)) # Noncompliant {{Use an assignment statement ("=") instead; ":=" operator is confusing in this context.}}
5+
# ^^^^^^^^^^
6+
7+
v = f(p)
8+
v0 = v1 = f(p)
9+
x = 42 + (y:=2) # OK, assignment expression within a larger expression.
10+
11+
# using an assignment expression in a function call when keyword arguments are also used.
12+
func(a=(b := f(p))) # Noncompliant {{Move this assignment out of the argument list; ":=" operator is confusing in this context.}}
13+
func(a := f(p), b=2) # Noncompliant {{Move this assignment out of the argument list; ":=" operator is confusing in this context.}}
14+
func((a := f(p)) + 42, b=2) # Noncompliant {{Move this assignment out of the argument list; ":=" operator is confusing in this context.}}
15+
func(a := f(p), 3) # OK
16+
func(a := f(p), (3, 4), *args) # OK
17+
def f(a=(x:=2)+42, b=1): # Noncompliant {{Move this assignment out of the function definition; ":=" operator is confusing in this context.}}
18+
pass
19+
20+
value = f(p)
21+
func(a=value)
22+
func(value, b=2)
23+
24+
def func(param=(p := 21)): # Noncompliant
25+
pass
26+
27+
def func(param=21):
28+
p = 21
29+
30+
# using an assignment expression in an annotation
31+
def func(param: (p := 21) = 3): # Noncompliant {{Move this assignment out of the function definition; ":=" operator is confusing in this context.}}
32+
pass
33+
def f(param: (x:=2)+42, b=1): # Noncompliant {{Move this assignment out of the function definition; ":=" operator is confusing in this context.}}
34+
pass
35+
p = 21
36+
def func(param: p = 3):
37+
pass
38+
39+
# using assignment expression in an f-string. Character ":" is also used as a formatting marker in f-strings.
40+
f'{(x:=10)}' # Noncompliant
41+
f'{foo(x:=10)}' # Noncompliant
42+
# ^^^^^
43+
44+
f'{foo(x:=10) + bar(y:=42)}' # Noncompliant
45+
# ^^^^^ ^^^^^<
46+
f'{x:=10}' # No issue raised but still not recommended. This is not an assignment expression. '=10' is passed to the f-string formatter.
47+
48+
x = 10
49+
f'{x}'

0 commit comments

Comments
 (0)