Merge pull request #18599 from joefarebrother/python-qual-not-named-self-cls

Python: Modernize py/not-named-self and py/not-named-cls queries

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)
+  )
+}

python/ql/src/Functions/NonCls.py

@@ -1,4 +1,4 @@
 class Entry(object):
     @classmethod
-    def make(klass):
+    def make(self):
         return Entry()

python/ql/src/Functions/NonCls.qhelp

@@ -5,20 +5,19 @@
 
 
 <overview>
-<p> The first parameter of a class method, a new method or any metaclass method
-should be called <code>cls</code>. This makes the purpose of the parameter clear to other developers.
+<p> The first parameter of a class method (including certain special methods such as <code>__new__</code>), or a method of a metaclass,
+should be named <code>cls</code>. 
 </p>
 
 </overview>
 <recommendation>
 
-<p>Change the name of the first parameter to <code>cls</code> as recommended by the style guidelines
+<p>Ensure that the first parameter of class methods is named <code>cls</code>, as recommended by the style guidelines
 in PEP 8.</p>
 
 </recommendation>
 <example>
-<p>In the example, the first parameter to <code>make()</code> is <code>klass</code> which should be changed to <code>cls</code>
-for ease of comprehension.
+<p>In the following example, the first parameter of the class method <code>make</code> is named <code>self</code> instead of <code>cls</code>.
 </p>
 
 <sample src="NonCls.py" />
@@ -29,6 +28,7 @@ for ease of comprehension.
 
 <li>Python PEP 8: <a href="http://www.python.org/dev/peps/pep-0008/#function-and-method-arguments">Function and method arguments</a>.</li>
 <li>Python Tutorial: <a href="http://docs.python.org/2/tutorial/classes.html">Classes</a>.</li>
+<li>Python Docs: <a href="https://docs.python.org/3/library/functions.html#classmethod">classmethod</a>.</li>
 
 
 </references>

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

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

python/ql/src/Functions/NonSelf.qhelp

@@ -6,22 +6,18 @@
 
 <overview>
 <p> Normal methods should have at least one parameter and the first parameter should be called <code>self</code>.
-This makes the purpose of the parameter clear to other developers.
 </p>
 </overview>
 
 <recommendation>
-<p>If there is at least one parameter, then change the name of the first parameter to <code>self</code> as recommended by the style guidelines
+<p>Ensure that the first parameter of a normal method is named <code>self</code>, as recommended by the style guidelines
 in PEP 8.</p>
-<p>If there are no parameters, then it cannot be a normal method. It may need to be marked as a <code>staticmethod</code>
-or it could be moved out of the class as a normal function.
+<p>If a <code>self</code> parameter is unneeded, the method should be decorated with <code>staticmethod</code>, or moved out of the class as a regular function.
 </p>
 </recommendation>
 
 <example>
-<p>The following methods can both be used to assign values to variables in a <code>point</code> 
-object. The second method makes the association clearer because the <code>self</code> parameter is 
-used.</p>
+<p>In the following cases, the first argument of <code>Point.__init__</code> is named <code>val</code> instead; whereas in <code>Point2.__init__</code> it is correctly named <code>self</code>.</p>
 <sample src="NonSelf.py" />
 
 
@@ -31,7 +27,7 @@ used.</p>
 <li>Python PEP 8: <a href="http://www.python.org/dev/peps/pep-0008/#function-and-method-arguments">Function and 
 method arguments</a>.</li>
 <li>Python Tutorial: <a href="http://docs.python.org/2/tutorial/classes.html">Classes</a>.</li>
-
+<li>Python Docs: <a href="https://docs.python.org/3/library/functions.html#staticmethod">staticmethod</a>.</li>
 
 
 </references>

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

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

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

python/ql/test/query-tests/Functions/general/NonCls.qlref

@@ -1 +0,0 @@
-Functions/NonCls.ql

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

@@ -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'). |

python/ql/test/query-tests/Functions/general/NonSelf.qlref

@@ -1 +0,0 @@
-Functions/NonSelf.ql

python/ql/test/query-tests/Functions/methodArgNames/parameter_names.py

@@ -14,46 +14,47 @@ class Normal(object):
 
     # 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 @@ class NonSelf(object):
     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 @@ class Z(zope.interface.Interface):
 
 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 @@ class SpecialMethodNames(object):
 
     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)

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<MethodArgTest>