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

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

merged 6 commits into from Mar 20, 2022

Conversation

sco1
Copy link
Contributor

@sco1 sco1 commented Jan 13, 2022

As described in #217, using functools.cache/functools.lru_cache on class methods can potentially cause their instances to live longer than expected.

I took a stab at adding a rule to catch this pattern but I have a few questions:

  1. flake8-bugbear's error messages generally try to propose a solution to make the linting error go away (besides ignoring the line), but I'm not sure about the verbiage to include. For reference, the starting point is the following:

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.

  1. Should this be restricted to caches that are explicitly unbounded, rather than any use of a built-in cache with a class method? i.e. this would only raise for functools.cache and functools.lru_cache(maxsize=None). This seems to more specifically address the issue though some still may not want to potentially keep these instances alive, bounded cache or not.

Related reading:

Resolves: #217
Resolves: #215
Resolves: #212
Resolves: #221

@sco1
Copy link
Contributor Author

sco1 commented Jan 13, 2022

For 2, I just discovered a related FAQ entry that was added a few months ago (via: bpo-44310) which might help craft some (brief) guidance?

@asottile
Copy link
Member

that whole should_warn should probably get removed, flake8 has had extend-ignore for a while now making ignore = mostly obsolete. there's also extend-select in the latest version which makes the select stuff not needed either

@sco1
Copy link
Contributor Author

sco1 commented Jan 14, 2022

At first glance I think dropping it is pretty easy, mostly just removing a conditional layer here:

for e in itertools.chain(visitor.errors, self.gen_line_based_checks()):
if self.should_warn(e.message[:4]):
yield self.adapt_error(e)

Which still passes the test suite.

Given that this method is an intentional design decision:

I think this behavior is surprising so Bugbear's opinionated warnings require explicit selection.

If its removal is desired, what would be the best way to update the language in the README, and what version of flake8 should it target? I believe the configuration options section was last touched before extend-ignore was introduced to flake8 (3.6?) and extend-select landed in 4.0.

Something like this?

To enable these checks, specify a ``--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 = B901

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.

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.

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. Specifying this option will override all codes that are disabled by
default, so you will need to specify these codes in your configuration to silence them.

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.
@cooperlees cooperlees marked this pull request as ready for review February 2, 2022 03:39
@cooperlees
Copy link
Collaborator

So I think we should look at moving to all the latest select hotness, my only worry is will we break existing configuration for users of flake8-bugbear - Is there anything we can do to support both for now and I can do a release advertising the deprecation of things we need to deprecate?

@sco1
Copy link
Contributor Author

sco1 commented Feb 2, 2022

The filter method can be left in & just made aware of --extend-select (if available), which should address #221. Even with it remaining for a deprecation period I think the configuration verbiage should still change to recommend --extend-select and --extend-ignore.

Depending on the answer to question 3 in the OP, the method's lru_cache can just be noqaed until the method is deprecated; I don't anticipate it being likely to cause any issues in this application.

@cooperlees
Copy link
Collaborator

The filter method can be left in & just made aware of --extend-select (if available), which should address #221. Even with it remaining for a deprecation period I think the configuration verbiage should still change to recommend --extend-select and --extend-ignore.

Depending on the answer to question 3 in the OP, the method's lru_cache can just be noqaed until the method is deprecated; I don't anticipate it being likely to cause any issues in this application.

Yeah I'm happy to start recommending the new and lets state we're deprecating the old in README + Change log (even tho its same file) as I think some tooling parse out he change log + I paste into the GitHub Release page.

* 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
@sco1
Copy link
Contributor Author

sco1 commented Feb 2, 2022

I've updated the README to (hopefully!) capture all the possible configuration caveats.

I've also included a few minor fixes for other issues:

I've edited the OP down to the remaining questions I have on this draft implementation.

* 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.
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks. Just some minor things:

