Update NonSelf and NonCls queries

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

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

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