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

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

Merged
merged 2 commits into from
Jul 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039.py
Original file line number Diff line number Diff line change
@@ -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())
Original file line number Diff line number Diff line change
@@ -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())
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
17 changes: 17 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(())
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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()));
}
}
Original file line number Diff line number Diff line change
@@ -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()``
Original file line number Diff line number Diff line change
@@ -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()``
2 changes: 2 additions & 0 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -1820,6 +1821,7 @@ bitflags! {
const TYPESHED = 1 << 16;
const DATACLASSES = 1 << 17;
const BUILTINS = 1 << 18;
const CONTEXTVARS = 1 << 19;
}
}

Expand Down
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading