Add B019 check to find cache decorators on class methods (#218)

* Add B019 check to find cache decorators on class methods

* B019: Change decorator resolution approach to retain lineno

Starting in Python 3.8, the function node definition's `lineno` is changed to index its `def ...` line rather than the first line where its decorators start. This causes inconsistent line numbers across Python versions for the line reported by Flake8.

We can use the decorator node location instead, which provides a consistent location, and makes sense because this hits on decorators.

* Update README verbiage

* Prefer `extend-select` and `extend-ignore` for configuring opinionated warnings (`B9`)
* Add deprecation note for Bugbear's internal handling of whether or not to emit `B9` codes
* Add an example for `extend-immutable-call` specification

* B9: Make Bugbear aware of flake8's `extend-select`

* The code for Bugbear's built-in filtering for opinionated warnings predates the addition of `extend-select` to flake8 (`v4.0`) so it's not part of the check for explicit specification of `B9` codes.
* Switch from `Mock` to `argparse.Namespace` for specifying options to tests to match the incoming type from `flake8` and avoid mocking side-effects.

* Style fixes from review

Co-authored-by: Cooper Ry Lees <[email protected]>

Co-authored-by: Cooper Ry Lees <[email protected]>

Author: sco1

README.rst

@@ -8,9 +8,9 @@ flake8-bugbear
 .. image:: https://img.shields.io/badge/code%20style-black-000000.svg
     :target: https://github.com/psf/black
 
-A plugin for Flake8 finding likely bugs and design problems in your
-program.  Contains warnings that don't belong in pyflakes and
-pycodestyle::
+A plugin for ``flake8`` finding likely bugs and design problems in your
+program.  Contains warnings that don't belong in ``pyflakes`` and
+``pycodestyle``::
 
     bug·bear  (bŭg′bâr′)
     n.
@@ -57,7 +57,7 @@ List of warnings
 **B001**: Do not use bare ``except:``, it also catches unexpected events
 like memory errors, interrupts, system exit, and so on.  Prefer ``except
 Exception:``.  If you're sure what you're doing, be explicit and write
-``except BaseException:``.  Disable E722 to avoid duplicate warnings.
+``except BaseException:``.  Disable ``E722`` to avoid duplicate warnings.
 
 **B002**: Python does not support the unary prefix increment. Writing
 ``++n`` is equivalent to ``+(+(n))``, which equals ``n``. You meant ``n
@@ -132,12 +132,15 @@ data available in ``ex``.
 
 **B018**: Found useless expression. Either assign it to a variable or remove it.
 
+**B019**: Use of ``functools.lru_cache`` or ``functools.cache`` on class methods
+can lead to memory leaks. The cache may retain instance references, preventing
+garbage collection.
+
 **B020**: Loop control variable overrides iterable it iterates
 
 **B021**: f-string used as docstring. This will be interpreted by python
 as a joined string rather than a docstring.
 
-
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 
@@ -170,15 +173,15 @@ See `the exception chaining tutorial <https://docs.python.org/3/tutorial/errors.
 for details.
 
 **B950**: Line too long. This is a pragmatic equivalent of
-``pycodestyle``'s E501: it considers "max-line-length" but only triggers
+``pycodestyle``'s ``E501``: it considers "max-line-length" but only triggers
 when the value has been exceeded by **more than 10%**. You will no
 longer be forced to reformat code due to the closing parenthesis being
 one character too far to satisfy the linter. At the same time, if you do
 significantly violate the line length, you will receive a message that
 states what the actual limit is. This is inspired by Raymond Hettinger's
 `"Beyond PEP 8" talk <https://www.youtube.com/watch?v=wf-BqAjZb8M>`_ and
 highway patrol not stopping you if you drive < 5mph too fast. Disable
-E501 to avoid duplicate warnings. Like E501, this error ignores long shebangs
+``E501`` to avoid duplicate warnings. Like ``E501``, this error ignores long shebangs
 on the first line and urls or paths that are on their own line::
 
   #! long shebang ignored
@@ -192,41 +195,57 @@ on the first line and urls or paths that are on their own line::
 How to enable opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-To enable these checks, specify a ``--select`` command-line option or
-``select=`` option in your config file.  As of Flake8 3.0, this option
-is a whitelist (checks not listed are being implicitly disabled), so you
-have to explicitly specify all checks you want enabled. For example::
-
-	[flake8]
-	max-line-length = 80
-	max-complexity = 12
-	...
-	ignore = E501
-	select = C,E,F,W,B,B901
-
-Note that we're enabling the complexity checks, the PEP8 ``pycodestyle``
-errors and warnings, the pyflakes fatals and all default Bugbear checks.
-Finally, we're also specifying B901 as a check that we want enabled.
-Some checks might need other flake8 checks disabled - e.g. E501 must be
-disabled for B950 to be hit.
-
-If you'd like all optional warnings to be enabled for you (future proof
-your config!), say ``B9`` instead of ``B901``. You will need Flake8 3.2+
-for this feature.
-
-Note that ``pycodestyle`` also has a bunch of warnings that are disabled
-by default.  Those get enabled as soon as there is an ``ignore =`` line
-in your configuration.  I think this behavior is surprising so Bugbear's
+To enable Bugbear's opinionated checks (``B9xx``), specify an ``--extend-select``
+command-line option or ``extend-select=`` option in your config file
+(requires ``flake8 >=4.0``)::
+
+  [flake8]
+  max-line-length = 80
+  max-complexity = 12
+  ...
+  extend-ignore = E501
+  extend-select = B950
+
+Some of Bugbear's checks require other ``flake8`` checks disabled - e.g. ``E501`` must
+be disabled when enabling ``B950``.
+
+If you'd like all optional warnings to be enabled for you (future proof your config!),
+say ``B9`` instead of ``B950``. You will need ``flake8 >=3.2`` for this feature.
+
+For ``flake8 <=4.0``, you will need to use the ``--select`` command-line option or
+``select=`` option in your config file. For ``flake8 >=3.0``, this option is a whitelist
+(checks not listed are implicitly disabled), so you have to explicitly specify all
+checks you want enabled (e.g. ``select = C,E,F,W,B,B950``).
+
+The ``--extend-ignore`` command-line option and ``extend-ignore=`` config file option
+require ``flake8 >=3.6``. For older ``flake8`` versions, the ``--ignore`` and
+``ignore=`` options can be used. Using ``ignore`` will override all codes that are
+disabled by default from all installed linters, so you will need to specify these codes
+in your configuration to silence them. I think this behavior is surprising so Bugbear's
 opinionated warnings require explicit selection.
 
+**Note:** Bugbear's enforcement of explicit opinionated warning selection is deprecated
+and will be removed in a future release. It is recommended to use ``extend-ignore`` and
+``extend-select`` in your ``flake8`` configuration to avoid implicitly altering selected
+and/or ignored codes.
+
 Configuration
 -------------
 
 The plugin currently has one setting:
 
 ``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.
+beside those already handled by ``flake8-bugbear``. Calls to these method will no longer
+raise a ``B008`` warning.
+
+For example::
+
+  [flake8]
+  max-line-length = 80
+  max-complexity = 12
+  ...
+  extend-immutable-calls = pathlib.Path, Path
 
 Tests / Lints
 ---------------

bugbear.py

@@ -127,7 +127,7 @@ def add_options(optmanager):
             help="Skip B008 test for additional immutable calls.",
         )
 
-    @lru_cache()
+    @lru_cache()  # noqa: B019
     def should_warn(self, code):
         """Returns `True` if Bugbear should emit a particular warning.
 
@@ -138,12 +138,17 @@ def should_warn(self, code):
 
         As documented in the README, the user is expected to explicitly select
         the warnings.
+
+        NOTE: This method is deprecated and will be removed in a future release. It is
+        recommended to use `extend-ignore` and `extend-select` in your flake8
+        configuration to avoid implicitly altering selected and ignored codes.
         """
         if code[:2] != "B9":
             # Normal warnings are safe for emission.
             return True
 
         if self.options is None:
+            # Without options configured, Bugbear will emit B9 but flake8 will ignore
             LOG.info(
                 "Options not provided to Bugbear, optional warning %s selected.", code
             )
@@ -153,6 +158,13 @@ def should_warn(self, code):
             if code[:i] in self.options.select:
                 return True
 
