diff --git a/python/ql/src/Functions/IncorrectlyOverriddenMethod.ql b/python/ql/src/Functions/IncorrectlyOverriddenMethod.ql index a46a2370c0e..a4e3bd6e0ce 100644 --- a/python/ql/src/Functions/IncorrectlyOverriddenMethod.ql +++ b/python/ql/src/Functions/IncorrectlyOverriddenMethod.ql @@ -1,4 +1,5 @@ /** + * @deprecated * @name Mismatch between signature and use of an overriding method * @description Method has a different signature from the overridden method and, if it were called, would be likely to cause an error. * @kind problem diff --git a/python/ql/src/Functions/SignatureOverriddenMethod.py b/python/ql/src/Functions/SignatureOverriddenMethod.py index 7beddcb9e95..e3522351fc0 100644 --- a/python/ql/src/Functions/SignatureOverriddenMethod.py +++ b/python/ql/src/Functions/SignatureOverriddenMethod.py @@ -1,9 +1,12 @@ -# Base class method -def runsource(self, source, filename="", symbol="single"): - ... # Definition +class Base: + def runsource(self, source, filename=""): + ... -# Extend base class method -def runsource(self, source): - ... # Definition \ No newline at end of file +class Sub(Base): + def runsource(self, source): # BAD: Does not match the signature of overridden method. + ... + +def run(obj: Base): + obj.runsource("source", filename="foo.txt") \ No newline at end of file diff --git a/python/ql/src/Functions/SignatureOverriddenMethod.qhelp b/python/ql/src/Functions/SignatureOverriddenMethod.qhelp index b7da2678e3d..0575d967d3e 100644 --- a/python/ql/src/Functions/SignatureOverriddenMethod.qhelp +++ b/python/ql/src/Functions/SignatureOverriddenMethod.qhelp @@ -5,32 +5,25 @@ -

There are one (or more) legal parameters for an overridden method that are -not legal for an overriding method. This will cause an error when the overriding -method is called with a number of parameters that is legal for the overridden method. -This violates the Liskov substitution principle. +

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 +may not be a valid call to the subclass method, and thus raise an exception if an instance of the subclass is passed instead. +If following the Liskov Substitution Principle, in which an instance of a subclass should be usable in every context as though it were an +instance of the base class, this behavior breaks the principle.

-

Ensure that the overriding method accepts all the parameters that are legal for -overridden method.

+

Ensure that the overriding method in the subclass accepts the same parameters as the base method.

-

In this example there is a mismatch between the legal parameters for the base -class method (self, source, filename, symbol) and the extension method -(self, source). The extension method can be used to override the base -method as long as values are not specified for the filename and -symbol parameters. If the extension method was passed the additional -parameters accepted by the base method then an error would occur.

+

In the following example, Base.runsource takes an optional filename argument. However, the overriding method +Sub.runsource does not. This means the run function will fail if passed an instance of Sub. +

-

The extension method should be updated to support the filename and -symbol parameters supported by the overridden method.

- diff --git a/python/ql/src/Functions/SignatureOverriddenMethod.ql b/python/ql/src/Functions/SignatureOverriddenMethod.ql index 3e3877bc139..15b3fa70640 100644 --- a/python/ql/src/Functions/SignatureOverriddenMethod.ql +++ b/python/ql/src/Functions/SignatureOverriddenMethod.ql @@ -13,23 +13,254 @@ */ import python -import Expressions.CallArgs +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.internal.DataFlowDispatch +import codeql.util.Option -from FunctionValue base, PythonFunctionValue derived -where - not exists(base.getACall()) and - not exists(FunctionValue a_derived | - a_derived.overrides(base) and - exists(a_derived.getACall()) - ) and - not derived.getScope().isSpecialMethod() and - derived.getName() != "__init__" and - derived.isNormalMethod() and - // call to overrides distributed for efficiency - ( - derived.overrides(base) and derived.minParameters() > base.maxParameters() - or - derived.overrides(base) and derived.maxParameters() < base.minParameters() +/** Holds if `base` is overridden by `sub` */ +predicate overrides(Function base, Function sub) { + base.getName() = sub.getName() and + base.getScope() = getADirectSuperclass+(sub.getScope()) +} + +/** Constructs a string to pluralize `str` depending on `num`. */ +bindingset[num, str] +string plural(int num, string str) { + num = 1 and result = "1 " + str + or + num != 1 and result = num.toString() + " " + str + "s" +} + +/** Describes the minimum number of arguments `func` can accept, using "at least" if it may accept more. */ +string describeMin(Function func) { + exists(string descr | descr = plural(func.getMinPositionalArguments(), "positional argument") | + if func.getMinPositionalArguments() = func.getMaxPositionalArguments() + then result = descr + else result = "at least " + descr ) -select derived, "Overriding method '" + derived.getName() + "' has signature mismatch with $@.", - base, "overridden method" +} + +/** 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. */ +string describeMax(Function func) { + if func.hasVarArg() + then result = "arbitrarily many positional arguments" + else + exists(string descr | descr = plural(func.getMaxPositionalArguments(), "positional argument") | + if func.getMinPositionalArguments() = func.getMaxPositionalArguments() + then result = descr + else result = "at most " + descr + ) +} + +/** Describes the minimum number of arguments `func` can accept, without repeating "positional arguments". */ +string describeMinShort(Function func) { + exists(string descr | descr = func.getMinPositionalArguments().toString() | + if func.getMinPositionalArguments() = func.getMaxPositionalArguments() + then result = descr + else result = "at least " + descr + ) +} + +/** Describes the maximum number of arguments `func` can accept, without repeating "positional arguments". */ +string describeMaxShort(Function func) { + if func.hasVarArg() + then result = "arbitrarily many" + else + exists(string descr | descr = func.getMaxPositionalArguments().toString() | + if func.getMinPositionalArguments() = func.getMaxPositionalArguments() + then result = descr + else result = "at most " + descr + ) +} + +/** Describe an upper bound on the number of arguments `func` may accept, without specifying "at most". */ +string describeMaxBound(Function func) { + if func.hasVarArg() + then result = "arbitrarily many" + else result = func.getMaxPositionalArguments().toString() +} + +/** Holds if no way to call `base` would be valid for `sub`. The `msg` applies to the `sub method. */ +predicate strongSignatureMismatch(Function base, Function sub, string msg) { + overrides(base, sub) and + ( + sub.getMinPositionalArguments() > base.getMaxPositionalArguments() and + msg = + "requires " + describeMin(sub) + ", whereas overridden $@ requires " + describeMaxShort(base) + + "." + or + sub.getMaxPositionalArguments() < base.getMinPositionalArguments() and + msg = + "requires " + describeMax(sub) + ", whereas overridden $@ requires " + describeMinShort(base) + + "." + ) +} + +/** Holds if there may be some ways to call `base` that would not be valid for `sub`. The `msg` applies to the `sub` method. */ +predicate weakSignatureMismatch(Function base, Function sub, string msg) { + overrides(base, sub) and + ( + sub.getMinPositionalArguments() > base.getMinPositionalArguments() and + msg = + "requires " + describeMin(sub) + ", whereas overridden $@ may be called with " + + base.getMinPositionalArguments().toString() + "." + or + sub.getMaxPositionalArguments() < base.getMaxPositionalArguments() and + msg = + "requires " + describeMax(sub) + ", whereas overridden $@ may be called with " + + describeMaxBound(base) + "." + or + sub.getMinPositionalArguments() <= base.getMinPositionalArguments() and + sub.getMaxPositionalArguments() >= base.getMaxPositionalArguments() and + exists(string arg | + // TODO: positional-only args not considered + // e.g. `def foo(x, y, /, z):` has x,y as positional only args, should not be considered as possible kw args + // However, this likely does not create FPs, as we require a 'witness' call to generate an alert. + arg = base.getAnArg().getName() and + not arg = sub.getAnArg().getName() and + not exists(sub.getKwarg()) and + msg = "does not accept keyword argument `" + arg + "`, which overridden $@ does." + ) + or + exists(base.getKwarg()) and + not exists(sub.getKwarg()) and + msg = "does not accept arbitrary keyword arguments, which overridden $@ does." + ) +} + +/** Holds if `f` should be ignored for considering signature mismatches. */ +predicate ignore(Function f) { + isClassmethod(f) + or + exists( + Function g // other functions with the same name, e.g. @property getters/setters. + | + g.getScope() = f.getScope() and + g.getName() = f.getName() and + g != f + ) +} + +/** Gets a function that `call` may resolve to. */ +Function resolveCall(Call call) { + exists(DataFlowCall dfc | call = dfc.getNode().(CallNode).getNode() | + result = viableCallable(dfc).(DataFlowFunction).getScope() + ) +} + +/** Holds if `call` may resolve to either `base` or `sub`, and `base` is overridden by `sub`. */ +predicate callViableForEitherOverride(Function base, Function sub, Call call) { + overrides(base, sub) and + base = resolveCall(call) and + sub = resolveCall(call) +} + +/** Holds if either both `base` and `sub` are static methods, or both are not static methods, and `base` is overridden by `sub`. */ +predicate matchingStatic(Function base, Function sub) { + overrides(base, sub) and + ( + isStaticmethod(base) and + isStaticmethod(sub) + or + not isStaticmethod(base) and + not isStaticmethod(sub) + ) +} + +int extraSelfArg(Function func) { if isStaticmethod(func) then result = 0 else result = 1 } + +/** Holds if the call `call` matches the signature for `func`. */ +predicate callMatchesSignature(Function func, Call call) { + func = resolveCall(call) and + ( + // Each parameter of the function is accounted for in the call + forall(Parameter param, int i | param = func.getArg(i) | + // self arg + i = 0 and not isStaticmethod(func) + or + // positional arg + i - extraSelfArg(func) < call.getPositionalArgumentCount() + or + // has default + exists(param.getDefault()) + or + // keyword arg + call.getANamedArgumentName() = param.getName() + ) + or + // arbitrary varargs or kwargs + exists(call.getStarArg()) + or + exists(call.getKwargs()) + ) and + // No excess parameters + call.getPositionalArgumentCount() + extraSelfArg(func) <= func.getMaxPositionalArguments() and + ( + exists(func.getKwarg()) + or + forall(string name | name = call.getANamedArgumentName() | exists(func.getArgByName(name))) + ) +} + +pragma[nomagic] +private File getFunctionFile(Function f) { result = f.getLocation().getFile() } + +/** Gets a call which matches the signature of `base`, but not of overridden `sub`. */ +Call getASignatureMismatchWitness(Function base, Function sub) { + callViableForEitherOverride(base, sub, result) and + callMatchesSignature(base, result) and + not callMatchesSignature(sub, result) +} + +pragma[inline] +string preferredFile(File callFile, Function base, Function sub) { + if callFile = getFunctionFile(base) + then result = " A" + else + if callFile = getFunctionFile(sub) + then result = " B" + else result = callFile.getAbsolutePath() +} + +/** Choose a 'witnessing' call that matches the signature of `base` but not of overridden `sub`. */ +Call chooseASignatureMismatchWitness(Function base, Function sub) { + exists(getASignatureMismatchWitness(base, sub)) and + result = + min(Call c | + c = getASignatureMismatchWitness(base, sub) + | + c + order by + preferredFile(c.getLocation().getFile(), base, sub), c.getLocation().getStartLine(), + c.getLocation().getStartColumn() + ) +} + +module CallOption = LocatableOption; + +from Function base, Function sub, string msg, string extraMsg, CallOption::Option call +where + not sub.isSpecialMethod() and + sub.getName() != "__init__" and + not ignore(sub) and + not ignore(base) and + matchingStatic(base, sub) and + ( + // If we have a witness, alert for a 'weak' mismatch, but prefer the message for a 'strong' mismatch if that holds. + call.asSome() = chooseASignatureMismatchWitness(base, sub) and + extraMsg = + " $@ correctly calls the base method, but does not match the signature of the overriding method." and + ( + strongSignatureMismatch(base, sub, msg) + or + not strongSignatureMismatch(base, sub, _) and + weakSignatureMismatch(base, sub, msg) + ) + or + // With no witness, only alert for 'strong' mismatches. + not exists(getASignatureMismatchWitness(base, sub)) and + call.isNone() and + strongSignatureMismatch(base, sub, msg) and + extraMsg = "" + ) +select sub, "This method " + msg + extraMsg, base, base.getQualifiedName(), call, "This call" diff --git a/python/ql/src/change-notes/2025-08-19-signature-mismatch.md b/python/ql/src/change-notes/2025-08-19-signature-mismatch.md new file mode 100644 index 00000000000..60c3efa32eb --- /dev/null +++ b/python/ql/src/change-notes/2025-08-19-signature-mismatch.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* The `py/inheritance/signature-mismatch` query has been modernized. It produces more precise results and more descriptive alert messages. +* The `py/inheritance/incorrect-overriding-signature` query has been deprecated. Its results have been consolidated into the `py/inheritance/signature-mismatch` query. \ No newline at end of file diff --git a/python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected b/python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected index 81c45ab8abe..21b11212075 100644 --- a/python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected +++ b/python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected @@ -1,2 +1,2 @@ -| 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 | -| 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 | +| 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 | +| 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 | diff --git a/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected b/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected index bcb9363a55a..07756ee4600 100644 --- a/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected +++ b/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected @@ -1 +1,13 @@ -| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | diff --git a/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref b/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref index a306477b3b4..5470a05e0e4 100644 --- a/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref +++ b/python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref @@ -1 +1,2 @@ -Functions/SignatureOverriddenMethod.ql +query: Functions/SignatureOverriddenMethod.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected b/python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected index d2fc2ef6784..0cadf5e5fbf 100644 --- a/python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected +++ b/python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected @@ -1 +1,2 @@ | 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 | +| 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 | diff --git a/python/ql/test/query-tests/Functions/overriding/test.py b/python/ql/test/query-tests/Functions/overriding/test.py index c4c7caaa1aa..63ee50e820c 100644 --- a/python/ql/test/query-tests/Functions/overriding/test.py +++ b/python/ql/test/query-tests/Functions/overriding/test.py @@ -21,13 +21,13 @@ class Base(object): class Derived(Base): - def meth1(self, spam): + def meth1(self, spam): # $Alert[py/inheritance/signature-mismatch] # Has 1 more arg, base called in Base.foo pass - def meth2(self): + def meth2(self): # $Alert[py/inheritance/signature-mismatch] # Has 1 fewer arg, base called in Base.foo pass - def meth3(self, eggs): #Incorrectly overridden and not called. + def meth3(self, eggs): # $Alert[py/inheritance/signature-mismatch] # Has 1 more arg. Method is not called. pass def bar(self): @@ -66,13 +66,84 @@ class BlameBase(object): class Correct1(BlameBase): - def meth(self, arg): + def meth(self, arg): # $Alert[py/inheritance/signature-mismatch] # Has 1 more arg. The incorrect-overridden-method query would alert for the base method in this case. pass class Correct2(BlameBase): - def meth(self, arg): + def meth(self, arg): # $Alert[py/inheritance/signature-mismatch] # Has 1 more arg pass c = Correct2() c.meth("hi") + +class Base2: + + def meth1(self, x=1): pass + + def meth2(self, x=1): pass + + def meth3(self): pass + + def meth4(self, x=1): pass + + def meth5(self, x, y, z, w=1): pass + + def meth6(self, x, *ys): pass + + def meth7(self, *ys): pass + + def meth8(self, x, y): pass + + def meth9(self, x, y): pass + + def meth10(self, x, *, y=3): pass + + def meth11(self, x, y): pass + + def meth12(self, **kwargs): pass + + def meth13(self, /, x): pass + + def call_some(self): + self.meth1() + self.meth1(x=2) + self.meth3() + self.meth3(x=2) + self.meth6(2, 3, 4) + self.meth7() + self.meth8(1,y=3) + self.meth9(1,2) + self.meth10(1,y=3) + self.meth11(1,y=3) + self.meth12(x=2) + self.meth13(x=2) + + +class Derrived2(Base2): + + def meth1(self): pass # $Alert[py/inheritance/signature-mismatch] # Weak mismatch (base may be called with 2 args. only alert if mismatching call exists) + + def meth2(self): pass # No alert (weak mismatch, but not called) + + def meth3(self, x=1): pass # No alert (no mismatch - all base calls are valid for sub) + + def meth4(self, x, y, z=1): pass # $Alert[py/inheritance/signature-mismatch] # sub min > base max (strong mismatch) + + def meth5(self, x, y=1): pass # $Alert[py/inheritance/signature-mismatch] + + def meth6(self, x): pass # $Alert[py/inheritance/signature-mismatch] # weak mismatch (base may be called with 3+ args) + + def meth7(self, x, *ys): pass # $Alert[py/inheritance/signature-mismatch] # weak mismatch (base may be called with 1 arg only) + + def meth8(self, x, z): pass # $Alert[py/inheritance/signature-mismatch] # weak mismatch (base may be called with arg named y) + + def meth9(self, x, z): pass # No alert (never called with wrong keyword arg) + + def meth10(self, x, **kwargs): pass # No alert (y is kw-only arg in base, calls that use it are valid for sub) + + def meth11(self, x, z, **kwargs): pass # $MISSING:Alert[py/inheritance/signature-mismatch] # call using y kw-arg is invalid due to not specifying z, but this is not detected. Likely a fairly niche situation. + + def meth12(self): pass # $Alert[py/inheritance/signature-mismatch] # call including extra kwarg invalid + + def meth13(self, /, y): pass # $Alert[py/inheritance/signature-mismatch] # weak mismatch (base may be called with arg named x), however meth13 is incorrectly detected as having 2 minimum positional arguments, whereas x is kw-only; resulting in the witness call not being detected as a valid call to Base2.meth13.