Skip to content

Commit 5f20d98

Browse files
guillaume-dequennesonartech
guillaume-dequenne
authored and
sonartech
committed
SONARPY-4110 Rule S8492: "set.discard()" should be used instead of checking member… (#1072)
GitOrigin-RevId: 1a980aea27cec87786435de44534bfa1a6bad843
1 parent 600fe6b commit 5f20d98

7 files changed

Lines changed: 406 additions & 0 deletions

File tree

‎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
@@ -402,6 +402,7 @@ public Stream<Class<?>> getChecks() {
402402
SecureCookieCheck.class,
403403
SecureModeEncryptionAlgorithmsCheck.class,
404404
SelfAssignmentCheck.class,
405+
SetDiscardOverRemoveCheck.class,
405406
SetDuplicateKeyCheck.class,
406407
SetUpdateOverForLoopCheck.class,
407408
SideEffectInTfFunctionCheck.class,

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
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 java.util.List;
20+
import java.util.Optional;
21+
import org.sonar.check.Rule;
22+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
23+
import org.sonar.plugins.python.api.SubscriptionContext;
24+
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
25+
import org.sonar.plugins.python.api.tree.Argument;
26+
import org.sonar.plugins.python.api.tree.CallExpression;
27+
import org.sonar.plugins.python.api.tree.ExpressionStatement;
28+
import org.sonar.plugins.python.api.tree.IfStatement;
29+
import org.sonar.plugins.python.api.tree.InExpression;
30+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
31+
import org.sonar.plugins.python.api.tree.RegularArgument;
32+
import org.sonar.plugins.python.api.tree.Statement;
33+
import org.sonar.plugins.python.api.tree.Tree;
34+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
35+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
36+
import org.sonar.python.checks.utils.CheckUtils;
37+
import org.sonar.python.quickfix.TextEditUtils;
38+
import org.sonar.python.tree.TreeUtils;
39+
40+
@Rule(key = "S8492")
41+
public class SetDiscardOverRemoveCheck extends PythonSubscriptionCheck {
42+
43+
private static final String MESSAGE = "Use \"discard()\" instead of checking membership before calling \"remove()\".";
44+
private static final String QUICK_FIX_MESSAGE = "Replace with \"discard()\"";
45+
private static final String REMOVE_METHOD_NAME = "remove";
46+
private static final TypeMatcher SET_TYPE_MATCHER = TypeMatchers.isObjectInstanceOf("builtins.set");
47+
48+
@Override
49+
public void initialize(Context context) {
50+
context.registerSyntaxNodeConsumer(Tree.Kind.IF_STMT, SetDiscardOverRemoveCheck::checkIfStatement);
51+
}
52+
53+
private static void checkIfStatement(SubscriptionContext ctx) {
54+
IfStatement ifStatement = (IfStatement) ctx.syntaxNode();
55+
if (ifStatement.isElif()
56+
|| ifStatement.elseBranch() != null
57+
|| !ifStatement.elifBranches().isEmpty()) {
58+
return;
59+
}
60+
if (!(ifStatement.condition() instanceof InExpression inExpression) || inExpression.notToken() != null) {
61+
return;
62+
}
63+
List<Statement> statements = ifStatement.body().statements();
64+
if (statements.size() != 1 || !(statements.get(0) instanceof ExpressionStatement expressionStatement)) {
65+
return;
66+
}
67+
if (expressionStatement.expressions().size() != 1
68+
|| !(expressionStatement.expressions().get(0) instanceof CallExpression callExpression)) {
69+
return;
70+
}
71+
if (!(callExpression.callee() instanceof QualifiedExpression qualifiedExpression)
72+
|| !REMOVE_METHOD_NAME.equals(qualifiedExpression.name().name())) {
73+
return;
74+
}
75+
List<Argument> arguments = callExpression.arguments();
76+
if (arguments.size() != 1 || !(arguments.get(0) instanceof RegularArgument regularArgument)
77+
|| regularArgument.keywordArgument() != null) {
78+
return;
79+
}
80+
if (!CheckUtils.areEquivalent(qualifiedExpression.qualifier(), inExpression.rightOperand())
81+
|| !CheckUtils.areEquivalent(regularArgument.expression(), inExpression.leftOperand())) {
82+
return;
83+
}
84+
if (!SET_TYPE_MATCHER.isTrueFor(qualifiedExpression.qualifier(), ctx)) {
85+
return;
86+
}
87+
var issue = ctx.addIssue(inExpression, MESSAGE);
88+
createQuickFix(ifStatement, qualifiedExpression, regularArgument).ifPresent(issue::addQuickFix);
89+
}
90+
91+
private static Optional<PythonQuickFix> createQuickFix(IfStatement ifStatement, QualifiedExpression qualifiedExpression, RegularArgument argument) {
92+
String qualifierText = TreeUtils.treeToString(qualifiedExpression.qualifier(), false);
93+
String argumentText = TreeUtils.treeToString(argument.expression(), false);
94+
if (qualifierText == null || argumentText == null) {
95+
return Optional.empty();
96+
}
97+
String replacement = "%s.discard(%s)".formatted(qualifierText, argumentText);
98+
return Optional.of(PythonQuickFix.newQuickFix(QUICK_FIX_MESSAGE)
99+
.addTextEdit(TextEditUtils.replace(ifStatement, replacement))
100+
.build());
101+
}
102+
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<p>This rule raises an issue when code checks if an element exists in a set using the <code>in</code> operator before calling <code>remove()</code> on
2+
that element.</p>
3+
<h2>Why is this an issue?</h2>
4+
<p>Checking whether an element exists in a set before removing it is unnecessary:</p>
5+
<pre>
6+
if x in my_set:
7+
my_set.remove(x)
8+
</pre>
9+
<p>This pattern has two problems:</p>
10+
<ol>
11+
<li><strong>double lookup</strong>: the code performs two hash table lookups - one for the membership check (<code>in</code>) and another for the
12+
removal (<code>remove()</code>). Each lookup has a cost.</li>
13+
<li><strong>less idiomatic</strong>: Python provides the <code>discard()</code> method specifically for this use case. Using it makes the code more
14+
Pythonic and signals intent more clearly.</li>
15+
</ol>
16+
<p>The <code>discard()</code> method combines both operations into a single call. It removes the element if it exists and does nothing otherwise:</p>
17+
<pre>
18+
my_set.discard(x) # Single lookup, no exception if x is not present
19+
</pre>
20+
<h3>What is the potential impact?</h3>
21+
<p>Using <code>discard()</code> improves readability and removes a redundant hash table lookup. In loops or performance-critical code, eliminating
22+
these extra lookups can provide measurable improvements.</p>
23+
<h2>How to fix it</h2>
24+
<p>Replace the conditional check and <code>remove()</code> call with a single <code>discard()</code> call. The <code>discard()</code> method safely
25+
removes the element if it exists and does nothing if it does not, without raising an exception.</p>
26+
<h3>Code examples</h3>
27+
<h4>Noncompliant code example</h4>
28+
<pre data-diff-id="1" data-diff-type="noncompliant">
29+
my_set = {1, 2, 3, 4, 5}
30+
value = 3
31+
32+
if value in my_set: # Noncompliant
33+
my_set.remove(value)
34+
</pre>
35+
<h4>Compliant solution</h4>
36+
<pre data-diff-id="1" data-diff-type="compliant">
37+
my_set = {1, 2, 3, 4, 5}
38+
value = 3
39+
40+
my_set.discard(value)
41+
</pre>
42+
<h2>Resources</h2>
43+
<h3>Documentation</h3>
44+
<ul>
45+
<li>Python Documentation - <a href="https://docs.python.org/3/library/stdtypes.html#frozenset.discard"><code>set.discard()</code> method</a></li>
46+
<li>Python Documentation - <a href="https://docs.python.org/3/library/stdtypes.html#frozenset.remove"><code>set.remove()</code> method</a></li>
47+
</ul>
48+

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
{
2+
"title": "\"set.discard()\" should be used instead of checking membership before removal",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "2 min"
8+
},
9+
"tags": [
10+
"performance",
11+
"pythonic",
12+
"idiom"
13+
],
14+
"defaultSeverity": "Minor",
15+
"ruleSpecification": "RSPEC-8492",
16+
"sqKey": "S8492",
17+
"scope": "All",
18+
"quickfix": "targeted",
19+
"code": {
20+
"impacts": {
21+
"MAINTAINABILITY": "LOW"
22+
},
23+
"attribute": "CLEAR"
24+
}
25+
}

‎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
@@ -329,6 +329,7 @@
329329
"S8414",
330330
"S8415",
331331
"S8490",
332+
"S8492",
332333
"S8493",
333334
"S8494",
334335
"S8495",

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
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 SetDiscardOverRemoveCheckTest {
24+
25+
@Test
26+
void test() {
27+
PythonCheckVerifier.verify("src/test/resources/checks/setDiscardOverRemove.py", new SetDiscardOverRemoveCheck());
28+
}
29+
30+
@Test
31+
void quickFixTest() {
32+
String before = "my_set = {1, 2, 3}\nif x in my_set:\n my_set.remove(x)\n";
33+
String after = "my_set = {1, 2, 3}\nmy_set.discard(x)";
34+
var check = new SetDiscardOverRemoveCheck();
35+
PythonQuickFixVerifier.verify(check, before, after);
36+
PythonQuickFixVerifier.verifyQuickFixMessages(check, before, "Replace with \"discard()\"");
37+
}
38+
}

0 commit comments

Comments
 (0)