Contextvar mutable or call default #476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cooperlees
approved these changes
Jun 24, 2024
| @@ -9,7 +9,7 @@ jobs: | |||
| runs-on: ${{ matrix.os }} | |||
| strategy: | |||
| matrix: | |||
| python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] | |||
| python-version: ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13-dev"] | |||
There was a problem hiding this comment.
Happy to take it as we've lucked out. We should do this in dedicated PRs in future in case there is fallout to deal with.
|
P.s. Let's make isort happy |
|
Added some comments to note where b006/b008 stuff also applies to b039, and fixed isort. Ready to merge unless @Zac-HD wants to have opinions |
Zac-HD
approved these changes
Jun 25, 2024
There was a problem hiding this comment.
Looks great - thanks, @jakkdl for implementation and @cooperlees for review 🙏
charliermarsh
pushed a commit
to astral-sh/ruff
that referenced
this pull request
Jul 1, 2024
## Summary Implement mutable-contextvar-default (B039) which was added to flake8-bugbear in PyCQA/flake8-bugbear#476. This rule is similar to [mutable-argument-default (B006)](https://docs.astral.sh/ruff/rules/mutable-argument-default) and [function-call-in-default-argument (B008)](https://docs.astral.sh/ruff/rules/function-call-in-default-argument), except that it checks the `default` keyword argument to `contextvars.ContextVar`. ``` B039.py:19:26: B039 Do not use mutable data structures for ContextVar defaults | 18 | # Bad 19 | ContextVar("cv", default=[]) | ^^ B039 20 | ContextVar("cv", default={}) 21 | ContextVar("cv", default=list()) | = help: Replace with `None`; initialize with `.set()` after checking for `None` ``` In the upstream flake8-plugin, this rule is written expressly as a corollary to B008 and shares much of its logic. Likewise, this implementation reuses the logic of the Ruff implementation of B008, namely https://github.com/astral-sh/ruff/blob/f765d194028ef0ebd01ef0a21e30732d4d5ff184/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs#L104-L106 and https://github.com/astral-sh/ruff/blob/f765d194028ef0ebd01ef0a21e30732d4d5ff184/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs#L106 Thus, this rule deliberately replicates B006's and B008's heuristics. For example, this rule assumes that all functions are mutable unless otherwise qualified. If improvements are to be made to B039 heuristics, they should probably be made to B006 and B008 as well (whilst trying to match the upstream implementation). This rule does not have an autofix as it is unknown where the ContextVar next used (and it might not be within the same file). Closes #12054 ## Test Plan `cargo nextest run`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #475
Not the prettiest extension of
FunctionDefDefaultsVisitor, and left e.g.B006.mutable_literalsas bound toB006and the naming ofb008_extend_immutable_calls. I'll fix those later unless we want to reuse B006/B008 forContextVars, or maybe just a couple comments saying that B039 also uses them.Also adding 3.13 to CI to check for any problems there