add --classmethod-decorators (#405)

* add --classmethod-decorators

* reproduce attribute handling like pep8-naming

* fix pep8-naming coexistence, add tox environment

* fix CI

* update readme.rst, also check metaclass logic

* broke out logic to helper function

* set->Set due to py38

* ignore C901

Author: jakkdl

README.rst

@@ -297,20 +297,24 @@ and/or ignored codes.
 Configuration
 -------------
 
-The plugin currently has one setting:
+The plugin currently has the following settings:
 
 ``extend-immutable-calls``: Specify a list of additional immutable calls.
 This could be useful, when using other libraries that provide more immutable calls,
 beside those already handled by ``flake8-bugbear``. Calls to these method will no longer
 raise a ``B008`` warning.
 
+``classmethod-decorators``: Specify a list of decorators to additionally mark a method as a ``classmethod`` as used by B902. Default values are ``classmethod, validator, root_validator``, and when an ``@obj.name`` decorator is specified it will match against either ``name`` or ``obj.name``.
+This functions similarly to how `pep8-naming <https://github.com/PyCQA/pep8-naming>` handles it, but with different defaults, and they don't support specifying attributes such that a decorator will never match against a specified value ``obj.name`` even if decorated with ``@obj.name``.
+
 For example::
 
   [flake8]
   max-line-length = 80
   max-complexity = 12
   ...
   extend-immutable-calls = pathlib.Path, Path
+  classmethod-decorators = myclassmethod, mylibrary.otherclassmethod
 
 Tests / Lints
 ---------------

bugbear.py

@@ -10,6 +10,7 @@
 from contextlib import suppress
 from functools import lru_cache, partial
 from keyword import iskeyword
+from typing import Dict, List, Set, Union
 
 import attr
 import pycodestyle
@@ -38,6 +39,8 @@
     "assertWarnsRegex",
 }
 
+B902_default_decorators = {"classmethod", "validator", "root_validator"}
+
 Context = namedtuple("Context", ["node", "stack"])
 
 
@@ -62,10 +65,15 @@ def run(self):
         else:
             b008_extend_immutable_calls = set()
 
+        b902_classmethod_decorators: set[str] = B902_default_decorators
+        if self.options and hasattr(self.options, "classmethod_decorators"):
+            b902_classmethod_decorators = set(self.options.classmethod_decorators)
+
         visitor = self.visitor(
             filename=self.filename,
             lines=self.lines,
             b008_extend_immutable_calls=b008_extend_immutable_calls,
+            b902_classmethod_decorators=b902_classmethod_decorators,
         )
         visitor.visit(self.tree)
         for e in itertools.chain(visitor.errors, self.gen_line_based_checks()):
@@ -143,6 +151,19 @@ def add_options(optmanager):
             default=[],
             help="Skip B008 test for additional immutable calls.",
         )
+        # you cannot register the same option in two different plugins, so we
+        # only register --classmethod-decorators if pep8-naming is not installed
+        if "pep8ext_naming" not in sys.modules.keys():
+            optmanager.add_option(
+                "--classmethod-decorators",
+                comma_separated_list=True,
+                parse_from_config=True,
+                default=B902_default_decorators,
+                help=(
+                    "List of method decorators that should be treated as classmethods"
+                    " by B902"
+                ),
+            )
 
     @lru_cache  # noqa: B019
     def should_warn(self, code):
@@ -308,6 +329,7 @@ class BugBearVisitor(ast.NodeVisitor):
     filename = attr.ib()
     lines = attr.ib()
     b008_extend_immutable_calls = attr.ib(default=attr.Factory(set))
+    b902_classmethod_decorators = attr.ib(default=attr.Factory(set))
     node_window = attr.ib(default=attr.Factory(list))
     errors = attr.ib(default=attr.Factory(list))
     futures = attr.ib(default=attr.Factory(set))
@@ -971,37 +993,51 @@ def check_for_b901(self, node):
                 self.errors.append(B901(return_node.lineno, return_node.col_offset))
                 break
 
