Skip to content

Commit

Permalink
Implement B039
Browse files Browse the repository at this point in the history
  • Loading branch information
tjkuson committed Jun 30, 2024
1 parent f765d19 commit 8b9ac49
Show file tree
Hide file tree
Showing 11 changed files with 318 additions and 0 deletions.
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,112 @@
use ruff_diagnostics::{Diagnostic, FixAvailability, 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 of 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 {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use mutable data structures for ContextVar defaults")
}

fn fix_title(&self) -> Option<String> {
Some(format!(
"Replace with `None`; initialize with `.set()` after checking for `None`"
))
}
}

/// 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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`

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()` after checking for `None`
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()` after checking for `None`
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.

0 comments on commit 8b9ac49

Please sign in to comment.