[flake8-bugbear] Implement mutable-contextvar-default (B039) (#12113)

## Summary

Implement mutable-contextvar-default (B039) which was added to
flake8-bugbear in https://github.com/PyCQA/flake8-bugbear/pull/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


f765d19402/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_call_in_argument_default.rs (L104-L106)

and 


f765d19402/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`
This commit is contained in:
Tom Kuson 2024-07-01 02:55:49 +01:00 committed by GitHub
parent 85ede4a88c
commit d80a9d9ce9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 314 additions and 0 deletions

View file

@ -0,0 +1,36 @@
from contextvars import ContextVar
from types import MappingProxyType
import re
import collections
import time
# Okay
ContextVar("cv")
ContextVar("cv", default=())
ContextVar("cv", default=(1, 2, 3))
ContextVar("cv", default="foo")
ContextVar("cv", default=tuple())
ContextVar("cv", default=frozenset())
ContextVar("cv", default=MappingProxyType({}))
ContextVar("cv", default=re.compile("foo"))
ContextVar("cv", default=float(1))
# Bad
ContextVar("cv", default=[])
ContextVar("cv", default={})
ContextVar("cv", default=list())
ContextVar("cv", default=set())
ContextVar("cv", default=dict())
ContextVar("cv", default=[char for char in "foo"])
ContextVar("cv", default={char for char in "foo"})
ContextVar("cv", default={char: idx for idx, char in enumerate("foo")})
ContextVar("cv", default=collections.deque())
def bar() -> list[int]:
return [1, 2, 3]
ContextVar("cv", default=bar())
ContextVar("cv", default=time.time())
def baz(): ...
ContextVar("cv", default=baz())

View file

@ -0,0 +1,7 @@
from contextvars import ContextVar
from fastapi import Query
ContextVar("cv", default=Query(None))
from something_else import Depends
ContextVar("cv", default=Depends())

View file

@ -580,6 +580,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::NoExplicitStacklevel) {
flake8_bugbear::rules::no_explicit_stacklevel(checker, call);
}
if checker.enabled(Rule::MutableContextvarDefault) {
flake8_bugbear::rules::mutable_contextvar_default(checker, call);
}
if checker.enabled(Rule::UnnecessaryDictKwargs) {
flake8_pie::rules::unnecessary_dict_kwargs(checker, call);
}

View file

@ -345,6 +345,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Bugbear, "033") => (RuleGroup::Stable, rules::flake8_bugbear::rules::DuplicateValue),
(Flake8Bugbear, "034") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ReSubPositionalArgs),
(Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension),
(Flake8Bugbear, "039") => (RuleGroup::Preview, rules::flake8_bugbear::rules::MutableContextvarDefault),
(Flake8Bugbear, "901") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ReturnInGenerator),
(Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept),
(Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict),

View file

@ -64,6 +64,7 @@ mod tests {
#[test_case(Rule::UselessExpression, Path::new("B018.py"))]
#[test_case(Rule::ReturnInGenerator, Path::new("B901.py"))]
#[test_case(Rule::LoopIteratorMutation, Path::new("B909.py"))]
#[test_case(Rule::MutableContextvarDefault, Path::new("B039.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
@ -124,4 +125,20 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}
#[test]
fn extend_mutable_contextvar_default() -> Result<()> {
let snapshot = "extend_mutable_contextvar_default".to_string();
let diagnostics = test_path(
Path::new("flake8_bugbear/B039_extended.py"),
&LinterSettings {
flake8_bugbear: super::settings::Settings {
extend_immutable_calls: vec!["fastapi.Query".to_string()],
},
..LinterSettings::for_rule(Rule::MutableContextvarDefault)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}

View file

@ -15,6 +15,7 @@ pub(crate) use jump_statement_in_finally::*;
pub(crate) use loop_iterator_mutation::*;
pub(crate) use loop_variable_overrides_iterator::*;
pub(crate) use mutable_argument_default::*;
pub(crate) use mutable_contextvar_default::*;
pub(crate) use no_explicit_stacklevel::*;
pub(crate) use raise_literal::*;
pub(crate) use raise_without_from_inside_except::*;
@ -52,6 +53,7 @@ mod jump_statement_in_finally;
mod loop_iterator_mutation;
mod loop_variable_overrides_iterator;
mod mutable_argument_default;
mod mutable_contextvar_default;
mod no_explicit_stacklevel;
mod raise_literal;
mod raise_without_from_inside_except;

View file

@ -0,0 +1,108 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::typing::{is_immutable_func, is_mutable_expr, is_mutable_func};
use ruff_python_semantic::Modules;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for uses of mutable objects as `ContextVar` defaults.
///
/// ## Why is this bad?
///
/// The `ContextVar` default is evaluated once, when the `ContextVar` is defined.
///
/// The same mutable object is then shared across all `.get()` method calls to
/// the `ContextVar`. If the object is modified, those modifications will persist
/// across calls, which can lead to unexpected behavior.
///
/// Instead, prefer to use immutable data structures; or, take `None` as a
/// default, and initialize a new mutable object inside for each call using the
/// `.set()` method.
///
/// Types outside the standard library can be marked as immutable with the
/// [`lint.flake8-bugbear.extend-immutable-calls`] configuration option.
///
/// ## Example
/// ```python
/// from contextvars import ContextVar
///
///
/// cv: ContextVar[list] = ContextVar("cv", default=[])
/// ```
///
/// Use instead:
/// ```python
/// from contextvars import ContextVar
///
///
/// cv: ContextVar[list | None] = ContextVar("cv", default=None)
///
/// ...
///
/// if cv.get() is None:
/// cv.set([])
/// ```
///
/// ## Options
/// - `lint.flake8-bugbear.extend-immutable-calls`
///
/// ## References
/// - [Python documentation: [`contextvars` — Context Variables](https://docs.python.org/3/library/contextvars.html)
#[violation]
pub struct MutableContextvarDefault;
impl Violation for MutableContextvarDefault {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use mutable data structures for `ContextVar` defaults")
}
fn fix_title(&self) -> Option<String> {
Some("Replace with `None`; initialize with `.set()``".to_string())
}
}
/// B039
pub(crate) fn mutable_contextvar_default(checker: &mut Checker, call: &ast::ExprCall) {
if !checker.semantic().seen_module(Modules::CONTEXTVARS) {
return;
}
let Some(default) = call
.arguments
.find_keyword("default")
.map(|keyword| &keyword.value)
else {
return;
};
let extend_immutable_calls: Vec<QualifiedName> = checker
.settings
.flake8_bugbear
.extend_immutable_calls
.iter()
.map(|target| QualifiedName::from_dotted_name(target))
.collect();
if (is_mutable_expr(default, checker.semantic())
|| matches!(
default,
Expr::Call(ast::ExprCall { func, .. })
if !is_mutable_func(func, checker.semantic())
&& !is_immutable_func(func, checker.semantic(), &extend_immutable_calls)))
&& checker
.semantic()
.resolve_qualified_name(&call.func)
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["contextvars", "ContextVar"])
})
{
checker
.diagnostics
.push(Diagnostic::new(MutableContextvarDefault, default.range()));
}
}

