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(