Python: Modernize the Signature Mismatch query

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

python/ql/src/Functions/SignatureOverriddenMethod.py

@@ -1,9 +1,12 @@
 
-# Base class method
-def runsource(self, source, filename="<input>", symbol="single"):
-    ... # Definition
+class Base:
+    def runsource(self, source, filename="<input>"):
+        ...
 
 
-# Extend base class method
-def runsource(self, source):
-    ... # Definition
+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")

python/ql/src/Functions/SignatureOverriddenMethod.qhelp

@@ -5,32 +5,25 @@
 
 
 <overview>
-<p> 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.
+<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 
+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.
 </p>
 
 </overview>
 <recommendation>
 
-<p>Ensure that the overriding method accepts all the parameters that are legal for
-overridden method.</p>
+<p>Ensure that the overriding method in the subclass accepts the same parameters as the base method. </p>
 
 </recommendation>
 <example>
-<p>In this example there is a mismatch between the legal parameters for the base 
-class method <code>(self, source, filename, symbol)</code> and the extension method 
-<code>(self, source)</code>. The extension method can be used to override the base 
-method as long as values are not specified for the <code>filename</code> and 
-<code>symbol</code> parameters. If the extension method was passed the additional 
-parameters accepted by the base method then an error would occur.</p>
+<p>In the following example, <code>Base.runsource</code> takes an optional <code>filename</code> argument. However, the overriding method 
+<code>Sub.runsource</code> does not. This means the <code>run</code> function will fail if passed an instance of <code>Sub</code>.
+</p>
 
 <sample src="SignatureOverriddenMethod.py" />
 
-<p>The extension method should be updated to support the <code>filename</code> and 
-<code>symbol</code> parameters supported by the overridden method.</p>
-
 </example>
 <references>
 

python/ql/src/Functions/SignatureOverriddenMethod.ql

@@ -13,23 +13,266 @@
  */
 
 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())
+/** Holds if `base` is overriden 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
+  )
+}
+
+/** 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 minumum 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 overiden 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 overriden 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
-  not derived.getScope().isSpecialMethod() and
-  derived.getName() != "__init__" and
-  derived.isNormalMethod() and
-  // call to overrides distributed for efficiency
+  // No excess parameters
+  call.getPositionalArgumentCount() + extraSelfArg(func) <= func.getMaxPositionalArguments() and
+  (
+    exists(func.getKwarg())
+    or
+    forall(string name | name = call.getANamedArgumentName() | exists(func.getArgByName(name)))
+  )
+}
+
+/** Gets a call which matches the signature of `base`, but not of overriden `sub`. */
+Call getASignatureMismatchWitness(Function base, Function sub) {
+  callViableForEitherOverride(base, sub, result) and
+  callMatchesSignature(base, result) and
+  not callMatchesSignature(sub, result)
+}
+
+/** Choose a 'witnessing' call that matches the signature of `base` but not of overridden `sub`, and is in the file `file`. */
+Call chooseASignatureMismatchWitnessInFile(Function base, Function sub, File file) {
+  result =
+    min(Call c |
+      c = getASignatureMismatchWitness(base, sub) and
+      c.getLocation().getFile() = file
+    |
+      c order by c.getLocation().getStartLine(), c.getLocation().getStartColumn()
+    )
+}
+
+/** 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 = chooseASignatureMismatchWitnessInFile(base, sub, base.getLocation().getFile())
+    or
+    not exists(Call c |
+      c = getASignatureMismatchWitness(base, sub) and
+      c.getLocation().getFile() = base.getLocation().getFile()
+    ) and
+    result = chooseASignatureMismatchWitnessInFile(base, sub, sub.getLocation().getFile())
+    or
+    not exists(Call c |
+      c = getASignatureMismatchWitness(base, sub) and
+      c.getLocation().getFile() = [base, sub].getLocation().getFile()
+    ) and
+    result =
+      min(Call c |
+        c = getASignatureMismatchWitness(base, sub)
+      |
+        c
+        order by
+          c.getLocation().getFile().getAbsolutePath(), c.getLocation().getStartLine(),
+          c.getLocation().getStartColumn()
+      )
+  )
+}
+
+module CallOption = LocatableOption<Location, Call>;
+
+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
   (
-    derived.overrides(base) and derived.minParameters() > base.maxParameters()
+    // 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
-    derived.overrides(base) and derived.maxParameters() < base.minParameters()
+    // With no witness, only alert for 'strong' mismatches.
+    not exists(getASignatureMismatchWitness(base, sub)) and
+    call.isNone() and
+    strongSignatureMismatch(base, sub, msg) and
+    extraMsg = ""
   )
-select derived, "Overriding method '" + derived.getName() + "' has signature mismatch with $@.",
-  base, "overridden method"
+select sub, "This method " + msg + extraMsg, base, base.getQualifiedName(), call, "This call"

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.

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 |

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 |

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

@@ -1 +1,2 @@
-Functions/SignatureOverriddenMethod.ql
+query: Functions/SignatureOverriddenMethod.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

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 |