Skip to content

Commit afe20b2

Browse files
joke1196sonartech
joke1196
authored and
sonartech
committed
SONARPY-634: Fix FP and on S2638 when positional-only parameters are used (#356)
GitOrigin-RevId: b92bcceba72cfa90b7c8e9728058f6bc1a5106ec
1 parent 49a84de commit afe20b2

4 files changed

Lines changed: 25 additions & 36 deletions

File tree

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ public void initialize(Context context) {
6969
}
7070

7171
private static void checkMethodContract(SubscriptionContext ctx, FunctionSymbol method) {
72-
SymbolUtils.getOverriddenMethod(method, SymbolUtils::getFirstAlternativeIfEqualArgumentNames)
72+
var potentialSymbols = SymbolUtils.getOverriddenMethods(method);
73+
SymbolUtils.getFirstAlternativeIfEqualArgumentNames(potentialSymbols)
7374
.ifPresent(overriddenMethod -> {
7475
if (overriddenMethod.hasVariadicParameter() || hasDecorators(overriddenMethod)) {
7576
// ignore function declarations with packed params
@@ -112,8 +113,7 @@ private static void reportOnMissingParameters(SubscriptionContext ctx, FunctionS
112113

113114
private static String getMissingParametersMessage(List<String> missingParameters) {
114115
if (missingParameters.contains(null)) {
115-
return missingParameters.size() == 1 ?
116-
"Add 1 missing parameter." : ("Add " + missingParameters.size() + " missing parameters.");
116+
return missingParameters.size() == 1 ? "Add 1 missing parameter." : ("Add " + missingParameters.size() + " missing parameters.");
117117
}
118118
return "Add missing parameters " + String.join(" ", missingParameters).trim() + ".";
119119
}
@@ -138,7 +138,7 @@ private static void reportOnExtraParameters(SubscriptionContext ctx, FunctionSym
138138
method.parameters().stream()
139139
.filter(parameter -> !parameter.hasDefaultValue() && parameter.name() != null)
140140
.filter(parameter -> overriddenMethod.parameters().stream().noneMatch(p -> Objects.equals(parameter.name(), p.name())))
141-
.forEach(parameter -> reportIssue(ctx,"Remove parameter " + parameter.name() + " or provide default value.", parameter.location(), overriddenMethod));
141+
.forEach(parameter -> reportIssue(ctx, "Remove parameter " + parameter.name() + " or provide default value.", parameter.location(), overriddenMethod));
142142
}
143143

144144

@@ -170,17 +170,14 @@ private static void checkDefaultValuesAndParamNames(SubscriptionContext ctx, Fun
170170
}
171171

172172
private static void checkDefaultValueAndKeywordOnly(SubscriptionContext ctx, FunctionSymbol overriddenMethod, FunctionSymbol.Parameter overriddenParam,
173-
FunctionSymbol.Parameter parameter) {
173+
FunctionSymbol.Parameter parameter) {
174174
String prefix = "Make parameter " + parameter.name();
175175
if (overriddenParam.hasDefaultValue() && !parameter.hasDefaultValue()) {
176176
reportIssue(ctx, "Add a default value to parameter " + parameter.name() + ".", parameter.location(), overriddenMethod);
177177
}
178178
if ((!overriddenParam.isKeywordOnly() && !overriddenParam.isPositionalOnly()) && (parameter.isKeywordOnly() || parameter.isPositionalOnly())) {
179179
reportIssue(ctx, prefix + " keyword-or-positional.", parameter.location(), overriddenMethod);
180180
}
181-
if (overriddenParam.isPositionalOnly() && !parameter.isPositionalOnly()) {
182-
reportIssue(ctx, prefix + " positional only.", parameter.location(), overriddenMethod);
183-
}
184181
if (overriddenParam.isKeywordOnly() && !parameter.isKeywordOnly()) {
185182
reportIssue(ctx, prefix + " keyword only.", parameter.location(), overriddenMethod);
186183
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,13 @@ def positional_only(self, param1, param2, /, *, param3): ... # Noncompliant {{Ma
126126
class ChildClassPosOnlyMovedBad2(ParentClass):
127127
def positional_only(self, param1, /, param2, param3): ... # Noncompliant {{Make parameter param3 keyword only.}}
128128
# ^^^^^^
129+
class ChildClassPosOnlyToKworPos(ParentClass):
130+
def positional_only(self, param1, param2,*, param3): ... # OK The substitution is correct
131+
129132
class ChildClassReorderingKW(ParentClass):
130133
def with_two_keyword_only(self, *, param2, param1): ... # OK. Reordering keyword only parameters is ok
131134

135+
132136
class ChildClassReorderingAndExtra(ParentClass):
133137
def my_method(self, inserted, param1): ... # Noncompliant
134138
# ^^^^^^^^

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

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -242,16 +242,11 @@ public static int firstParameterOffset(FunctionSymbol functionSymbol, boolean is
242242
}
243243

244244
public static Optional<FunctionSymbol> getOverriddenMethod(FunctionSymbol functionSymbol) {
245-
return getOverriddenMethod(functionSymbol, symbols -> Optional.of(symbols)
246-
.filter(s -> s.size() == 1)
247-
.map(Collection::stream)
248-
.flatMap(Stream::findFirst)
249-
);
250-
}
251-
252-
public static Optional<FunctionSymbol> getOverriddenMethod(FunctionSymbol functionSymbol,
253-
Function<List<FunctionSymbol>, Optional<FunctionSymbol>> symbolPicker) {
254-
return symbolPicker.apply(getOverriddenMethods(functionSymbol));
245+
var symbols = getOverriddenMethods(functionSymbol);
246+
if(symbols.size() == 1){
247+
return symbols.stream().findFirst();
248+
}
249+
return Optional.empty();
255250
}
256251

257252
public static List<FunctionSymbol> getOverriddenMethods(FunctionSymbol functionSymbol) {

‎python-frontend/src/test/java/org/sonar/python/semantic/SymbolUtilsTest.java‎

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,9 @@ void first_parameter_offset() {
121121
assertThat(SymbolUtils.firstParameterOffset(functionSymbol, true)).isZero();
122122
}
123123

124-
125124
@Test
126125
void get_overridden_method() {
127-
FileInput file = PythonTestUtils.parse( new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
126+
FileInput file = PythonTestUtils.parse(new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
128127
"def foo(): pass",
129128
"def foo2():",
130129
" def foo3(): pass",
@@ -142,15 +141,14 @@ void get_overridden_method() {
142141
"class E(foo2):",
143142
" def foo8(): pass",
144143
"class MyStr(str):",
145-
" def capitalize(self): pass"
146-
);
144+
" def capitalize(self): pass");
147145

148146
FunctionSymbol foo = (FunctionSymbol) descendantFunction(file, "foo").name().symbol();
149147
FunctionSymbol foo2 = (FunctionSymbol) descendantFunction(file, "foo2").name().symbol();
150148
FunctionSymbol foo3 = (FunctionSymbol) descendantFunction(file, "foo3").name().symbol();
151149
FunctionSymbol foo4 = (FunctionSymbol) descendantFunction(file, "foo4").name().symbol();
152150
FunctionSymbol foo5 = (FunctionSymbol) descendantFunction(file, "foo5").name().symbol();
153-
FunctionSymbol foo5_override = (FunctionSymbol) ((FunctionDef) ((ClassDefImpl)file.statements().statements().get(4)).body().statements().get(0)).name().symbol();
151+
FunctionSymbol foo5_override = (FunctionSymbol) ((FunctionDef) ((ClassDefImpl) file.statements().statements().get(4)).body().statements().get(0)).name().symbol();
154152
FunctionSymbol foo6 = (FunctionSymbol) descendantFunction(file, "foo6").name().symbol();
155153
FunctionSymbol foo7 = (FunctionSymbol) descendantFunction(file, "foo7").name().symbol();
156154
FunctionSymbol foo8 = (FunctionSymbol) descendantFunction(file, "foo8").name().symbol();
@@ -176,8 +174,8 @@ void get_overridden_method() {
176174
assertThat(SymbolUtils.canBeAnOverridingMethod(foo8)).isTrue();
177175
assertThat(SymbolUtils.getOverriddenMethod(foo_int)).isEmpty();
178176
assertThat(SymbolUtils.canBeAnOverridingMethod(foo_int)).isTrue();
179-
assertThat(SymbolUtils.getOverriddenMethod(capitalize, SymbolUtils::getFirstAlternativeIfEqualArgumentNames)).isNotEmpty();
180-
177+
List<FunctionSymbol> overriddenMethod = SymbolUtils.getOverriddenMethods(capitalize);
178+
assertThat(SymbolUtils.getFirstAlternativeIfEqualArgumentNames(overriddenMethod)).isNotEmpty();
181179
assertThat(SymbolUtils.canBeAnOverridingMethod(null)).isTrue();
182180
String[] strings = {
183181
"class F:",
@@ -207,15 +205,13 @@ void get_overridden_method() {
207205
void getFunctionSymbolsTest() {
208206
assertThat(SymbolUtils.getFunctionSymbols(null)).isEmpty();
209207

210-
var file = PythonTestUtils.parse( new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
208+
var file = PythonTestUtils.parse(new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
211209
"class MyStr(str):",
212-
" def capitalize(self): pass"
213-
);
210+
" def capitalize(self): pass");
214211
var capitalize = (FunctionSymbolImpl) descendantFunction(file, "capitalize").name().symbol();
215212
assertThat(capitalize).isNotNull();
216213
assertThat(SymbolUtils.getFunctionSymbols(capitalize)).isNotEmpty().contains(capitalize);
217214

218-
219215
var owner = (ClassSymbol) capitalize.owner();
220216
assertThat(SymbolUtils.getFunctionSymbols(owner)).isEmpty();
221217
var capitalizeParentSymbol = ((ClassSymbol) owner.superClasses().get(0)).resolveMember("capitalize").orElse(null);
@@ -224,11 +220,11 @@ void getFunctionSymbolsTest() {
224220

225221
@Test
226222
void getFirstAlternativeIfEqualArgumentNamesTest() {
227-
var file = PythonTestUtils.parse( new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
223+
var file = PythonTestUtils.parse(new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
228224
"""
229225
class MyClass(dict):
230226
def get(self, key): ...
231-
227+
232228
def foo1(a, b, c): ...
233229
def foo2(a, b, c): ...
234230
def bar(b, c, a): ...
@@ -250,7 +246,7 @@ def qix(c, a, b): ...
250246

251247
@Test
252248
void isEqualParameterCountAndNamesTest() {
253-
var file = PythonTestUtils.parse( new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
249+
var file = PythonTestUtils.parse(new SymbolTableBuilder("my_package", pythonFile("my_module.py")),
254250
"class A:",
255251
" def foo1(self, a):",
256252
" ...",
@@ -259,8 +255,7 @@ void isEqualParameterCountAndNamesTest() {
259255
" ...",
260256
"class C:",
261257
" def foo3(self, b):",
262-
" ..."
263-
);
258+
" ...");
264259

265260
FunctionSymbol foo1 = (FunctionSymbol) descendantFunction(file, "foo1").name().symbol();
266261
FunctionSymbol foo2 = (FunctionSymbol) descendantFunction(file, "foo2").name().symbol();
@@ -273,7 +268,6 @@ void isEqualParameterCountAndNamesTest() {
273268
assertThat(SymbolUtils.isEqualParameterCountAndNames(List.of(foo1, foo3))).isFalse();
274269
}
275270

276-
277271
@Nullable
278272
private static FunctionDef descendantFunction(Tree tree, String name) {
279273
if (tree.is(Tree.Kind.FUNCDEF)) {
@@ -288,7 +282,6 @@ private static FunctionDef descendantFunction(Tree tree, String name) {
288282
.findFirst().orElse(null);
289283
}
290284

291-
292285
@Test
293286
void qualifiedNameOrEmpty() {
294287
var callExpr1 = mock(CallExpression.class);

0 commit comments

Comments
 (0)