diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll new file mode 100644 index 000000000000..0d775b9dbf03 --- /dev/null +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -0,0 +1,108 @@ +/** Definitions for reasoning about the expected first argument names for methods. */ + +import python +import semmle.python.ApiGraphs +import semmle.python.dataflow.new.internal.DataFlowDispatch +import DataFlow + +/** Holds if `f` is a method of the class `c`. */ +private predicate methodOfClass(Function f, Class c) { + exists(FunctionDef d | d.getDefinedFunction() = f and d.getScope() = c) +} + +/** Holds if `c` is a metaclass. */ +private predicate isMetaclass(Class c) { + c = API::builtin("type").getASubclass*().asSource().asExpr().(ClassExpr).getInnerScope() +} + +/** Holds if `c` is a Zope interface. */ +private predicate isZopeInterface(Class c) { + c = + API::moduleImport("zope") + .getMember("interface") + .getMember("Interface") + .getASubclass*() + .asSource() + .asExpr() + .(ClassExpr) + .getInnerScope() +} + +/** + * Holds if `f` is used in the initialisation of `c`. + * This means `f` isn't being used as a normal method. + * Ideally it should be a `@staticmethod`; however this wasn't possible prior to Python 3.10. + * We exclude this case from the `not-named-self` query. + * However there is potential for a new query that specifically covers and alerts for this case. + */ +private predicate usedInInit(Function f, Class c) { + methodOfClass(f, c) and + exists(Call call | + call.getScope() = c and + DataFlow::localFlow(DataFlow::exprNode(f.getDefinition()), DataFlow::exprNode(call.getFunc())) + ) +} + +/** + * Holds if `f` has no arguments, and also has a decorator. + * We assume that the decorator affect the method in such a way that a `self` parameter is unneeded. + */ +private predicate noArgsWithDecorator(Function f) { + not exists(f.getAnArg()) and + exists(f.getADecorator()) +} + +/** Holds if the first parameter of `f` should be named `self`. */ +predicate shouldBeSelf(Function f, Class c) { + methodOfClass(f, c) and + not isStaticmethod(f) and + not isClassmethod(f) and + not isMetaclass(c) and + not isZopeInterface(c) and + not usedInInit(f, c) and + not noArgsWithDecorator(f) +} + +/** Holds if the first parameter of `f` should be named `cls`. */ +predicate shouldBeCls(Function f, Class c) { + methodOfClass(f, c) and + not isStaticmethod(f) and + ( + isClassmethod(f) and not isMetaclass(c) + or + isMetaclass(c) and not isClassmethod(f) + ) +} + +/** Holds if the first parameter of `f` is named `self`. */ +predicate firstArgNamedSelf(Function f) { f.getArgName(0) = "self" } + +/** Holds if the first parameter of `f` refers to the class - it is either named `cls`, or it is named `self` and is a method of a metaclass. */ +predicate firstArgRefersToCls(Function f, Class c) { + methodOfClass(f, c) and + exists(string argname | argname = f.getArgName(0) | + argname = "cls" + or + /* Not PEP8, but relatively common */ + argname = "mcls" + or + /* If c is a metaclass, allow arguments named `self`. */ + argname = "self" and + isMetaclass(c) + ) +} + +/** Holds if the first parameter of `f` should be named `self`, but isn't. */ +predicate firstArgShouldBeNamedSelfAndIsnt(Function f) { + shouldBeSelf(f, _) and + not firstArgNamedSelf(f) +} + +/** Holds if the first parameter of `f` should be named `cls`, but isn't. */ +predicate firstArgShouldReferToClsAndDoesnt(Function f) { + exists(Class c | + methodOfClass(f, c) and + shouldBeCls(f, c) and + not firstArgRefersToCls(f, c) + ) +} diff --git a/python/ql/src/Functions/NonCls.py b/python/ql/src/Functions/NonCls.py index f4959d89ebd3..3fc118253891 100644 --- a/python/ql/src/Functions/NonCls.py +++ b/python/ql/src/Functions/NonCls.py @@ -1,4 +1,4 @@ class Entry(object): @classmethod - def make(klass): + def make(self): return Entry() diff --git a/python/ql/src/Functions/NonCls.qhelp b/python/ql/src/Functions/NonCls.qhelp index 6ac9170a9915..cf064fc50566 100644 --- a/python/ql/src/Functions/NonCls.qhelp +++ b/python/ql/src/Functions/NonCls.qhelp @@ -5,20 +5,19 @@ -

