Skip to content

Commit f3802ec

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #20217 from joefarebrother/python-qual-signature-mismatch
Python: Modernize the Signature Mismatch query
2 parents c653d93 + f9e094d commit f3802ec

10 files changed

+363
-45
lines changed

‎python/ql/src/Functions/IncorrectlyOverriddenMethod.ql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
/**
2+
* @deprecated
23
* @name Mismatch between signature and use of an overriding method
34
* @description Method has a different signature from the overridden method and, if it were called, would be likely to cause an error.
45
* @kind problem

‎python/ql/src/Functions/SignatureOverriddenMethod.py‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11

2-
# Base class method
3-
def runsource(self, source, filename="<input>", symbol="single"):
4-
... # Definition
2+
class Base:
3+
def runsource(self, source, filename="<input>"):
4+
...
55

66

7-
# Extend base class method
8-
def runsource(self, source):
9-
... # Definition
7+
class Sub(Base):
8+
def runsource(self, source): # BAD: Does not match the signature of overridden method.
9+
...
10+
11+
def run(obj: Base):
12+
obj.runsource("source", filename="foo.txt")

‎python/ql/src/Functions/SignatureOverriddenMethod.qhelp‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,32 +5,25 @@
55

66

77
<overview>
8-
<p> There are one (or more) legal parameters for an overridden method that are
9-
not legal for an overriding method. This will cause an error when the overriding
10-
method is called with a number of parameters that is legal for the overridden method.
11-
This violates the Liskov substitution principle.
8+
<p> When the signature of a method of a base class and a method of a subclass that overrides it don't match, a call to the base class method
9+
may not be a valid call to the subclass method, and thus raise an exception if an instance of the subclass is passed instead.
10+
If following the Liskov Substitution Principle, in which an instance of a subclass should be usable in every context as though it were an
11+
instance of the base class, this behavior breaks the principle.
1212
</p>
1313

1414
</overview>
1515
<recommendation>
1616

17-
<p>Ensure that the overriding method accepts all the parameters that are legal for
18-
overridden method.</p>
17+
<p>Ensure that the overriding method in the subclass accepts the same parameters as the base method. </p>
1918

2019
</recommendation>
2120
<example>
22-
<p>In this example there is a mismatch between the legal parameters for the base
23-
class method <code>(self, source, filename, symbol)</code> and the extension method
24-
<code>(self, source)</code>. The extension method can be used to override the base
25-
method as long as values are not specified for the <code>filename</code> and
26-
<code>symbol</code> parameters. If the extension method was passed the additional
27-
parameters accepted by the base method then an error would occur.</p>
21+
<p>In the following example, <code>Base.runsource</code> takes an optional <code>filename</code> argument. However, the overriding method
22+
<code>Sub.runsource</code> does not. This means the <code>run</code> function will fail if passed an instance of <code>Sub</code>.
23+
</p>
2824

2925
<sample src="SignatureOverriddenMethod.py" />
3026

31-
<p>The extension method should be updated to support the <code>filename</code> and
32-
<code>symbol</code> parameters supported by the overridden method.</p>
33-
3427
</example>
3528
<references>
3629

‎python/ql/src/Functions/SignatureOverriddenMethod.ql‎

Lines changed: 246 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,254 @@
1313
*/
1414

1515
import python
16-
import Expressions.CallArgs
16+
import semmle.python.dataflow.new.DataFlow
17+
import semmle.python.dataflow.new.internal.DataFlowDispatch
18+
import codeql.util.Option
1719