-    def check_for_b902(self, node):
+    # taken from pep8-naming
+    @classmethod
+    def find_decorator_name(cls, d):
+        if isinstance(d, ast.Name):
+            return d.id
+        elif isinstance(d, ast.Attribute):
+            return d.attr
+        elif isinstance(d, ast.Call):
+            return cls.find_decorator_name(d.func)
+
+    def check_for_b902(  # noqa: C901 (too complex)
+        self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef]
+    ) -> None:
+        def is_classmethod(decorators: Set[str]) -> bool:
+            return (
+                any(name in decorators for name in self.b902_classmethod_decorators)
+                or node.name in B902.implicit_classmethods
+            )
+
         if len(self.contexts) < 2 or not isinstance(
             self.contexts[-2].node, ast.ClassDef
         ):
             return
 
         cls = self.contexts[-2].node
-        decorators = NameFinder()
-        decorators.visit(node.decorator_list)
 
-        if "staticmethod" in decorators.names:
+        decorators: set[str] = {
+            self.find_decorator_name(d) for d in node.decorator_list
+        }
+
+        if "staticmethod" in decorators:
             # TODO: maybe warn if the first argument is surprisingly `self` or
             # `cls`?
             return
 
         bases = {b.id for b in cls.bases if isinstance(b, ast.Name)}
         if "type" in bases:
-            if (
-                "classmethod" in decorators.names
-                or node.name in B902.implicit_classmethods
-            ):
+            if is_classmethod(decorators):
                 expected_first_args = B902.metacls
                 kind = "metaclass class"
             else:
                 expected_first_args = B902.cls
                 kind = "metaclass instance"
         else:
-            if (
-                "classmethod" in decorators.names
-                or node.name in B902.implicit_classmethods
-            ):
+            if is_classmethod(decorators):
                 expected_first_args = B902.cls
                 kind = "class"
             else:
