Update tests, update alert messages

Author: joefarebrother

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.ql

@@ -29,21 +29,63 @@ string plural(int num, string str) {
   num != 1 and result = num.toString() + " " + str + "s"
 }
 
+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
+  )
+}
+
+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
+    )
+}
+
+string describeMinShort(Function func) {
+  exists(string descr | descr = func.getMinPositionalArguments().toString() |
+    if func.getMinPositionalArguments() = func.getMaxPositionalArguments()
+    then result = descr
+    else result = "at least " + descr
+  )
+}
+
+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
+    )
+}
+
+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 " +
-        plural(sub.getMinPositionalArguments() - base.getMaxPositionalArguments(),
-          "more positional argument") + " than overridden $@ allows."
+      "requires " + describeMin(sub) + ", whereas overridden $@ requires " + describeMaxShort(base) +
+        "."
     or
     sub.getMaxPositionalArguments() < base.getMinPositionalArguments() and
     msg =
-      "requires " +
-        plural(base.getMinPositionalArguments() - sub.getMaxPositionalArguments(),
-          "fewer positional argument") + " than overridden $@ allows."
+      "requires " + describeMax(sub) + ", whereas overridden $@ requires " + describeMinShort(base) +
+        "."
   )
 }
 
@@ -53,15 +95,13 @@ predicate weakSignatureMismatch(Function base, Function sub, string msg) {
   (
     sub.getMinPositionalArguments() > base.getMinPositionalArguments() and
     msg =
-      "requires " +
-        plural(sub.getMinPositionalArguments() - base.getMinPositionalArguments(),
-          "more positional argument") + " than some possible calls to overridden $@."
+      "requires " + describeMin(sub) + ", whereas overridden $@ may be called with " +
+        base.getMinPositionalArguments().toString() + "."
     or
     sub.getMaxPositionalArguments() < base.getMaxPositionalArguments() and
     msg =
-      "requires " +
-        plural(base.getMaxPositionalArguments() - sub.getMaxPositionalArguments(),
-          "fewer positional argument") + " than some possible calls to overridden $@."
+      "requires " + describeMax(sub) + ", whereas overridden $@ may be called with " +
+        describeMaxBound(base) + "."
     or
     sub.getMinPositionalArguments() <= base.getMinPositionalArguments() and
     sub.getMaxPositionalArguments() >= base.getMaxPositionalArguments() and
@@ -83,7 +123,9 @@ predicate weakSignatureMismatch(Function base, Function sub, string msg) {
 predicate ignore(Function f) {
   isClassmethod(f)
   or
-  exists(Function g |
+  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
@@ -96,7 +138,7 @@ Function resolveCall(Call call) {
   )
 }
 
-predicate callViableForEither(Function base, Function sub, Call call) {
+predicate callViableForEitherOverride(Function base, Function sub, Call call) {
   overrides(base, sub) and
   base = resolveCall(call) and
   sub = resolveCall(call)
@@ -132,7 +174,7 @@ predicate callMatchesSignature(Function func, Call call) {
 }
 
 Call getASignatureMismatchWitness(Function base, Function sub) {
-  callViableForEither(base, sub, result) and
+  callViableForEitherOverride(base, sub, result) and
   callMatchesSignature(base, result) and
   not callMatchesSignature(sub, result)
 }
@@ -195,6 +237,7 @@ where
     )
     or
     not exists(getASignatureMismatchWitness(base, sub)) and
+    call.isNone() and
     strongSignatureMismatch(base, sub, msg) and
     extraMsg = ""
   )

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

@@ -1 +1,5 @@
-| 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 positional arguments. $@ 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 |

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/test.py

@@ -21,13 +21,13 @@ def foo(self):
 
 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,12 +66,12 @@ def meth(self):
 
 class Correct1(BlameBase):
 
-    def meth(self, arg):
+    def meth(self, arg): # $Alert[py/inheritance/signature-mismatch] # Has 1 more arg. The incorrect-overriden-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()