18-
from FunctionValue base, PythonFunctionValue derived
19-
where
20-
not exists(base.getACall()) and
21-
not exists(FunctionValue a_derived |
22-
a_derived.overrides(base) and
23-
exists(a_derived.getACall())
20+
/** Holds if `base` is overridden by `sub` */
21+
predicate overrides(Function base, Function sub) {
22+
base.getName() = sub.getName() and
23+
base.getScope() = getADirectSuperclass+(sub.getScope())
24+
}
25+
26+
/** Constructs a string to pluralize `str` depending on `num`. */
27+
bindingset[num, str]
28+
string plural(int num, string str) {
29+
num = 1 and result = "1 " + str
30+
or
31+
num != 1 and result = num.toString() + " " + str + "s"
32+
}
33+
34+
/** Describes the minimum number of arguments `func` can accept, using "at least" if it may accept more. */
35+
string describeMin(Function func) {
36+
exists(string descr | descr = plural(func.getMinPositionalArguments(), "positional argument") |
37+
if func.getMinPositionalArguments() = func.getMaxPositionalArguments()
38+
then result = descr
39+
else result = "at least " + descr
40+
)
41+
}
42+
43+
/** Described the maximum number of arguments `func` can accept, using "at most" if it may accept fewer, and "arbitrarily many" if it has a vararg. */
44+
string describeMax(Function func) {
45+
if func.hasVarArg()
46+
then result = "arbitrarily many positional arguments"
47+
else
48+
exists(string descr | descr = plural(func.getMaxPositionalArguments(), "positional argument") |
49+
if func.getMinPositionalArguments() = func.getMaxPositionalArguments()
50+
then result = descr
51+
else result = "at most " + descr
52+
)
53+
}
54+
55+
/** Describes the minimum number of arguments `func` can accept, without repeating "positional arguments". */
56+
string describeMinShort(Function func) {
57+
exists(string descr | descr = func.getMinPositionalArguments().toString() |
58+
if func.getMinPositionalArguments() = func.getMaxPositionalArguments()
59+
then result = descr
60+
else result = "at least " + descr
61+
)
62+
}
63+
64+
/** Describes the maximum number of arguments `func` can accept, without repeating "positional arguments". */
65+
string describeMaxShort(Function func) {
66+
if func.hasVarArg()
67+
then result = "arbitrarily many"
68+
else
69+
exists(string descr | descr = func.getMaxPositionalArguments().toString() |
70+
if func.getMinPositionalArguments() = func.getMaxPositionalArguments()
71+
then result = descr
72+
else result = "at most " + descr
73+
)
74+
}
75+
76+
/** Describe an upper bound on the number of arguments `func` may accept, without specifying "at most". */
77+
string describeMaxBound(Function func) {
78+
if func.hasVarArg()
79+
then result = "arbitrarily many"
80+
else result = func.getMaxPositionalArguments().toString()
81+
}
82+
83+
/** Holds if no way to call `base` would be valid for `sub`. The `msg` applies to the `sub method. */
84+
predicate strongSignatureMismatch(Function base, Function sub, string msg) {
85+
overrides(base, sub) and
86+
(
87+
sub.getMinPositionalArguments() > base.getMaxPositionalArguments() and
88+
msg =
89+
"requires " + describeMin(sub) + ", whereas overridden $@ requires " + describeMaxShort(base) +
90+
"."
91+
or
92+
sub.getMaxPositionalArguments() < base.getMinPositionalArguments() and
93+
msg =
94+
"requires " + describeMax(sub) + ", whereas overridden $@ requires " + describeMinShort(base) +
95+
"."
96+
)
97+
}
98+
99+
/** Holds if there may be some ways to call `base` that would not be valid for `sub`. The `msg` applies to the `sub` method. */
100+
predicate weakSignatureMismatch(Function base, Function sub, string msg) {
101+
overrides(base, sub) and
102+
(
103+
sub.getMinPositionalArguments() > base.getMinPositionalArguments() and
104+
msg =
105+
"requires " + describeMin(sub) + ", whereas overridden $@ may be called with " +
106+
base.getMinPositionalArguments().toString() + "."
107+
or
108+
sub.getMaxPositionalArguments() < base.getMaxPositionalArguments() and
109+
msg =
110+
"requires " + describeMax(sub) + ", whereas overridden $@ may be called with " +
111+
describeMaxBound(base) + "."
112+
or
113+
sub.getMinPositionalArguments() <= base.getMinPositionalArguments() and
114+
sub.getMaxPositionalArguments() >= base.getMaxPositionalArguments() and
115+
exists(string arg |
116+
// TODO: positional-only args not considered
117+
// e.g. `def foo(x, y, /, z):` has x,y as positional only args, should not be considered as possible kw args
118+
// However, this likely does not create FPs, as we require a 'witness' call to generate an alert.
119+
arg = base.getAnArg().getName() and
120+
not arg = sub.getAnArg().getName() and
121+
not exists(sub.getKwarg()) and
122+
msg = "does not accept keyword argument `" + arg + "`, which overridden $@ does."
123+
)
124+
or
125+
exists(base.getKwarg()) and
126+
not exists(sub.getKwarg()) and
127+
msg = "does not accept arbitrary keyword arguments, which overridden $@ does."
128+
)
129+
}
130+
131+
/** Holds if `f` should be ignored for considering signature mismatches. */
132+
predicate ignore(Function f) {
133+
isClassmethod(f)
134+
or
135+
exists(
136+
Function g // other functions with the same name, e.g. @property getters/setters.
137+
|
138+
g.getScope() = f.getScope() and
139+
g.getName() = f.getName() and
140+
g != f
141+
)
142+
}
143+
144+
/** Gets a function that `call` may resolve to. */
145+
Function resolveCall(Call call) {
146+
exists(DataFlowCall dfc | call = dfc.getNode().(CallNode).getNode() |
147+
result = viableCallable(dfc).(DataFlowFunction).getScope()
148+
)
149+
}
150+
151+
/** Holds if `call` may resolve to either `base` or `sub`, and `base` is overridden by `sub`. */
152+
predicate callViableForEitherOverride(Function base, Function sub, Call call) {
153+
overrides(base, sub) and
154+
base = resolveCall(call) and
155+
sub = resolveCall(call)
156+
}
157+
158+
/** Holds if either both `base` and `sub` are static methods, or both are not static methods, and `base` is overridden by `sub`. */
159+
predicate matchingStatic(Function base, Function sub) {
160+
overrides(base, sub) and
161+
(
162+
isStaticmethod(base) and
163+
isStaticmethod(sub)
164+
or
165+
not isStaticmethod(base) and
166+
not isStaticmethod(sub)
167+
)
168+
}
169+
170+
int extraSelfArg(Function func) { if isStaticmethod(func) then result = 0 else result = 1 }
171+
172+
/** Holds if the call `call` matches the signature for `func`. */
173+
predicate callMatchesSignature(Function func, Call call) {
174+
func = resolveCall(call) and
175+
(
176+
// Each parameter of the function is accounted for in the call
177+
forall(Parameter param, int i | param = func.getArg(i) |
178+
// self arg
179+
i = 0 and not isStaticmethod(func)
180+
or
181+
// positional arg
182+
i - extraSelfArg(func) < call.getPositionalArgumentCount()
183+
or
184+
// has default
185+
exists(param.getDefault())
186+
or
187+
// keyword arg
188+
call.getANamedArgumentName() = param.getName()
189+
)
190+
or
191+
// arbitrary varargs or kwargs
192+
exists(call.getStarArg())
193+
or
194+
exists(call.getKwargs())
24195
) and
25-
not derived.getScope().isSpecialMethod() and
26-
derived.getName() != "__init__" and
27-
derived.isNormalMethod() and
28-
// call to overrides distributed for efficiency
196+
// No excess parameters
197+
call.getPositionalArgumentCount() + extraSelfArg(func) <= func.getMaxPositionalArguments() and
198+
(
199+
exists(func.getKwarg())
200+
or
201+
forall(string name | name = call.getANamedArgumentName() | exists(func.getArgByName(name)))
202+
)
203+
}
204+
205+
pragma[nomagic]
206+
private File getFunctionFile(Function f) { result = f.getLocation().getFile() }
207+
208+
/** Gets a call which matches the signature of `base`, but not of overridden `sub`. */
209+
Call getASignatureMismatchWitness(Function base, Function sub) {
210+
callViableForEitherOverride(base, sub, result) and
211+
callMatchesSignature(base, result) and
212+
not callMatchesSignature(sub, result)
213+
}
214+
215+
pragma[inline]
216+
string preferredFile(File callFile, Function base, Function sub) {
217+
if callFile = getFunctionFile(base)
218+
then result = " A"
219+
else
220+
if callFile = getFunctionFile(sub)
221+
then result = " B"
222+
else result = callFile.getAbsolutePath()
223+
}
224+
225+
/** Choose a 'witnessing' call that matches the signature of `base` but not of overridden `sub`. */
226+
Call chooseASignatureMismatchWitness(Function base, Function sub) {
227+
exists(getASignatureMismatchWitness(base, sub)) and
228+
result =
229+
min(Call c |
230+
c = getASignatureMismatchWitness(base, sub)
231+
|
232+
c
233+
order by
234+
preferredFile(c.getLocation().getFile(), base, sub), c.getLocation().getStartLine(),
235+
c.getLocation().getStartColumn()
236+
)
237+
}
238+
239+
module CallOption = LocatableOption<Location, Call>;
240+
241+
from Function base, Function sub, string msg, string extraMsg, CallOption::Option call
242+
where
243+
not sub.isSpecialMethod() and
244+
sub.getName() != "__init__" and
245+
not ignore(sub) and
246+
not ignore(base) and
247+
matchingStatic(base, sub) and
29248
(
30-
derived.overrides(base) and derived.minParameters() > base.maxParameters()
249+
// If we have a witness, alert for a 'weak' mismatch, but prefer the message for a 'strong' mismatch if that holds.
250+
call.asSome() = chooseASignatureMismatchWitness(base, sub) and
251+
extraMsg =
252+
" $@ correctly calls the base method, but does not match the signature of the overriding method." and
253+
(
254+
strongSignatureMismatch(base, sub, msg)
255+
or
256+
not strongSignatureMismatch(base, sub, _) and
257+
weakSignatureMismatch(base, sub, msg)
258+
)
31259
or
32-
derived.overrides(base) and derived.maxParameters() < base.minParameters()
260+
// With no witness, only alert for 'strong' mismatches.
261+
not exists(getASignatureMismatchWitness(base, sub)) and
262+
call.isNone() and
263+
strongSignatureMismatch(base, sub, msg) and
264+
extraMsg = ""
33265
)
34-
select derived, "Overriding method '" + derived.getName() + "' has signature mismatch with $@.",
35-
base, "overridden method"
266+
select sub, "This method " + msg + extraMsg, base, base.getQualifiedName(), call, "This call"

‎python/ql/src/change-notes/2025-08-19-signature-mismatch.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `py/inheritance/signature-mismatch` query has been modernized. It produces more precise results and more descriptive alert messages.
5+
* The `py/inheritance/incorrect-overriding-signature` query has been deprecated. Its results have been consolidated into the `py/inheritance/signature-mismatch` query.

‎python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| om_test.py:32:5:32:35 | Function Derived.grossly_wrong1 | Overriding method 'grossly_wrong1' has signature mismatch with $@. | om_test.py:12:5:12:41 | Function Base.grossly_wrong1 | overridden method |
2-
| om_test.py:35:5:35:47 | Function Derived.grossly_wrong2 | Overriding method 'grossly_wrong2' has signature mismatch with $@. | om_test.py:15:5:15:41 | Function Base.grossly_wrong2 | overridden method |
1+
| om_test.py:32:5:32:35 | Function grossly_wrong1 | This method requires 2 positional arguments, whereas overridden $@ requires 3. | om_test.py:12:5:12:41 | Function grossly_wrong1 | Base.grossly_wrong1 | file://:0:0:0:0 | (none) | This call |
2+
| om_test.py:35:5:35:47 | Function grossly_wrong2 | This method requires 4 positional arguments, whereas overridden $@ requires 3. | om_test.py:15:5:15:41 | Function grossly_wrong2 | Base.grossly_wrong2 | file://:0:0:0:0 | (none) | This call |

‎python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,13 @@
1-
| test.py:30:5:30:26 | Function Derived.meth3 | Overriding method 'meth3' has signature mismatch with $@. | test.py:11:5:11:20 | Function Base.meth3 | overridden method |
1+
| test.py:24:5:24:26 | Function meth1 | This method requires 2 positional arguments, whereas overridden $@ requires 1. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:5:5:5:20 | Function meth1 | Base.meth1 | test.py:15:9:15:20 | Attribute() | This call |
2+
| test.py:27:5:27:20 | Function meth2 | This method requires 1 positional argument, whereas overridden $@ requires 2. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:8:5:8:26 | Function meth2 | Base.meth2 | test.py:18:9:18:21 | Attribute() | This call |
3+
| test.py:30:5:30:26 | Function meth3 | This method requires 2 positional arguments, whereas overridden $@ requires 1. | test.py:11:5:11:20 | Function meth3 | Base.meth3 | file://:0:0:0:0 | (none) | This call |
4+
| test.py:69:5:69:24 | Function meth | This method requires 2 positional arguments, whereas overridden $@ requires 1. | test.py:64:5:64:19 | Function meth | BlameBase.meth | file://:0:0:0:0 | (none) | This call |
5+
| test.py:74:5:74:24 | Function meth | This method requires 2 positional arguments, whereas overridden $@ requires 1. | test.py:64:5:64:19 | Function meth | BlameBase.meth | file://:0:0:0:0 | (none) | This call |
6+
| test.py:125:5:125:20 | Function meth1 | This method requires 1 positional argument, whereas overridden $@ may be called with 2. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:82:5:82:25 | Function meth1 | Base2.meth1 | test.py:110:9:110:23 | Attribute() | This call |
7+
| test.py:131:5:131:31 | Function meth4 | This method requires at least 3 positional arguments, whereas overridden $@ requires at most 2. | test.py:88:5:88:25 | Function meth4 | Base2.meth4 | file://:0:0:0:0 | (none) | This call |
8+
| test.py:133:5:133:28 | Function meth5 | This method requires at most 3 positional arguments, whereas overridden $@ requires at least 4. | test.py:90:5:90:34 | Function meth5 | Base2.meth5 | file://:0:0:0:0 | (none) | This call |
9+
| test.py:135:5:135:23 | Function meth6 | This method requires 2 positional arguments, whereas overridden $@ may be called with arbitrarily many. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:92:5:92:28 | Function meth6 | Base2.meth6 | test.py:113:9:113:27 | Attribute() | This call |
10+
| test.py:137:5:137:28 | Function meth7 | This method requires at least 2 positional arguments, whereas overridden $@ may be called with 1. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:94:5:94:25 | Function meth7 | Base2.meth7 | test.py:114:9:114:20 | Attribute() | This call |
11+
| test.py:139:5:139:26 | Function meth8 | This method does not accept keyword argument `y`, which overridden $@ does. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:96:5:96:26 | Function meth8 | Base2.meth8 | test.py:115:9:115:25 | Attribute() | This call |
12+
| test.py:147:5:147:21 | Function meth12 | This method does not accept arbitrary keyword arguments, which overridden $@ does. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:104:5:104:31 | Function meth12 | Base2.meth12 | test.py:119:9:119:24 | Attribute() | This call |
13+
| test.py:149:5:149:27 | Function meth13 | This method does not accept keyword argument `x`, which overridden $@ does. $@ correctly calls the base method, but does not match the signature of the overriding method. | test.py:106:5:106:27 | Function meth13 | Base2.meth13 | test.py:120:9:120:24 | Attribute() | This call |

‎python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
Functions/SignatureOverriddenMethod.ql
1+
query: Functions/SignatureOverriddenMethod.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
| test.py:19:9:19:31 | Attribute() | Keyword argument 'spam' is not a supported parameter name of $@. | test.py:5:5:5:20 | Function meth1 | method Base.meth1 |
2+
| test.py:112:9:112:23 | Attribute() | Keyword argument 'x' is not a supported parameter name of $@. | test.py:86:5:86:20 | Function meth3 | method Base2.meth3 |

0 commit comments

Comments
 (0)