Skip to content

Commit 8744029

Browse files
sonar-nigel[bot]marc-jasper-sonarsource
authored and
sonartech
committed
SONARPY-3960 Rule S8502 "set.update()" should be used instead of a for-loop with "add()" (#997)
Co-authored-by: Marc Jasper <marc.jasper@sonarsource.com> GitOrigin-RevId: fe167c270ae6ebb80df1f9e804c3671a1680c168
1 parent 0453648 commit 8744029

File tree

6 files changed

+433
-0
lines changed

6 files changed

+433
-0
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,7 @@ public Stream<Class<?>> getChecks() {
397397
SecureModeEncryptionAlgorithmsCheck.class,
398398
SelfAssignmentCheck.class,
399399
SetDuplicateKeyCheck.class,
400+
SetUpdateOverForLoopCheck.class,
400401
SideEffectInTfFunctionCheck.class,
401402
SillyEqualityCheck.class,
402403
SillyIdentityCheck.class,

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

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* You can redistribute and/or modify this program under the terms of
7+
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks;
18+
19+
import org.sonar.check.Rule;
20+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
21+
import org.sonar.plugins.python.api.SubscriptionContext;
22+
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
23+
import org.sonar.plugins.python.api.symbols.v2.SymbolV2;
24+
import org.sonar.plugins.python.api.tree.CallExpression;
25+
import org.sonar.plugins.python.api.tree.Expression;
26+
import org.sonar.plugins.python.api.tree.ExpressionStatement;
27+
import org.sonar.plugins.python.api.tree.ForStatement;
28+
import org.sonar.plugins.python.api.tree.Name;
29+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
30+
import org.sonar.plugins.python.api.tree.RegularArgument;
31+
import org.sonar.plugins.python.api.tree.Tree;
32+
import org.sonar.plugins.python.api.tree.UnpackingExpression;
33+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
34+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
35+
import org.sonar.python.quickfix.TextEditUtils;
36+
import org.sonar.python.tree.TreeUtils;
37+
38+
@Rule(key = "S8502")
39+
public class SetUpdateOverForLoopCheck extends PythonSubscriptionCheck {
40+
41+
private static final TypeMatcher SET_TYPE_MATCHER = TypeMatchers.isObjectOfType("builtins.set");
42+
43+
private static final String MESSAGE = "Use \"set.update()\" instead of a for-loop with \"add()\".";
44+
45+
@Override
46+
public void initialize(Context context) {
47+
context.registerSyntaxNodeConsumer(Tree.Kind.FOR_STMT, SetUpdateOverForLoopCheck::checkForStatement);
48+
}
49+
50+
private static void checkForStatement(SubscriptionContext ctx) {
51+
ForStatement forStatement = (ForStatement) ctx.syntaxNode();
52+
53+
if (forStatement.expressions().size() != 1) {
54+
return;
55+
}
56+
if (forStatement.elseClause() != null) {
57+
return;
58+
}
59+
Expression loopVarExpr = forStatement.expressions().get(0);
60+
if (!(loopVarExpr instanceof Name loopVar)) {
61+
return;
62+
}
63+
if (forStatement.testExpressions().size() != 1) {
64+
return;
65+
}
66+
Expression iterable = forStatement.testExpressions().get(0);
67+
68+
CallExpression callExpr = extractSingleBodyCallExpression(forStatement);
69+
if (callExpr == null) {
70+
return;
71+
}
72+
if (!(callExpr.callee() instanceof QualifiedExpression qualifiedExpr)) {
73+
return;
74+
}
75+
// TypeMatchers operate on types, not method names; the callee name must be checked syntactically
76+
if (!"add".equals(qualifiedExpr.name().name())) {
77+
return;
78+
}
79+
Expression receiver = qualifiedExpr.qualifier();
80+
81+
// Exclude transient objects like get_set().add(item)
82+
if (receiver instanceof CallExpression) {
83+
return;
84+
}
85+
if (!SET_TYPE_MATCHER.isTrueFor(receiver, ctx)) {
86+
return;
87+
}
88+
if (!isLoopVariableTheOnlyArgument(callExpr, loopVar)) {
89+
return;
90+
}
91+
92+
PreciseIssue issue = ctx.addIssue(qualifiedExpr, MESSAGE);
93+
addQuickFix(issue, forStatement, receiver, iterable);
94+
}
95+
96+
private static boolean isLoopVariableTheOnlyArgument(CallExpression callExpr, Name loopVar) {
97+
if (callExpr.arguments().size() != 1) {
98+
return false;
99+
}
100+
if (!(callExpr.arguments().get(0) instanceof RegularArgument regularArgument)) {
101+
return false;
102+
}
103+
if (regularArgument.keywordArgument() != null) {
104+
return false;
105+
}
106+
Expression argExpr = regularArgument.expression();
107+
if (argExpr instanceof UnpackingExpression || !(argExpr instanceof Name argName)) {
108+
return false;
109+
}
110+
SymbolV2 loopVarSymbol = loopVar.symbolV2();
111+
return loopVarSymbol != null && loopVarSymbol.equals(argName.symbolV2());
112+
}
113+
114+
private static CallExpression extractSingleBodyCallExpression(ForStatement forStatement) {
115+
if (forStatement.body().statements().size() != 1) {
116+
return null;
117+
}
118+
if (!(forStatement.body().statements().get(0) instanceof ExpressionStatement exprStmt)) {
119+
return null;
120+
}
121+
if (exprStmt.expressions().size() != 1) {
122+
return null;
123+
}
124+
if (!(exprStmt.expressions().get(0) instanceof CallExpression callExpr)) {
125+
return null;
126+
}
127+
return callExpr;
128+
}
129+
130+
private static void addQuickFix(PreciseIssue issue, ForStatement forStatement, Expression receiver, Expression iterable) {
131+
String receiverText = TreeUtils.treeToString(receiver, false);
132+
String iterableText = TreeUtils.treeToString(iterable, false);
133+
if (receiverText == null || iterableText == null) {
134+
return;
135+
}
136+
137+
String replacement = receiverText + ".update(" + iterableText + ")";
138+
139+
PythonQuickFix quickFix = PythonQuickFix.newQuickFix("Use \"set.update()\" instead", TextEditUtils.replace(forStatement, replacement));
140+
issue.addQuickFix(quickFix);
141+
}
142+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<p>This rule raises an issue when a for-loop is used to add all elements from one collection to a set using the <code>add()</code> method.</p>
2+
<h2>Why is this an issue?</h2>
3+
<p>Adding elements to a set one at a time in a loop is unnecessary when the <code>update()</code> method exists. The <code>update()</code> method
4+
combines all additions into a single call and avoids the overhead of repeated <code>add()</code> invocations. Using <code>update()</code> improves
5+
readability and removes redundant method call overhead. In loops or performance-critical code, the difference can become measurable.</p>
6+
<h2>How to fix it</h2>
7+
<p>Replace the for-loop with a single <code>update()</code> call. The method accepts any iterable.</p>
8+
<h3>Code examples</h3>
9+
<h4>Noncompliant code example</h4>
10+
<pre data-diff-id="1" data-diff-type="noncompliant">
11+
my_set = {1, 2, 3}
12+
other_collection = [4, 5, 6]
13+
14+
for element in other_collection:
15+
my_set.add(element) # Noncompliant
16+
</pre>
17+
<h4>Compliant solution</h4>
18+
<pre data-diff-id="1" data-diff-type="compliant">
19+
my_set = {1, 2, 3}
20+
other_collection = [4, 5, 6]
21+
22+
my_set.update(other_collection)
23+
</pre>
24+
<h2>Resources</h2>
25+
<h3>Documentation</h3>
26+
<ul>
27+
<li>Python Documentation - <a href="https://docs.python.org/3/library/stdtypes.html#frozenset.update"><code>set.update()</code> method</a></li>
28+
<li>Python Documentation - <a href="https://docs.python.org/3/library/stdtypes.html#set-types-set-frozenset">Set Types — <code>set</code>, <code>frozenset</code></a></li>
29+
</ul>
30+

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
{
2+
"title": "\"set.update()\" should be used instead of a for-loop with \"add()\"",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [],
10+
"defaultSeverity": "Minor",
11+
"ruleSpecification": "RSPEC-8502",
12+
"sqKey": "S8502",
13+
"scope": "All",
14+
"quickfix": "targeted",
15+
"code": {
16+
"impacts": {
17+
"MAINTAINABILITY": "LOW"
18+
},
19+
"attribute": "CONVENTIONAL"
20+
}
21+
}

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* You can redistribute and/or modify this program under the terms of
7+
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks;
18+
19+
import org.junit.jupiter.api.Test;
20+
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
21+
import org.sonar.python.checks.utils.PythonCheckVerifier;
22+
23+
class SetUpdateOverForLoopCheckTest {
24+
25+
private static final SetUpdateOverForLoopCheck check = new SetUpdateOverForLoopCheck();
26+
27+
@Test
28+
void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/setUpdateOverForLoop.py", check);
30+
}
31+
32+
@Test
33+
void test_quick_fix() {
34+
String codeWithIssue = """
35+
my_set = set()
36+
other_collection = [4, 5, 6]
37+
for element in other_collection:
38+
my_set.add(element)
39+
""";
40+
41+
PythonQuickFixVerifier.verifyQuickFixMessages(check, codeWithIssue, "Use \"set.update()\" instead");
42+
43+
String correctCode = """
44+
my_set = set()
45+
other_collection = [4, 5, 6]
46+
my_set.update(other_collection)""";
47+
48+
PythonQuickFixVerifier.verify(check, codeWithIssue, correctCode);
49+
}
50+
51+
@Test
52+
void test_no_quick_fix_on_multiline_iterable() {
53+
String codeWithIssue = """
54+
my_set = set()
55+
for element in [
56+
1,
57+
2,
58+
3,
59+
]:
60+
my_set.add(element)
61+
""";
62+
63+
PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
64+
}
65+
66+
@Test
67+
void test_no_quick_fix_on_multiline_receiver() {
68+
String codeWithIssue = """
69+
my_set = set()
70+
items = [1, 2, 3]
71+
for element in items:
72+
(my_set
73+
).add(element)
74+
""";
75+
76+
PythonQuickFixVerifier.verifyNoQuickFixes(check, codeWithIssue);
77+
}
78+
}

0 commit comments

Comments
 (0)