From ed030d04ae7591d136f8bacde21a9792395ca0c1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 23 Apr 2024 23:08:57 -0400 Subject: [PATCH] Flag mutable defaults based on mutable annotations --- .../test/fixtures/flake8_bugbear/B006_9.py | 13 ++++ .../src/rules/flake8_bugbear/mod.rs | 1 + .../rules/mutable_argument_default.rs | 17 ++++- ...flake8_bugbear__tests__B006_B006_9.py.snap | 20 ++++++ .../src/analyze/typing.rs | 66 ++++++++++++++++++- 5 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_9.py create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_9.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_9.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_9.py new file mode 100644 index 0000000000..ef15b78aa4 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_9.py @@ -0,0 +1,13 @@ +LIST = [] + + +def foo(a: list = LIST): # B006 + raise NotImplementedError("") + + +def foo(a: list = None): # OK + raise NotImplementedError("") + + +def foo(a: list | None = LIST): # OK + raise NotImplementedError("") diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index 640007b307..826ff6a032 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -42,6 +42,7 @@ mod tests { #[test_case(Rule::MutableArgumentDefault, Path::new("B006_6.py"))] #[test_case(Rule::MutableArgumentDefault, Path::new("B006_7.py"))] #[test_case(Rule::MutableArgumentDefault, Path::new("B006_8.py"))] + #[test_case(Rule::MutableArgumentDefault, Path::new("B006_9.py"))] #[test_case(Rule::MutableArgumentDefault, Path::new("B006_B008.py"))] #[test_case(Rule::NoExplicitStacklevel, Path::new("B028.py"))] #[test_case(Rule::RaiseLiteral, Path::new("B016.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index e028eb02c2..ebfc63c9a3 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -6,7 +6,9 @@ use ruff_python_ast::{self as ast, Expr, Parameter, ParameterWithDefault}; use ruff_python_codegen::{Generator, Stylist}; use ruff_python_index::Indexer; use ruff_python_semantic::analyze::function_type::is_stub; -use ruff_python_semantic::analyze::typing::{is_immutable_annotation, is_mutable_expr}; +use ruff_python_semantic::analyze::typing::{ + is_immutable_annotation, is_immutable_expr, is_mutable_annotation, is_mutable_expr, +}; use ruff_python_semantic::SemanticModel; use ruff_python_trivia::{indentation_at_offset, textwrap}; use ruff_source_file::Locator; @@ -108,10 +110,19 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, function_def: &ast .map(|target| QualifiedName::from_dotted_name(target)) .collect(); - if is_mutable_expr(default, checker.semantic()) + // Either the expression _or_ the annotation must be mutable. + if (is_mutable_expr(default, checker.semantic()) && !parameter.annotation.as_ref().is_some_and(|expr| { is_immutable_annotation(expr, checker.semantic(), extend_immutable_calls.as_slice()) - }) + })) + || (parameter.annotation.as_ref().is_some_and(|annotation| { + is_mutable_annotation(annotation, checker.semantic()) + && !is_immutable_expr( + default, + checker.semantic(), + extend_immutable_calls.as_slice(), + ) + })) { let mut diagnostic = Diagnostic::new(MutableArgumentDefault, default.range()); diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_9.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_9.py.snap new file mode 100644 index 0000000000..5386a2a7ad --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_9.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +B006_9.py:4:19: B006 [*] Do not use mutable data structures for argument defaults + | +4 | def foo(a: list = LIST): # B006 + | ^^^^ B006 +5 | raise NotImplementedError("") + | + = help: Replace with `None`; initialize within function + +ℹ Unsafe fix +1 1 | LIST = [] +2 2 | +3 3 | +4 |-def foo(a: list = LIST): # B006 + 4 |+def foo(a: list = None): # B006 +5 5 | raise NotImplementedError("") +6 6 | +7 7 | diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 4d239017dd..fb61180d15 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -212,6 +212,45 @@ pub fn to_pep604_operator( }) } +/// Return `true` if `Expr` represents a reference to a type annotation that resolves to a mutable +/// type. +pub fn is_mutable_annotation(expr: &Expr, semantic: &SemanticModel) -> bool { + match expr { + Expr::Name(_) | Expr::Attribute(_) => semantic + .resolve_qualified_name(expr) + .is_some_and(|qualified_name| is_mutable_return_type(qualified_name.segments())), + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => semantic + .resolve_qualified_name(value) + .is_some_and(|qualified_name| { + if is_mutable_return_type(qualified_name.segments()) { + true + } else if semantic.match_typing_qualified_name(&qualified_name, "Union") { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { + elts.iter().all(|elt| is_mutable_annotation(elt, semantic)) + } else { + false + } + } else if is_pep_593_generic_type(qualified_name.segments()) { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { + elts.first() + .is_some_and(|elt| is_mutable_annotation(elt, semantic)) + } else { + false + } + } else { + false + } + }), + Expr::BinOp(ast::ExprBinOp { + left, + op: Operator::BitOr, + right, + range: _, + }) => is_mutable_annotation(left, semantic) && is_mutable_annotation(right, semantic), + _ => false, + } +} + /// Return `true` if `Expr` represents a reference to a type annotation that resolves to an /// immutable type. pub fn is_immutable_annotation( @@ -236,7 +275,7 @@ pub fn is_immutable_annotation( .is_some_and(|qualified_name| { if is_immutable_generic_type(qualified_name.segments()) { true - } else if matches!(qualified_name.segments(), ["typing", "Union"]) { + } else if semantic.match_typing_qualified_name(&qualified_name, "Union") { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { elts.iter().all(|elt| { is_immutable_annotation(elt, semantic, extend_immutable_calls) @@ -244,7 +283,7 @@ pub fn is_immutable_annotation( } else { false } - } else if matches!(qualified_name.segments(), ["typing", "Optional"]) { + } else if semantic.match_typing_qualified_name(&qualified_name, "Optional") { is_immutable_annotation(slice, semantic, extend_immutable_calls) } else if is_pep_593_generic_type(qualified_name.segments()) { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { @@ -311,6 +350,29 @@ pub fn is_mutable_expr(expr: &Expr, semantic: &SemanticModel) -> bool { } } +/// Return `true` if `expr` is an expression that resolves to an immutable value. +pub fn is_immutable_expr( + expr: &Expr, + semantic: &SemanticModel, + extend_immutable_calls: &[QualifiedName], +) -> bool { + match expr { + Expr::NoneLiteral(_) => true, + Expr::BooleanLiteral(_) => true, + Expr::NumberLiteral(_) => true, + Expr::StringLiteral(_) => true, + Expr::BytesLiteral(_) => true, + Expr::EllipsisLiteral(_) => true, + Expr::Tuple(ast::ExprTuple { elts, .. }) => elts + .iter() + .all(|elt| is_immutable_expr(elt, semantic, extend_immutable_calls)), + Expr::Call(ast::ExprCall { func, .. }) => { + is_immutable_func(func, semantic, extend_immutable_calls) + } + _ => false, + } +} + /// Return `true` if [`ast::StmtIf`] is a guard for a type-checking block. pub fn is_type_checking_block(stmt: &ast::StmtIf, semantic: &SemanticModel) -> bool { let ast::StmtIf { test, .. } = stmt;