Skip to content

Commit fb20cab

Browse files
taus-semmletaus-semmle
authored
Merge pull request github#2012 from RasmusWL/python-modernise-cls-self-checks
Python: modernise cls self argument name checks
2 parents f417640 + 4a5aae0 commit fb20cab

File tree

14 files changed

+266
-174
lines changed

14 files changed

+266
-174
lines changed

‎python/ql/src/Functions/NonCls.qhelp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@
55

66

77
<overview>
8-
<p> The first argument of a class method, a new method or any metaclass method
9-
should be called <code>cls</code>. This makes the purpose of the argument clear to other developers.
8+
<p> The first parameter of a class method, a new method or any metaclass method
9+
should be called <code>cls</code>. This makes the purpose of the parameter clear to other developers.
1010
</p>
1111

1212
</overview>
1313
<recommendation>
1414

15-
<p>Change the name of the first argument to <code>cls</code> as recommended by the style guidelines
15+
<p>Change the name of the first parameter to <code>cls</code> as recommended by the style guidelines
1616
in PEP 8.</p>
1717

1818
</recommendation>

‎python/ql/src/Functions/NonCls.ql

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @name First parameter of a class method is not named 'cls'
3-
* @description Using an alternative name for the first argument of a class method makes code more
4-
* difficult to read; PEP8 states that the first argument to class methods should be 'cls'.
3+
* @description Using an alternative name for the first parameter of a class method makes code more
4+
* difficult to read; PEP8 states that the first parameter to class methods should be 'cls'.
55
* @kind problem
66
* @tags maintainability
77
* readability
@@ -16,32 +16,33 @@ import python
1616

1717
predicate first_arg_cls(Function f) {
1818
exists(string argname | argname = f.getArgName(0) |
19-
argname = "cls" or
19+
argname = "cls"
20+
or
2021
/* Not PEP8, but relatively common */
2122
argname = "mcls"
2223
)
2324
}
2425

2526
predicate is_type_method(Function f) {
26-
exists(ClassObject c | c.getPyClass() = f.getScope() and c.getASuperType() = theTypeType())
27+
exists(ClassValue c | c.getScope() = f.getScope() and c.getASuperType() = ClassValue::type())
2728
}
2829

2930
predicate classmethod_decorators_only(Function f) {
30-
forall(Expr decorator |
31-
decorator = f.getADecorator() |
32-
((Name) decorator).getId() = "classmethod")
31+
forall(Expr decorator | decorator = f.getADecorator() | decorator.(Name).getId() = "classmethod")
3332
}
3433

3534
from Function f, string message
36-
where (f.getADecorator().(Name).getId() = "classmethod" or is_type_method(f)) and
37-
not first_arg_cls(f) and classmethod_decorators_only(f) and
38-
not f.getName() = "__new__" and
39-
(
40-
if exists(f.getArgName(0)) then
41-
message = "Class methods or methods of a type deriving from type should have 'cls', rather than '" +
42-
f.getArgName(0) + "', as their first argument."
43-
else
44-
message = "Class methods or methods of a type deriving from type should have 'cls' as their first argument."
45-
)
46-
35+
where
36+
(f.getADecorator().(Name).getId() = "classmethod" or is_type_method(f)) and
37+
not first_arg_cls(f) and
38+
classmethod_decorators_only(f) and
39+
not f.getName() = "__new__" and
40+
(
41+
if exists(f.getArgName(0))
42+
then
43+
message = "Class methods or methods of a type deriving from type should have 'cls', rather than '"
44+
+ f.getArgName(0) + "', as their first parameter."
45+
else
46+
message = "Class methods or methods of a type deriving from type should have 'cls' as their first parameter."
47+
)
4748
select f, message

‎python/ql/src/Functions/NonSelf.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
class Point:
2-
def __init__(val, x, y): # first argument is mis-named 'val'
2+
def __init__(val, x, y): # first parameter is mis-named 'val'
33
val._x = x
44
val._y = y
5-
5+
66
class Point2:
7-
def __init__(self, x, y): # first argument is correctly named 'self'
7+
def __init__(self, x, y): # first parameter is correctly named 'self'
88
self._x = x
99
self._y = y

‎python/ql/src/Functions/NonSelf.ql

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
2-
* @name First argument of a method is not named 'self'
3-
* @description Using an alternative name for the first argument of an instance method makes
4-
* code more difficult to read; PEP8 states that the first argument to instance
2+
* @name First parameter of a method is not named 'self'
3+
* @description Using an alternative name for the first parameter of an instance method makes
4+
* code more difficult to read; PEP8 states that the first parameter to instance
55
* methods should be 'self'.
66
* @kind problem
77
* @tags maintainability
@@ -16,39 +16,39 @@
1616
import python
1717
import semmle.python.libraries.Zope
1818