The first parameter of a class method, a new method or any metaclass method -should be called cls. This makes the purpose of the parameter clear to other developers. +

The first parameter of a class method (including certain special methods such as __new__), or a method of a metaclass, +should be named cls.

-

Change the name of the first parameter to cls as recommended by the style guidelines +

Ensure that the first parameter of class methods is named cls, as recommended by the style guidelines in PEP 8.

 -

In the example, the first parameter to make() is klass which should be changed to cls -for ease of comprehension. +

In the following example, the first parameter of the class method make is named self instead of cls.

@@ -29,6 +28,7 @@ for ease of comprehension.
  • Python PEP 8: Function and method arguments.
  • Python Tutorial: Classes.
    • +
  • Python Docs: classmethod.
    • diff --git a/python/ql/src/Functions/NonCls.ql b/python/ql/src/Functions/NonCls.ql index 5cb9fafab895..d36eeb9a6ec4 100644 --- a/python/ql/src/Functions/NonCls.ql +++ b/python/ql/src/Functions/NonCls.ql @@ -1,7 +1,6 @@ /** * @name First parameter of a class method is not named 'cls' - * @description Using an alternative name for the first parameter of a class method makes code more - * difficult to read; PEP8 states that the first parameter to class methods should be 'cls'. + * @description By the PEP8 style guide, the first parameter of a class method should be named `cls`. * @kind problem * @tags maintainability * readability @@ -13,30 +12,11 @@ */ import python - -predicate first_arg_cls(Function f) { - exists(string argname | argname = f.getArgName(0) | - argname = "cls" - or - /* Not PEP8, but relatively common */ - argname = "mcls" - ) -} - -predicate is_type_method(Function f) { - exists(ClassValue c | c.getScope() = f.getScope() and c.getASuperType() = ClassValue::type()) -} - -predicate classmethod_decorators_only(Function f) { - forall(Expr decorator | decorator = f.getADecorator() | decorator.(Name).getId() = "classmethod") -} +import MethodArgNames from Function f, string message where - (f.getADecorator().(Name).getId() = "classmethod" or is_type_method(f)) and - not first_arg_cls(f) and - classmethod_decorators_only(f) and - not f.getName() = "__new__" and + firstArgShouldReferToClsAndDoesnt(f) and ( if exists(f.getArgName(0)) then diff --git a/python/ql/src/Functions/NonSelf.py b/python/ql/src/Functions/NonSelf.py index 00d3c2ff6f5d..69e2dde34a9a 100644 --- a/python/ql/src/Functions/NonSelf.py +++ b/python/ql/src/Functions/NonSelf.py @@ -1,9 +1,9 @@ class Point: - def __init__(val, x, y): # first parameter is mis-named 'val' + def __init__(val, x, y): # BAD: first parameter is mis-named 'val' val._x = x val._y = y class Point2: - def __init__(self, x, y): # first parameter is correctly named 'self' + def __init__(self, x, y): # GOOD: first parameter is correctly named 'self' self._x = x self._y = y \ No newline at end of file diff --git a/python/ql/src/Functions/NonSelf.qhelp b/python/ql/src/Functions/NonSelf.qhelp index c4cef70e731b..ac1639ba625a 100644 --- a/python/ql/src/Functions/NonSelf.qhelp +++ b/python/ql/src/Functions/NonSelf.qhelp @@ -6,22 +6,18 @@

    Normal methods should have at least one parameter and the first parameter should be called self. -This makes the purpose of the parameter clear to other developers.

    -

    If there is at least one parameter, then change the name of the first parameter to self as recommended by the style guidelines +

    Ensure that the first parameter of a normal method is named self, as recommended by the style guidelines in PEP 8.

    -

    If there are no parameters, then it cannot be a normal method. It may need to be marked as a staticmethod -or it could be moved out of the class as a normal function. +

    If a self parameter is unneeded, the method should be decorated with staticmethod, or moved out of the class as a regular function.

    -

    The following methods can both be used to assign values to variables in a point -object. The second method makes the association clearer because the self parameter is -used.

    +

    In the following cases, the first argument of Point.__init__ is named val instead; whereas in Point2.__init__ it is correctly named self.

    @@ -31,7 +27,7 @@ used.

  • Python PEP 8: Function and method arguments.
  • Python Tutorial: Classes.
    • - +
  • Python Docs: staticmethod.
    • diff --git a/python/ql/src/Functions/NonSelf.ql b/python/ql/src/Functions/NonSelf.ql index cb8924c071ae..cea15d3661a8 100644 --- a/python/ql/src/Functions/NonSelf.ql +++ b/python/ql/src/Functions/NonSelf.ql @@ -1,8 +1,6 @@ /** * @name First parameter of a method is not named 'self' - * @description Using an alternative name for the first parameter of an instance method makes - * code more difficult to read; PEP8 states that the first parameter to instance - * methods should be 'self'. + * @description By the PEP8 style guide, the first parameter of a normal method should be named `self`. * @kind problem * @tags maintainability * readability @@ -14,45 +12,19 @@ */ import python -import semmle.python.libraries.Zope +import MethodArgNames -predicate is_type_method(FunctionValue fv) { - exists(ClassValue c | c.declaredAttribute(_) = fv and c.getASuperType() = ClassValue::type()) -} - -predicate used_in_defining_scope(FunctionValue fv) { - exists(Call c | c.getScope() = fv.getScope().getScope() and c.getFunc().pointsTo(fv)) -} - -from Function f, FunctionValue fv, string message +from Function f, string message where - exists(ClassValue cls, string name | - cls.declaredAttribute(name) = fv and - cls.isNewStyle() and - not name = "__new__" and - not name = "__metaclass__" and - not name = "__init_subclass__" and - not name = "__class_getitem__" and - /* declared in scope */ - f.getScope() = cls.getScope() - ) and - not f.getArgName(0) = "self" and - not is_type_method(fv) and - fv.getScope() = f and - not f.getName() = "lambda" and - not used_in_defining_scope(fv) and + firstArgShouldBeNamedSelfAndIsnt(f) and ( - ( - if exists(f.getArgName(0)) - then - message = - "Normal methods should have 'self', rather than '" + f.getArgName(0) + - "', as their first parameter." - else - message = - "Normal methods should have at least one parameter (the first of which should be 'self')." - ) and - not f.hasVarArg() - ) and - not fv instanceof ZopeInterfaceMethodValue + if exists(f.getArgName(0)) + then + message = + "Normal methods should have 'self', rather than '" + f.getArgName(0) + + "', as their first parameter." + else + message = + "Normal methods should have at least one parameter (the first of which should be 'self')." + ) select f, message diff --git a/python/ql/test/query-tests/Functions/general/NonCls.expected b/python/ql/test/query-tests/Functions/general/NonCls.expected deleted file mode 100644 index 991c344c9d7f..000000000000 --- a/python/ql/test/query-tests/Functions/general/NonCls.expected +++ /dev/null @@ -1,3 +0,0 @@ -| parameter_names.py:17:5:17:24 | Function n_cmethod | Class methods or methods of a type deriving from type should have 'cls', rather than 'self', as their first parameter. | -| parameter_names.py:22:5:22:21 | Function n_cmethod2 | Class methods or methods of a type deriving from type should have 'cls' as their first parameter. | -| parameter_names.py:37:5:37:20 | Function c_method | Class methods or methods of a type deriving from type should have 'cls', rather than 'y', as their first parameter. | diff --git a/python/ql/test/query-tests/Functions/general/NonCls.qlref b/python/ql/test/query-tests/Functions/general/NonCls.qlref deleted file mode 100644 index b3525b101df2..000000000000 --- a/python/ql/test/query-tests/Functions/general/NonCls.qlref +++ /dev/null @@ -1 +0,0 @@ -Functions/NonCls.ql diff --git a/python/ql/test/query-tests/Functions/general/NonSelf.expected b/python/ql/test/query-tests/Functions/general/NonSelf.expected deleted file mode 100644 index 17d5f9818dd7..000000000000 --- a/python/ql/test/query-tests/Functions/general/NonSelf.expected +++ /dev/null @@ -1,4 +0,0 @@ -| om_test.py:71:5:71:19 | Function __repr__ | Normal methods should have at least one parameter (the first of which should be 'self'). | -| parameter_names.py:50:5:50:20 | Function __init__ | Normal methods should have 'self', rather than 'x', as their first parameter. | -| parameter_names.py:53:5:53:20 | Function s_method | Normal methods should have 'self', rather than 'y', as their first parameter. | -| parameter_names.py:56:5:56:20 | Function s_method2 | Normal methods should have at least one parameter (the first of which should be 'self'). | diff --git a/python/ql/test/query-tests/Functions/general/NonSelf.qlref b/python/ql/test/query-tests/Functions/general/NonSelf.qlref deleted file mode 100644 index 2629570c2a17..000000000000 --- a/python/ql/test/query-tests/Functions/general/NonSelf.qlref +++ /dev/null @@ -1 +0,0 @@ -Functions/NonSelf.ql diff --git a/python/ql/test/query-tests/Functions/general/parameter_names.py b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py similarity index 71% rename from python/ql/test/query-tests/Functions/general/parameter_names.py rename to python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py index b42aaaebb1de..3a2fddc9f02c 100644 --- a/python/ql/test/query-tests/Functions/general/parameter_names.py +++ b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py @@ -14,46 +14,47 @@ def n_smethod(ok): # not ok @classmethod - def n_cmethod(self): + def n_cmethod(self): # $shouldBeCls pass # not ok @classmethod - def n_cmethod2(): + def n_cmethod2(): # $shouldBeCls pass - # this is allowed because it has a decorator other than @classmethod @classmethod @id - def n_suppress(any_name): + def n_dec(any_name): # $shouldBeCls pass +# Metaclass - normal methods should be named cls, though self is also accepted class Class(type): def __init__(cls): pass - def c_method(y): + def c_method(y): # $shouldBeCls pass def c_ok(cls): pass - @id - def c_suppress(any_name): + # technically we could alert on mixing self for metaclasses with cls for metaclasses in the same codebase, + # but it's probably not too significant. + def c_self_ok(self): pass class NonSelf(object): - def __init__(x): + def __init__(x): # $shouldBeSelf pass - def s_method(y): + def s_method(y): # $shouldBeSelf pass - def s_method2(): + def s_method2(): # $shouldBeSelf pass def s_ok(self): @@ -67,11 +68,12 @@ def s_smethod(ok): def s_cmethod(cls): pass - def s_smethod2(ok): + # we allow methods that are used in class initialization, but only detect this case when they are called. + def s_smethod2(ok): # $ SPURIOUS: shouldBeSelf pass s_smethod2 = staticmethod(s_smethod2) - def s_cmethod2(cls): + def s_cmethod2(cls): # $ SPURIOUS: shouldBeSelf pass s_cmethod2 = classmethod(s_cmethod2) @@ -118,7 +120,15 @@ def meth(arg): Z().meth(0) +def weird_decorator(f): + def g(self): + return f() + return g +class B: + @weird_decorator + def f(): # allow no-arg functions with a decorator + pass # The `__init_subclass__` (introduced in Python 3.6) # and `__class_getitem__` (introduced in Python 3.7) methods are methods @@ -139,3 +149,10 @@ def __init_subclass__(cls): def __class_getitem__(cls): pass + +from dataclasses import dataclass, field + +@dataclass +class A: + # Lambdas used in initilisation aren't methods. + x: int = field(default_factory = lambda: 2) \ No newline at end of file diff --git a/python/ql/test/query-tests/Functions/methodArgNames/test.expected b/python/ql/test/query-tests/Functions/methodArgNames/test.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/python/ql/test/query-tests/Functions/methodArgNames/test.ql b/python/ql/test/query-tests/Functions/methodArgNames/test.ql new file mode 100644 index 000000000000..526a648b8da3 --- /dev/null +++ b/python/ql/test/query-tests/Functions/methodArgNames/test.ql @@ -0,0 +1,24 @@ +import python +import Functions.MethodArgNames +import utils.test.InlineExpectationsTest + +module MethodArgTest implements TestSig { + string getARelevantTag() { result = ["shouldBeSelf", "shouldBeCls"] } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(Function f | + element = f.toString() and + location = f.getLocation() and + value = "" and + ( + firstArgShouldBeNamedSelfAndIsnt(f) and + tag = "shouldBeSelf" + or + firstArgShouldReferToClsAndDoesnt(f) and + tag = "shouldBeCls" + ) + ) + } +} + +import MakeTest