Skip to content

Commit 5c70d38

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARPY-745 S5864: Iteration operation on a non-iterable type (SonarSource#842)
1 parent 2f71dcc commit 5c70d38

6 files changed

Lines changed: 239 additions & 105 deletions

File tree

‎its/ruling/src/test/resources/expected/python-S5864.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
'project:mypy-0.782/test-data/samples/crawl.py':[
3+
335,
4+
343,
5+
344,
6+
],
7+
}

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2727
import org.sonar.plugins.python.api.symbols.Symbol;
2828
import org.sonar.plugins.python.api.tree.Expression;
29+
import org.sonar.plugins.python.api.tree.Name;
2930
import org.sonar.plugins.python.api.tree.Token;
31+
import org.sonar.plugins.python.api.tree.Tree;
3032
import org.sonar.plugins.python.api.types.InferredType;
3133
import org.sonar.python.types.InferredTypes;
3234

@@ -40,6 +42,7 @@ public void initialize(Context context) {
4042
new NonCallableCalledCheck().initialize(context);
4143
new IncompatibleOperandsCheck().initialize(context);
4244
new ItemOperationsTypeCheck().initialize(context);
45+
new IterationOnNonIterableCheck().initialize(context);
4346
}
4447

4548
private static class NonCallableCalledCheck extends NonCallableCalled {
@@ -106,4 +109,39 @@ public String message(@Nullable String name, String missingMethod) {
106109
return String.format("Fix this \"%s\" operation; Previous type checks suggest that this expression does not have this method.", missingMethod);
107110
}
108111
}
112+
113+
private static class IterationOnNonIterableCheck extends IterationOnNonIterable {
114+
115+
@Override
116+
boolean isValidIterable(Expression expression, List<LocationInFile> secondaries) {
117+
InferredType type = expression.type();
118+
secondaries.add(InferredTypes.typeClassLocation(type));
119+
return !containsDeclaredType(type) || type.declaresMember("__iter__") || type.declaresMember("__getitem__");
120+
}
121+
122+
@Override
123+
String message(Expression expression, boolean isForLoop) {
124+
String typeName = InferredTypes.typeName(expression.type());
125+
String expressionName = nameFromExpression(expression);
126+
String expressionNameString = expressionName != null ? String.format("\"%s\"", expressionName) : "it";
127+
String typeNameString = typeName != null ? String.format("has type \"%s\" and", typeName) : "";
128+
return isForLoop && isAsyncIterable(expression) ?
129+
String.format("Add \"async\" before \"for\"; Previous type checks suggest that %s %s is an async generator.", expressionNameString, typeNameString) :
130+
String.format("Replace this expression; Previous type checks suggest that %s %s isn't iterable.", expressionNameString, typeNameString);
131+
}
132+
133+
@Override
134+
boolean isAsyncIterable(Expression expression) {
135+
InferredType type = expression.type();
136+
// No need to check again if the type contains a declared type
137+
return type.declaresMember("__aiter__");
138+
}
139+
140+
private static String nameFromExpression(Expression expression) {
141+
if (expression.is(Tree.Kind.NAME)) {
142+
return ((Name) expression).name();
143+
}
144+
return null;
145+
}
146+
}
109147
}

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

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
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.Objects;
25+
import org.sonar.plugins.python.api.LocationInFile;
26+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
27+
import org.sonar.plugins.python.api.SubscriptionContext;
28+
import org.sonar.plugins.python.api.tree.AssignmentStatement;
29+
import org.sonar.plugins.python.api.tree.ComprehensionFor;
30+
import org.sonar.plugins.python.api.tree.Expression;
31+
import org.sonar.plugins.python.api.tree.ExpressionList;
32+
import org.sonar.plugins.python.api.tree.ForStatement;
33+
import org.sonar.plugins.python.api.tree.Tree;
34+
import org.sonar.plugins.python.api.tree.UnpackingExpression;
35+
import org.sonar.plugins.python.api.tree.YieldExpression;
36+
import org.sonar.plugins.python.api.tree.YieldStatement;
37+
import org.sonar.python.api.PythonPunctuator;
38+
39+
public abstract class IterationOnNonIterable extends PythonSubscriptionCheck {
40+
41+
@Override
42+
public void initialize(Context context) {
43+
context.registerSyntaxNodeConsumer(Tree.Kind.UNPACKING_EXPR, this::checkUnpackingExpression);
44+
context.registerSyntaxNodeConsumer(Tree.Kind.ASSIGNMENT_STMT, this::checkAssignment);
45+
context.registerSyntaxNodeConsumer(Tree.Kind.FOR_STMT, this::checkForStatement);
46+
context.registerSyntaxNodeConsumer(Tree.Kind.COMP_FOR, this::checkForComprehension);
47+
context.registerSyntaxNodeConsumer(Tree.Kind.YIELD_STMT, this::checkYieldStatement);
48+
}
49+
50+
private void checkAssignment(SubscriptionContext ctx) {
51+
AssignmentStatement assignmentStatement = (AssignmentStatement) ctx.syntaxNode();
52+
ExpressionList expressionList = assignmentStatement.lhsExpressions().get(0);
53+
List<LocationInFile> secondaries = new ArrayList<>();
54+
if (isLhsIterable(expressionList) && !isValidIterable(assignmentStatement.assignedValue(), secondaries)) {
55+
reportIssue(ctx, assignmentStatement.assignedValue(), secondaries, message(assignmentStatement.assignedValue(), false));
56+
}
57+
}
58+
59+
private static boolean isLhsIterable(ExpressionList expressionList) {
60+
if (expressionList.expressions().size() > 1) {
61+
return true;
62+
}
63+
Expression expression = expressionList.expressions().get(0);
64+
return expression.is(Tree.Kind.LIST_LITERAL) || expression.is(Tree.Kind.TUPLE);
65+
}
66+
67+
private void checkForComprehension(SubscriptionContext ctx) {
68+
ComprehensionFor comprehensionFor = (ComprehensionFor) ctx.syntaxNode();
69+
Expression expression = comprehensionFor.iterable();
70+
List<LocationInFile> secondaries = new ArrayList<>();
71+
if (!isValidIterable(expression, secondaries)) {
72+
reportIssue(ctx, expression, secondaries, message(expression, false));
73+
}
74+
}
75+
76+
private static void reportIssue(SubscriptionContext ctx, Expression expression, List<LocationInFile> secondaries, String message) {
77+
PreciseIssue preciseIssue = ctx.addIssue(expression, message);
78+
secondaries.stream().filter(Objects::nonNull).forEach(location -> preciseIssue.secondary(location, null));
79+
}
80+
81+
private void checkYieldStatement(SubscriptionContext ctx) {
82+
YieldStatement yieldStatement = (YieldStatement) ctx.syntaxNode();
83+
YieldExpression yieldExpression = yieldStatement.yieldExpression();
84+
if (yieldExpression.fromKeyword() == null) {
85+
return;
86+
}
87+
Expression expression = yieldExpression.expressions().get(0);
88+
List<LocationInFile> secondaries = new ArrayList<>();
89+
if (!isValidIterable(expression, secondaries)) {
90+
reportIssue(ctx, expression, secondaries, message(expression, false));
91+
}
92+
}
93+
94+
private void checkUnpackingExpression(SubscriptionContext ctx) {
95+
UnpackingExpression unpackingExpression = (UnpackingExpression) ctx.syntaxNode();
96+
if (unpackingExpression.starToken().type().equals(PythonPunctuator.MUL_MUL)) {
97+
return;
98+
}
99+
Expression expression = unpackingExpression.expression();
100+
List<LocationInFile> secondaries = new ArrayList<>();
101+
if (!isValidIterable(expression, secondaries)) {
102+
reportIssue(ctx, expression, secondaries, message(expression, false));
103+
}
104+
}
105+
106+
private void checkForStatement(SubscriptionContext ctx) {
107+
ForStatement forStatement = (ForStatement) ctx.syntaxNode();
108+
List<Expression> testExpressions = forStatement.testExpressions();
109+
boolean isAsync = forStatement.asyncKeyword() != null;
110+
if (testExpressions.size() > 1) {
111+
return;
112+
}
113+
Expression expression = testExpressions.get(0);
114+
List<LocationInFile> secondaries = new ArrayList<>();
115+
if (!isAsync && !isValidIterable(expression, secondaries)) {
116+
reportIssue(ctx, expression, secondaries, message(expression, true));
117+
}
118+
}
119+
120+
abstract boolean isAsyncIterable(Expression expression);
121+
122+
abstract boolean isValidIterable(Expression expression, List<LocationInFile> secondaries);
123+
124+
abstract String message(Expression expression, boolean isForLoop);
125+
}

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

