Skip to content
Draft
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
Original file line number Diff line number Diff line change
@@ -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("")
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
)
}))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(At the very least I think these should return enums rather than bools, with a single call to check immutability.)

{
let mut diagnostic = Diagnostic::new(MutableArgumentDefault, default.range());

Expand Down
Original file line number Diff line number Diff line change
@@ -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 |
66 changes: 64 additions & 2 deletions crates/ruff_python_semantic/src/analyze/typing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -236,15 +275,15 @@ 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)
})
} 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() {
Expand Down Expand Up @@ -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;
Expand Down