Skip to content

Commit aeabc27

Browse files
pynicolaspynicolas
authored
SONARPY-626 Fix handling of keyword only args in S930 (SonarSource#638)
* SONARPY-626 Fix handling of keyword only args in S930 * SONARPY-626 Fix FP on method defined in typeshed
1 parent 043bb29 commit aeabc27

3 files changed

Lines changed: 100 additions & 31 deletions

File tree

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

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

22+
import java.util.HashSet;
2223
import java.util.List;
2324
import java.util.Set;
2425
import java.util.stream.Collectors;
@@ -45,16 +46,6 @@ public class ArgumentNumberCheck extends PythonSubscriptionCheck {
4546

4647
private static final String FUNCTION_DEFINITION = "Function definition.";
4748

48-
private static String message(String functionName, long minRequiredPositionalArguments, int nArguments, long nPositionalParamWithDefaultValue) {
49-
String message = "";
50-
if (minRequiredPositionalArguments > nArguments) {
51-
message = "Add " + (minRequiredPositionalArguments - nArguments) + " missing arguments; ";
52-
} else {
53-
message = "Remove " + (nArguments - minRequiredPositionalArguments - nPositionalParamWithDefaultValue) + " unexpected arguments; ";
54-
}
55-
return message + "'" + functionName + "' expects " + minRequiredPositionalArguments + " positional arguments.";
56-
}
57-
5849
@Override
5950
public void initialize(Context context) {
6051
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, ctx -> {
@@ -65,29 +56,77 @@ public void initialize(Context context) {
6556
if (isException(callExpression, functionSymbol)) {
6657
return;
6758
}
68-
int nArguments = callExpression.arguments().size();
69-
int self = 0;
70-
if (functionSymbol.isInstanceMethod() && callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR)) {
71-
self = 1;
72-
}
73-
long minRequiredPositionalArguments = functionSymbol.parameters().stream()
74-
.filter(parameterName -> !parameterName.isKeywordOnly() && !parameterName.hasDefaultValue()).count();
75-
if ((nArguments + self < minRequiredPositionalArguments || nArguments + self > functionSymbol.parameters().size())) {
76-
PreciseIssue preciseIssue = ctx.addIssue(callExpression.callee(),
77-
message(functionSymbol.name(), minRequiredPositionalArguments - self, nArguments, functionSymbol.parameters().size() - minRequiredPositionalArguments));
78-
addSecondary(functionSymbol, preciseIssue);
79-
}
80-
59+
checkPositionalParameters(ctx, callExpression, functionSymbol);
8160
checkKeywordArguments(ctx, callExpression, functionSymbol, callExpression.callee());
8261
}
8362

8463
});
8564
}
8665

66+
private static void checkPositionalParameters(SubscriptionContext ctx, CallExpression callExpression, FunctionSymbol functionSymbol) {
67+
int self = 0;
68+
if (functionSymbol.isInstanceMethod() && callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR)) {
69+
self = 1;
70+
}
71+
Set<String> positionalParamsWithoutDefault = positionalParamsWithoutDefault(functionSymbol);
72+
long nbPositionalParamsWithDefault = functionSymbol.parameters().stream()
73+
.filter(parameterName -> !parameterName.isKeywordOnly() && parameterName.hasDefaultValue())
74+
.count();
75+
76+
List<RegularArgument> arguments = callExpression.arguments().stream()
77+
.map(RegularArgument.class::cast)
78+
.collect(Collectors.toList());
79+
long nbPositionalArgs = arguments.stream().filter(a -> a.keywordArgument() == null).count();
80+
long nbNonKeywordOnlyPassedWithKeyword = arguments.stream()
81+
.map(RegularArgument::keywordArgument)
82+
.filter(k -> k != null && positionalParamsWithoutDefault.contains(k.name()))
83+
.count();
84+
85+
int minimumPositionalArgs = positionalParamsWithoutDefault.size();
86+
String expected = "" + (minimumPositionalArgs - self);
87+
long nbMissingArgs = minimumPositionalArgs - nbPositionalArgs - self - nbNonKeywordOnlyPassedWithKeyword;
88+
if (nbMissingArgs > 0) {
89+
String message = "Add " + nbMissingArgs + " missing arguments; ";
90+
if (nbPositionalParamsWithDefault > 0) {
91+
expected = "at least " + expected;
92+
}
93+
addPositionalIssue(ctx, callExpression.callee(), functionSymbol, message, expected);
94+
} else if (nbMissingArgs + nbPositionalParamsWithDefault < 0) {
95+
String message = "Remove " + (- nbMissingArgs - nbPositionalParamsWithDefault) + " unexpected arguments; ";
96+
if (nbPositionalParamsWithDefault > 0) {
97+
expected = "at most " + (minimumPositionalArgs - self + nbPositionalParamsWithDefault);
98+
}
99+
addPositionalIssue(ctx, callExpression.callee(), functionSymbol, message, expected);
100+
}
101+
}
102+
103+
private static Set<String> positionalParamsWithoutDefault(FunctionSymbol functionSymbol) {
104+
int unnamedIndex = 0;
105+
Set<String> result = new HashSet<>();
106+
for (FunctionSymbol.Parameter parameter : functionSymbol.parameters()) {
107+
if (!parameter.isKeywordOnly() && !parameter.hasDefaultValue()) {
108+
String name = parameter.name();
109+
if (name == null) {
110+
result.add("!unnamed" + unnamedIndex);
111+
unnamedIndex++;
112+
} else {
113+
result.add(name);
114+
}
115+
}
116+
}
117+
return result;
118+
}
119+
120+
private static void addPositionalIssue(SubscriptionContext ctx, Tree tree, FunctionSymbol functionSymbol, String message, String expected) {
121+
String msg = message + "'" + functionSymbol.name() + "' expects " + expected + " positional arguments.";
122+
PreciseIssue preciseIssue = ctx.addIssue(tree, msg);
123+
addSecondary(functionSymbol, preciseIssue);
124+
}
125+
87126
private static boolean isReceiverClassSymbol(QualifiedExpression qualifiedExpression) {
88127
return TreeUtils.getSymbolFromTree(qualifiedExpression.qualifier())
89-
.filter(symbol -> symbol.kind() == Symbol.Kind.CLASS)
90-
.isPresent();
128+
.filter(symbol -> symbol.kind() == Symbol.Kind.CLASS)
129+
.isPresent();
91130
}
92131

