From 9cd74253edede2ef2d0a24975262c7204c06b974 Mon Sep 17 00:00:00 2001 From: Zanie Date: Tue, 22 Aug 2023 11:45:25 -0500 Subject: [PATCH 1/5] Update `B006` to respect `extend_immutable_calls` when determining if annotations are immutable --- crates/ruff/src/rules/flake8_bugbear/mod.rs | 23 ++++++++++++++-- .../rules/mutable_argument_default.rs | 16 +++++++++--- ...extend_immutable_calls_arg_annotation.snap | 22 ++++++++++++++++ ...__extend_immutable_calls_arg_default.snap} | 0 .../rules/ruff/rules/mutable_class_default.rs | 2 +- .../ruff/rules/mutable_dataclass_default.rs | 2 +- .../src/analyze/typing.rs | 26 ++++++++++++++----- 7 files changed, 76 insertions(+), 15 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_annotation.snap rename crates/ruff/src/rules/flake8_bugbear/snapshots/{ruff__rules__flake8_bugbear__tests__extend_immutable_calls.snap => ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap} (100%) diff --git a/crates/ruff/src/rules/flake8_bugbear/mod.rs b/crates/ruff/src/rules/flake8_bugbear/mod.rs index 1d92306633aa6..fa275427cc387 100644 --- a/crates/ruff/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff/src/rules/flake8_bugbear/mod.rs @@ -71,8 +71,27 @@ mod tests { } #[test] - fn extend_immutable_calls() -> Result<()> { - let snapshot = "extend_immutable_calls".to_string(); + fn extend_immutable_calls_arg_annotation() -> Result<()> { + let snapshot = "extend_immutable_calls_arg_annotation".to_string(); + let diagnostics = test_path( + Path::new("flake8_bugbear/B006_extended.py"), + &Settings { + flake8_bugbear: super::settings::Settings { + extend_immutable_calls: vec![ + "custom.ImmutableTypeA".to_string(), + "custom.ImmutableTypeB".to_string(), + ], + }, + ..Settings::for_rule(Rule::MutableArgumentDefault) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + #[test] + fn extend_immutable_calls_arg_default() -> Result<()> { + let snapshot = "extend_immutable_calls_arg_default".to_string(); let diagnostics = test_path( Path::new("flake8_bugbear/B008_extended.py"), &Settings { diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index bee4481a28e80..dc66765b9265b 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -1,3 +1,4 @@ +use ast::call_path::{from_qualified_name, CallPath}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::is_docstring_stmt; @@ -84,11 +85,18 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, function_def: &ast continue; }; + let extend_immutable_calls: Vec = checker + .settings + .flake8_bugbear + .extend_immutable_calls + .iter() + .map(|target| from_qualified_name(target)) + .collect(); + if is_mutable_expr(default, checker.semantic()) - && !parameter - .annotation - .as_ref() - .is_some_and(|expr| is_immutable_annotation(expr, checker.semantic())) + && !parameter.annotation.as_ref().is_some_and(|expr| { + is_immutable_annotation(expr, checker.semantic(), extend_immutable_calls.as_slice()) + }) { let mut diagnostic = Diagnostic::new(MutableArgumentDefault, default.range()); diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_annotation.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_annotation.snap new file mode 100644 index 0000000000000..7bc5a28612e31 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_annotation.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff/src/rules/flake8_bugbear/mod.rs +--- +B006_extended.py:17:55: B006 [*] Do not use mutable data structures for argument defaults + | +17 | def error_due_to_missing_import(foo: ImmutableTypeA = []): + | ^^ B006 +18 | ... + | + = help: Replace with `None`; initialize within function + +ℹ Possible fix +14 14 | ... +15 15 | +16 16 | +17 |-def error_due_to_missing_import(foo: ImmutableTypeA = []): + 17 |+def error_due_to_missing_import(foo: ImmutableTypeA = None): + 18 |+ if foo is None: + 19 |+ foo = [] +18 20 | ... + + diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap similarity index 100% rename from crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls.snap rename to crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap diff --git a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs index 11fbcd0374c17..998c6cf368719 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs @@ -60,7 +60,7 @@ pub(crate) fn mutable_class_default(checker: &mut Checker, class_def: &ast::Stmt && is_mutable_expr(value, checker.semantic()) && !is_class_var_annotation(annotation, checker.semantic()) && !is_final_annotation(annotation, checker.semantic()) - && !is_immutable_annotation(annotation, checker.semantic()) + && !is_immutable_annotation(annotation, checker.semantic(), vec![].as_slice()) && !is_dataclass(class_def, checker.semantic()) { // Avoid Pydantic models, which end up copying defaults on instance creation. diff --git a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs index 24f2ff9f1e351..564be3e921ffe 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs @@ -76,7 +76,7 @@ pub(crate) fn mutable_dataclass_default(checker: &mut Checker, class_def: &ast:: { if is_mutable_expr(value, checker.semantic()) && !is_class_var_annotation(annotation, checker.semantic()) - && !is_immutable_annotation(annotation, checker.semantic()) + && !is_immutable_annotation(annotation, checker.semantic(), vec![].as_slice()) { checker .diagnostics diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index fbc481b779632..b2c4b52f3523a 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -186,12 +186,19 @@ pub fn to_pep604_operator( /// Return `true` if `Expr` represents a reference to a type annotation that resolves to an /// immutable type. -pub fn is_immutable_annotation(expr: &Expr, semantic: &SemanticModel) -> bool { +pub fn is_immutable_annotation( + expr: &Expr, + semantic: &SemanticModel, + extend_immutable_calls: &[CallPath], +) -> bool { match expr { Expr::Name(_) | Expr::Attribute(_) => { semantic.resolve_call_path(expr).is_some_and(|call_path| { is_immutable_non_generic_type(call_path.as_slice()) || is_immutable_generic_type(call_path.as_slice()) + || extend_immutable_calls + .iter() + .any(|target| call_path == *target) }) } Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { @@ -200,17 +207,19 @@ pub fn is_immutable_annotation(expr: &Expr, semantic: &SemanticModel) -> bool { true } else if matches!(call_path.as_slice(), ["typing", "Union"]) { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { - elts.iter() - .all(|elt| is_immutable_annotation(elt, semantic)) + elts.iter().all(|elt| { + is_immutable_annotation(elt, semantic, extend_immutable_calls) + }) } else { false } } else if matches!(call_path.as_slice(), ["typing", "Optional"]) { - is_immutable_annotation(slice, semantic) + is_immutable_annotation(slice, semantic, extend_immutable_calls) } else if is_pep_593_generic_type(call_path.as_slice()) { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { - elts.first() - .is_some_and(|elt| is_immutable_annotation(elt, semantic)) + elts.first().is_some_and(|elt| { + is_immutable_annotation(elt, semantic, extend_immutable_calls) + }) } else { false } @@ -224,7 +233,10 @@ pub fn is_immutable_annotation(expr: &Expr, semantic: &SemanticModel) -> bool { op: Operator::BitOr, right, range: _, - }) => is_immutable_annotation(left, semantic) && is_immutable_annotation(right, semantic), + }) => { + is_immutable_annotation(left, semantic, extend_immutable_calls) + && is_immutable_annotation(right, semantic, extend_immutable_calls) + } Expr::Constant(ast::ExprConstant { value: Constant::None, .. From da38b71a816c214fefe1c60dae53b58bef139d1a Mon Sep 17 00:00:00 2001 From: Zanie Date: Tue, 22 Aug 2023 11:48:16 -0500 Subject: [PATCH 2/5] Add docs reference to option --- .../src/rules/flake8_bugbear/rules/mutable_argument_default.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index dc66765b9265b..bc42f8de7f26e 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -50,6 +50,9 @@ use crate::registry::AsRule; /// l2 = add_to_list(1) # [1] /// ``` /// +/// ## Options +/// - `flake8-bugbear.extend-immutable-calls` +/// /// ## References /// - [Python documentation: Default Argument Values](https://docs.python.org/3/tutorial/controlflow.html#default-argument-values) #[violation] From 548af45642a91060f6b7cb527249d9b817a7b570 Mon Sep 17 00:00:00 2001 From: Zanie Date: Tue, 22 Aug 2023 12:04:34 -0500 Subject: [PATCH 3/5] Commit test fixture file --- .../fixtures/flake8_bugbear/B006_extended.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bugbear/B006_extended.py diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B006_extended.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B006_extended.py new file mode 100644 index 0000000000000..15da0cc3fdf00 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bugbear/B006_extended.py @@ -0,0 +1,18 @@ +import custom +from custom import ImmutableTypeB + + +def okay(foo: ImmutableTypeB = []): + ... + + +def okay(foo: custom.ImmutableTypeA = []): + ... + + +def okay(foo: custom.ImmutableTypeB = []): + ... + + +def error_due_to_missing_import(foo: ImmutableTypeA = []): + ... From 42cfcd2e2ec223588c1237db310c9b411d81963c Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 23 Aug 2023 10:15:25 -0500 Subject: [PATCH 4/5] Document option behavior --- .../rules/flake8_bugbear/rules/mutable_argument_default.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index bc42f8de7f26e..0ea4f69059c73 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -26,6 +26,10 @@ use crate::registry::AsRule; /// default, and initialize a new mutable object inside the function body /// for each call. /// +/// Arguments with immutable type annotations will be ignored by this rule. +/// Types outside of the standard library can be marked as immutable with the +/// [`flake8-bugbear.extend-immutable-calls`] configuration option. +/// /// ## Example /// ```python /// def add_to_list(item, some_list=[]): From 9eba982f80914c73dd0a016ec0e4c07215a119ae Mon Sep 17 00:00:00 2001 From: Zanie Date: Wed, 23 Aug 2023 10:16:25 -0500 Subject: [PATCH 5/5] Simplify empty slices --- crates/ruff/src/rules/ruff/rules/mutable_class_default.rs | 2 +- crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs index 998c6cf368719..d411a6fadb49b 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs @@ -60,7 +60,7 @@ pub(crate) fn mutable_class_default(checker: &mut Checker, class_def: &ast::Stmt && is_mutable_expr(value, checker.semantic()) && !is_class_var_annotation(annotation, checker.semantic()) && !is_final_annotation(annotation, checker.semantic()) - && !is_immutable_annotation(annotation, checker.semantic(), vec![].as_slice()) + && !is_immutable_annotation(annotation, checker.semantic(), &[]) && !is_dataclass(class_def, checker.semantic()) { // Avoid Pydantic models, which end up copying defaults on instance creation. diff --git a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs index 564be3e921ffe..e916d6de18fde 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs @@ -76,7 +76,7 @@ pub(crate) fn mutable_dataclass_default(checker: &mut Checker, class_def: &ast:: { if is_mutable_expr(value, checker.semantic()) && !is_class_var_annotation(annotation, checker.semantic()) - && !is_immutable_annotation(annotation, checker.semantic(), vec![].as_slice()) + && !is_immutable_annotation(annotation, checker.semantic(), &[]) { checker .diagnostics