diff --git a/python/ql/src/Functions/SignatureSpecialMethods.ql b/python/ql/src/Functions/SignatureSpecialMethods.ql index feedb6c94b6..caa6f8c1614 100644 --- a/python/ql/src/Functions/SignatureSpecialMethods.ql +++ b/python/ql/src/Functions/SignatureSpecialMethods.ql @@ -4,6 +4,7 @@ * @kind problem * @tags reliability * correctness + * quality * @problem.severity error * @sub-severity low * @precision high @@ -11,12 +12,15 @@ */ 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) { ) } -from - PythonFunctionValue f, string message, string sizes, boolean show_counts, string name, - ClassValue owner -where +/** 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 ( - incorrect_special_method_defn(f, message, show_counts, name, owner) + // Body is a string literal. This is a common pattern for Zope interfaces. + f.getBody().getLastItem().(ExprStmt).getValue() instanceof StringLiteral or - incorrect_pow(f, message, show_counts, owner) and name = "__pow__" + // Body just raises an exception. + f.getBody().getLastItem() instanceof Raise or - incorrect_get(f, message, show_counts, owner) and name = "__get__" + // Body is a pass statement. + f.getBody().getLastItem() instanceof Pass + ) +} + +from + 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, show_unused_defaults) + or + incorrect_pow(f, message, show_counts, show_unused_defaults) and name = "__pow__" + or + incorrect_get(f, message, show_counts, show_unused_defaults) and name = "__get__" + or + 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() diff --git a/python/ql/src/change-notes/2025-03-20-modernize-special-method-wrong-signature-query.md b/python/ql/src/change-notes/2025-03-20-modernize-special-method-wrong-signature-query.md new file mode 100644 index 00000000000..e871b7510d9 --- /dev/null +++ b/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. diff --git a/python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected b/python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected index 94fba173b3b..55f1e7381f1 100644 --- a/python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected +++ b/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 | diff --git a/python/ql/test/query-tests/Functions/general/om_test.py b/python/ql/test/query-tests/Functions/general/om_test.py index cbee20625aa..959ed6bfe34 100644 --- a/python/ql/test/query-tests/Functions/general/om_test.py +++ b/python/ql/test/query-tests/Functions/general/om_test.py @@ -69,11 +69,11 @@ class WrongSpecials(object): 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 @@ class LoggingDict(dict): +class MoreSpecialMethods: + @staticmethod + def __abs__(): + return 42