Skip to content

Commit

Permalink
Flag mutable defaults based on mutable annotations
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Apr 24, 2024
1 parent 7c8c1c7 commit ed030d0
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 5 deletions.
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(),
)
}))
{
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

0 comments on commit ed030d0

Please sign in to comment.