Lines changed: 19 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -19,129 +19,26 @@
1919
*/
2020
package org.sonar.python.checks;
2121

22-
import java.util.ArrayList;
2322
import java.util.List;
24-
import java.util.Objects;
2523
import org.sonar.check.Rule;
2624
import org.sonar.plugins.python.api.LocationInFile;
27-
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
28-
import org.sonar.plugins.python.api.SubscriptionContext;
2925
import org.sonar.plugins.python.api.symbols.ClassSymbol;
3026
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
3127
import org.sonar.plugins.python.api.symbols.Symbol;
32-
import org.sonar.plugins.python.api.tree.AssignmentStatement;
3328
import org.sonar.plugins.python.api.tree.CallExpression;
34-
import org.sonar.plugins.python.api.tree.ComprehensionFor;
3529
import org.sonar.plugins.python.api.tree.Expression;
36-
import org.sonar.plugins.python.api.tree.ExpressionList;
37-
import org.sonar.plugins.python.api.tree.ForStatement;
3830
import org.sonar.plugins.python.api.tree.HasSymbol;
3931
import org.sonar.plugins.python.api.tree.Tree;
40-
import org.sonar.plugins.python.api.tree.UnpackingExpression;
41-
import org.sonar.plugins.python.api.tree.YieldExpression;
42-
import org.sonar.plugins.python.api.tree.YieldStatement;
4332
import org.sonar.plugins.python.api.types.InferredType;
44-
import org.sonar.python.api.PythonPunctuator;
4533
import org.sonar.python.semantic.ClassSymbolImpl;
4634
import org.sonar.python.types.InferredTypes;
4735

