Skip to content

Commit 022c4f6

Browse files
Seppli11sonartech
Seppli11
authored and
sonartech
committed
SONARPY-1606 S930: collection methods (from dict, list, ...) such as items or keys expect wrongly a positional argument (#723)
GitOrigin-RevId: af439fbf65cb8a525ffb4bbc878d82cc0ea8ff7a
1 parent 6f96fed commit 022c4f6

File tree

22 files changed

+773
-140
lines changed

22 files changed

+773
-140
lines changed

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

Lines changed: 58 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -16,35 +16,25 @@
1616
*/
1717
package org.sonar.python.checks;
1818

19-
import java.util.Collection;
2019
import java.util.HashMap;
2120
import java.util.List;
2221
import java.util.Map;
23-
import java.util.Optional;
2422
import java.util.Set;
2523
import java.util.stream.Collectors;
26-
import java.util.stream.Stream;
27-
import javax.annotation.Nullable;
2824
import org.sonar.check.Rule;
29-
import org.sonar.plugins.python.api.LocationInFile;
3025
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
3126
import org.sonar.plugins.python.api.SubscriptionContext;
32-
import org.sonar.plugins.python.api.symbols.ClassSymbol;
33-
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
34-
import org.sonar.plugins.python.api.symbols.Symbol;
3527
import org.sonar.plugins.python.api.tree.Argument;
3628
import org.sonar.plugins.python.api.tree.CallExpression;
3729
import org.sonar.plugins.python.api.tree.Expression;
38-
import org.sonar.plugins.python.api.tree.FunctionDef;
3930
import org.sonar.plugins.python.api.tree.Name;
4031
import org.sonar.plugins.python.api.tree.QualifiedExpression;
4132
import org.sonar.plugins.python.api.tree.RegularArgument;
4233
import org.sonar.plugins.python.api.tree.Tree;
43-
import org.sonar.python.semantic.FunctionSymbolImpl;
44-
import org.sonar.python.semantic.SymbolUtils;
45-
import org.sonar.python.tree.TreeUtils;
46-
47-
import static org.sonar.plugins.python.api.symbols.Usage.Kind.PARAMETER;
34+
import org.sonar.plugins.python.api.types.v2.FunctionType;
35+
import org.sonar.plugins.python.api.types.v2.ObjectType;
36+
import org.sonar.plugins.python.api.types.v2.ParameterV2;
37+
import org.sonar.python.api.types.v2.matchers.TypeMatchers;
4838

4939
@Rule(key = "S930")
5040
public class ArgumentNumberCheck extends PythonSubscriptionCheck {
@@ -56,31 +46,27 @@ public void initialize(Context context) {
5646
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, ctx -> {
5747
CallExpression callExpression = (CallExpression) ctx.syntaxNode();
5848

59-
Optional.of(callExpression)
60-
.map(CallExpression::calleeSymbol)
61-
.map(SymbolUtils::getFunctionSymbols)
62-
.filter(SymbolUtils::isEqualParameterCountAndNames)
63-
.map(Collection::stream)
64-
.flatMap(Stream::findFirst)
65-
.ifPresent(functionSymbol -> checkFunctionSymbol(ctx, callExpression, functionSymbol));
49+
if (callExpression.callee().typeV2() instanceof FunctionType functionType) {
50+
checkFunctionType(ctx, callExpression, functionType);
51+
}
6652
});
6753
}
6854

69-
private static void checkFunctionSymbol(SubscriptionContext ctx, CallExpression callExpression, FunctionSymbol functionSymbol) {
70-
if (isException(callExpression, functionSymbol)) {
55+
private static void checkFunctionType(SubscriptionContext ctx, CallExpression callExpression, FunctionType functionType) {
56+
if (isException(ctx, callExpression, functionType)) {
7157
return;
7258
}
73-
checkPositionalParameters(ctx, callExpression, functionSymbol);
74-
checkKeywordArguments(ctx, callExpression, functionSymbol, callExpression.callee());
59+
checkPositionalParameters(ctx, callExpression, functionType);
60+
checkKeywordArguments(ctx, callExpression, functionType, callExpression.callee());
7561
}
7662

77-
private static void checkPositionalParameters(SubscriptionContext ctx, CallExpression callExpression, FunctionSymbol functionSymbol) {
63+
private static void checkPositionalParameters(SubscriptionContext ctx, CallExpression callExpression, FunctionType functionType) {
7864
int self = 0;
79-
if (functionSymbol.isInstanceMethod() && callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR) && !isCalledAsClassMethod((QualifiedExpression) callExpression.callee())) {
65+
if (isCalledAsInstanceMethod(callExpression, functionType) && functionHasSelfParameter(functionType)) {
8066
self = 1;
8167
}
82-
Map<String, FunctionSymbol.Parameter> positionalParamsWithoutDefault = positionalParamsWithoutDefault(functionSymbol);
83-
long nbPositionalParamsWithDefault = functionSymbol.parameters().stream()
68+
Map<String, ParameterV2> positionalParamsWithoutDefault = positionalParamsWithoutDefault(functionType);
69+
long nbPositionalParamsWithDefault = functionType.parameters().stream()
8470
.filter(parameterName -> !parameterName.isKeywordOnly() && parameterName.hasDefaultValue())
8571
.count();
8672

@@ -101,42 +87,33 @@ private static void checkPositionalParameters(SubscriptionContext ctx, CallExpre
10187
if (nbPositionalParamsWithDefault > 0) {
10288
expected = "at least " + expected;
10389
}
104-
addPositionalIssue(ctx, callExpression.callee(), functionSymbol, message, expected);
90+
addPositionalIssue(ctx, callExpression.callee(), functionType, message, expected);
10591
} else if (nbMissingArgs + nbPositionalParamsWithDefault + nbNonKeywordOnlyPassedWithKeyword < 0) {
10692
String message = "Remove " + (- nbMissingArgs - nbPositionalParamsWithDefault) + " unexpected arguments; ";
10793
if (nbPositionalParamsWithDefault > 0) {
10894
expected = "at most " + (minimumPositionalArgs - self + nbPositionalParamsWithDefault);
10995
}
110-
addPositionalIssue(ctx, callExpression.callee(), functionSymbol, message, expected);
96+
addPositionalIssue(ctx, callExpression.callee(), functionType, message, expected);
11197
}
11298
}
11399

114-
private static boolean isCalledAsClassMethod(QualifiedExpression callee) {
115-
return TreeUtils.getSymbolFromTree(callee.qualifier())
116-
.filter(ArgumentNumberCheck::isParamOfClassMethod)
117-
.isPresent();
100+
private static boolean isCalledAsInstanceMethod(CallExpression callExpression, FunctionType functionType) {
101+
return functionType.isInstanceMethod()
102+
&& callExpression.callee() instanceof QualifiedExpression qualifiedExpression
103+
&& qualifiedExpression.qualifier().typeV2() instanceof ObjectType;
118104
}
119105

120-
// no need to check that's the first parameter (i.e. cls)
121-
// the assumption is that another method can be called only using the first parameter of a class method
122-
private static boolean isParamOfClassMethod(Symbol symbol) {
123-
return symbol.usages().stream().anyMatch(usage -> usage.kind() == PARAMETER && isParamOfClassMethod(usage.tree()));
106+
private static boolean functionHasSelfParameter(FunctionType functionType) {
107+
return !functionType.parameters().isEmpty();
124108
}
125109

126-
private static boolean isParamOfClassMethod(Tree tree) {
127-
FunctionDef functionDef = (FunctionDef) TreeUtils.firstAncestorOfKind(tree, Tree.Kind.FUNCDEF);
128-
return Optional.ofNullable(TreeUtils.getFunctionSymbolFromDef(functionDef))
129-
.filter(functionSymbol -> functionSymbol.decorators().stream().anyMatch("classmethod"::equals))
130-
.isPresent();
131-
}
132-
133-
private static Map<String, FunctionSymbol.Parameter> positionalParamsWithoutDefault(FunctionSymbol functionSymbol) {
110+
private static Map<String, ParameterV2> positionalParamsWithoutDefault(FunctionType functionType) {
134111
int unnamedIndex = 0;
135-
Map<String, FunctionSymbol.Parameter> result = new HashMap<>();
136-
for (FunctionSymbol.Parameter parameter : functionSymbol.parameters()) {
112+
Map<String, ParameterV2> result = new HashMap<>();
113+
for (ParameterV2 parameter : functionType.parameters()) {
137114
if (!parameter.isKeywordOnly() && !parameter.hasDefaultValue()) {
138115
String name = parameter.name();
139-
if (name == null) {
116+
if (name == null || name.isEmpty()) {
140117
result.put("!unnamed" + unnamedIndex, parameter);
141118
unnamedIndex++;
142119
} else {
@@ -147,54 +124,59 @@ private static Map<String, FunctionSymbol.Parameter> positionalParamsWithoutDefa
147124
return result;
148125
}
149126

150-
private static void addPositionalIssue(SubscriptionContext ctx, Tree tree, FunctionSymbol functionSymbol, String message, String expected) {
151-
String msg = message + "'" + functionSymbol.name() + "' expects " + expected + " positional arguments.";
127+
private static void addPositionalIssue(SubscriptionContext ctx, Tree tree, FunctionType functionType, String message, String expected) {
128+
String msg = message + "'" + functionType.name() + "' expects " + expected + " positional arguments.";
152129
PreciseIssue preciseIssue = ctx.addIssue(tree, msg);
153-
addSecondary(functionSymbol, preciseIssue);
130+
addSecondary(functionType, preciseIssue);
154131
}
155132

156-
private static boolean isReceiverClassSymbol(QualifiedExpression qualifiedExpression) {
157-
return TreeUtils.getSymbolFromTree(qualifiedExpression.qualifier())
158-
.filter(symbol -> symbol.kind() == Symbol.Kind.CLASS)
159-
.isPresent();
133+
private static boolean isException(SubscriptionContext ctx, CallExpression callExpression, FunctionType functionType) {
134+
return functionType.hasDecorators()
135+
|| functionType.hasVariadicParameter()
136+
|| callExpression.arguments().stream().anyMatch(argument -> argument.is(Tree.Kind.UNPACKING_EXPR))
137+
|| extendsZopeInterface(ctx, callExpression)
138+
|| isCalledAsBoundInstanceMethod(callExpression)
139+
|| isSuperCall(ctx, callExpression);
160140
}
161141

162-
private static boolean isException(CallExpression callExpression, FunctionSymbol functionSymbol) {
163-
return functionSymbol.hasDecorators()
164-
|| functionSymbol.hasVariadicParameter()
165-
|| callExpression.arguments().stream().anyMatch(argument -> argument.is(Tree.Kind.UNPACKING_EXPR))
166-
|| extendsZopeInterface(((FunctionSymbolImpl) functionSymbol).owner())
167-
// TODO: distinguish between class methods (new and old style) from other methods
168-
|| (callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR) && isReceiverClassSymbol(((QualifiedExpression) callExpression.callee())));
142+
private static boolean extendsZopeInterface(SubscriptionContext ctx, CallExpression callExpression) {
143+
var matcher = TypeMatchers.isFunctionOwnerSatisfying(
144+
TypeMatchers.isOrExtendsType("zope.interface.Interface")
145+
);
146+
return matcher.isTrueFor(callExpression.callee(), ctx);
169147
}
170148

171-
private static boolean extendsZopeInterface(@Nullable Symbol symbol) {
172-
if (symbol != null && symbol.kind() == Symbol.Kind.CLASS) {
173-
return ((ClassSymbol) symbol).isOrExtends("zope.interface.Interface");
149+
private static boolean isCalledAsBoundInstanceMethod(CallExpression callExpression) {
150+
if (callExpression.callee().typeV2() instanceof FunctionType functionType) {
151+
return functionType.isInstanceMethod() && !callExpression.callee().is(Tree.Kind.QUALIFIED_EXPR);
174152
}
175153
return false;
176154
}
177155

178-
private static void addSecondary(FunctionSymbol functionSymbol, PreciseIssue preciseIssue) {
179-
LocationInFile definitionLocation = functionSymbol.definitionLocation();
180-
if (definitionLocation != null) {
181-
preciseIssue.secondary(definitionLocation, FUNCTION_DEFINITION);
156+
private static boolean isSuperCall(SubscriptionContext ctx, CallExpression callExpression) {
157+
if (callExpression.callee() instanceof QualifiedExpression qualifiedExpression) {
158+
return TypeMatchers.isObjectOfType("super").isTrueFor(qualifiedExpression.qualifier(), ctx);
182159
}
160+
return false;
161+
}
162+
163+
private static void addSecondary(FunctionType functionType, PreciseIssue preciseIssue) {
164+
functionType.definitionLocation().ifPresent(location -> preciseIssue.secondary(location, FUNCTION_DEFINITION));
183165
}
184166

185-
private static void checkKeywordArguments(SubscriptionContext ctx, CallExpression callExpression, FunctionSymbol functionSymbol, Expression callee) {
186-
List<FunctionSymbol.Parameter> parameters = functionSymbol.parameters();
167+
private static void checkKeywordArguments(SubscriptionContext ctx, CallExpression callExpression, FunctionType functionType, Expression callee) {
168+
List<ParameterV2> parameters = functionType.parameters();
187169
Set<String> mandatoryParamNamesKeywordOnly = parameters.stream()
188170
.filter(parameterName -> parameterName.isKeywordOnly() && !parameterName.hasDefaultValue())
189-
.map(FunctionSymbol.Parameter::name).collect(Collectors.toSet());
171+
.map(ParameterV2::name).collect(Collectors.toSet());
190172

191173
for (Argument argument : callExpression.arguments()) {
192174
RegularArgument arg = (RegularArgument) argument;
193175
Name keyword = arg.keywordArgument();
194176
if (keyword != null) {
195177
if (parameters.stream().noneMatch(parameter -> keyword.name().equals(parameter.name()) && !parameter.isPositionalOnly())) {
196178
PreciseIssue preciseIssue = ctx.addIssue(argument, "Remove this unexpected named argument '" + keyword.name() + "'.");
197-
addSecondary(functionSymbol, preciseIssue);
179+
addSecondary(functionType, preciseIssue);
198180
} else {
199181
mandatoryParamNamesKeywordOnly.remove(keyword.name());
200182
}
@@ -206,7 +188,7 @@ private static void checkKeywordArguments(SubscriptionContext ctx, CallExpressio
206188
message.append("'").append(param).append("' ");
207189
}
208190
PreciseIssue preciseIssue = ctx.addIssue(callee, message.toString().trim());
209-
addSecondary(functionSymbol, preciseIssue);
191+
addSecondary(functionType, preciseIssue);
210192
}
211193
}
212194
}

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

Lines changed: 72 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -79,37 +79,55 @@ def python2_tuple_params2((p1, p2), (p3, p4)): pass
7979
python2_tuple_params2(x, y, z) # Noncompliant
8080

8181
def methods():
82-
def meth(p1, p2): pass
82+
def meth(p1, p2, p3, p4): pass
8383
class A:
84-
def __new__(cls, a, b):
85-
cls.__new__(A, 1, 2)
86-
def meth(self, p1, p2):
87-
meth(1, 2) # OK
88-
meth(1, 2, 3) # Noncompliant
89-
@classmethod
90-
def class_meth(cls, p1, p2): pass
91-
@staticmethod
92-
def static_meth(p1, p2): pass
93-
def foo(p1): pass
94-
foo(42)
95-
@classmethod
96-
def bar_class_method(cls):
97-
cls.bar_instance_method(cls)
98-
def bar_instance_method(self): pass
84+
def __new__(cls, a, b):
85+
cls.__new__(A, 1, 2)
86+
def meth(self, p1, p2):
87+
meth(1, 2) # Noncompliant
88+
meth(1, 2, 3, 5) # oK
89+
90+
def bar(self, p1, p2): pass
91+
92+
def other_method(self):
93+
self.bar(1, 2)
94+
self.other_method()
95+
96+
@classmethod
97+
def class_meth(cls, p1, p2): pass
98+
@staticmethod
99+
def static_meth(p1, p2): pass
100+
def foo(): pass
101+
102+
@classmethod
103+
def bar_class_method(cls):
104+
cls.bar_instance_method(cls)
105+
cls.bar_instance_method(cls, 1) # FN
106+
def bar_instance_method(self): pass
99107

100108
A.class_meth(42) # FN {{'class_meth' expects 2 positional arguments, but 1 was provided}}
101109

102110
A.class_meth(1, 2) # OK
103111
A.static_meth(42) # FN {{'static_meth' expects 2 positional arguments, but 1 was provided}}
104112

105113
A.static_meth(1, 2) # OK
114+
106115
a = A()
107116
a.meth(42, 43)
108117
a.meth(42) # Noncompliant {{Add 1 missing arguments; 'meth' expects 2 positional arguments.}}
109118
a.meth(42, 43, 44) # Noncompliant {{Remove 1 unexpected arguments; 'meth' expects 2 positional arguments.}}
110119

111-
A.foo() # FN
112-
A.foo(42)
120+
a.other_method() # OK
121+
122+
A.bar(42, 43) # Noncompliant {{Add 1 missing arguments; 'bar' expects 3 positional arguments.}}
123+
A.bar(a, 42, 53)
124+
125+
126+
A.foo()
127+
A.foo(42) # Noncompliant {{Remove 1 unexpected arguments; 'foo' expects 0 positional arguments.}}
128+
129+
a.foo()
130+
a.foo(42) # Noncompliant {{Remove 1 unexpected arguments; 'foo' expects 0 positional arguments.}}
113131

114132
m = a.meth
115133
m(42, 43) # OK
@@ -142,6 +160,40 @@ def __reduce__(self, p1, p2):
142160
class B2(B1):
143161
def foo(self):
144162
super().__reduce__(1, 2) # OK, __reduce__ is not 'object.__reduce__' but B1.__reduce__
163+
super().__reduce__(1, 2, 3) # FN
164+
165+
class MethodsWithKeywordArguments:
166+
@classmethod
167+
def from_dict(cls, **kwargs):
168+
kwargs.items()
169+
kwargs.items(1) # FN
170+
171+
172+
173+
class MethodsWithTryExcept(object):
174+
def get_all_config(self):
175+
pass
176+
177+
def replace(self, config):
178+
try:
179+
pass
180+
except Exception as e:
181+
pass
182+
self.get_all_config()
183+
self.get_all_config(3) # Noncompliant
184+
185+
self.replace(3) # OK
186+
self.replace(3, 4) # Noncompliant
187+
MethodsWithTryExcept.replace(self, 4) # OK
188+
MethodsWithTryExcept.replace(4) # Noncompliant
189+
190+
class MethodsWithControlFlow:
191+
def something(self, importing=None):
192+
if importing is None:
193+
importing = set()
194+
importing.discard(1)
195+
importing.discard(1, 2) # FN
196+
145197

146198
def builtin_method():
147199
myList = list(42, 43)
@@ -212,7 +264,7 @@ def logging_api():
212264
logging.basicConfig(format="42", force=True) # OK
213265

214266
def foo(day, tz):
215-
b = datetime.date.fromordinal(day).replace(tzinfo=tz) # FN SONARPY-1472
267+
b = datetime.date.fromordinal(day).replace(tzinfo=tz) # Noncompliant
216268
a = datetime.datetime.fromordinal(day).replace(tzinfo=tz) # OK
217269

218270

@@ -234,4 +286,4 @@ def some_method(self, param): ...
234286

235287
def torch():
236288
import torch
237-
torch.cat(torch.Tensor(), axis=-1)
289+
torch.cat(torch.Tensor(), axis=-1)

‎python-frontend/src/main/java/org/sonar/python/api/types/v2/matchers/TypeMatchers.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@
2626
import org.sonar.python.types.v2.matchers.HasFQNPredicate;
2727
import org.sonar.python.types.v2.matchers.HasMemberPredicate;
2828
import org.sonar.python.types.v2.matchers.HasMemberSatisfyingPredicate;
29+
import org.sonar.python.types.v2.matchers.IsFunctionOwnerSatisfyingPredicate;
2930
import org.sonar.python.types.v2.matchers.IsObjectSatisfyingPredicate;
3031
import org.sonar.python.types.v2.matchers.IsObjectSubtypeOfPredicate;
32+
import org.sonar.python.types.v2.matchers.IsTypeOrSuperTypeSatisfyingPredicate;
3133
import org.sonar.python.types.v2.matchers.IsTypePredicate;
3234
import org.sonar.python.types.v2.matchers.TypePredicate;
3335
import org.sonar.python.types.v2.matchers.TypeSourcePredicate;
@@ -97,6 +99,24 @@ public static TypeMatcher isObjectOfSubType(String fqn) {
9799
return new TypeMatcherImpl(new IsObjectSubtypeOfPredicate(fqn));
98100
}
99101

102+
public static TypeMatcher isOrExtendsType(String fqn) {
103+
return isTypeOrSuperTypeSatisfying(isType(fqn));
104+
}
105+
106+
public static TypeMatcher isTypeOrSuperTypeWithFQN(String fqn) {
107+
return isTypeOrSuperTypeSatisfying(withFQN(fqn));
108+
}
109+
110+
public static TypeMatcher isTypeOrSuperTypeSatisfying(TypeMatcher matcher) {
111+
TypePredicate predicate = getTypePredicate(matcher);
112+
return new TypeMatcherImpl(new IsTypeOrSuperTypeSatisfyingPredicate(predicate));
113+
}
114+
115+
public static TypeMatcher isFunctionOwnerSatisfying(TypeMatcher matcher) {
116+
TypePredicate predicate = getTypePredicate(matcher);
117+
return new TypeMatcherImpl(new IsFunctionOwnerSatisfyingPredicate(predicate));
118+
}
119+
100120
public static TypeMatcher hasTypeSource(TypeSource typeSource) {
101121
return new TypeMatcherImpl(new TypeSourcePredicate(typeSource));
102122
}

0 commit comments

Comments
 (0)