Python: Modernize special method query

python/ql/src/Functions/SignatureSpecialMethods.ql

@@ -4,19 +4,23 @@
  * @kind problem
  * @tags reliability
  *       correctness
+ *       quality
  * @problem.severity error
  * @sub-severity low
  * @precision high
  * @id py/special-method-wrong-signature
  */
 
 import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch as DD
 
 predicate is_unary_op(string name) {
   name in [
       "__del__", "__repr__", "__neg__", "__pos__", "__abs__", "__invert__", "__complex__",
       "__int__", "__float__", "__long__", "__oct__", "__hex__", "__str__", "__index__", "__enter__",
-      "__hash__", "__bool__", "__nonzero__", "__unicode__", "__len__", "__iter__", "__reversed__"
+      "__hash__", "__bool__", "__nonzero__", "__unicode__", "__len__", "__iter__", "__reversed__",
+      "__aenter__", "__aiter__", "__anext__", "__await__", "__ceil__", "__floor__", "__trunc__",
+      "__length_hint__", "__dir__", "__bytes__"
     ]
 }
 
@@ -28,91 +32,138 @@ predicate is_binary_op(string name) {
       "__and__", "__xor__", "__or__", "__ne__", "__radd__", "__rsub__", "__rmul__", "__rfloordiv__",
       "__rdiv__", "__rtruediv__", "__rmod__", "__rdivmod__", "__rpow__", "__rlshift__", "__gt__",
       "__rrshift__", "__rand__", "__rxor__", "__ror__", "__iadd__", "__isub__", "__imul__",
-      "__ifloordiv__", "__idiv__", "__itruediv__", "__ge__", "__imod__", "__idivmod__", "__ipow__",
-      "__ilshift__", "__irshift__", "__iand__", "__ixor__", "__ior__", "__coerce__", "__cmp__",
-      "__rcmp__", "__getattr___", "__getattribute___"
+      "__ifloordiv__", "__idiv__", "__itruediv__", "__ge__", "__imod__", "__ipow__", "__ilshift__",
+      "__irshift__", "__iand__", "__ixor__", "__ior__", "__coerce__", "__cmp__", "__rcmp__",
+      "__getattr__", "__getattribute__", "__buffer__", "__release_buffer__", "__matmul__",
+      "__rmatmul__", "__imatmul__", "__missing__", "__class_getitem__", "__mro_entries__",
+      "__format__"
     ]
 }
 
 predicate is_ternary_op(string name) {
-  name in ["__setattr__", "__set__", "__setitem__", "__getslice__", "__delslice__"]
+  name in ["__setattr__", "__set__", "__setitem__", "__getslice__", "__delslice__", "__set_name__"]
 }
 
-predicate is_quad_op(string name) { name = "__setslice__" or name = "__exit__" }
+predicate is_quad_op(string name) { name in ["__setslice__", "__exit__", "__aexit__"] }
 