MD syntax only needs and not `` - e.g.select=`

All other requests are inline.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
bugbear.py Show resolved Hide resolved
bugbear.py Outdated
Comment on lines 533 to 534
# 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).

tests/test_bugbear.py Show resolved Hide resolved
@cooperlees
Copy link
Collaborator

If 3.6 is a pain, since it's EOL'd cpython support, I'm happy to drop it here too.

@sco1
Copy link
Contributor Author

sco1 commented Feb 3, 2022

It's not a major pain, it's mostly me just doing a dumb thing and trying to use sys.version_info > (3, 6) to detect Python >= 3.7. It's fixed now locally but I can also just drop it.

Co-authored-by: Cooper Ry Lees <me@cooperlees.com>
@cooperlees
Copy link
Collaborator

If you have it fixed lets do 1 more 3.6 capable release and drop it next release :) Thanks!

@cooperlees cooperlees merged commit 4a92ce5 into PyCQA:master Mar 20, 2022
@sco1 sco1 deleted the class-lru branch March 20, 2022 21:10
toofar added a commit to qutebrowser/qutebrowser that referenced this pull request Mar 26, 2022
Flake8-bugbear correctly pointed out that TabBar instances would not be
reliably cleaned up because the `self` reference would be cached in the
lru_caches on a couple of the methods. And the caches are on the class
so they last for the whole lifetime of the process.

This commit move the caches to be created per instance, instead of on
the class.

Other options:

1. get rid of the caches

From running the benchmark tests (eg `python3 -m pytest --benchmark-columns Min,Max,Median,Rounds -k test_update_tab_titles_benchmark`)
it seems like the caches can still be helpful (even though when they
were introduced in #3122 we didn't have the config cache either)

2. use cachetools to manage our own cache instead of using lru_cache in
   a non-standard way

I don't feel like introducing a new dependency given this change didn't
end up being too offensive.

3. clear the cache whenever a window get closed

That would solve the "not getting deleted issue" but flake8 would still
complain :)

Possibly the cache size could be reduced now that there is going to be
one per window. But the aren't caching large objects anyway.

Flake8-bugbear change: PyCQA/flake8-bugbear#218
Video that pointed out this way of using lru_cache: https://youtu.be/sVjtp6tGo0g
@khrapovs
Copy link

@sco1 I am totally confused. The error message says "class methods", but the actual behavior is to throw an error when "instance methods" are cached. I looked into the tests and this is how I read them. What do I miss?

@sco1
Copy link
Contributor Author

sco1 commented Mar 30, 2022

@khrapovs I don't understand your question. It would probably be a good idea open a new issue with an example.

@jpy-git
Copy link
Contributor

jpy-git commented Mar 30, 2022

Answered in #250

@Julian
Copy link

Julian commented Apr 15, 2022

I may be missing it just scanning the above, but can someone mention the thinking on:

Should this be restricted to caches that are explicitly unbounded, rather than any use of a built-in cache with a class method? i.e. this would only raise for functools.cache and functools.lru_cache(maxsize=None).

My reading of the upstream issue also seems to agree with the above (that you're likely OK if you're not setting maxsize=None and not creating hundreds of thousands of the object the method is on). Just confirming then, was the decision to warn always (and not just for maxsize=None) explicit?

Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Apr 15, 2022
Creating lots of RefResolvers shouldn't be common (as creating lots
of Validators should be reasonably uncommon itself), and even if
one did so, we have a fixed size LRU cache, so this seems OK.

Refs: PyCQA/flake8-bugbear#218
@sco1
Copy link
Contributor Author

sco1 commented Apr 15, 2022

Just some thoughts as a non-maintainer:

that you're likely OK if you're not setting maxsize=None and not creating hundreds of thousands of the object the method is on

Maybe, but I'm not sure many are even aware of the potential pitfall. Not to get into the hypothetical weeds too much, but the point where you'd incur a non-trivial performance cost is going to vary between projects, if you even approach it at all; theoretically it could even be well below the default maxsize=128 if your objects are large enough. Rather than bugbear trying to guess at what that line is, IMO it being the "very opinionated" checker allows it to just say "hey this could be a problem" and let the project make the decision. Maybe this is to far into "what-if" land and ends up being more annoying than practical.

FWIW, there's also a similar discussion happening over at Pylint: pylint-dev/pylint#5670. It looks like they're opting to only warn for functools.cache and maxsize=None.

twigleingrid pushed a commit to twigleingrid/qutebrowser that referenced this pull request May 13, 2022
Flake8-bugbear correctly pointed out that TabBar instances would not be
reliably cleaned up because the `self` reference would be cached in the
lru_caches on a couple of the methods. And the caches are on the class
so they last for the whole lifetime of the process.

This commit move the caches to be created per instance, instead of on
the class.

Other options:

1. get rid of the caches

From running the benchmark tests (eg `python3 -m pytest --benchmark-columns Min,Max,Median,Rounds -k test_update_tab_titles_benchmark`)
it seems like the caches can still be helpful (even though when they
were introduced in qutebrowser#3122 we didn't have the config cache either)

2. use cachetools to manage our own cache instead of using lru_cache in
   a non-standard way

I don't feel like introducing a new dependency given this change didn't
end up being too offensive.

3. clear the cache whenever a window get closed

That would solve the "not getting deleted issue" but flake8 would still
complain :)

Possibly the cache size could be reduced now that there is going to be
one per window. But the aren't caching large objects anyway.

Flake8-bugbear change: PyCQA/flake8-bugbear#218
Video that pointed out this way of using lru_cache: https://youtu.be/sVjtp6tGo0g
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants