From 4a92ce5ee1b2a8c2a6864c7a56476b1c4008924d Mon Sep 17 00:00:00 2001 From: "S. Co1" Date: Sun, 20 Mar 2022 15:11:55 -0400 Subject: [PATCH] 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 Co-authored-by: Cooper Ry Lees --- README.rst | 85 ++++++++++++++++++++-------------- bugbear.py | 57 ++++++++++++++++++++++- tests/b019.py | 103 ++++++++++++++++++++++++++++++++++++++++++ tests/test_bugbear.py | 63 ++++++++++++++++++++++++-- 4 files changed, 271 insertions(+), 37 deletions(-) create mode 100644 tests/b019.py diff --git a/README.rst b/README.rst index 7994a6c..5e38d85 100644 --- a/README.rst +++ b/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,7 +173,7 @@ See `the exception chaining tutorial `_ 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,33 +195,40 @@ 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 ------------- @@ -226,7 +236,16 @@ 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 --------------- diff --git a/bugbear.py b/bugbear.py index 4c0223d..031a2fa 100644 --- a/bugbear.py +++ b/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 " diff --git a/tests/b019.py b/tests/b019.py new file mode 100644 index 0000000..b074df1 --- /dev/null +++ b/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): + ... diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 05f9cbf..517c7ca 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -4,8 +4,8 @@ import subprocess import sys import unittest +from argparse import Namespace from pathlib import Path -from unittest.mock import Mock from hypothesis import HealthCheck, given, settings from hypothesmith import from_grammar @@ -29,6 +29,7 @@ B016, B017, B018, + B019, B020, B021, B901, @@ -131,8 +132,9 @@ def test_b007(self): def test_b008_extended(self): filename = Path(__file__).absolute().parent / "b008_extended.py" - mock_options = Mock() - mock_options.extend_immutable_calls = ["fastapi.Depends", "fastapi.Query"] + mock_options = Namespace( + extend_immutable_calls=["fastapi.Depends", "fastapi.Query"] + ) bbc = BugBearChecker(filename=str(filename), options=mock_options) errors = list(bbc.run()) @@ -251,6 +253,27 @@ def test_b018_classes(self): expected.append(B018(33, 4)) self.assertEqual(errors, self.errors(*expected)) + def test_b019(self): + filename = Path(__file__).absolute().parent / "b019.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + + # AST Decorator column location for callable decorators changes in 3.7 + col = 5 if sys.version_info >= (3, 7) else 4 + self.assertEqual( + errors, + self.errors( + B019(73, 5), + B019(77, 5), + B019(81, col), + B019(85, col), + B019(89, 5), + B019(93, 5), + B019(97, col), + B019(101, col), + ), + ) + def test_b020(self): filename = Path(__file__).absolute().parent / "b020.py" bbc = BugBearChecker(filename=str(filename)) @@ -359,6 +382,40 @@ def test_b950(self): ), ) + def test_b9_select(self): + filename = Path(__file__).absolute().parent / "b950.py" + + mock_options = Namespace(select=["B950"]) + bbc = BugBearChecker(filename=str(filename), options=mock_options) + errors = list(bbc.run()) + self.assertEqual( + errors, + self.errors( + B950(7, 92, vars=(92, 79)), + B950(12, 103, vars=(103, 79)), + B950(14, 103, vars=(103, 79)), + B950(21, 97, vars=(97, 79)), + ), + ) + + def test_b9_extend_select(self): + filename = Path(__file__).absolute().parent / "b950.py" + + # select is always going to have a value, usually the default codes, but can + # also be empty + mock_options = Namespace(select=[], extend_select=["B950"]) + bbc = BugBearChecker(filename=str(filename), options=mock_options) + errors = list(bbc.run()) + self.assertEqual( + errors, + self.errors( + B950(7, 92, vars=(92, 79)), + B950(12, 103, vars=(103, 79)), + B950(14, 103, vars=(103, 79)), + B950(21, 97, vars=(97, 79)), + ), + ) + def test_selfclean_bugbear(self): filename = Path(__file__).absolute().parent.parent / "bugbear.py" proc = subprocess.run(