93132
private static boolean isException(CallExpression callExpression, FunctionSymbol functionSymbol) {

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

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ def foo(p1,p2): pass
1212

1313
foo(1,2,3) # Noncompliant {{Remove 1 unexpected arguments; 'foo' expects 2 positional arguments.}}
1414
# ^^^
15-
foo(1, x = 42) # Noncompliant {{Remove this unexpected named argument 'x'.}}
16-
# ^^^^^^
17-
15+
foo(1, x = 42) # Noncompliant {{Add 1 missing arguments; 'foo' expects 2 positional arguments.}} {{Remove this unexpected named argument 'x'.}}
16+
foo(1, 2, x = 42) # Noncompliant {{Remove this unexpected named argument 'x'.}}
17+
# ^^^^^^
1818

1919
foo(1, 2) # OK
2020
foo(1, p2 = 2) # OK
@@ -24,10 +24,15 @@ def foo(p1,p2): pass
2424

2525
def foo_with_default_value(p1, p2 = 42): pass
2626

27-
foo_with_default_value() # Noncompliant {{Add 1 missing arguments; 'foo_with_default_value' expects 1 positional arguments.}}
27+
foo_with_default_value() # Noncompliant {{Add 1 missing arguments; 'foo_with_default_value' expects at least 1 positional arguments.}}
2828
# ^^^^^^^^^^^^^^^^^^^^^^
2929
foo_with_default_value(42) # OK
30-
foo_with_default_value(1,2,3) # Noncompliant {{Remove 1 unexpected arguments; 'foo_with_default_value' expects 1 positional arguments.}}
30+
foo_with_default_value(1,2,3) # Noncompliant {{Remove 1 unexpected arguments; 'foo_with_default_value' expects at most 2 positional arguments.}}
31+
32+
def default_values(p1, p2 = 42, p3 = 43): pass
33+
default_values(1)
34+
default_values(1, 2)
35+
default_values(1, 2, 3)
3136

3237
if True:
3338
def foo_with_multiple_binding(p1, p2): pass
@@ -49,13 +54,26 @@ def foo_with_variadics(**kwargs): pass
4954
foo_with_variadics(1, 2, 3) # OK
5055

5156
def foo_with_optional_keyword_only(p1, *, p2 = 42): pass
57+
foo_with_optional_keyword_only() # Noncompliant {{Add 1 missing arguments; 'foo_with_optional_keyword_only' expects 1 positional arguments.}}
5258
foo_with_optional_keyword_only(42) # OK
59+
foo_with_optional_keyword_only(42, 43) # Noncompliant {{Remove 1 unexpected arguments; 'foo_with_optional_keyword_only' expects 1 positional arguments.}}
60+
foo_with_optional_keyword_only(42, p2 = 43)
61+
5362
from mod import dec
5463
@dec
5564
def foo_with_decorator(): pass
5665

5766
foo_with_decorator(1, 2, 3) # OK
5867

68+
def python2_tuple_params(p1, (p2, p3)): pass
69+
python2_tuple_params(1, something)
70+
python2_tuple_params(1, (2, 3))
71+
python2_tuple_params(1, (2, 3), 4) # Noncompliant
72+
def python2_tuple_params2((p1, p2), (p3, p4)): pass
73+
python2_tuple_params2(x) # Noncompliant
74+
python2_tuple_params2(x, y)
75+
python2_tuple_params2(x, y, z) # Noncompliant
76+
5977
def methods():
6078
def meth(p1, p2): pass
6179
class A:
@@ -116,3 +134,13 @@ def __reduce__(self, p1, p2):
116134
class B2(B1):
117135
def foo(self):
118136
super().__reduce__(1, 2) # OK, __reduce__ is not 'object.__reduce__' but B1.__reduce__
137+
138+
def builtin_method():
139+
myList = list(42, 43)
140+
myList.append(44)
141+
myList.append(1, 2) # Noncompliant
142+
143+
def builtin_method_different_for_python_2_and_3():
144+
myList = list(42, 43)
145+
myList.sort()
146+
myList.sort(lambda x: x)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,9 @@ public FunctionSymbolImpl(String name, @Nullable String fullyQualifiedName, bool
9595

9696
@Override
9797
FunctionSymbolImpl copyWithoutUsages() {
98-
return new FunctionSymbolImpl(name(), this);
98+
FunctionSymbolImpl copy = new FunctionSymbolImpl(name(), this);
99+
copy.setKind(kind());
100+
return copy;
99101
}
100102

101103
private static boolean isInstanceMethod(FunctionDef functionDef) {

0 commit comments

Comments
 (0)