4836
@Rule(key = "S3862")
49-
public class IterationOnNonIterableCheck extends PythonSubscriptionCheck {
37+
public class IterationOnNonIterableCheck extends IterationOnNonIterable {
5038

5139
private static final String MESSAGE = "Replace this expression with an iterable object.";
5240

53-
@Override
54-
public void initialize(Context context) {
55-
context.registerSyntaxNodeConsumer(Tree.Kind.UNPACKING_EXPR, IterationOnNonIterableCheck::checkUnpackingExpression);
56-
context.registerSyntaxNodeConsumer(Tree.Kind.ASSIGNMENT_STMT, IterationOnNonIterableCheck::checkAssignment);
57-
context.registerSyntaxNodeConsumer(Tree.Kind.FOR_STMT, IterationOnNonIterableCheck::checkForStatement);
58-
context.registerSyntaxNodeConsumer(Tree.Kind.COMP_FOR, IterationOnNonIterableCheck::checkForComprehension);
59-
context.registerSyntaxNodeConsumer(Tree.Kind.YIELD_STMT, IterationOnNonIterableCheck::checkYieldStatement);
60-
}
61-
62-
private static void checkAssignment(SubscriptionContext ctx) {
63-
AssignmentStatement assignmentStatement = (AssignmentStatement) ctx.syntaxNode();
64-
ExpressionList expressionList = assignmentStatement.lhsExpressions().get(0);
65-
List<LocationInFile> secondaries = new ArrayList<>();
66-
if (isLhsIterable(expressionList) && !isValidIterable(assignmentStatement.assignedValue(), secondaries)) {
67-
reportIssue(ctx, assignmentStatement.assignedValue(), secondaries, MESSAGE);
68-
}
69-
}
70-
71-
private static boolean isLhsIterable(ExpressionList expressionList) {
72-
if (expressionList.expressions().size() > 1) {
73-
return true;
74-
}
75-
Expression expression = expressionList.expressions().get(0);
76-
return expression.is(Tree.Kind.LIST_LITERAL) || expression.is(Tree.Kind.TUPLE);
77-
}
78-
79-
private static void checkForComprehension(SubscriptionContext ctx) {
80-
ComprehensionFor comprehensionFor = (ComprehensionFor) ctx.syntaxNode();
81-
Expression expression = comprehensionFor.iterable();
82-
List<LocationInFile> secondaries = new ArrayList<>();
83-
if (!isValidIterable(expression, secondaries)) {
84-
reportIssue(ctx, expression, secondaries, MESSAGE);
85-
}
86-
}
87-
88-
private static void reportIssue(SubscriptionContext ctx, Expression expression, List<LocationInFile> secondaries, String message) {
89-
PreciseIssue preciseIssue = ctx.addIssue(expression, message);
90-
secondaries.stream().filter(Objects::nonNull).forEach(location -> preciseIssue.secondary(location, null));
91-
}
92-
93-
private static void checkYieldStatement(SubscriptionContext ctx) {
94-
YieldStatement yieldStatement = (YieldStatement) ctx.syntaxNode();
95-
YieldExpression yieldExpression = yieldStatement.yieldExpression();
96-
if (yieldExpression.fromKeyword() == null) {
97-
return;
98-
}
99-
Expression expression = yieldExpression.expressions().get(0);
100-
List<LocationInFile> secondaries = new ArrayList<>();
101-
if (!isValidIterable(expression, secondaries)) {
102-
reportIssue(ctx, expression, secondaries, MESSAGE);
103-
}
104-
}
105-
106-
private static void checkUnpackingExpression(SubscriptionContext ctx) {
107-
UnpackingExpression unpackingExpression = (UnpackingExpression) ctx.syntaxNode();
108-
if (unpackingExpression.starToken().type().equals(PythonPunctuator.MUL_MUL)) {
109-
return;
110-
}
111-
Expression expression = unpackingExpression.expression();
112-
List<LocationInFile> secondaries = new ArrayList<>();
113-
if (!isValidIterable(expression, secondaries)) {
114-
reportIssue(ctx, expression, secondaries, MESSAGE);
115-
}
116-
}
117-
118-
private static void checkForStatement(SubscriptionContext ctx) {
119-
ForStatement forStatement = (ForStatement) ctx.syntaxNode();
120-
List<Expression> testExpressions = forStatement.testExpressions();
121-
boolean isAsync = forStatement.asyncKeyword() != null;
122-
if (testExpressions.size() > 1) {
123-
return;
124-
}
125-
Expression expression = testExpressions.get(0);
126-
List<LocationInFile> secondaries = new ArrayList<>();
127-
if (!isAsync && !isValidIterable(expression, secondaries)) {
128-
String message = isAsyncIterable(expression) ? "Add \"async\" before \"for\"; This expression is an async generator." : MESSAGE;
129-
reportIssue(ctx, expression, secondaries, message);
130-
}
131-
}
132-
133-
private static boolean isAsyncIterable(Expression expression) {
134-
if (expression.is(Tree.Kind.CALL_EXPR)) {
135-
CallExpression callExpression = (CallExpression) expression;
136-
Symbol calleeSymbol = callExpression.calleeSymbol();
137-
if (calleeSymbol != null && calleeSymbol.is(Symbol.Kind.FUNCTION)) {
138-
return ((FunctionSymbol) calleeSymbol).isAsynchronous();
139-
}
140-
}
141-
return expression.type().canHaveMember("__aiter__");
142-
}
143-
144-
private static boolean isValidIterable(Expression expression, List<LocationInFile> secondaries) {
41+
boolean isValidIterable(Expression expression, List<LocationInFile> secondaries) {
14542
if (expression.is(Tree.Kind.CALL_EXPR)) {
14643
CallExpression callExpression = (CallExpression) expression;
14744
Symbol calleeSymbol = callExpression.calleeSymbol();
@@ -170,4 +67,21 @@ private static boolean isValidIterable(Expression expression, List<LocationInFil
17067
secondaries.add(InferredTypes.typeClassLocation(type));
17168
return type.canHaveMember("__iter__") || type.canHaveMember("__getitem__");
17269
}
70+
71+
@Override
72+
boolean isAsyncIterable(Expression expression) {
73+
if (expression.is(Tree.Kind.CALL_EXPR)) {
74+
CallExpression callExpression = (CallExpression) expression;
75+
Symbol calleeSymbol = callExpression.calleeSymbol();
76+
if (calleeSymbol != null && calleeSymbol.is(Symbol.Kind.FUNCTION)) {
77+
return ((FunctionSymbol) calleeSymbol).isAsynchronous();
78+
}
79+
}
80+
return expression.type().canHaveMember("__aiter__");
81+
}
82+
83+
@Override
84+
String message(Expression expression, boolean isForLoop) {
85+
return isForLoop && isAsyncIterable(expression) ? "Add \"async\" before \"for\"; This expression is an async generator." : MESSAGE;
86+
}
17387
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ public void item_operations() {
5454
assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/itemOperationsTypeCheck/itemOperations_setitem.py");
5555
}
5656

57+
@Test
58+
public void iteration_on_non_iterable() {
59+
PythonCheckVerifier.verify("src/test/resources/checks/confusingTypeChecking/iterationOnNonIterable.py", check);
60+
assertNoIssuesInCorrespondingBugRule("src/test/resources/checks/iterationOnNonIterable.py");
61+
}
62+
5763
private void assertNoIssuesInCorrespondingBugRule(String path) {
5864
PythonVisitorContext context = TestPythonVisitorRunner.createContext(new File(path));
5965
SubscriptionVisitor.analyze(Collections.singletonList(check), context);

0 commit comments

Comments
 (0)