+            # flake8 >=4.0: Also check for codes in extend_select
+            if (
+                hasattr(self.options, "extend_select")
+                and code[:i] in self.options.extend_select
+            ):
+                return True
+
         LOG.info(
             "Optional warning %s not present in selected warnings: %r. Not "
             "firing it at all.",
@@ -350,6 +362,7 @@ def visit_FunctionDef(self, node):
         self.check_for_b902(node)
         self.check_for_b006(node)
         self.check_for_b018(node)
+        self.check_for_b019(node)
         self.check_for_b021(node)
         self.generic_visit(node)
 
@@ -380,6 +393,8 @@ def compose_call_path(self, node):
         if isinstance(node, ast.Attribute):
             yield from self.compose_call_path(node.value)
             yield node.attr
+        elif isinstance(node, ast.Call):
+            yield from self.compose_call_path(node.func)
         elif isinstance(node, ast.Name):
             yield node.id
 
@@ -509,6 +524,33 @@ def check_for_b017(self, node):
         ):
             self.errors.append(B017(node.lineno, node.col_offset))
 
+    def check_for_b019(self, node):
+        if (
+            len(node.decorator_list) == 0
+            or len(self.contexts) < 2
+            or not isinstance(self.contexts[-2].node, ast.ClassDef)
+        ):
+            return
+
+        # Preserve decorator order so we can get the lineno from the decorator node
+        # rather than the function node (this location definition changes in Python 3.8)
+        resolved_decorators = (
+            ".".join(self.compose_call_path(decorator))
+            for decorator in node.decorator_list
+        )
+        for idx, decorator in enumerate(resolved_decorators):
+            if decorator in {"classmethod", "staticmethod"}:
+                return
+
+            if decorator in B019.caches:
+                self.errors.append(
+                    B019(
+                        node.decorator_list[idx].lineno,
+                        node.decorator_list[idx].col_offset,
+                    )
+                )
+                return
+
     def check_for_b020(self, node):
         targets = NameFinder()
         targets.visit(node.target)
@@ -898,6 +940,19 @@ def visit(self, node):
         "B018 Found useless expression. Either assign it to a variable or remove it."
     )
 )
+B019 = Error(
+    message=(
+        "B019 Use of `functools.lru_cache` or `functools.cache` on class methods "
+        "can lead to memory leaks. The cache may retain instance references, "
+        "preventing garbage collection."
+    )
+)
+B019.caches = {
+    "functools.cache",
+    "functools.lru_cache",
+    "cache",
+    "lru_cache",
+}
 B020 = Error(
     message=(
         "B020 Found for loop that reassigns the iterable it is iterating "

tests/b019.py

@@ -0,0 +1,103 @@
+"""
+Should emit:
+B019 - on lines 73, 77, 81, 85, 89, 93, 97, 101
+"""
+import functools
+from functools import cache, cached_property, lru_cache
+
+
+def some_other_cache():
+    ...
+
+
+class Foo:
+    def __init__(self, x):
+        self.x = x
+
+    def compute_method(self, y):
+        ...
+
+    @some_other_cache
+    def user_cached_method(self, y):
+        ...
+
+    @classmethod
+    @functools.cache
+    def cached_classmethod(cls, y):
+        ...
+
+    @classmethod
+    @cache
+    def other_cached_classmethod(cls, y):
+        ...
+
+    @classmethod
+    @functools.lru_cache
+    def lru_cached_classmethod(cls, y):
+        ...
+
+    @classmethod
+    @lru_cache
+    def other_lru_cached_classmethod(cls, y):
+        ...
+
+    @staticmethod
+    @functools.cache
+    def cached_staticmethod(y):
+        ...
+
+    @staticmethod
+    @cache
+    def other_cached_staticmethod(y):
+        ...
+
+    @staticmethod
+    @functools.lru_cache
+    def lru_cached_staticmethod(y):
+        ...
+
+    @staticmethod
+    @lru_cache
+    def other_lru_cached_staticmethod(y):
+        ...
+
+    @functools.cached_property
+    def some_cached_property(self):
+        ...
+
+    @cached_property
+    def some_other_cached_property(self):
+        ...
+
+    # Remaining methods should emit B019
+    @functools.cache
+    def cached_method(self, y):
+        ...
+
+    @cache
+    def another_cached_method(self, y):
+        ...
+
+    @functools.cache()
+    def called_cached_method(self, y):
+        ...
+
+    @cache()
+    def another_called_cached_method(self, y):
+        ...
+
+    @functools.lru_cache
+    def lru_cached_method(self, y):
+        ...
+
+    @lru_cache
+    def another_lru_cached_method(self, y):
+        ...
+
+    @functools.lru_cache()
+    def called_lru_cached_method(self, y):
+        ...
+
+    @lru_cache()
+    def another_called_lru_cached_method(self, y):
+        ...