Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Mar 20, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 46 additions & 28 deletions README.rst
Expand Up @@ -8,7 +8,7 @@ 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
A plugin for flake8 finding likely bugs and design problems in your
program. Contains warnings that don't belong in pyflakes and
pycodestyle::

Expand Down Expand Up @@ -132,8 +132,11 @@ data available in ``ex``.

**B018**: Found useless expression. Either assign it to a variable or remove it.

**B020**: Loop control variable overrides iterable it iterates
**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

Opinionated warnings
~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -189,41 +192,56 @@ 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 these checks, specify a ``--extend-select`` command-line option or
sco1 marked this conversation as resolved.
Show resolved Hide resolved
``extend-select=`` option in your config file (requires flake8 4.0+)::

[flake8]
max-line-length = 80
max-complexity = 12
...
extend-ignore = E501
extend-select = B901
sco1 marked this conversation as resolved.
Show resolved Hide resolved

Some checks might need other flake8 checks disabled - e.g. E501 must be disabled for
B950 to be hit.
sco1 marked this conversation as resolved.
Show resolved Hide resolved

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.

For flake8 versions older than 4.0, you will need to use the ``--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 implicitly disabled), so you have to explicitly specify
all checks you want enabled (e.g. ``select = C,E,F,W,B,B901``).

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
sco1 marked this conversation as resolved.
Show resolved Hide resolved
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
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
sco1 marked this conversation as resolved.
Show resolved Hide resolved

Tests / Lints
---------------
Expand Down
57 changes: 56 additions & 1 deletion bugbear.py
Expand Up @@ -127,7 +127,7 @@ def add_options(optmanager):
help="Skip B008 test for additional immutable calls.",
)

@lru_cache()
@lru_cache() # noqa: B019
sco1 marked this conversation as resolved.
Show resolved Hide resolved
def should_warn(self, code):
"""Returns `True` if Bugbear should emit a particular warning.

Expand All @@ -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
)
Expand All @@ -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.",
Expand Down Expand Up @@ -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.generic_visit(node)

def visit_ClassDef(self, node):
Expand Down Expand Up @@ -378,6 +391,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

Expand Down Expand Up @@ -507,6 +522,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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can clean this up when >=3.8 as minimum version, yes? If so maybe explicitly say that so I and other maintainers remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends where we ultimately want the emitted error to point. This isn't as slick as the set intersection but does consistently point the error at the first offending decorator rather than the function's def line, or the line where the decorators start (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)
Expand Down Expand Up @@ -886,6 +928,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 "
Expand Down
103 changes: 103 additions & 0 deletions 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):
...