pylint-dev / pylint-django

Pylint plugin for improving code analysis for when using Django
Other
590 stars 117 forks source link

method decorated with django's `@classproperty` raises `E0213: Method should have "self" as first argument` #240

Open waterimp opened 5 years ago

waterimp commented 5 years ago

problem

pylint does not recognize django.utils.decorators.classproperty is actually like a class method, and expects self as the first argument instead of cls.

example

In the example below, my_class_property() (decorated with @classproperty) should pass linting but does not. For comparison, we can see that my_class_method() passes linting with the @classmethod decorator, though.

pylint --load-plugins=pylint_django my_project output:

************* Module my_project.my_module
my_project/my_module.py:10:4: E0213: Method should have "self" as first argument (no-self-argument)
my_project/my_module.py:10:4: R0201: Method could be a function (no-self-use)

contents of my_project/my_module.py:

"""Demonstrate pylint problem."""
from django.utils.decorators import classproperty

class MyClass:
    """
    Just a test class to demonstrate the problem.
    """
    @classproperty
    def my_class_property(cls):
        """
        This demonstrates the problem with pylint and django.
        """
        return 'hello world.'

    @classmethod
    def my_class_method(cls):
        """
        This is a normal Python class method and will pass pylint.
        """
        return 'hello world 2.'

software versions

Python 3.5.2

full pip freeze output:

astroid==2.2.5
Django==2.2.3
isort==4.3.21
lazy-object-proxy==1.4.1
mccabe==0.6.1
pkg-resources==0.0.0
pylint==2.3.1
pylint-django==2.0.10
pylint-plugin-utils==0.5
pytz==2019.1
six==1.12.0
sqlparse==0.3.0
typed-ast==1.4.0
wrapt==1.11.2
ProProgrammer commented 4 years ago

reproducible on Python 3.6.10 with django-dev (as of now) installed


asgiref==3.2.7
bcrypt==3.1.7
certifi==2019.11.28
cffi==1.14.0
chardet==3.0.4
-e git+git@github.com:ProProgrammer/django.git@3728ac15a01a0dd973466af813cd91ab9c43fc70#egg=Django
docutils==0.16
geoip2==3.0.0
idna==2.9
Jinja2==2.11.1
MarkupSafe==1.1.1
maxminddb==1.5.2
numpy==1.18.2
Pillow==7.0.0
pycparser==2.20
pylibmc==1.6.1
python-memcached==1.59
pytz==2019.3
pywatchman==1.4.1
PyYAML==5.3.1
requests==2.23.0
selenium==3.141.0
six==1.14.0
sqlparse==0.3.1
tblib==1.6.0
urllib3==1.25.8```
stefan6419846 commented 2 years ago

I did not yet test it within a real pylint plugin run, but judging from the code something like this should probably be added to pylint_django.augmentations.apply_augmentations(linter):

suppress_message(linter, ClassChecker.visit_functiondef, "no-self-argument", has_classproperty)

The check is implemented like this:

def _extract_decorator_name(node):
    from astroid.node_classes import Attribute, Call, Name
    if isinstance(node, Attribute):
        # Objects accessed from imported module.
        return node.attrname
    if isinstance(node, Name):
        # Objects imported directly.
        return node.name
    if isinstance(node, Call):
        # Unwrap (method) calls and retrieve the name from the inner function node.
        return _extract_decorator_name(node.func)

def has_classproperty(node):
    if node.decorators is None:
        return False
    decorator_names = {
        _extract_decorator_name(decorator) for decorator in node.decorators.nodes
    }
    return 'classproperty' in decorator_names

Please note that this just disables reporting the false positives, while we probably still want a dedicated checker which makes sure that the first argument actually is cls.

A rough checker method (without the actual message declaration and class) might look like this:

def visit_functiondef(self, node: FunctionDef) -> NoReturn:
    if not has_classproperty(node):
        return
    # Copied from `pylint.checkers.classes.class_checker.ClassChecker._check_first_arg_for_type`.
    if node.args.posonlyargs:
        first_arg = node.args.posonlyargs[0].name
    elif node.args.args is not None:
        first_arg = node.argnames()[0]
    else:
        first_arg = None

    if not first_arg == "cls":
        self.add_message("classproperty-without-cls-argument", node=node)