Skip to content

Commit 87011c0

Browse files
Seppli11sonartech
Seppli11
authored and
sonartech
committed
SONARPY-3163 S7504: Fix FP when copying the original list is required (#458)
GitOrigin-RevId: 36e43d90859338caf4fa5710b615746eab930b16
1 parent 29a3265 commit 87011c0

2 files changed

Lines changed: 125 additions & 18 deletions

File tree

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

Lines changed: 93 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,25 +18,35 @@
1818

1919
import java.util.List;
2020
import java.util.Optional;
21+
import java.util.Set;
2122
import org.sonar.check.Rule;
2223
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2324
import org.sonar.plugins.python.api.SubscriptionContext;
2425
import org.sonar.plugins.python.api.TriBool;
2526
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
27+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
2628
import org.sonar.plugins.python.api.tree.CallExpression;
2729
import org.sonar.plugins.python.api.tree.ComprehensionFor;
2830
import org.sonar.plugins.python.api.tree.Expression;
2931
import org.sonar.plugins.python.api.tree.ForStatement;
32+
import org.sonar.plugins.python.api.tree.Name;
33+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
3034
import org.sonar.plugins.python.api.tree.RegularArgument;
3135
import org.sonar.plugins.python.api.tree.Tree;
3236
import org.sonar.python.quickfix.TextEditUtils;
37+
import org.sonar.python.semantic.v2.SymbolV2;
3338
import org.sonar.python.tree.TreeUtils;
3439
import org.sonar.python.types.v2.TypeCheckBuilder;
40+
import org.sonar.python.types.v2.TypeCheckMap;
3541

3642
@Rule(key = "S7504")
3743
public class UnnecessaryListCastCheck extends PythonSubscriptionCheck {
3844
private TypeCheckBuilder isListCallCheck;
3945

46+
private static final Set<String> MODIFYING_LIST_METHODS = Set.of("append", "extend", "insert", "remove", "pop", "clear",
47+
"sort", "reverse");
48+
private TypeCheckMap<Object> typeCheckMap;
49+
4050
@Override
4151
public void initialize(Context context) {
4252
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::initChecks);
@@ -46,34 +56,33 @@ public void initialize(Context context) {
4656

4757
private void initChecks(SubscriptionContext ctx) {
4858
isListCallCheck = ctx.typeChecker().typeCheckBuilder().isBuiltinWithName("list");
59+
60+
var marker = new Object();
61+
typeCheckMap = new TypeCheckMap<>();
62+
MODIFYING_LIST_METHODS.forEach(method -> {
63+
var checker = ctx.typeChecker().typeCheckBuilder().isTypeWithName("list." + method);
64+
typeCheckMap.put(checker, marker);
65+
});
4966
}
5067

5168
private void checkForStatements(SubscriptionContext ctx) {
5269
ForStatement stmt = ((ForStatement) ctx.syntaxNode());
53-
checkListCastCheck(stmt.testExpressions(), ctx);
70+
hasListCallOnIterable(stmt.testExpressions())
71+
.filter(listCall -> !isListModifiedInLoop(listCall, stmt))
72+
.ifPresent(listCall -> raiseIssue(ctx, listCall));
5473
}
5574

5675
private void checkComprehensions(SubscriptionContext ctx) {
5776
ComprehensionFor comprehensionFor = ((ComprehensionFor) ctx.syntaxNode());
58-
checkListCastCheck(List.of(comprehensionFor.iterable()), ctx);
59-
}
60-
61-
private void checkListCastCheck(List<Expression> expressions, SubscriptionContext ctx) {
62-
hasListCallOnIterable(expressions)
63-
.ifPresent(listCall -> {
64-
PreciseIssue issue = ctx.addIssue(listCall.callee(), "Remove this unnecessary `list()` call on an already iterable object.");
65-
Optional.ofNullable(TreeUtils.treeToString(listCall.argumentList(), false))
66-
.map(replacementText -> TextEditUtils.replace(listCall, replacementText))
67-
.map(textEdit -> PythonQuickFix.newQuickFix("Remove the \"list\" call", textEdit))
68-
.ifPresent(issue::addQuickFix);
69-
});
77+
hasListCallOnIterable(List.of(comprehensionFor.iterable()))
78+
.ifPresent(listCall -> raiseIssue(ctx, listCall));
7079
}
7180

7281
private Optional<CallExpression> hasListCallOnIterable(List<Expression> testExpressions) {
7382
if (testExpressions.size() == 1
74-
&& testExpressions.get(0) instanceof CallExpression callExpression
75-
&& isListCall(callExpression)
76-
&& hasOnlyOneRegularArg(callExpression)) {
83+
&& testExpressions.get(0) instanceof CallExpression callExpression
84+
&& isListCall(callExpression)
85+
&& getFirstRegularArgument(callExpression).isPresent()) {
7786
return Optional.of(callExpression);
7887
}
7988
return Optional.empty();
@@ -83,7 +92,73 @@ private boolean isListCall(CallExpression callExpression) {
8392
return isListCallCheck.check(callExpression.callee().typeV2()) == TriBool.TRUE;
8493
}
8594

86-
private static boolean hasOnlyOneRegularArg(CallExpression callExpression) {
87-
return callExpression.arguments().size() == 1 && callExpression.arguments().get(0) instanceof RegularArgument;
95+
private boolean isListModifiedInLoop(CallExpression callExpression, ForStatement forStatement) {
96+
var listName = getFirstNameArgument(callExpression);
97+
return listName.map(name -> {
98+
ModifyingListMethodTreeVisitor visitor = new ModifyingListMethodTreeVisitor(name, typeCheckMap);
99+
forStatement.accept(visitor);
100+
return visitor.isModifyingListMethod();
101+
}).orElse(false);
102+
}
103+
104+
public static Optional<Name> getFirstNameArgument(CallExpression callExpression) {
105+
return getFirstRegularArgument(callExpression)
106+
.map(RegularArgument::expression)
107+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class));
108+
}
109+
110+
private static Optional<RegularArgument> getFirstRegularArgument(CallExpression callExpression) {
111+
if (callExpression.arguments().size() == 1
112+
&& callExpression.arguments().get(0) instanceof RegularArgument regularArgument) {
113+
return Optional.of(regularArgument);
114+
}
115+
return Optional.empty();
116+
}
117+
118+
private static void raiseIssue(SubscriptionContext ctx, CallExpression listCall) {
119+
PreciseIssue issue = ctx.addIssue(listCall.callee(),
120+
"Remove this unnecessary `list()` call on an already iterable object.");
121+
Optional.ofNullable(listCall.argumentList())
122+
.map(argList -> TreeUtils.treeToString(argList, false))
123+
.map(replacementText -> TextEditUtils.replace(listCall, replacementText))
124+
.map(textEdit -> PythonQuickFix.newQuickFix("Remove the \"list\" call", textEdit))
125+
.ifPresent(issue::addQuickFix);
126+
}
127+
128+
private static class ModifyingListMethodTreeVisitor extends BaseTreeVisitor {
129+
private final Name listName;
130+
private final TypeCheckMap<Object> modifyingListTypeCheckMap;
131+
132+
private boolean isModifyingListMethod = false;
133+
134+
ModifyingListMethodTreeVisitor(Name listName, TypeCheckMap<Object> modifyingListTypeCheckMap) {
135+
this.listName = listName;
136+
this.modifyingListTypeCheckMap = modifyingListTypeCheckMap;
137+
}
138+
139+
public boolean isModifyingListMethod() {
140+
return isModifyingListMethod;
141+
}
142+
143+
@Override
144+
public void visitCallExpression(CallExpression callExpr) {
145+
super.visitCallExpression(callExpr);
146+
var symbol = listName.symbolV2();
147+
if(symbol != null) {
148+
isModifyingListMethod |= isMethodReceiverInstanceOf(callExpr, symbol)
149+
&& isModifyingListMethod(callExpr);
150+
}
151+
}
152+
153+
private static boolean isMethodReceiverInstanceOf(CallExpression callExpr, SymbolV2 listSymbol) {
154+
return callExpr.callee() instanceof QualifiedExpression qualifiedExpression &&
155+
qualifiedExpression.qualifier() instanceof Name name &&
156+
name.symbolV2().equals(listSymbol);
157+
}
158+
159+
private boolean isModifyingListMethod(CallExpression callExpression) {
160+
return modifyingListTypeCheckMap.containsForType(callExpression.callee().typeV2());
161+
}
88162
}
163+
89164
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,35 @@ def foo(a: Union[str, Iterable[str]]):
3131
for i in list(some_val): # Noncompliant
3232
print(i)
3333

34+
def compliant_modifying_list_in_loop():
35+
some_list = [1, 2, 3]
36+
# Compliant, copying is necessary since the list is modified in the loop
37+
for i in list(some_list):
38+
if i % 2 == 0:
39+
some_list.remove(i)
40+
41+
for i in list(some_list): some_list.append(5)
42+
for i in list(some_list): some_list.extend([1, 2])
43+
for i in list(some_list): some_list.insert(0, 1)
44+
for i in list(some_list): some_list.remove(i)
45+
for i in list(some_list): some_list.pop()
46+
for i in list(some_list): some_list.clear()
47+
for i in list(some_list): some_list.sort()
48+
for i in list(some_list): some_list.reverse()
49+
for i in list(some_list): some_list.copy() # Noncompliant
50+
51+
def noncompliant_modifying_list_outside_loop():
52+
some_list = [1, 2, 3]
53+
for i in list(some_list): # Noncompliant
54+
print(i)
55+
some_list.append(5)
56+
57+
[some_list.pop() for i in list(some_list)] # Noncompliant
58+
59+
some_other_list = [10, 20, 30]
60+
for i in list(some_other_list): # Noncompliant
61+
some_list.append(i)
62+
3463
#=============== COVERAGE =============
3564

3665
for i,y in list(range(3)), list(range(4)):
@@ -46,3 +75,6 @@ def foo(a: Union[str, Iterable[str]]):
4675

4776
for i in test("1"):
4877
print(i)
78+
79+
for i in list(test): # Noncompliant
80+
test().append(i)

0 commit comments

Comments
 (0)