19-
predicate first_arg_self(Function f) {
20-
f.getArgName(0) = "self"
19+
predicate is_type_method(FunctionValue fv) {
20+
exists(ClassValue c | c.declaredAttribute(_) = fv and c.getASuperType() = ClassValue::type())
2121
}
2222

23-
predicate is_type_method(FunctionObject f) {
24-
exists(ClassObject c | c.lookupAttribute(_) = f and c.getASuperType() = theTypeType())
23+
predicate used_in_defining_scope(FunctionValue fv) {
24+
exists(Call c | c.getScope() = fv.getScope().getScope() and c.getFunc().pointsTo(fv))
2525
}
2626

27-
predicate used_in_defining_scope(FunctionObject f) {
28-
exists(Call c |
29-
c.getScope() = f.getFunction().getScope() and
30-
c.getFunc().refersTo(f)
31-
)
32-
}
33-
34-
from Function f, PyFunctionObject func, string message
27+
from Function f, FunctionValue fv, string message
3528
where
36-
exists(ClassObject cls, string name |
37-
cls.declaredAttribute(name) = func and cls.isNewStyle() and
38-
not name = "__new__" and
39-
not name = "__metaclass__" and
40-
/* declared in scope */
41-
f.getScope() = cls.getPyClass()
42-
) and
43-
not first_arg_self(f) and not is_type_method(func) and
44-
func.getFunction() = f and not f.getName() = "lambda" and
45-
not used_in_defining_scope(func) and
46-
(
47-
if exists(f.getArgName(0)) then
48-
message = "Normal methods should have 'self', rather than '" + f.getArgName(0) + "', as their first parameter."
49-
else
50-
message = "Normal methods should have at least one parameter (the first of which should be 'self')." and not f.hasVarArg()
51-
) and
52-
not func instanceof ZopeInterfaceMethod
53-
29+
exists(ClassValue cls, string name |
30+
cls.declaredAttribute(name) = fv and
31+
cls.isNewStyle() and
32+
not name = "__new__" and
33+
not name = "__metaclass__" and
34+
/* declared in scope */
35+
f.getScope() = cls.getScope()
36+
) and
37+
not f.getArgName(0) = "self" and
38+
not is_type_method(fv) and
39+
fv.getScope() = f and
40+
not f.getName() = "lambda" and
41+
not used_in_defining_scope(fv) and
42+
(
43+
(
44+
if exists(f.getArgName(0))
45+
then
46+
message = "Normal methods should have 'self', rather than '" + f.getArgName(0) +
47+
"', as their first parameter."
48+
else
49+
message = "Normal methods should have at least one parameter (the first of which should be 'self')."
50+
) and
51+
not f.hasVarArg()
52+
) and
53+
not fv instanceof ZopeInterfaceMethodValue
5454
select f, message

‎python/ql/src/semmle/python/libraries/Zope.qll

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22

33
import python
44

5+
private import semmle.python.pointsto.PointsTo
6+
57
/** A method that to a sub-class of `zope.interface.Interface` */
6-
class ZopeInterfaceMethod extends PyFunctionObject {
8+
deprecated class ZopeInterfaceMethod extends PyFunctionObject {
79

810
/** Holds if this method belongs to a class that sub-classes `zope.interface.Interface` */
911
ZopeInterfaceMethod() {
@@ -26,3 +28,31 @@ class ZopeInterfaceMethod extends PyFunctionObject {
2628
}
2729

2830
}
31+
32+
/** A method that belongs to a sub-class of `zope.interface.Interface` */
33+
class ZopeInterfaceMethodValue extends PythonFunctionValue {
34+
35+
/** Holds if this method belongs to a class that sub-classes `zope.interface.Interface` */
36+
ZopeInterfaceMethodValue() {
37+
exists(Value interface, ClassValue owner |
38+
interface = Module::named("zope.interface").attr("Interface") and
39+
owner.declaredAttribute(_) = this and
40+
// `zope.interface.Interface` will be recognized as a Value by the pointsTo analysis,
41+
// because it is the result of instantiating a "meta" class. getASuperType only returns
42+
// ClassValues, so we do this little trick to make things work
43+
Types::getBase(owner.getASuperType(), _) = interface
44+
)
45+
}
46+
47+
override int minParameters() {
48+
result = super.minParameters() + 1
49+
}
50+
51+
override int maxParameters() {
52+
if exists(this.getScope().getVararg()) then
53+
result = super.maxParameters()
54+
else
55+
result = super.maxParameters() + 1
56+
}
57+
58+
}

‎python/ql/src/semmle/python/objects/ObjectAPI.qll

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,11 @@ abstract class FunctionValue extends CallableValue {
434434

435435
abstract string getQualifiedName();
436436

437+
/** Gets the minimum number of parameters that can be correctly passed to this function */
438+
abstract int minParameters();
439+
440+
/** Gets the maximum number of parameters that can be correctly passed to this function */
441+
abstract int maxParameters();
437442
}
438443

439444
/** Class representing Python functions */
@@ -447,6 +452,23 @@ class PythonFunctionValue extends FunctionValue {
447452
result = this.(PythonFunctionObjectInternal).getScope().getQualifiedName()
448453
}
449454

455+
override int minParameters() {
456+
exists(Function f |
457+
f = this.getScope() and
458+
result = count(f.getAnArg()) - count(f.getDefinition().getArgs().getADefault())
459+
)
460+
}
461+
462+
override int maxParameters() {
463+
exists(Function f |
464+
f = this.getScope() and
465+
if exists(f.getVararg()) then
466+
result = 2147483647 // INT_MAX
467+
else
468+
result = count(f.getAnArg())
469+
)
470+
}
471+
450472
}
451473

452474
/** Class representing builtin functions, such as `len` or `print` */
@@ -460,6 +482,13 @@ class BuiltinFunctionValue extends FunctionValue {
460482
result = this.(BuiltinFunctionObjectInternal).getName()
461483
}
462484

485+
override int minParameters() {
486+
none()
487+
}
488+
489+
override int maxParameters() {
490+
none()
491+
}
463492
}
464493

465494
/** Class representing builtin methods, such as `list.append` or `set.add` */
@@ -477,6 +506,14 @@ class BuiltinMethodValue extends FunctionValue {
477506
)
478507
}
479508

509+
override int minParameters() {
510+
none()
511+
}
512+
513+
override int maxParameters() {
514+
none()
515+
}
516+
480517
}
481518

