Skip to content

Commit 17c0595

Browse files
anton-haubner-sonarsourceanton-haubner-sonarsource
authored
SONARPY-1406 (+ SONARPY-1402): Addressing FNs and FPs on Peach (#1524)
* Reduce FPs by excluding Protocols * Fix FNs by more closely inspecting `return self` statements In iterators, `self` is often used as the return value of `__iter__`, see https://docs.python.org/3/library/stdtypes.html#iterator.__iter__ Furthermore, iterator implementations tend to implement the incrementation method as `next()` instead of `__next__`, see ruling tests. Hence, we were missing many invalid iterator implementations in the ruling tests, and also in peach. * Reduce FPs by handling methods annotated with NoReturn and Never Functions annotated with NoReturn or Never can be expected to raise exceptions and hence we should treat calls to them like `raise` statements.
1 parent ccbab7b commit 17c0595

File tree

14 files changed

+568
-60
lines changed

14 files changed

+568
-60
lines changed

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,32 @@
66
130,
77
146
88
],
9+
"project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_set.py": [
10+
1619
11+
],
912
"project:tensorflow/python/types/distribute.py": [
1013
28,
1114
60
1215
],
13-
"project:twisted-12.1.0/twisted/application/service.py": [
14-
223
16+
"project:twisted-12.1.0/twisted/conch/ssh/filetransfer.py": [
17+
814
18+
],
19+
"project:twisted-12.1.0/twisted/conch/unix.py": [
20+
439
21+
],
22+
"project:twisted-12.1.0/twisted/mail/pop3.py": [
23+
128
1524
],
1625
"project:twisted-12.1.0/twisted/python/test/test_sendmsg.py": [
1726
149
1827
],
1928
"project:twisted-12.1.0/twisted/python/util.py": [
2029
486
2130
],
22-
"project:twisted-12.1.0/twisted/words/protocols/jabber/ijabber.py": [
23-
137
31+
"project:twisted-12.1.0/twisted/python/zipstream.py": [
32+
154
33+
],
34+
"project:twisted-12.1.0/twisted/test/test_compat.py": [
35+
23
2436
]
2537
}

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.List;
2323
import javax.annotation.CheckForNull;
2424
import javax.annotation.Nullable;
25+
import org.sonar.plugins.python.api.symbols.Symbol;
2526
import org.sonar.plugins.python.api.tree.ArgList;
2627
import org.sonar.plugins.python.api.tree.Argument;
2728
import org.sonar.plugins.python.api.tree.CallExpression;
@@ -31,6 +32,8 @@
3132
import org.sonar.plugins.python.api.tree.FunctionDef;
3233
import org.sonar.plugins.python.api.tree.ListLiteral;
3334
import org.sonar.plugins.python.api.tree.Name;
35+
import org.sonar.plugins.python.api.tree.Parameter;
36+
import org.sonar.plugins.python.api.tree.ParameterList;
3437
import org.sonar.plugins.python.api.tree.SetLiteral;
3538
import org.sonar.plugins.python.api.tree.Tree;
3639
import org.sonar.plugins.python.api.tree.Tuple;
@@ -41,6 +44,7 @@
4144

4245
import static org.sonar.plugins.python.api.tree.Tree.Kind.GENERATOR_EXPR;
4346
import static org.sonar.plugins.python.api.tree.Tree.Kind.LAMBDA;
47+
import static org.sonar.plugins.python.api.tree.Tree.Kind.NAME;
4448
import static org.sonar.plugins.python.api.tree.Tree.Kind.NONE;
4549
import static org.sonar.plugins.python.api.tree.Tree.Kind.NUMERIC_LITERAL;
4650
import static org.sonar.plugins.python.api.tree.Tree.Kind.STRING_LITERAL;
@@ -154,6 +158,21 @@ public static boolean isNone(InferredType type) {
154158
return type.canOnlyBe(BuiltinTypes.NONE_TYPE);
155159
}
156160

161+
private static final List<String> PROTOCOL_LIKE_BASE_TYPES = List.of("typing.Protocol", "zope.interface.Interface");
162+
163+
/**
164+
* Determines whether the given class must be a <a href="https://docs.python.org/3/library/typing.html#typing.Protocol">Protocol</a>
165+
* or a similar Protocol-like definition (e.g. {@code zope.interface.Interface}).
166+
*/
167+
public static boolean mustBeAProtocolLike(ClassDef classDef) {
168+
var classSymbol = TreeUtils.getClassSymbolFromDef(classDef);
169+
if (classSymbol != null) {
170+
return PROTOCOL_LIKE_BASE_TYPES.stream().anyMatch(classSymbol::isOrExtends);
171+
}
172+
173+
return false;
174+
}
175+
157176
private static final List<String> ABC_ABSTRACTMETHOD_DECORATORS = List.of("abstractmethod", "abc.abstractmethod");
158177

159178
public static boolean isAbstract(FunctionDef funDef) {
@@ -163,4 +182,35 @@ public static boolean isAbstract(FunctionDef funDef) {
163182
.map(decorator -> TreeUtils.decoratorNameFromExpression(decorator.expression()))
164183
.anyMatch(foundDeco -> ABC_ABSTRACTMETHOD_DECORATORS.stream().anyMatch(abcDeco -> abcDeco.equals(foundDeco)));
165184
}
185+
186+
/**
187+
* Simple check whether the given expression is the "self" name expression.
188+
*
189+
* Carefully check the context when relying on this method!
190+
* This implementation does not ensure that the name is actually referring to a method parameter or whether the surrounding method might
191+
* be static, etc.
192+
*/
193+
public static boolean isSelf(Expression expression) {
194+
// TODO: Instead of performing a manual string comparison, maybe check the symbol instead for being a SelfSymbolImpl symbol
195+
// (This might require exposing some more information about the symbol kind being a "Self"-symbol)
196+
return expression.is(NAME) && "self".equals(((Name) expression).name());
197+
}
198+
199+
@CheckForNull
200+
public static Symbol findFirstParameterSymbol(FunctionDef functionDef) {
201+
ParameterList parameters = functionDef.parameters();
202+
if (parameters == null) {
203+
return null;
204+
}
205+
List<Parameter> params = parameters.nonTuple();
206+
if (params.isEmpty()) {
207+
return null;
208+
}
209+
Name firstParameterName = params.get(0).name();
210+
if (firstParameterName == null) {
211+
return null;
212+
}
213+
214+
return firstParameterName.symbol();
215+
}
166216
}

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

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,12 @@
3030
import javax.annotation.CheckForNull;
3131
import javax.annotation.Nullable;
3232
import org.sonar.check.Rule;
33+
import org.sonar.plugins.python.api.PythonFile;
3334
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
3435
import org.sonar.plugins.python.api.cfg.CfgBlock;
3536
import org.sonar.plugins.python.api.cfg.ControlFlowGraph;
37+
import org.sonar.plugins.python.api.symbols.Symbol;
38+
import org.sonar.plugins.python.api.symbols.Usage;
3639
import org.sonar.plugins.python.api.tree.AssignmentStatement;
3740
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
3841
import org.sonar.plugins.python.api.tree.BinaryExpression;
@@ -41,21 +44,14 @@
4144
import org.sonar.plugins.python.api.tree.ComprehensionExpression;
4245
import org.sonar.plugins.python.api.tree.ComprehensionIf;
4346
import org.sonar.plugins.python.api.tree.ConditionalExpression;
44-
import org.sonar.plugins.python.api.tree.Decorator;
45-
import org.sonar.plugins.python.api.tree.DottedName;
4647
import org.sonar.plugins.python.api.tree.Expression;
4748
import org.sonar.plugins.python.api.tree.ExpressionList;
4849
import org.sonar.plugins.python.api.tree.FunctionDef;
4950
import org.sonar.plugins.python.api.tree.LambdaExpression;
5051
import org.sonar.plugins.python.api.tree.Name;
51-
import org.sonar.plugins.python.api.tree.Parameter;
52-
import org.sonar.plugins.python.api.tree.ParameterList;
5352
import org.sonar.plugins.python.api.tree.QualifiedExpression;
5453
import org.sonar.plugins.python.api.tree.Tree;
55-
import org.sonar.plugins.python.api.PythonFile;
5654
import org.sonar.python.api.PythonKeyword;
57-
import org.sonar.plugins.python.api.symbols.Symbol;
58-
import org.sonar.plugins.python.api.symbols.Usage;
5955
import org.sonar.python.tree.DictCompExpressionImpl;
6056
import org.sonar.python.tree.TreeUtils;
6157

@@ -141,7 +137,7 @@ private RecursiveCallCollector(FunctionDef currentFunction, Symbol functionSymbo
141137
selfSymbol = null;
142138
className = findParentClassName(currentFunction);
143139
} else {
144-
selfSymbol = findSelfParameterSymbol(currentFunction);
140+
selfSymbol = CheckUtils.findFirstParameterSymbol(currentFunction);
145141
className = null;
146142
}
147143
} else {
@@ -259,20 +255,6 @@ private boolean matchesLookupClassName(Expression expression) {
259255
return className.equals(((Name) expression).name());
260256
}
261257

262-
@CheckForNull
263-
private static Symbol findSelfParameterSymbol(FunctionDef functionDef) {
264-
ParameterList parameters = functionDef.parameters();
265-
if (parameters == null) {
266-
return null;
267-
}
268-
List<Parameter> params = parameters.nonTuple();
269-
if (params.isEmpty()) {
270-
return null;
271-
}
272-
Name firstParameterName = params.get(0).name();
273-
return firstParameterName != null ? firstParameterName.symbol() : null;
274-
}
275-
276258
@CheckForNull
277259
private static String findParentClassName(FunctionDef functionDef) {
278260
ClassDef parentClass = CheckUtils.getParentClassDef(functionDef);

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

Lines changed: 58 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,14 @@
2323
import org.sonar.check.Rule;
2424
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2525
import org.sonar.plugins.python.api.SubscriptionContext;
26+
import org.sonar.plugins.python.api.tree.ClassDef;
2627
import org.sonar.plugins.python.api.tree.Expression;
2728
import org.sonar.plugins.python.api.tree.FunctionDef;
29+
import org.sonar.plugins.python.api.tree.Name;
2830
import org.sonar.plugins.python.api.tree.ReturnStatement;
2931
import org.sonar.plugins.python.api.tree.Tree;
3032
import org.sonar.plugins.python.api.types.InferredType;
33+
import org.sonar.python.tree.TreeUtils;
3134

3235
@Rule(key = "S2876")
3336
public class IterMethodReturnTypeCheck extends PythonSubscriptionCheck {
@@ -36,16 +39,20 @@ public class IterMethodReturnTypeCheck extends PythonSubscriptionCheck {
3639
+ " Consider explicitly raising a NotImplementedError if this class is not (yet) meant to support this method.";
3740
private static final String COROUTINE_METHOD_MESSAGE = INVALID_RETURN_VALUE_MESSAGE + " The method can not be a coroutine and have the `async` keyword.";
3841

42+
private static final List<String> REQUIRED_ITERATOR_METHODS = List.of("__iter__", "__next__");
43+
3944
@Override
4045
public void initialize(Context context) {
41-
context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, ctx -> checkFunctionDefinition(ctx, (FunctionDef) ctx.syntaxNode()));
46+
context.registerSyntaxNodeConsumer(Tree.Kind.CLASSDEF, ctx -> checkClassDefinition(ctx, (ClassDef) ctx.syntaxNode()));
4247
}
4348

44-
private static void checkFunctionDefinition(SubscriptionContext ctx, FunctionDef funDef) {
45-
if (!funDef.isMethodDefinition()) {
46-
return;
49+
private static void checkClassDefinition(SubscriptionContext ctx, ClassDef classDef) {
50+
for (var methodDef : TreeUtils.topLevelFunctionDefs(classDef)) {
51+
checkFunctionDefinition(ctx, classDef, methodDef, CheckUtils.mustBeAProtocolLike(classDef));
4752
}
53+
}
4854

55+
private static void checkFunctionDefinition(SubscriptionContext ctx, ClassDef classDef, FunctionDef funDef, boolean classIsProtocolLike) {
4956
String funNameString = funDef.name().name();
5057
if (!"__iter__".equals(funNameString)) {
5158
return;
@@ -64,30 +71,71 @@ private static void checkFunctionDefinition(SubscriptionContext ctx, FunctionDef
6471
// If there are no return statements, we trigger this rule since this effectively means that the method is returning `None`.
6572
// However, there are exceptions to this:
6673
// * if the method raises exceptions, then it likely does not return a value on purpose
67-
// * if the method is marked as abstract, then it is likely not implemented on purpose
74+
// * if the method is marked as abstract or the surrounding class is a Protocol, then it is likely not implemented on purpose
6875
if (returnStmts.isEmpty() &&
6976
!returnStmtCollector.raisesExceptions() &&
70-
!CheckUtils.isAbstract(funDef)) {
77+
!CheckUtils.isAbstract(funDef) &&
78+
!classIsProtocolLike) {
7179
ctx.addIssue(funDef.defKeyword(), funDef.colon(), NO_RETURN_STMTS_MESSAGE);
7280
return;
7381
}
7482

7583
for (ReturnStatement returnStmt : returnStmts) {
76-
checkReturnStmt(ctx, returnStmt);
84+
checkReturnStmt(ctx, classDef, funDef, returnStmt);
7785
}
7886
}
7987

80-
private static void checkReturnStmt(SubscriptionContext ctx, ReturnStatement returnStmt) {
88+
private static void checkReturnStmt(SubscriptionContext ctx, ClassDef classDef, FunctionDef functionDef, ReturnStatement returnStmt) {
8189
List<Expression> returnedExpressions = returnStmt.expressions();
8290
if (returnedExpressions.isEmpty()) {
8391
ctx.addIssue(returnStmt.returnKeyword(), INVALID_RETURN_VALUE_MESSAGE);
8492
return;
8593
}
8694

95+
// The type analysis will report ANY for the type of "self".
96+
// This is technically the case, but in practice, self should almost always be an instance of the surrounding class if the associated
97+
// symbol is the same as the first parameter of the method.
98+
// Hence, to avoid FNs for methods returning self, we should report an issue if the surrounding class does not support the iterator
99+
// protocol.
100+
// Especially because it is quite common that iterator objects return self in __iter__,
101+
// see https://docs.python.org/3/library/stdtypes.html#iterator.__iter__
102+
if (returnsJustSelf(functionDef, returnedExpressions)) {
103+
var classSymbol = TreeUtils.getClassSymbolFromDef(classDef);
104+
if (classSymbol != null) {
105+
if (REQUIRED_ITERATOR_METHODS.stream().anyMatch(method -> !classSymbol.canHaveMember(method))) {
106+
ReturnCheckUtils.addIssueOnReturnedExpressions(ctx, returnStmt, INVALID_RETURN_VALUE_MESSAGE);
107+
}
108+
return;
109+
}
110+
}
111+
112+
// if the returned expression is not `self`, we just rely on the type analysis.
87113
InferredType returnStmtType = returnStmt.returnValueType();
88-
if (!returnStmtType.canHaveMember("__iter__") ||
89-
!returnStmtType.canHaveMember("__next__")) {
114+
if (REQUIRED_ITERATOR_METHODS.stream().anyMatch(method -> !returnStmtType.canHaveMember(method))) {
90115
ReturnCheckUtils.addIssueOnReturnedExpressions(ctx, returnStmt, INVALID_RETURN_VALUE_MESSAGE);
91116
}
92117
}
118+
119+
private static boolean returnsJustSelf(FunctionDef funDef, List<Expression> returnedExpressions) {
120+
if (returnedExpressions.size() != 1) {
121+
return false;
122+
}
123+
124+
var firstReturnedExpression = returnedExpressions.get(0);
125+
if (!CheckUtils.isSelf(firstReturnedExpression)) {
126+
return false;
127+
}
128+
129+
var returnedSelfSymbol = ((Name) firstReturnedExpression).symbol();
130+
if (returnedSelfSymbol == null) {
131+
return false;
132+
}
133+
134+
var selfParameterSymbol = CheckUtils.findFirstParameterSymbol(funDef);
135+
if (selfParameterSymbol == null) {
136+
return false;
137+
}
138+
139+
return returnedSelfSymbol == selfParameterSymbol;
140+
}
93141
}

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

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,18 @@
3333
import javax.annotation.Nullable;
3434
import org.sonar.check.Rule;
3535
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
36+
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
3637
import org.sonar.plugins.python.api.symbols.Symbol;
3738
import org.sonar.plugins.python.api.symbols.Usage;
3839
import org.sonar.plugins.python.api.tree.AssignmentStatement;
3940
import org.sonar.plugins.python.api.tree.CompoundAssignmentStatement;
4041
import org.sonar.plugins.python.api.tree.Expression;
4142
import org.sonar.plugins.python.api.tree.FunctionDef;
42-
import org.sonar.plugins.python.api.tree.Name;
4343
import org.sonar.plugins.python.api.tree.Parameter;
4444
import org.sonar.plugins.python.api.tree.QualifiedExpression;
4545
import org.sonar.plugins.python.api.tree.SubscriptionExpression;
4646
import org.sonar.plugins.python.api.tree.Tree;
4747
import org.sonar.plugins.python.api.types.BuiltinTypes;
48-
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
4948
import org.sonar.python.tree.TreeUtils;
5049

5150
import static org.sonar.plugins.python.api.tree.Tree.Kind.ASSIGNMENT_STMT;
@@ -55,7 +54,6 @@
5554
import static org.sonar.plugins.python.api.tree.Tree.Kind.DICTIONARY_LITERAL;
5655
import static org.sonar.plugins.python.api.tree.Tree.Kind.FUNCDEF;
5756
import static org.sonar.plugins.python.api.tree.Tree.Kind.LIST_LITERAL;
58-
import static org.sonar.plugins.python.api.tree.Tree.Kind.NAME;
5957
import static org.sonar.plugins.python.api.tree.Tree.Kind.QUALIFIED_EXPR;
6058
import static org.sonar.plugins.python.api.tree.Tree.Kind.SET_LITERAL;
6159
import static org.sonar.plugins.python.api.tree.Tree.Kind.SUBSCRIPTION;
@@ -75,7 +73,7 @@ public class ModifiedParameterValueCheck extends PythonSubscriptionCheck {
7573
private static final String CLEAR = "clear";
7674
private static final Set<String> LIST_MUTATING_METHODS = new HashSet<>(Arrays.asList("append", CLEAR, "extend", "insert", "pop", "remove", "reverse", "sort"));
7775
private static final Set<String> SET_MUTATING_METHODS = new HashSet<>(
78-
Arrays.asList("update", "intersection_update", "difference_update", "symmetric_difference_update", "add", "remove", "discard", "pop", CLEAR));
76+
Arrays.asList("update", "intersection_update", "difference_update", "symmetric_difference_update", "add", "remove", "discard", "pop", CLEAR));
7977
private static final Set<String> DICT_MUTATING_METHODS = new HashSet<>(Arrays.asList("pop", CLEAR, "popitem", "setdefault", "update"));
8078
private static final Set<String> DEQUE_MUTATING_METHODS = new HashSet<>(Arrays.asList("appendleft", "extendleft", "popleft", "rotate"));
8179
static {
@@ -139,8 +137,7 @@ public void initialize(Context context) {
139137
getQuickFix(functionDef, defaultValue, paramSymbol)
140138
.ifPresent(issue::addQuickFix);
141139
}
142-
}
143-
);
140+
});
144141
}
145142
});
146143
}
@@ -156,8 +153,7 @@ private static Optional<PythonQuickFix> getQuickFix(FunctionDef functionDef, Exp
156153
paramInit -> PythonQuickFix.newQuickFix("Initialize this parameter inside the function/method")
157154
.addTextEdit(replace(defaultValue, "None"))
158155
.addTextEdit(insertLineBefore(firstStatement, String.format("if %1$s is None:\n %1$s = %2$s", paramName, paramInit)))
159-
.build()
160-
);
156+
.build());
161157
}
162158

