From 526e235fc1200241ef2e5488fc3c76c4fd417cdf Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 27 Jan 2025 13:46:15 +0000 Subject: [PATCH 1/9] Update NonSelf and NonCls queries --- python/ql/src/Functions/MethodArgNames.qll | 79 ++++++++++++++++++++++ python/ql/src/Functions/NonCls.ql | 23 +------ python/ql/src/Functions/NonSelf.ql | 32 ++------- 3 files changed, 85 insertions(+), 49 deletions(-) create mode 100644 python/ql/src/Functions/MethodArgNames.qll diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll new file mode 100644 index 000000000000..e84a59977b6b --- /dev/null +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -0,0 +1,79 @@ +/** Definitions for reasoning about the expected first argument names for methods. */ + +import python +import semmle.python.ApiGraphs + +/** Holds if `f` is a method of the class `c`. */ +private predicate methodOfClass(Function f, Class c) { f.getScope() = c } + +/** Holds if `c` is a metaclass. */ +private predicate isMetaclass(Class c) { + c.getABase() = API::builtin("type").getASubclass*().asSource().asExpr() +} + +/** Holds if `f` is a class method. */ +private predicate isClassMethod(Function f) { + f.getADecorator() = API::builtin("classmethod").asSource().asExpr() +} + +/** Holds if `f` is a static method. */ +private predicate isStaticMethod(Function f) { + f.getADecorator() = API::builtin("staticmethod").asSource().asExpr() +} + +/** Holds if `c` is a Zope interface. */ +private predicate isZopeInterface(Class c) { + c.getABase() = + API::moduleImport("zone") + .getMember("interface") + .getMember("interface") + .getASubclass*() + .asSource() + .asExpr() +} + +/** 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 f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] and + isMetaclass(c) and + not isZopeInterface(c) +} + +/** 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) + or + f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] + ) +} + +/** 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` is named `cls`. */ +predicate firstArgNamedCls(Function f) { + exists(string argname | argname = f.getArgName(0) | + argname = "cls" + or + /* Not PEP8, but relatively common */ + argname = "mcls" + ) +} + +/** Holds if the first parameter of `f` should be named `self`, but isn't. */ +predicate firstArgShouldBeNamedSelfAndIsnt(Function f) { + exists(Class c | shouldBeSelf(f, c)) and + not firstArgNamedSelf(f) +} + +/** Holds if the first parameter of `f` should be named `cls`, but isn't. */ +predicate firstArgShouldBeNamedClsAndIsnt(Function f) { + exists(Class c | shouldBeCls(f, c)) and + not firstArgNamedCls(f) +} diff --git a/python/ql/src/Functions/NonCls.ql b/python/ql/src/Functions/NonCls.ql index 5cb9fafab895..ae9ab0846d6f 100644 --- a/python/ql/src/Functions/NonCls.ql +++ b/python/ql/src/Functions/NonCls.ql @@ -13,30 +13,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 + firstArgShouldBeNamedClsAndIsnt(f) and ( if exists(f.getArgName(0)) then diff --git a/python/ql/src/Functions/NonSelf.ql b/python/ql/src/Functions/NonSelf.ql index cb8924c071ae..2ea72014a0e5 100644 --- a/python/ql/src/Functions/NonSelf.ql +++ b/python/ql/src/Functions/NonSelf.ql @@ -14,33 +14,11 @@ */ 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)) @@ -52,7 +30,5 @@ where 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 + ) select f, message From fa76bf3c9ff16a8200b6e23db45b48902d65ed34 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 27 Jan 2025 16:58:19 +0000 Subject: [PATCH 2/9] Fix formatting and rewrite redundant exists --- python/ql/src/Functions/MethodArgNames.qll | 13 +++++++------ python/ql/src/Functions/NonSelf.ql | 20 +++++++++----------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll index e84a59977b6b..1843bcd93782 100644 --- a/python/ql/src/Functions/MethodArgNames.qll +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -14,6 +14,8 @@ private predicate isMetaclass(Class c) { /** Holds if `f` is a class method. */ private predicate isClassMethod(Function f) { f.getADecorator() = API::builtin("classmethod").asSource().asExpr() + or + f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] } /** Holds if `f` is a static method. */ @@ -37,8 +39,7 @@ predicate shouldBeSelf(Function f, Class c) { methodOfClass(f, c) and not isStaticMethod(f) and not isClassMethod(f) and - not f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] and - isMetaclass(c) and + not isMetaclass(c) and not isZopeInterface(c) } @@ -47,9 +48,9 @@ predicate shouldBeCls(Function f, Class c) { methodOfClass(f, c) and not isStaticMethod(f) and ( - isClassMethod(f) + isClassMethod(f) and not isMetaclass(c) or - f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] + isMetaclass(c) and not isClassMethod(f) ) } @@ -68,12 +69,12 @@ predicate firstArgNamedCls(Function f) { /** Holds if the first parameter of `f` should be named `self`, but isn't. */ predicate firstArgShouldBeNamedSelfAndIsnt(Function f) { - exists(Class c | shouldBeSelf(f, c)) and + shouldBeSelf(f, _) and not firstArgNamedSelf(f) } /** Holds if the first parameter of `f` should be named `cls`, but isn't. */ predicate firstArgShouldBeNamedClsAndIsnt(Function f) { - exists(Class c | shouldBeCls(f, c)) and + shouldBeCls(f, _) and not firstArgNamedCls(f) } diff --git a/python/ql/src/Functions/NonSelf.ql b/python/ql/src/Functions/NonSelf.ql index 2ea72014a0e5..94bc8a2fa378 100644 --- a/python/ql/src/Functions/NonSelf.ql +++ b/python/ql/src/Functions/NonSelf.ql @@ -20,15 +20,13 @@ from Function f, string message where 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 - ) + 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 From 0bf8d4ec4b32c4234ae6d457da57309413ff96d2 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 30 Jan 2025 13:55:49 +0000 Subject: [PATCH 3/9] Exclude 'methods' used in initialisation, and allow self for metaclass methods --- python/ql/src/Functions/MethodArgNames.qll | 30 ++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll index 1843bcd93782..d1a609fd4112 100644 --- a/python/ql/src/Functions/MethodArgNames.qll +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -2,6 +2,7 @@ import python import semmle.python.ApiGraphs +import DataFlow /** Holds if `f` is a method of the class `c`. */ private predicate methodOfClass(Function f, Class c) { f.getScope() = c } @@ -34,13 +35,29 @@ private predicate isZopeInterface(Class c) { .asExpr() } +/** + * 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 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) + not isZopeInterface(c) and + not usedInInit(f, c) } /** Holds if the first parameter of `f` should be named `cls`. */ @@ -73,8 +90,17 @@ predicate firstArgShouldBeNamedSelfAndIsnt(Function f) { not firstArgNamedSelf(f) } +/** Holds if `f` is a regular method of a metaclass, and its first argument is named `self`. */ +private predicate metaclassNamedSelf(Function f, Class c) { + methodOfClass(f, c) and + firstArgNamedSelf(f) and + isMetaclass(c) and + not isClassMethod(f) +} + /** Holds if the first parameter of `f` should be named `cls`, but isn't. */ predicate firstArgShouldBeNamedClsAndIsnt(Function f) { shouldBeCls(f, _) and - not firstArgNamedCls(f) + not firstArgNamedCls(f) and + not metaclassNamedSelf(f, _) } From aa2c84ea36f5922acf6e2053858061cf62ee351a Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 31 Jan 2025 12:03:52 +0000 Subject: [PATCH 4/9] Move tests to separate folder --- .../Functions/{general => methodArgNames}/NonCls.expected | 0 .../Functions/{general => methodArgNames}/NonCls.qlref | 0 .../Functions/{general => methodArgNames}/NonSelf.expected | 0 .../Functions/{general => methodArgNames}/NonSelf.qlref | 0 .../Functions/{general => methodArgNames}/parameter_names.py | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename python/ql/test/query-tests/Functions/{general => methodArgNames}/NonCls.expected (100%) rename python/ql/test/query-tests/Functions/{general => methodArgNames}/NonCls.qlref (100%) rename python/ql/test/query-tests/Functions/{general => methodArgNames}/NonSelf.expected (100%) rename python/ql/test/query-tests/Functions/{general => methodArgNames}/NonSelf.qlref (100%) rename python/ql/test/query-tests/Functions/{general => methodArgNames}/parameter_names.py (100%) diff --git a/python/ql/test/query-tests/Functions/general/NonCls.expected b/python/ql/test/query-tests/Functions/methodArgNames/NonCls.expected similarity index 100% rename from python/ql/test/query-tests/Functions/general/NonCls.expected rename to python/ql/test/query-tests/Functions/methodArgNames/NonCls.expected diff --git a/python/ql/test/query-tests/Functions/general/NonCls.qlref b/python/ql/test/query-tests/Functions/methodArgNames/NonCls.qlref similarity index 100% rename from python/ql/test/query-tests/Functions/general/NonCls.qlref rename to python/ql/test/query-tests/Functions/methodArgNames/NonCls.qlref diff --git a/python/ql/test/query-tests/Functions/general/NonSelf.expected b/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.expected similarity index 100% rename from python/ql/test/query-tests/Functions/general/NonSelf.expected rename to python/ql/test/query-tests/Functions/methodArgNames/NonSelf.expected diff --git a/python/ql/test/query-tests/Functions/general/NonSelf.qlref b/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.qlref similarity index 100% rename from python/ql/test/query-tests/Functions/general/NonSelf.qlref rename to python/ql/test/query-tests/Functions/methodArgNames/NonSelf.qlref 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 100% rename from python/ql/test/query-tests/Functions/general/parameter_names.py rename to python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py From e8adef18a3ff27d5cbe92b5a77c2ed3521c023af Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 3 Feb 2025 17:36:43 +0000 Subject: [PATCH 5/9] Update to inline expectations + fixes --- python/ql/src/Functions/MethodArgNames.qll | 10 ++++--- .../Functions/methodArgNames/NonCls.expected | 3 --- .../Functions/methodArgNames/NonCls.qlref | 1 - .../Functions/methodArgNames/NonSelf.expected | 4 --- .../Functions/methodArgNames/NonSelf.qlref | 1 - .../methodArgNames/parameter_names.py | 26 ++++++++++--------- .../Functions/methodArgNames/test.expected | 0 .../Functions/methodArgNames/test.ql | 24 +++++++++++++++++ 8 files changed, 44 insertions(+), 25 deletions(-) delete mode 100644 python/ql/test/query-tests/Functions/methodArgNames/NonCls.expected delete mode 100644 python/ql/test/query-tests/Functions/methodArgNames/NonCls.qlref delete mode 100644 python/ql/test/query-tests/Functions/methodArgNames/NonSelf.expected delete mode 100644 python/ql/test/query-tests/Functions/methodArgNames/NonSelf.qlref create mode 100644 python/ql/test/query-tests/Functions/methodArgNames/test.expected create mode 100644 python/ql/test/query-tests/Functions/methodArgNames/test.ql diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll index d1a609fd4112..0dc4cc3c5fdf 100644 --- a/python/ql/src/Functions/MethodArgNames.qll +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -9,7 +9,7 @@ private predicate methodOfClass(Function f, Class c) { f.getScope() = c } /** Holds if `c` is a metaclass. */ private predicate isMetaclass(Class c) { - c.getABase() = API::builtin("type").getASubclass*().asSource().asExpr() + c = API::builtin("type").getASubclass*().asSource().asExpr().(ClassExpr).getInnerScope() } /** Holds if `f` is a class method. */ @@ -26,13 +26,15 @@ private predicate isStaticMethod(Function f) { /** Holds if `c` is a Zope interface. */ private predicate isZopeInterface(Class c) { - c.getABase() = - API::moduleImport("zone") - .getMember("interface") + c = + API::moduleImport("zope") .getMember("interface") + .getMember("Interface") .getASubclass*() .asSource() .asExpr() + .(ClassExpr) + .getInnerScope() } /** diff --git a/python/ql/test/query-tests/Functions/methodArgNames/NonCls.expected b/python/ql/test/query-tests/Functions/methodArgNames/NonCls.expected deleted file mode 100644 index 991c344c9d7f..000000000000 --- a/python/ql/test/query-tests/Functions/methodArgNames/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/methodArgNames/NonCls.qlref b/python/ql/test/query-tests/Functions/methodArgNames/NonCls.qlref deleted file mode 100644 index b3525b101df2..000000000000 --- a/python/ql/test/query-tests/Functions/methodArgNames/NonCls.qlref +++ /dev/null @@ -1 +0,0 @@ -Functions/NonCls.ql diff --git a/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.expected b/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.expected deleted file mode 100644 index 17d5f9818dd7..000000000000 --- a/python/ql/test/query-tests/Functions/methodArgNames/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/methodArgNames/NonSelf.qlref b/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.qlref deleted file mode 100644 index 2629570c2a17..000000000000 --- a/python/ql/test/query-tests/Functions/methodArgNames/NonSelf.qlref +++ /dev/null @@ -1 +0,0 @@ -Functions/NonSelf.ql diff --git a/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py index b42aaaebb1de..1fd87b2c0d34 100644 --- a/python/ql/test/query-tests/Functions/methodArgNames/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) 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..01c67d45b055 --- /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 + firstArgShouldBeNamedClsAndIsnt(f) and + tag = "shouldBeCls" + ) + ) + } +} + +import MakeTest From 3802a73f47766ae2fa56d8adce8ffade0d568796 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 4 Feb 2025 10:13:25 +0000 Subject: [PATCH 6/9] Update docs --- python/ql/src/Functions/NonCls.py | 2 +- python/ql/src/Functions/NonCls.qhelp | 10 +++++----- python/ql/src/Functions/NonCls.ql | 3 +-- python/ql/src/Functions/NonSelf.py | 4 ++-- python/ql/src/Functions/NonSelf.qhelp | 12 ++++-------- python/ql/src/Functions/NonSelf.ql | 4 +--- 6 files changed, 14 insertions(+), 21 deletions(-) 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..418218e5a1fa 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 ae9ab0846d6f..bca9a57c9080 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 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..eb796f9b8833 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 94bc8a2fa378..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 From 287cf0121df625d27fbad05233c5bb82148959a4 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 4 Feb 2025 10:46:05 +0000 Subject: [PATCH 7/9] Fix docs --- python/ql/src/Functions/NonCls.qhelp | 2 +- python/ql/src/Functions/NonSelf.qhelp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/Functions/NonCls.qhelp b/python/ql/src/Functions/NonCls.qhelp index 418218e5a1fa..cf064fc50566 100644 --- a/python/ql/src/Functions/NonCls.qhelp +++ b/python/ql/src/Functions/NonCls.qhelp @@ -28,7 +28,7 @@ in PEP 8.

  • Python PEP 8: Function and method arguments.
  • Python Tutorial: Classes.
    • -
  • Python Docs: classmethod.
    • +
  • Python Docs: classmethod.
    • diff --git a/python/ql/src/Functions/NonSelf.qhelp b/python/ql/src/Functions/NonSelf.qhelp index eb796f9b8833..ac1639ba625a 100644 --- a/python/ql/src/Functions/NonSelf.qhelp +++ b/python/ql/src/Functions/NonSelf.qhelp @@ -27,7 +27,7 @@ in PEP 8.

  • Python PEP 8: Function and method arguments.
  • Python Tutorial: Classes.
    • -
  • Python Docs: staticmethod.
    • . +
  • Python Docs: staticmethod.
    • From 61d5a692fb962bc4e3c463457ad306909d5604b2 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 7 Feb 2025 21:46:30 +0000 Subject: [PATCH 8/9] Refactor metaclass logic a bit, ensure lambdas are excluded --- python/ql/src/Functions/MethodArgNames.qll | 54 ++++++++----------- python/ql/src/Functions/NonCls.ql | 2 +- .../methodArgNames/parameter_names.py | 7 +++ .../Functions/methodArgNames/test.ql | 2 +- 4 files changed, 31 insertions(+), 34 deletions(-) diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll index 0dc4cc3c5fdf..0cbe3ba21965 100644 --- a/python/ql/src/Functions/MethodArgNames.qll +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -2,28 +2,19 @@ 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) { f.getScope() = 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 `f` is a class method. */ -private predicate isClassMethod(Function f) { - f.getADecorator() = API::builtin("classmethod").asSource().asExpr() - or - f.getName() in ["__new__", "__init_subclass__", "__metaclass__", "__class_getitem__"] -} - -/** Holds if `f` is a static method. */ -private predicate isStaticMethod(Function f) { - f.getADecorator() = API::builtin("staticmethod").asSource().asExpr() -} - /** Holds if `c` is a Zope interface. */ private predicate isZopeInterface(Class c) { c = @@ -55,8 +46,8 @@ private predicate usedInInit(Function f, Class c) { /** 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 isStaticmethod(f) and + not isClassmethod(f) and not isMetaclass(c) and not isZopeInterface(c) and not usedInInit(f, c) @@ -65,24 +56,29 @@ predicate shouldBeSelf(Function f, Class c) { /** 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 + not isStaticmethod(f) and ( - isClassMethod(f) and not isMetaclass(c) + isClassmethod(f) and not isMetaclass(c) or - isMetaclass(c) and not isClassMethod(f) + 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` is named `cls`. */ -predicate firstArgNamedCls(Function f) { +/** 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) ) } @@ -92,17 +88,11 @@ predicate firstArgShouldBeNamedSelfAndIsnt(Function f) { not firstArgNamedSelf(f) } -/** Holds if `f` is a regular method of a metaclass, and its first argument is named `self`. */ -private predicate metaclassNamedSelf(Function f, Class c) { - methodOfClass(f, c) and - firstArgNamedSelf(f) and - isMetaclass(c) and - not isClassMethod(f) -} - /** Holds if the first parameter of `f` should be named `cls`, but isn't. */ -predicate firstArgShouldBeNamedClsAndIsnt(Function f) { - shouldBeCls(f, _) and - not firstArgNamedCls(f) and - not metaclassNamedSelf(f, _) +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.ql b/python/ql/src/Functions/NonCls.ql index bca9a57c9080..d36eeb9a6ec4 100644 --- a/python/ql/src/Functions/NonCls.ql +++ b/python/ql/src/Functions/NonCls.ql @@ -16,7 +16,7 @@ import MethodArgNames from Function f, string message where - firstArgShouldBeNamedClsAndIsnt(f) and + firstArgShouldReferToClsAndDoesnt(f) and ( if exists(f.getArgName(0)) then diff --git a/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py index 1fd87b2c0d34..6a93c35568e3 100644 --- a/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py +++ b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py @@ -141,3 +141,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.ql b/python/ql/test/query-tests/Functions/methodArgNames/test.ql index 01c67d45b055..526a648b8da3 100644 --- a/python/ql/test/query-tests/Functions/methodArgNames/test.ql +++ b/python/ql/test/query-tests/Functions/methodArgNames/test.ql @@ -14,7 +14,7 @@ module MethodArgTest implements TestSig { firstArgShouldBeNamedSelfAndIsnt(f) and tag = "shouldBeSelf" or - firstArgShouldBeNamedClsAndIsnt(f) and + firstArgShouldReferToClsAndDoesnt(f) and tag = "shouldBeCls" ) ) From f46a2a177384ef5c3ed63302a0e067dc2f1ab8cf Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 12 Feb 2025 09:40:45 +0000 Subject: [PATCH 9/9] Exclude some decorators --- python/ql/src/Functions/MethodArgNames.qll | 12 +++++++++++- .../Functions/methodArgNames/parameter_names.py | 8 ++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/python/ql/src/Functions/MethodArgNames.qll b/python/ql/src/Functions/MethodArgNames.qll index 0cbe3ba21965..0d775b9dbf03 100644 --- a/python/ql/src/Functions/MethodArgNames.qll +++ b/python/ql/src/Functions/MethodArgNames.qll @@ -43,6 +43,15 @@ private predicate usedInInit(Function f, Class c) { ) } +/** + * 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 @@ -50,7 +59,8 @@ predicate shouldBeSelf(Function f, Class c) { not isClassmethod(f) and not isMetaclass(c) and not isZopeInterface(c) and - not usedInInit(f, c) + not usedInInit(f, c) and + not noArgsWithDecorator(f) } /** Holds if the first parameter of `f` should be named `cls`. */ diff --git a/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py index 6a93c35568e3..3a2fddc9f02c 100644 --- a/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py +++ b/python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py @@ -120,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