482519
/** A class representing sequence objects with a length and tracked items.
@@ -560,6 +597,11 @@ module ClassValue {
560597
result = TBuiltinClassObject(Builtin::special("FunctionType"))
561598
}
562599

600+
/** Get the `ClassValue` for the `type` class. */
601+
ClassValue type() {
602+
result = TType()
603+
}
604+
563605
/** Get the `ClassValue` for the class of builtin functions. */
564606
ClassValue builtinFunction() {
565607
result = Value::named("len").getClass()

‎python/ql/test/library-tests/types/functions/Zope.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
| Function yes |
1+
| Function Z.yes |

‎python/ql/test/library-tests/types/functions/Zope.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@
22
import python
33
import semmle.python.libraries.Zope
44

5-
from ZopeInterfaceMethod f
5+
from ZopeInterfaceMethodValue f
66
select f.toString()

‎python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
edges
22
| functions_test.py:39:9:39:9 | empty mutable value | functions_test.py:40:5:40:5 | empty mutable value |
33
| functions_test.py:133:15:133:15 | empty mutable value | functions_test.py:134:5:134:5 | empty mutable value |
4-
| functions_test.py:185:25:185:25 | empty mutable value | functions_test.py:186:5:186:5 | empty mutable value |
5-
| functions_test.py:188:21:188:21 | empty mutable value | functions_test.py:189:5:189:5 | empty mutable value |
6-
| functions_test.py:191:27:191:27 | empty mutable value | functions_test.py:192:25:192:25 | empty mutable value |
7-
| functions_test.py:191:27:191:27 | empty mutable value | functions_test.py:193:21:193:21 | empty mutable value |
8-
| functions_test.py:192:25:192:25 | empty mutable value | functions_test.py:185:25:185:25 | empty mutable value |
9-
| functions_test.py:193:21:193:21 | empty mutable value | functions_test.py:188:21:188:21 | empty mutable value |
4+
| functions_test.py:151:25:151:25 | empty mutable value | functions_test.py:152:5:152:5 | empty mutable value |
5+
| functions_test.py:154:21:154:21 | empty mutable value | functions_test.py:155:5:155:5 | empty mutable value |
6+
| functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:158:25:158:25 | empty mutable value |
7+
| functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:159:21:159:21 | empty mutable value |
8+
| functions_test.py:158:25:158:25 | empty mutable value | functions_test.py:151:25:151:25 | empty mutable value |
9+
| functions_test.py:159:21:159:21 | empty mutable value | functions_test.py:154:21:154:21 | empty mutable value |
1010
#select
1111
| functions_test.py:40:5:40:5 | x | functions_test.py:39:9:39:9 | empty mutable value | functions_test.py:40:5:40:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:39:9:39:9 | x | Default value |
1212
| functions_test.py:134:5:134:5 | x | functions_test.py:133:15:133:15 | empty mutable value | functions_test.py:134:5:134:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:133:15:133:15 | x | Default value |
13-
| functions_test.py:186:5:186:5 | x | functions_test.py:191:27:191:27 | empty mutable value | functions_test.py:186:5:186:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:191:27:191:27 | y | Default value |
14-
| functions_test.py:189:5:189:5 | x | functions_test.py:191:27:191:27 | empty mutable value | functions_test.py:189:5:189:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:191:27:191:27 | y | Default value |
13+
| functions_test.py:152:5:152:5 | x | functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:152:5:152:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:157:27:157:27 | y | Default value |
14+
| functions_test.py:155:5:155:5 | x | functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:155:5:155:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:157:27:157:27 | y | Default value |

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
1-
| argument_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 argument. |
2-
| argument_names.py:32:5:32:20 | Function c_method | Class methods or methods of a type deriving from type should have 'cls', rather than 'y', as their first argument. |
1+
| 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. |
2+
| 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. |
3+
| 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. |

0 commit comments

Comments
 (0)