-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
11 changed files
with
417 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
from dataclasses import dataclass, field | ||
|
||
KNOWINGLY_MUTABLE_DEFAULT = [] | ||
|
||
|
||
@dataclass() | ||
class A: | ||
mutable_default: list[int] = [] | ||
without_annotation = [] | ||
ignored_via_comment: list[int] = [] # noqa: RUF200 | ||
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT | ||
perfectly_fine: list[int] = field(default_factory=list) | ||
|
||
|
||
@dataclass | ||
class B: | ||
mutable_default: list[int] = [] | ||
without_annotation = [] | ||
ignored_via_comment: list[int] = [] # noqa: RUF200 | ||
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT | ||
perfectly_fine: list[int] = field(default_factory=list) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
from dataclasses import dataclass | ||
from typing import NamedTuple | ||
|
||
|
||
def default_function() -> list[int]: | ||
return [] | ||
|
||
|
||
class ImmutableType(NamedTuple): | ||
something: int = 8 | ||
|
||
|
||
@dataclass() | ||
class A: | ||
hidden_mutable_default: list[int] = default_function() | ||
|
||
|
||
DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES = ImmutableType(40) | ||
DEFAULT_A_FOR_ALL_DATACLASSES = A([1, 2, 3]) | ||
|
||
|
||
@dataclass | ||
class B: | ||
hidden_mutable_default: list[int] = default_function() | ||
another_dataclass: A = A() | ||
not_optimal: ImmutableType = ImmutableType(20) | ||
good_variant: ImmutableType = DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES | ||
okay_variant: A = DEFAULT_A_FOR_ALL_DATACLASSES |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
207 changes: 207 additions & 0 deletions
207
crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,207 @@ | ||
use crate::checkers::ast::Checker; | ||
use ruff_diagnostics::{Diagnostic, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::{call_path::compose_call_path, types::Range}; | ||
use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind}; | ||
|
||
/// ## What it does | ||
/// Checks for mutable default values in dataclasses without the usage of | ||
/// `dataclasses.field`. | ||
/// | ||
/// ## Why is it bad? | ||
/// Mutable default values share state across all instances of the dataclass, | ||
/// while not being obvious. This can lead to nasty bugs when the attributes | ||
/// are changed, but expected not to in another instance. | ||
/// | ||
/// ## Examples: | ||
/// ```python | ||
/// from dataclasses import dataclass | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = [] | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// from dataclasses import dataclass, field | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = field(default_factory=list) | ||
/// ``` | ||
/// | ||
/// Alternatively, if you __want__ the shared behaviour, make it more obvious | ||
/// by assigning it to a module-level variable: | ||
/// ```python | ||
/// from dataclasses import dataclass | ||
/// | ||
/// I_KNOW_THIS_IS_SHARED_STATE = [1, 2, 3, 4] | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = I_KNOW_THIS_IS_SHARED_STATE | ||
/// ``` | ||
#[violation] | ||
pub struct MutableDataclassDefault; | ||
|
||
impl Violation for MutableDataclassDefault { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
format!("Do not use mutable default values for dataclass attributes") | ||
} | ||
} | ||
|
||
/// ## What it does | ||
/// Checks for function calls in dataclass defaults. | ||
/// | ||
/// ## Why is it bad? | ||
/// Function calls are only performed once during definition time. The result | ||
/// gets reused in all created instances of the dataclass. | ||
/// | ||
/// ## Examples: | ||
/// ```python | ||
/// from dataclasses import dataclass | ||
/// | ||
/// def creating_list() -> list[]: | ||
/// return [1, 2, 3, 4] | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = creating_list() | ||
/// | ||
/// # also: | ||
/// | ||
/// @dataclass | ||
/// class B: | ||
/// also_mutable_default_but_sneakier: A = A() | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// from dataclasses import dataclass, field | ||
/// | ||
/// def creating_list() -> list[]: | ||
/// return [1, 2, 3, 4] | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = field(default_factory=creating_list) | ||
/// | ||
/// @dataclass | ||
/// class B: | ||
/// also_mutable_default_but_sneakier: A = field(default_factory=A) | ||
/// ``` | ||
/// | ||
/// Alternatively, if you __want__ the shared behaviour, make it more obvious | ||
/// by assigning it to a module-level variable: | ||
/// ```python | ||
/// from dataclasses import dataclass | ||
/// | ||
/// def creating_list() -> list[]: | ||
/// return [1, 2, 3, 4] | ||
/// | ||
/// I_KNOW_THIS_IS_SHARED_STATE = creating_list() | ||
/// | ||
/// @dataclass | ||
/// class A: | ||
/// mutable_default: list[int] = I_KNOW_THIS_IS_SHARED_STATE | ||
/// ``` | ||
#[violation] | ||
pub struct FunctionCallInDataclassDefaultArgument { | ||
pub name: Option<String>, | ||
} | ||
|
||
impl Violation for FunctionCallInDataclassDefaultArgument { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let FunctionCallInDataclassDefaultArgument { name } = self; | ||
if let Some(name) = name { | ||
format!("Do not perform function call `{name}` in dataclass defaults") | ||
} else { | ||
format!("Do not perform function call in dataclass defaults") | ||
} | ||
} | ||
} | ||
|
||
fn is_mutable_expr(expr: &Expr) -> bool { | ||
matches!( | ||
&expr.node, | ||
ExprKind::List { .. } | ||
| ExprKind::Dict { .. } | ||
| ExprKind::Set { .. } | ||
| ExprKind::ListComp { .. } | ||
| ExprKind::DictComp { .. } | ||
| ExprKind::SetComp { .. } | ||
) | ||
} | ||
|
||
const ALLOWED_FUNCS: &[&[&str]] = &[&["dataclasses", "field"]]; | ||
|
||
fn is_allowed_func(checker: &Checker, func: &Expr) -> bool { | ||
checker | ||
.ctx | ||
.resolve_call_path(func) | ||
.map_or(false, |call_path| { | ||
ALLOWED_FUNCS | ||
.iter() | ||
.any(|target| call_path.as_slice() == *target) | ||
}) | ||
} | ||
|
||
pub fn function_call_in_dataclass_defaults(checker: &mut Checker, body: &[Stmt]) { | ||
for statement in body { | ||
if let StmtKind::AnnAssign { | ||
value: Some(expr), .. | ||
} = &statement.node | ||
{ | ||
if let ExprKind::Call { func, .. } = &expr.node { | ||
if !is_allowed_func(checker, func) { | ||
checker.diagnostics.push(Diagnostic::new( | ||
FunctionCallInDataclassDefaultArgument { | ||
name: compose_call_path(func), | ||
}, | ||
Range::from(expr), | ||
)); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
pub fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { | ||
for statement in body { | ||
match &statement.node { | ||
StmtKind::AnnAssign { | ||
value: Some(value), .. | ||
} | ||
| StmtKind::Assign { value, .. } => { | ||
if is_mutable_expr(value) { | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(MutableDataclassDefault, Range::from(value))); | ||
} | ||
} | ||
_ => (), | ||
} | ||
} | ||
} | ||
|
||
pub fn is_dataclass(checker: &Checker, decorator_list: &[Expr]) -> bool { | ||
for decorator in decorator_list.iter() { | ||
if checker | ||
.ctx | ||
.resolve_call_path(if let ExprKind::Call { func, .. } = &decorator.node { | ||
func | ||
} else { | ||
decorator | ||
}) | ||
.map_or(false, |call_path| { | ||
call_path.as_slice() == ["dataclasses", "dataclass"] | ||
}) | ||
{ | ||
return true; | ||
} | ||
} | ||
false | ||
} |
61 changes: 61 additions & 0 deletions
61
crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF200_RUF200.py.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
--- | ||
source: crates/ruff/src/rules/ruff/mod.rs | ||
expression: diagnostics | ||
--- | ||
- kind: | ||
name: MutableDataclassDefault | ||
body: Do not use mutable default values for dataclass attributes | ||
suggestion: ~ | ||
fixable: false | ||
location: | ||
row: 8 | ||
column: 33 | ||
end_location: | ||
row: 8 | ||
column: 35 | ||
fix: | ||
edits: [] | ||
parent: ~ | ||
- kind: | ||
name: MutableDataclassDefault | ||
body: Do not use mutable default values for dataclass attributes | ||
suggestion: ~ | ||
fixable: false | ||
location: | ||
row: 9 | ||
column: 25 | ||
end_location: | ||
row: 9 | ||
column: 27 | ||
fix: | ||
edits: [] | ||
parent: ~ | ||
- kind: | ||
name: MutableDataclassDefault | ||
body: Do not use mutable default values for dataclass attributes | ||
suggestion: ~ | ||
fixable: false | ||
location: | ||
row: 17 | ||
column: 33 | ||
end_location: | ||
row: 17 | ||
column: 35 | ||
fix: | ||
edits: [] | ||
parent: ~ | ||
- kind: | ||
name: MutableDataclassDefault | ||
body: Do not use mutable default values for dataclass attributes | ||
suggestion: ~ | ||
fixable: false | ||
location: | ||
row: 18 | ||
column: 25 | ||
end_location: | ||
row: 18 | ||
column: 27 | ||
fix: | ||
edits: [] | ||
parent: ~ | ||
|
Oops, something went wrong.