View file

@ -0,0 +1,127 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
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()``
B039.py:20:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
18 | # Bad
19 | ContextVar("cv", default=[])
20 | ContextVar("cv", default={})
| ^^ B039
21 | ContextVar("cv", default=list())
22 | ContextVar("cv", default=set())
|
= help: Replace with `None`; initialize with `.set()``
B039.py:21:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
19 | ContextVar("cv", default=[])
20 | ContextVar("cv", default={})
21 | ContextVar("cv", default=list())
| ^^^^^^ B039
22 | ContextVar("cv", default=set())
23 | ContextVar("cv", default=dict())
|
= help: Replace with `None`; initialize with `.set()``
B039.py:22:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
20 | ContextVar("cv", default={})
21 | ContextVar("cv", default=list())
22 | ContextVar("cv", default=set())
| ^^^^^ B039
23 | ContextVar("cv", default=dict())
24 | ContextVar("cv", default=[char for char in "foo"])
|
= help: Replace with `None`; initialize with `.set()``
B039.py:23:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
21 | ContextVar("cv", default=list())
22 | ContextVar("cv", default=set())
23 | ContextVar("cv", default=dict())
| ^^^^^^ B039
24 | ContextVar("cv", default=[char for char in "foo"])
25 | ContextVar("cv", default={char for char in "foo"})
|
= help: Replace with `None`; initialize with `.set()``
B039.py:24:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
22 | ContextVar("cv", default=set())
23 | ContextVar("cv", default=dict())
24 | ContextVar("cv", default=[char for char in "foo"])
| ^^^^^^^^^^^^^^^^^^^^^^^^ B039
25 | ContextVar("cv", default={char for char in "foo"})
26 | ContextVar("cv", default={char: idx for idx, char in enumerate("foo")})
|
= help: Replace with `None`; initialize with `.set()``
B039.py:25:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
23 | ContextVar("cv", default=dict())
24 | ContextVar("cv", default=[char for char in "foo"])
25 | ContextVar("cv", default={char for char in "foo"})
| ^^^^^^^^^^^^^^^^^^^^^^^^ B039
26 | ContextVar("cv", default={char: idx for idx, char in enumerate("foo")})
27 | ContextVar("cv", default=collections.deque())
|
= help: Replace with `None`; initialize with `.set()``
B039.py:26:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
24 | ContextVar("cv", default=[char for char in "foo"])
25 | ContextVar("cv", default={char for char in "foo"})
26 | ContextVar("cv", default={char: idx for idx, char in enumerate("foo")})
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B039
27 | ContextVar("cv", default=collections.deque())
|
= help: Replace with `None`; initialize with `.set()``
B039.py:27:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
25 | ContextVar("cv", default={char for char in "foo"})
26 | ContextVar("cv", default={char: idx for idx, char in enumerate("foo")})
27 | ContextVar("cv", default=collections.deque())
| ^^^^^^^^^^^^^^^^^^^ B039
28 |
29 | def bar() -> list[int]:
|
= help: Replace with `None`; initialize with `.set()``
B039.py:32:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
30 | return [1, 2, 3]
31 |
32 | ContextVar("cv", default=bar())
| ^^^^^ B039
33 | ContextVar("cv", default=time.time())
|
= help: Replace with `None`; initialize with `.set()``
B039.py:33:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
32 | ContextVar("cv", default=bar())
33 | ContextVar("cv", default=time.time())
| ^^^^^^^^^^^ B039
34 |
35 | def baz(): ...
|
= help: Replace with `None`; initialize with `.set()``
B039.py:36:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
35 | def baz(): ...
36 | ContextVar("cv", default=baz())
| ^^^^^ B039
|
= help: Replace with `None`; initialize with `.set()``

View file

@ -0,0 +1,10 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B039_extended.py:7:26: B039 Do not use mutable data structures for `ContextVar` defaults
|
6 | from something_else import Depends
7 | ContextVar("cv", default=Depends())
| ^^^^^^^^^ B039
|
= help: Replace with `None`; initialize with `.set()``

View file

@ -1233,6 +1233,7 @@ impl<'a> SemanticModel<'a> {
"_typeshed" => self.seen.insert(Modules::TYPESHED),
"builtins" => self.seen.insert(Modules::BUILTINS),
"collections" => self.seen.insert(Modules::COLLECTIONS),
"contextvars" => self.seen.insert(Modules::CONTEXTVARS),
"dataclasses" => self.seen.insert(Modules::DATACLASSES),
"datetime" => self.seen.insert(Modules::DATETIME),
"django" => self.seen.insert(Modules::DJANGO),
@ -1820,6 +1821,7 @@ bitflags! {
const TYPESHED = 1 << 16;
const DATACLASSES = 1 << 17;
const BUILTINS = 1 << 18;
const CONTEXTVARS = 1 << 19;
}
}

1
ruff.schema.json generated
View file

@ -2760,6 +2760,7 @@
"B033",
"B034",
"B035",
"B039",
"B9",
"B90",
"B901",