Skip to content

Commit 841f35c

Browse files
Seppli11Seppli11
authored
SONARPY-1814: S1481: Fix detection of mutation of dict using the |= operator (#2029)
* SONARPY-1814 raise when not updaing a dict with "|=" * SONARPY-1814 add test coverage and remove @SuppressWarnings
1 parent cff542f commit 841f35c

File tree

3 files changed

+65
-16
lines changed

3 files changed

+65
-16
lines changed

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

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.Objects;
2525
import java.util.Optional;
2626
import java.util.Set;
27+
import java.util.function.Predicate;
2728
import java.util.regex.Pattern;
2829
import org.sonar.check.Rule;
2930
import org.sonar.check.RuleProperty;
@@ -36,9 +37,11 @@
3637
import org.sonar.plugins.python.api.tree.AnnotatedAssignment;
3738
import org.sonar.plugins.python.api.tree.AssignmentExpression;
3839
import org.sonar.plugins.python.api.tree.AssignmentStatement;
40+
import org.sonar.plugins.python.api.tree.CompoundAssignmentStatement;
3941
import org.sonar.plugins.python.api.tree.ComprehensionExpression;
4042
import org.sonar.plugins.python.api.tree.DictCompExpression;
4143
import org.sonar.plugins.python.api.tree.ExceptClause;
44+
import org.sonar.plugins.python.api.tree.Expression;
4245
import org.sonar.plugins.python.api.tree.ExpressionList;
4346
import org.sonar.plugins.python.api.tree.ForStatement;
4447
import org.sonar.plugins.python.api.tree.FunctionDef;
@@ -77,11 +80,16 @@ public class UnusedLocalVariableCheck extends PythonSubscriptionCheck {
7780
public void initialize(Context context) {
7881
pattern = Pattern.compile(format);
7982
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::checkTemplateVariablesAccessEnabled);
80-
context.registerSyntaxNodeConsumer(Kind.FUNCDEF, ctx -> checkLocalVars(ctx, ctx.syntaxNode(), ((FunctionDef) ctx.syntaxNode()).localVariables()));
81-
context.registerSyntaxNodeConsumer(Kind.DICT_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(), ((DictCompExpression) ctx.syntaxNode()).localVariables()));
82-
context.registerSyntaxNodeConsumer(Kind.LIST_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(), ((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
83-
context.registerSyntaxNodeConsumer(Kind.SET_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(), ((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
84-
context.registerSyntaxNodeConsumer(Kind.GENERATOR_EXPR, ctx -> checkLocalVars(ctx, ctx.syntaxNode(), ((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
83+
context.registerSyntaxNodeConsumer(Kind.FUNCDEF, ctx -> checkLocalVars(ctx, ctx.syntaxNode(),
84+
((FunctionDef) ctx.syntaxNode()).localVariables()));
85+
context.registerSyntaxNodeConsumer(Kind.DICT_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(),
86+
((DictCompExpression) ctx.syntaxNode()).localVariables()));
87+
context.registerSyntaxNodeConsumer(Kind.LIST_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(),
88+
((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
89+
context.registerSyntaxNodeConsumer(Kind.SET_COMPREHENSION, ctx -> checkLocalVars(ctx, ctx.syntaxNode(),
90+
((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
91+
context.registerSyntaxNodeConsumer(Kind.GENERATOR_EXPR, ctx -> checkLocalVars(ctx, ctx.syntaxNode(),
92+
((ComprehensionExpression) ctx.syntaxNode()).localVariables()));
8593
}
8694

8795
private void checkTemplateVariablesAccessEnabled(SubscriptionContext ctx) {
@@ -103,6 +111,7 @@ private void checkLocalVars(SubscriptionContext ctx, Tree functionTree, Set<Symb
103111
symbols.stream()
104112
.filter(s -> !pattern.matcher(s.name()).matches())
105113
.filter(UnusedLocalVariableCheck::hasOnlyBindingUsages)
114+
.filter(UnusedLocalVariableCheck::isNotUpdatingParameterDict)
106115
.filter(symbol -> !isVariableAccessedInStringTemplate(symbol, stringLiteralValuesCollector))
107116
.forEach(symbol -> {
108117
var usages = symbol.usages().stream()
@@ -149,7 +158,8 @@ public PreciseIssue createIssue(SubscriptionContext ctx, Symbol symbol, Usage us
149158

150159
private static void createAssignmentQuickFix(Usage usage, PreciseIssue issue) {
151160
if (usage.kind().equals(Usage.Kind.ASSIGNMENT_LHS)) {
152-
Statement assignmentStatement = ((Statement) TreeUtils.firstAncestorOfKind(usage.tree(), Kind.ASSIGNMENT_STMT, Kind.ANNOTATED_ASSIGNMENT));
161+
Statement assignmentStatement = ((Statement) TreeUtils.firstAncestorOfKind(usage.tree(), Kind.ASSIGNMENT_STMT,
162+
Kind.ANNOTATED_ASSIGNMENT));
153163

154164
Optional.ofNullable(assignmentStatement).filter(stmt -> stmt.is(Kind.ASSIGNMENT_STMT)).map(AssignmentStatement.class::cast).ifPresent(stmt -> {
155165
PythonQuickFix quickFix = PythonQuickFix.newQuickFix(ASSIGNMENT_QUICK_FIX_MESSAGE,
@@ -167,7 +177,7 @@ private static void createAssignmentQuickFix(Usage usage, PreciseIssue issue) {
167177
Tree assignmentTree = TreeUtils.firstAncestorOfKind(usage.tree(), Kind.ASSIGNMENT_EXPRESSION);
168178
Optional.ofNullable(assignmentTree).map(AssignmentExpression.class::cast).ifPresent(assignmentExpr -> {
169179
PythonQuickFix quickFix = PythonQuickFix.newQuickFix(ASSIGNMENT_QUICK_FIX_MESSAGE,
170-
createAssignmentExpressionQuickFix(usage, assignmentExpr));
180+
createAssignmentExpressionQuickFix(usage, assignmentExpr));
171181
issue.addQuickFix(quickFix);
172182
});
173183
}
@@ -188,9 +198,11 @@ private static boolean isUnderscoreSymbolAlreadyAssigned(SubscriptionContext ctx
188198
Tree searchTree = usage.kind().equals(Usage.Kind.LOOP_DECLARATION) ? ctx.syntaxNode() : null;
189199
while (foundUnderscoreSymbol == null && searchTree != null) {
190200
if (searchTree.is(Kind.FUNCDEF)) {
191-
foundUnderscoreSymbol = ((FunctionDef) searchTree).localVariables().stream().filter(symbol1 -> "_".equals(symbol1.name())).findAny().orElse(null);
201+
foundUnderscoreSymbol =
202+
((FunctionDef) searchTree).localVariables().stream().filter(symbol1 -> "_".equals(symbol1.name())).findAny().orElse(null);
192203
} else if (searchTree.is(Kind.FILE_INPUT)) {
193-
foundUnderscoreSymbol = ((FileInputImpl) searchTree).globalVariables().stream().filter(symbol1 -> "_".equals(symbol1.name())).findAny().orElse(null);
204+
foundUnderscoreSymbol =
205+
((FileInputImpl) searchTree).globalVariables().stream().filter(symbol1 -> "_".equals(symbol1.name())).findAny().orElse(null);
194206
}
195207
searchTree = TreeUtils.firstAncestor(searchTree, a -> a.is(Kind.FUNCDEF, Kind.FILE_INPUT));
196208
}
@@ -199,7 +211,8 @@ private static boolean isUnderscoreSymbolAlreadyAssigned(SubscriptionContext ctx
199211

200212
private static boolean isLoopIndex(Usage usage, Symbol symbol) {
201213
var allowedKinds = EnumSet.of(Usage.Kind.LOOP_DECLARATION, Usage.Kind.COMP_DECLARATION);
202-
Optional<Symbol> optionalSymbol = Optional.of(usage).filter(u -> allowedKinds.contains(u.kind())).map(Usage::tree).map(a -> ((Name) a).symbol());
214+
Optional<Symbol> optionalSymbol =
215+
Optional.of(usage).filter(u -> allowedKinds.contains(u.kind())).map(Usage::tree).map(a -> ((Name) a).symbol());
203216
return optionalSymbol.map(value -> value.equals(symbol)).orElse(false);
204217
}
205218

@@ -227,19 +240,39 @@ private static boolean hasOnlyBindingUsages(Symbol symbol) {
227240
return false;
228241
}
229242
return usages.stream().noneMatch(usage -> usage.kind() == Usage.Kind.IMPORT)
230-
&& usages.stream().allMatch(Usage::isBindingUsage);
243+
&& usages.stream().allMatch(Usage::isBindingUsage);
244+
}
245+
246+
private static boolean isNotUpdatingParameterDict(Symbol symbol) {
247+
List<Usage> usages = symbol.usages();
248+
return usages.stream().noneMatch(UnusedLocalVariableCheck::isDictAssignmentExpressionUsage) ||
249+
usages.stream().noneMatch(usage -> usage.kind() == Usage.Kind.PARAMETER);
250+
}
251+
252+
private static boolean isDictAssignmentExpressionUsage(Usage usage) {
253+
Tree compoundAssignmentTree = TreeUtils.firstAncestorOfKind(usage.tree(), Kind.COMPOUND_ASSIGNMENT);
254+
return compoundAssignmentTree instanceof CompoundAssignmentStatement compoundAssignmentStatement &&
255+
"|=".equals(compoundAssignmentStatement.compoundAssignmentToken().value()) &&
256+
compoundAssignmentStatement.lhsExpression().type().mustBeOrExtend("dict");
231257
}
232258

233259
private static boolean isOnlyTypeAnnotation(List<Usage> usages) {
234260
return usages.size() == 1 && usages.get(0).isBindingUsage() &&
235-
TreeUtils.firstAncestor(usages.get(0).tree(), t -> t.is(Kind.ANNOTATED_ASSIGNMENT) && ((AnnotatedAssignment) t).assignedValue() == null) != null;
261+
TreeUtils.firstAncestor(usages.get(0).tree(),
262+
t -> t.is(Kind.ANNOTATED_ASSIGNMENT) && ((AnnotatedAssignment) t).assignedValue() == null) != null;
236263
}
237264

238265
private static boolean isTupleDeclaration(Usage usage) {
239266
var tree = usage.tree();
240-
return !isSequenceUnpacking(usage) && TreeUtils.firstAncestor(tree, t -> t.is(Kind.TUPLE)
241-
|| (t.is(Kind.EXPRESSION_LIST) && ((ExpressionList) t).expressions().size() > 1)
242-
|| (t.is(Kind.FOR_STMT) && ((ForStatement) t).expressions().size() > 1 && ((ForStatement) t).expressions().contains(tree))) != null;
267+
268+
Predicate<Tree> isTupleDeclaration = t -> t.is(Kind.TUPLE)
269+
|| (t.is(Kind.EXPRESSION_LIST) && ((ExpressionList) t).expressions().size() > 1)
270+
|| (t.is(Kind.FOR_STMT)
271+
&& ((ForStatement) t).expressions().size() > 1
272+
&& tree instanceof Expression treeExpr
273+
&& ((ForStatement) t).expressions().contains(treeExpr));
274+
275+
return !isSequenceUnpacking(usage) && TreeUtils.firstAncestor(tree, isTupleDeclaration) != null;
243276
}
244277

245278
private static boolean isSequenceUnpacking(Usage usage) {

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ def using_tuples():
6464
for (c, d) in foo():
6565
pass
6666

67+
68+
6769
def for_loops():
6870
for _ in range(10):
6971
do_something()
@@ -195,3 +197,17 @@ def assignment_target_in_set_comprenhension():
195197

196198
if {(unused_last := i) for i in range(5)}: # Noncompliant
197199
return
200+
201+
def param_dict_union_assignment(values: dict):
202+
values |= dict(b=2)
203+
204+
def param_dict_assignment(values: dict):
205+
values = dict(b=2) # Noncompliant
206+
207+
def local_dict_union_assignment():
208+
values = {} # Noncompliant
209+
values |= dict(b=2)
210+
211+
def local_not_dict_union_assignment():
212+
values = True #Noncompliant
213+
values |= False

‎python-frontend/src/main/java/org/sonar/plugins/python/api/tree/CompoundAssignmentStatement.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
*
2727
* Example: <pre>x += y</pre>
2828
*
29-
* See https://docs.python.org/3/reference/simple_stmts.html#grammar-token-augmented-assignment-stmt
29+
* See https://docs.python.org/3/reference/simple_stmts.html#grammar-token-python-grammar-augmented_assignment_stmt
3030
*/
3131
public interface CompoundAssignmentStatement extends Statement {
3232
Expression lhsExpression();

0 commit comments

Comments
 (0)