163159
@CheckForNull
@@ -238,25 +234,21 @@ private static boolean mightBeReferencedOutsideOfFunction(Tree tree) {
238234
}
239235
return assignment.assignedValue() == tree
240236
&& assignment.lhsExpressions().stream()
241-
.flatMap(expressionList -> expressionList.expressions().stream())
242-
.anyMatch(ModifiedParameterValueCheck::isAccessingSelf);
237+
.flatMap(expressionList -> expressionList.expressions().stream())
238+
.anyMatch(ModifiedParameterValueCheck::isAccessingSelf);
243239
}
244240

245241
private static boolean isAccessingSelf(Expression expression) {
246242
switch (expression.getKind()) {
247243
case QUALIFIED_EXPR:
248-
return isSelf(((QualifiedExpression) expression).qualifier());
244+
return CheckUtils.isSelf(((QualifiedExpression) expression).qualifier());
249245
case SUBSCRIPTION:
250-
return isSelf(((SubscriptionExpression) expression).object());
246+
return CheckUtils.isSelf(((SubscriptionExpression) expression).object());
251247
default:
252248
return false;
253249
}
254250
}
255251

256-
private static boolean isSelf(Expression expression) {
257-
return expression.is(NAME) && ((Name) expression).name().equals("self");
258-
}
259-
260252
private static List<Tree> getAttributeSet(Symbol paramSymbol) {
261253
return paramSymbol.usages().stream()
262254
.map(Usage::tree)

0 commit comments

Comments
 (0)