-int argument_count(PythonFunctionValue f, string name, ClassValue cls) {
-  cls.declaredAttribute(name) = f and
-  (
-    is_unary_op(name) and result = 1
-    or
-    is_binary_op(name) and result = 2
-    or
-    is_ternary_op(name) and result = 3
-    or
-    is_quad_op(name) and result = 4
-  )
+int argument_count(string name) {
+  is_unary_op(name) and result = 1
+  or
+  is_binary_op(name) and result = 2
+  or
+  is_ternary_op(name) and result = 3
+  or
+  is_quad_op(name) and result = 4
+}
+
+/**
+ * Returns 1 if `func` is a static method, and 0 otherwise. This predicate is used to adjust the
+ * number of expected arguments for a special method accordingly.
+ */
+int staticmethod_correction(Function func) {
+  if DD::isStaticmethod(func) then result = 1 else result = 0
 }
 
 predicate incorrect_special_method_defn(
-  PythonFunctionValue func, string message, boolean show_counts, string name, ClassValue owner
+  Function func, string message, boolean show_counts, string name, boolean is_unused_default
 ) {
-  exists(int required | required = argument_count(func, name, owner) |
+  exists(int required, int correction |
+    required = argument_count(name) - correction and correction = staticmethod_correction(func)
+  |
     /* actual_non_default <= actual */
-    if required > func.maxParameters()
-    then message = "Too few parameters" and show_counts = true
+    if required > func.getMaxPositionalArguments()
+    then message = "Too few parameters" and show_counts = true and is_unused_default = false
     else
-      if required < func.minParameters()
-      then message = "Too many parameters" and show_counts = true
+      if required < func.getMinPositionalArguments()
+      then message = "Too many parameters" and show_counts = true and is_unused_default = false
       else (
-        func.minParameters() < required and
-        not func.getScope().hasVarArg() and
-        message = (required - func.minParameters()) + " default values(s) will never be used" and
-        show_counts = false
+        func.getMinPositionalArguments() < required and
+        not func.hasVarArg() and
+        message =
+          (required - func.getMinPositionalArguments()) + " default values(s) will never be used" and
+        show_counts = false and
+        is_unused_default = true
       )
   )
 }
 
-predicate incorrect_pow(FunctionValue func, string message, boolean show_counts, ClassValue owner) {
-  owner.declaredAttribute("__pow__") = func and
-  (
-    func.maxParameters() < 2 and message = "Too few parameters" and show_counts = true
+predicate incorrect_pow(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 2 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() > 3 and message = "Too many parameters" and show_counts = true
+    func.getMinPositionalArguments() > 3 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() < 2 and
-    message = (2 - func.minParameters()) + " default value(s) will never be used" and
-    show_counts = false
+    func.getMinPositionalArguments() < 2 - correction and
+    message = (2 - func.getMinPositionalArguments()) + " default value(s) will never be used" and
+    show_counts = false and
+    is_unused_default = true
     or
-    func.minParameters() = 3 and
+    func.getMinPositionalArguments() = 3 - correction and
     message = "Third parameter to __pow__ should have a default value" and
-    show_counts = false
+    show_counts = false and
+    is_unused_default = false
   )
 }
 
-predicate incorrect_get(FunctionValue func, string message, boolean show_counts, ClassValue owner) {
-  owner.declaredAttribute("__get__") = func and
-  (
-    func.maxParameters() < 3 and message = "Too few parameters" and show_counts = true
+predicate incorrect_round(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 1 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() > 3 and message = "Too many parameters" and show_counts = true
+    func.getMinPositionalArguments() > 2 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() < 2 and
-    not func.getScope().hasVarArg() and
-    message = (2 - func.minParameters()) + " default value(s) will never be used" and
-    show_counts = false
+    func.getMinPositionalArguments() = 2 - correction and
+    message = "Second parameter to __round__ should have a default value" and
+    show_counts = false and
+    is_unused_default = false
   )
 }
 
-string should_have_parameters(PythonFunctionValue f, string name, ClassValue owner) {
-  exists(int i | i = argument_count(f, name, owner) | result = i.toString())
-  or
-  owner.declaredAttribute(name) = f and
-  (name = "__get__" or name = "__pow__") and
-  result = "2 or 3"
+predicate incorrect_get(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 3 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
+    or
+    func.getMinPositionalArguments() > 3 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
+    or
+    func.getMinPositionalArguments() < 2 - correction and
+    not func.hasVarArg() and
+    message = (2 - func.getMinPositionalArguments()) + " default value(s) will never be used" and
+    show_counts = false and
+    is_unused_default = true
+  )
 }
 
-string has_parameters(PythonFunctionValue f) {
-  exists(int i | i = f.minParameters() |
+string should_have_parameters(string name) {
+  if name in ["__pow__", "__get__"]
+  then result = "2 or 3"
+  else result = argument_count(name).toString()
+}
+
+string has_parameters(Function f) {
+  exists(int i | i = f.getMinPositionalArguments() |
     i = 0 and result = "no parameters"
     or
     i = 1 and result = "1 parameter"
@@ -121,23 +172,44 @@ string has_parameters(PythonFunctionValue f) {
   )
 }
 
+/** Holds if `f` is likely to be a placeholder, and hence not interesting enough to report. */
+predicate isLikelyPlaceholderFunction(Function f) {
+  // Body has only a single statement.
+  f.getBody().getItem(0) = f.getBody().getLastItem() and
+  (
+    // Body is a string literal. This is a common pattern for Zope interfaces.
+    f.getBody().getLastItem().(ExprStmt).getValue() instanceof StringLiteral
+    or
+    // Body just raises an exception.
+    f.getBody().getLastItem() instanceof Raise
+    or
+    // Body is a pass statement.
+    f.getBody().getLastItem() instanceof Pass
+  )
+}
+
 from
-  PythonFunctionValue f, string message, string sizes, boolean show_counts, string name,
-  ClassValue owner
+  Function f, string message, string sizes, boolean show_counts, string name, Class owner,
+  boolean show_unused_defaults
 where
+  owner.getAMethod() = f and
+  f.getName() = name and
   (
-    incorrect_special_method_defn(f, message, show_counts, name, owner)
+    incorrect_special_method_defn(f, message, show_counts, name, show_unused_defaults)
+    or
+    incorrect_pow(f, message, show_counts, show_unused_defaults) and name = "__pow__"
     or
-    incorrect_pow(f, message, show_counts, owner) and name = "__pow__"
+    incorrect_get(f, message, show_counts, show_unused_defaults) and name = "__get__"
     or
-    incorrect_get(f, message, show_counts, owner) and name = "__get__"
+    incorrect_round(f, message, show_counts, show_unused_defaults) and
+    name = "__round__"
   ) and
+  not isLikelyPlaceholderFunction(f) and
+  show_unused_defaults = false and
   (
     show_counts = false and sizes = ""
     or
     show_counts = true and
-    sizes =
-      ", which has " + has_parameters(f) + ", but should have " +
-        should_have_parameters(f, name, owner)
+    sizes = ", which has " + has_parameters(f) + ", but should have " + should_have_parameters(name)
   )
 select f, message + " for special method " + name + sizes + ", in class $@.", owner, owner.getName()

python/ql/src/change-notes/2025-03-20-modernize-special-method-wrong-signature-query.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+- The `py/special-method-wrong-signature` has been modernized and rewritten to no longer rely on outdated APIs. Moreover, the query no longer flags cases where a default value is never used, as these alerts were rarely useful.

python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected

@@ -1,9 +1,6 @@
-| om_test.py:59:5:59:28 | Function WrongSpecials.__div__ | Too many parameters for special method __div__, which has 3 parameters, but should have 2, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:62:5:62:22 | Function WrongSpecials.__mul__ | Too few parameters for special method __mul__, which has 1 parameter, but should have 2, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:65:5:65:29 | Function WrongSpecials.__neg__ | Too many parameters for special method __neg__, which has 2 parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:68:5:68:35 | Function WrongSpecials.__exit__ | Too few parameters for special method __exit__, which has 3 parameters, but should have 4, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:71:5:71:19 | Function WrongSpecials.__repr__ | Too few parameters for special method __repr__, which has no parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:74:5:74:46 | Function WrongSpecials.__add__ | 1 default values(s) will never be used for special method __add__, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:97:15:97:34 | Function NotOKSpecials.lambda | Too few parameters for special method __sub__, which has 1 parameter, but should have 2, in class $@. | om_test.py:95:1:95:28 | class NotOKSpecials | NotOKSpecials |
-| protocols.py:107:1:107:12 | Function f | Too few parameters for special method __add__, which has 1 parameter, but should have 2, in class $@. | protocols.py:110:1:110:29 | class MissingMethods | MissingMethods |
-| protocols.py:107:1:107:12 | Function f | Too few parameters for special method __set__, which has 1 parameter, but should have 3, in class $@. | protocols.py:110:1:110:29 | class MissingMethods | MissingMethods |
+| om_test.py:59:5:59:28 | Function __div__ | Too many parameters for special method __div__, which has 3 parameters, but should have 2, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:62:5:62:22 | Function __mul__ | Too few parameters for special method __mul__, which has 1 parameter, but should have 2, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:65:5:65:29 | Function __neg__ | Too many parameters for special method __neg__, which has 2 parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:68:5:68:35 | Function __exit__ | Too few parameters for special method __exit__, which has 3 parameters, but should have 4, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:71:5:71:19 | Function __repr__ | Too few parameters for special method __repr__, which has no parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:83:5:83:18 | Function __del__ | Too few parameters for special method __del__, which has no parameters, but should have 1, in class $@. | om_test.py:81:1:81:25 | Class OKSpecials | OKSpecials |

python/ql/test/query-tests/Functions/general/om_test.py

@@ -69,11 +69,11 @@ def __exit__(self, arg0, arg1):
         return arg0 == arg1
 
     def __repr__():
-        pass
+        return ""
 
     def __add__(self, other="Unused default"):
-       pass
-   
+       return 4
+
     @staticmethod
     def __abs__():
         return 42
@@ -105,3 +105,7 @@ def pop(self):
 
 
 
+class MoreSpecialMethods:
+    @staticmethod
+    def __abs__():
+        return 42