From 4212d1b5b65bede97298d5d9e67efe8dd1563971 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Wed, 13 Aug 2025 10:18:29 +0100
Subject: [PATCH] Update alert messages and choose one witness

---
 .../Functions/SignatureOverriddenMethod.ql    | 134 ++++++++++++++++--
 1 file changed, 120 insertions(+), 14 deletions(-)

diff --git a/python/ql/src/Functions/SignatureOverriddenMethod.ql b/python/ql/src/Functions/SignatureOverriddenMethod.ql
index 74c5aaa477e..3bd8fb6bf31 100644
--- a/python/ql/src/Functions/SignatureOverriddenMethod.ql
+++ b/python/ql/src/Functions/SignatureOverriddenMethod.ql
@@ -13,11 +13,19 @@
  */
 
 import python
+import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.internal.DataFlowDispatch
 
 predicate overrides(Function base, Function sub) {
   base.getName() = sub.getName() and
-  base.getScope() = getADirectSuperclass*(sub.getScope())
+  base.getScope() = getADirectSuperclass+(sub.getScope())
+}
+
+bindingset[num, str]
+string plural(int num, string str) {
+  num = 1 and result = "1 " + str
+  or
+  num != 1 and result = num.toString() + " " + str + "s"
 }
 
 /** Holds if no way to call `base` would be valid for `sub`. The `msg` applies to the `sub method. */
@@ -25,10 +33,16 @@ predicate strongSignatureMismatch(Function base, Function sub, string msg) {
   overrides(base, sub) and
   (
     sub.getMinPositionalArguments() > base.getMaxPositionalArguments() and
-    msg = "requires more positional arguments than overridden $@ allows."
+    msg =
+      "requires " +
+        plural(sub.getMinPositionalArguments() - base.getMaxPositionalArguments(),
+          "more positional argument") + " than overridden $@ allows."
     or
     sub.getMaxPositionalArguments() < base.getMinPositionalArguments() and
-    msg = "requires fewer positional arguments than overridden $@ allows."
+    msg =
+      "requires " +
+        plural(base.getMinPositionalArguments() - sub.getMaxPositionalArguments(),
+          "fewer positional argument") + " than overridden $@ allows."
   )
 }
 
@@ -37,18 +51,26 @@ predicate weakSignatureMismatch(Function base, Function sub, string msg) {
   overrides(base, sub) and
   (
     sub.getMinPositionalArguments() > base.getMinPositionalArguments() and
-    msg = "requires more positional arguments than overridden $@ may accept."
+    msg =
+      "requires " +
+        plural(sub.getMinPositionalArguments() - base.getMinPositionalArguments(),
+          "more positional argument") + "than some possible calls to overridden $@."
     or
     sub.getMaxPositionalArguments() < base.getMaxPositionalArguments() and
-    msg = "requires fewer positional arguments than overridden $@ may accept."
+    msg =
+      "requires " +
+        plural(base.getMaxPositionalArguments() - sub.getMaxPositionalArguments(),
+          "fewer positional argument") + " than some possible calls to overridden $@."
     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
       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."
+      msg = "does not accept keyword argument `" + arg + "`, which overridden $@ does."
     )
     or
     exists(base.getKwarg()) and
@@ -67,16 +89,100 @@ predicate ignore(Function f) {
   )
 }
 
-from Function base, Function sub, string msg
+Function resolveCall(Call call) {
+  exists(DataFlowCall dfc | call = dfc.getNode().(CallNode).getNode() |
+    result = viableCallable(dfc).(DataFlowFunction).getScope()
+  )
+}
+
+predicate callViableForEither(Function base, Function sub, Call call) {
+  overrides(base, sub) and
+  base = resolveCall(call) and
+  sub = resolveCall(call)
+}
+
+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 }
+
+predicate callMatchesSignature(Function func, Call call) {
+  (
+    call.getPositionalArgumentCount() + extraSelfArg(func) >= func.getMinPositionalArguments()
+    or
+    exists(call.getStarArg())
+    or
+    exists(call.getKwargs())
+  ) and
+  call.getPositionalArgumentCount() + extraSelfArg(func) <= func.getMaxPositionalArguments() and
+  (
+    exists(func.getKwarg())
+    or
+    forall(string name | name = call.getANamedArgumentName() | exists(func.getArgByName(name)))
+  )
+}
+
+Call getASignatureMismatchWitness(Function base, Function sub) {
+  callViableForEither(base, sub, result) and
+  callMatchesSignature(base, result) and
+  not callMatchesSignature(sub, result)
+}
+
+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()
+    )
+}
+
+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, base.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()
+      )
+  )
+}
+
+from Function base, Function sub, string msg, string extraMsg, Call call
 where
-  // not exists(base.getACall()) and
-  // not exists(FunctionValue a_derived |
-  //   a_derived.overrides(base) and
-  //   exists(a_derived.getACall())
-  // ) and
   not sub.isSpecialMethod() and
   sub.getName() != "__init__" and
   not ignore(sub) and
   not ignore(base) and
-  strongSignatureMismatch(base, sub, msg)
-select sub, "This method " + msg, base, base.getQualifiedName()
+  matchingStatic(base, sub) and
+  weakSignatureMismatch(base, sub, msg) and
+  //msg = " has a different signature to $@." and
+  call = chooseASignatureMismatchWitness(base, sub) and
+  extraMsg =
+    " $@ correctly calls the base method, but does not match the signature of the overriding method."
+select sub, "This method " + msg + extraMsg, base, base.getQualifiedName(), call, "This call"