@@ -1438,9 +1474,11 @@ class NameFinder(ast.NodeVisitor):
     key is name string, value is the node (useful for location purposes).
     """
 
-    names = attr.ib(default=attr.Factory(dict))
+    names: Dict[str, List[ast.Name]] = attr.ib(default=attr.Factory(dict))
 
-    def visit_Name(self, node):  # noqa: B906 # names don't contain other names
+    def visit_Name(  # noqa: B906 # names don't contain other names
+        self, node: ast.Name
+    ):
         self.names.setdefault(node.id, []).append(node)
 
     def visit(self, node):

tests/b902_extended.py

@@ -0,0 +1,104 @@
+# parameters: --classmethod-decorators=["mylibrary.classmethod", "validator"]
+class Errors:
+    # correctly registered as classmethod
+    @validator
+    def foo_validator(self) -> None: ...
+
+    @other.validator
+    def foo_other_validator(self) -> None: ...
+
+    @foo.bar.validator
+    def foo_foo_bar_validator(self) -> None: ...
+
+    @validator.blah
+    def foo_validator_blah(cls) -> None: ...
+
+    # specifying attribute in options is not valid
+    @mylibrary.makeclassmethod
+    def foo2(cls) -> None: ...
+
+    # specified attribute in options
+    @makeclassmethod
+    def foo6(cls) -> None: ...
+
+    # classmethod is default, but if not specified it's ignored
+    @classmethod
+    def foo3(cls) -> None: ...
+
+    # random unknown decorator
+    @aoeuaoeu
+    def foo5(cls) -> None: ...
+
+
+class NoErrors:
+    @validator
+    def foo1(cls) -> None: ...
+
+    @other.validator
+    def foo4(cls) -> None: ...
+
+    @mylibrary.makeclassmethod
+    def foo2(self) -> None: ...
+
+    @classmethod
+    def foo3(self) -> None: ...
+
+    @aoeuaoeu
+    def foo5(self) -> None: ...
+
+    @makeclassmethod
+    def foo6(self) -> None: ...
+
+
+# Above tests, duplicated to check that the separate logic for metaclasses also works
+
+
+class ErrorsMeta(type):
+    # correctly registered as classmethod
+    @validator
+    def foo_validator(cls) -> None: ...
+
+    @other.validator
+    def foo_other_validator(cls) -> None: ...
+
+    @foo.bar.validator
+    def foo_foo_bar_validator(cls) -> None: ...
+
+    @validator.blah
+    def foo_validator_blah(metacls) -> None: ...
+
+    # specifying attribute in options is not valid
+    @mylibrary.makeclassmethod
+    def foo2(metacls) -> None: ...
+
+    # specified attribute in options
+    @makeclassmethod
+    def foo6(metacls) -> None: ...
+
+    # classmethod is default, but if not specified it's ignored
+    @classmethod
+    def foo3(metacls) -> None: ...
+
+    # random unknown decorator
+    @aoeuaoeu
+    def foo5(metacls) -> None: ...
+
+
+class NoErrorsMeta(type):
+    @validator
+    def foo1(metacls) -> None: ...
+
+    @other.validator
+    def foo4(metacls) -> None: ...
+
+    @mylibrary.makeclassmethod
+    def foo2(cls) -> None: ...
+
+    @classmethod
+    def foo3(cls) -> None: ...
+
+    @aoeuaoeu
+    def foo5(cls) -> None: ...
+
+    @makeclassmethod
+    def foo6(cls) -> None: ...

tests/test_bugbear.py

@@ -667,6 +667,39 @@ def test_b902(self):
             ),
         )
 
+    def test_b902_extended(self):
+        filename = Path(__file__).absolute().parent / "b902_extended.py"
+
+        mock_options = Namespace(
+            classmethod_decorators=["mylibrary.makeclassmethod", "validator"],
+            select=["B902"],
+        )
+        bbc = BugBearChecker(filename=str(filename), options=mock_options)
+        errors = list(bbc.run())
+
+        self.assertEqual(
+            errors,
+            self.errors(
+                B902(5, 22, vars=("'self'", "class", "cls")),
+                B902(8, 28, vars=("'self'", "class", "cls")),
+                B902(11, 30, vars=("'self'", "class", "cls")),
+                B902(14, 27, vars=("'cls'", "instance", "self")),
+                B902(18, 13, vars=("'cls'", "instance", "self")),
+                B902(22, 13, vars=("'cls'", "instance", "self")),
+                B902(26, 13, vars=("'cls'", "instance", "self")),
+                B902(30, 13, vars=("'cls'", "instance", "self")),
+                # metaclass
+                B902(59, 22, vars=("'cls'", "metaclass class", "metacls")),
+                B902(62, 28, vars=("'cls'", "metaclass class", "metacls")),
+                B902(65, 30, vars=("'cls'", "metaclass class", "metacls")),
+                B902(68, 27, vars=("'metacls'", "metaclass instance", "cls")),
+                B902(72, 13, vars=("'metacls'", "metaclass instance", "cls")),
+                B902(76, 13, vars=("'metacls'", "metaclass instance", "cls")),
+                B902(80, 13, vars=("'metacls'", "metaclass instance", "cls")),
+                B902(84, 13, vars=("'metacls'", "metaclass instance", "cls")),
+            ),
+        )
+
     def test_b902_py38(self):
         filename = Path(__file__).absolute().parent / "b902_py38.py"
         bbc = BugBearChecker(filename=str(filename))

tox.ini

@@ -1,14 +1,14 @@
 # The test environment and commands
 [tox]
 # default environments to run without `-e`
-envlist = py38, py39, py310, py311, py312
+envlist = py38, py39, py310, py311, py312, pep8_naming
 
 [gh-actions]
 python =
     3.8: py38
     3.9: py39
     3.10: py310
-    3.11: py311
+    3.11: py311,pep8_naming
     3.12: py312
 
 [testenv]
@@ -21,6 +21,16 @@ commands =
     coverage run tests/test_bugbear.py {posargs}
     coverage report -m
 
+[testenv:pep8_naming]
+deps =
+    coverage
+    hypothesis
+    hypothesmith
+    pep8-naming
+commands =
+    coverage run tests/test_bugbear.py -k b902 {posargs}
+    coverage report -m
+
 [testenv:py312]
 deps =
     # the other dependencies aren't yet installable on py312+