From d80a9d9ce9ecb9b04427673eb21555103359a9b5 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Mon, 1 Jul 2024 02:55:49 +0100 Subject: [PATCH] [`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 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` --- .../test/fixtures/flake8_bugbear/B039.py | 36 +++++ .../fixtures/flake8_bugbear/B039_extended.py | 7 + .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_bugbear/mod.rs | 17 +++ .../src/rules/flake8_bugbear/rules/mod.rs | 2 + .../rules/mutable_contextvar_default.rs | 108 +++++++++++++++ ...__flake8_bugbear__tests__B039_B039.py.snap | 127 ++++++++++++++++++ ...ts__extend_mutable_contextvar_default.snap | 10 ++ crates/ruff_python_semantic/src/model.rs | 2 + ruff.schema.json | 1 + 11 files changed, 314 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039_extended.py create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_contextvar_default.rs create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B039_B039.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_mutable_contextvar_default.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039.py new file mode 100644 index 0000000000000..7d96075f15697 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039.py @@ -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()) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039_extended.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039_extended.py new file mode 100644 index 0000000000000..71d46cc7a5b0f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B039_extended.py @@ -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()) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index a86fdc2bb1bbe..5e362d96ae2bc 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 4ca985e2d3b77..70bbaab4ebf94 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index 2fa8d5b7a67aa..1f122f15438b3 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -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(()) + } } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs index 4f7fd0eebf42b..b4af7755dd435 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs @@ -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; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_contextvar_default.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_contextvar_default.rs new file mode 100644 index 0000000000000..c6dc1394a14f3 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_contextvar_default.rs @@ -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 { + 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 = 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())); + } +} diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B039_B039.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B039_B039.py.snap new file mode 100644 index 0000000000000..e2bbb851aa3c8 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B039_B039.py.snap @@ -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()`` diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_mutable_contextvar_default.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_mutable_contextvar_default.snap new file mode 100644 index 0000000000000..d03f11124f91f --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_mutable_contextvar_default.snap @@ -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()`` diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index e567a5e936f84..6a813c55c2ccd 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -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; } } diff --git a/ruff.schema.json b/ruff.schema.json index 4c77a7dd895ce..cfe10e3a4cbc7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2760,6 +2760,7 @@ "B033", "B034", "B035", + "B039", "B9", "B90", "B901",