Skip to content

Commit 5242cb8

Browse files
andrea-guarino-sonarsourceguillaume-dequenne
andrea-guarino-sonarsource
authored and
guillaume-dequenne
committed
SONARPY-937 S5655 (ArgumentTypeCheck) should report also on incompati… (#1035)
1 parent fb4e7cf commit 5242cb8

File tree

6 files changed

+116
-38
lines changed

6 files changed

+116
-38
lines changed

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

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

22+
import java.util.Collections;
23+
import java.util.HashSet;
24+
import java.util.List;
2225
import java.util.Optional;
26+
import java.util.Set;
27+
import java.util.SortedSet;
28+
import java.util.TreeSet;
2329
import java.util.function.Predicate;
2430
import org.sonar.check.Rule;
2531
import org.sonar.plugins.python.api.LocationInFile;
2632
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
27-
import org.sonar.plugins.python.api.SubscriptionContext;
2833
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
2934
import org.sonar.plugins.python.api.symbols.Symbol;
3035
import org.sonar.plugins.python.api.tree.Argument;
@@ -43,6 +48,12 @@
4348
@Rule(key = "S5655")
4449
public class ArgumentTypeCheck extends PythonSubscriptionCheck {
4550

51+
private static class IssueToReport {
52+
SortedSet<Integer> nonCompliantArgs = new TreeSet<>();
53+
String message;
54+
final Set<LocationInFile> secondaryLocations = new HashSet<>();
55+
}
56+
4657
@Override
4758
public void initialize(Context context) {
4859
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, ctx -> {
@@ -51,46 +62,57 @@ public void initialize(Context context) {
5162
if (calleeSymbol == null) {
5263
return;
5364
}
54-
if (!calleeSymbol.is(Symbol.Kind.FUNCTION)) {
55-
// We might want to support ambiguous symbols for which every definition is a function
56-
return;
57-
}
58-
FunctionSymbol functionSymbol = (FunctionSymbol) calleeSymbol;
59-
if (functionSymbol.hasVariadicParameter()) {
60-
return;
65+
Set<Symbol> symbols = SymbolUtils.flattenAmbiguousSymbols(Collections.singleton(calleeSymbol));
66+
if (symbols.stream().anyMatch(s -> !s.is(Symbol.Kind.FUNCTION))) return;
67+
IssueToReport issue = new IssueToReport();
68+
if (symbols.stream().allMatch(symbol -> isNonCompliantFunctionCall(issue, ((FunctionSymbol) symbol), callExpression))) {
69+
List<Argument> args = callExpression.arguments();
70+
Integer firstArgIndex = issue.nonCompliantArgs.first();
71+
PreciseIssue preciseIssue = ctx.addIssue(args.get(firstArgIndex), issue.message);
72+
issue.nonCompliantArgs.forEach(index -> { if(!index.equals(firstArgIndex)) preciseIssue.secondary(args.get(index), issue.message); });
73+
issue.secondaryLocations.forEach(locationInFile -> preciseIssue.secondary(locationInFile, "Function definition"));
6174
}
62-
boolean isStaticCall = callExpression.callee().is(Tree.Kind.NAME) || Optional.of(callExpression.callee())
63-
.filter(c -> c.is(Tree.Kind.QUALIFIED_EXPR))
64-
.flatMap(q -> TreeUtils.getSymbolFromTree(((QualifiedExpression) q).qualifier()).filter(s -> s.is(Symbol.Kind.CLASS)))
65-
.isPresent();
66-
checkFunctionCall(ctx, callExpression, functionSymbol, isStaticCall);
6775
});
6876
}
6977

70-
private static void checkFunctionCall(SubscriptionContext ctx, CallExpression callExpression, FunctionSymbol functionSymbol, boolean isStaticCall) {
78+
private static boolean isNonCompliantFunctionCall(IssueToReport issue, FunctionSymbol functionSymbol, CallExpression callExpression) {
79+
if (functionSymbol.hasVariadicParameter()) {
80+
return false;
81+
}
82+
boolean isStaticCall = callExpression.callee().is(Tree.Kind.NAME) || Optional.of(callExpression.callee())
83+
.filter(c -> c.is(Tree.Kind.QUALIFIED_EXPR))
84+
.flatMap(q -> TreeUtils.getSymbolFromTree(((QualifiedExpression) q).qualifier()).filter(s -> s.is(Symbol.Kind.CLASS)))
85+
.isPresent();
86+
return checkFunctionCall(issue, callExpression, functionSymbol, isStaticCall);
87+
}
88+
89+
private static boolean checkFunctionCall(IssueToReport issue, CallExpression callExpression, FunctionSymbol functionSymbol, boolean isStaticCall) {
7190

7291
boolean isKeyword = false;
7392
int firstParameterOffset = SymbolUtils.firstParameterOffset(functionSymbol, isStaticCall);
7493
if (firstParameterOffset < 0) {
75-
return;
94+
return false;
7695
}
96+
boolean hasIncompatibleArgumentType = false;
7797
for (int i = 0; i < callExpression.arguments().size(); i++) {
7898
Argument argument = callExpression.arguments().get(i);
7999
int parameterIndex = i + firstParameterOffset;
80100
if (parameterIndex >= functionSymbol.parameters().size()) {
81101
// S930 will raise the issue
82-
return;
102+
return false;
83103
}
84104
if (argument.is(Tree.Kind.REGULAR_ARGUMENT)) {
85105
RegularArgument regularArgument = (RegularArgument) argument;
86106
isKeyword |= regularArgument.keywordArgument() != null;
87107
boolean shouldReport = isKeyword ? shouldReportKeywordArgument(regularArgument, functionSymbol)
88108
: shouldReportPositionalArgument(regularArgument, functionSymbol, parameterIndex);
89109
if (shouldReport) {
90-
reportIssue(ctx, functionSymbol, regularArgument);
110+
hasIncompatibleArgumentType = true;
111+
reportIssue(issue, functionSymbol, i);
91112
}
92113
}
93114
}
115+
return hasIncompatibleArgumentType;
94116
}
95117

96118
private static boolean shouldReportPositionalArgument(RegularArgument regularArgument, FunctionSymbol functionSymbol, int index) {
@@ -122,11 +144,11 @@ private static boolean shouldReportKeywordArgument(RegularArgument regularArgume
122144
.orElse(false);
123145
}
124146

125-
private static void reportIssue(SubscriptionContext ctx, FunctionSymbol functionSymbol, RegularArgument regularArgument) {
126-
PreciseIssue issue = ctx.addIssue(regularArgument, String.format("Change this argument; Function \"%s\" expects a different type", functionSymbol.name()));
127-
LocationInFile locationInFile = functionSymbol.definitionLocation();
128-
if (locationInFile != null) {
129-
issue.secondary(locationInFile, "Function definition");
147+
private static void reportIssue(IssueToReport issue, FunctionSymbol functionSymbol, int argumentIndex) {
148+
issue.nonCompliantArgs.add(argumentIndex);
149+
issue.message = String.format("Change this argument; Function \"%s\" expects a different type", functionSymbol.name());
150+
if (functionSymbol.definitionLocation() != null) {
151+
issue.secondaryLocations.add(functionSymbol.definitionLocation());
130152
}
131153
}
132154

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,9 @@ public class ArgumentTypeCheckTest {
2828
public void test() {
2929
PythonCheckVerifier.verify("src/test/resources/checks/argumentType.py", new ArgumentTypeCheck());
3030
}
31+
32+
@Test
33+
public void test_overloaded_functions() {
34+
PythonCheckVerifier.verify("src/test/resources/checks/argumentType_overloaded_functions.py", new ArgumentTypeCheck());
35+
}
3136
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ def stdlib_functions():
6868
time.sleep(True) # OK, converted to 1
6969
time.sleep(1j) # FN, considered duck type compatible
7070
genericpath.isfile("some/path")
71-
genericpath.isfile(42) # FN - see SONARPY-937
71+
genericpath.isfile(42) # Noncompliant
7272
my_list = [1,2,3]
7373
_heapq.heapify(42) # Noncompliant {{Change this argument; Function "heapify" expects a different type}}
7474
# ^^
@@ -89,7 +89,7 @@ def builtin_functions():
8989
number.__add__(x = unexpected) # Noncompliant {{Change this argument; Function "__add__" expects a different type}}
9090
# ^^^^^^^^^^^^^^
9191
float.fromhex(42) # Noncompliant
92-
eval(42)
92+
eval(42) # Noncompliant
9393
"Some string literal".format(1, 2)
9494
exit(1)
9595
repr(A)
@@ -207,4 +207,3 @@ def another_function(param: ChildB):
207207

208208
a_function(Parent()) # OK, still duck type compatible with ChildA
209209
another_function(Parent()) # Noncompliant
210-

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
from mod import predicate
2+
3+
class A:
4+
def make_a(self): ...
5+
6+
class B:
7+
def make_b(self): ...
8+
9+
if predicate():
10+
def overloaded_foo(param1: A, param2: int) : ...
11+
# ^^^^^^^^^^^^^^>
12+
else:
13+
def overloaded_foo(param1: B, param2: int): ...
14+
# ^^^^^^^^^^^^^^>
15+
16+
overloaded_foo(42, 42) # Noncompliant {{Change this argument; Function "overloaded_foo" expects a different type}}
17+
# ^^
18+
19+
20+
if predicate():
21+
def overloaded_bar(param1: A, param2: A): ...
22+
# ^^^^^^^^^^^^^^>
23+
else:
24+
def overloaded_bar(param1: B, param2: B): ...
25+
# ^^^^^^^^^^^^^^>
26+
27+
overloaded_bar(42, 42) # Noncompliant
28+
# ^^ ^^<
29+
30+
31+
if predicate():
32+
def overloaded_baz(param1: A): ...
33+
else:
34+
def overloaded_baz(param1: B): ...
35+
36+
overloaded_baz(A()) # OK
37+
38+
39+
if predicate():
40+
def overloaded_fn(param1: A, param2: A): ...
41+
# ^^^^^^^^^^^^^>
42+
else:
43+
def overloaded_fn(param1: B, param2: B): ...
44+
# ^^^^^^^^^^^^^>
45+
46+
overloaded_fn(A(), B()) # Noncompliant
47+
# ^^^ ^^^<

‎python-frontend/src/main/java/org/sonar/python/semantic/AmbiguousSymbolImpl.java

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
3030
import org.sonar.plugins.python.api.symbols.Symbol;
3131

32+
import static org.sonar.python.semantic.SymbolUtils.flattenAmbiguousSymbols;
33+
3234
public class AmbiguousSymbolImpl extends SymbolImpl implements AmbiguousSymbol {
3335

3436
private final Set<Symbol> symbols;
@@ -58,19 +60,6 @@ public static AmbiguousSymbol create(Set<Symbol> symbols) {
5860
return new AmbiguousSymbolImpl(resultingSymbolName, firstSymbol.fullyQualifiedName(), flattenAmbiguousSymbols(symbols));
5961
}
6062

61-
private static Set<Symbol> flattenAmbiguousSymbols(Set<Symbol> symbols) {
62-
Set<Symbol> alternatives = new HashSet<>();
63-
for (Symbol symbol : symbols) {
64-
if (symbol.is(Kind.AMBIGUOUS)) {
65-
Set<Symbol> flattenedAlternatives = flattenAmbiguousSymbols(((AmbiguousSymbol) symbol).alternatives());
66-
alternatives.addAll(flattenedAlternatives);
67-
} else {
68-
alternatives.add(symbol);
69-
}
70-
}
71-
return alternatives;
72-
}
73-
7463
public static AmbiguousSymbol create(Symbol... symbols) {
7564
return create(new HashSet<>(Arrays.asList(symbols)));
7665
}

‎python-frontend/src/main/java/org/sonar/python/semantic/SymbolUtils.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,16 @@
2727
import java.util.ArrayDeque;
2828
import java.util.ArrayList;
2929
import java.util.Deque;
30+
import java.util.HashSet;
3031
import java.util.List;
3132
import java.util.Map;
3233
import java.util.Optional;
34+
import java.util.Set;
3335
import java.util.stream.Collectors;
3436
import javax.annotation.CheckForNull;
3537
import javax.annotation.Nullable;
3638
import org.sonar.plugins.python.api.PythonFile;
39+
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
3740
import org.sonar.plugins.python.api.symbols.ClassSymbol;
3841
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
3942
import org.sonar.plugins.python.api.symbols.Symbol;
@@ -288,4 +291,17 @@ public static Symbol typeshedSymbolWithFQN(String fullyQualifiedName) {
288291
Symbol symbol = TypeShed.symbolWithFQN(fullyQualifiedName);
289292
return symbol == null ? new SymbolImpl(localName, fullyQualifiedName) : ((SymbolImpl) symbol).copyWithoutUsages();
290293
}
294+
295+
public static Set<Symbol> flattenAmbiguousSymbols(Set<Symbol> symbols) {
296+
Set<Symbol> alternatives = new HashSet<>();
297+
for (Symbol symbol : symbols) {
298+
if (symbol.is(Symbol.Kind.AMBIGUOUS)) {
299+
Set<Symbol> flattenedAlternatives = flattenAmbiguousSymbols(((AmbiguousSymbol) symbol).alternatives());
300+
alternatives.addAll(flattenedAlternatives);
301+
} else {
302+
alternatives.add(symbol);
303+
}
304+
}
305+
return alternatives;
306+
}
291307
}

0 commit comments

Comments
 (0)