diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF008.py b/crates/ruff/resources/test/fixtures/ruff/RUF008.py new file mode 100644 index 0000000000000..d19a9bdc647e5 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF008.py @@ -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: RUF008 + 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: RUF008 + correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + perfectly_fine: list[int] = field(default_factory=list) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF009.py b/crates/ruff/resources/test/fixtures/ruff/RUF009.py new file mode 100644 index 0000000000000..cfe4e6b871b02 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF009.py @@ -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 diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index e6815c8974e4f..c14710fb76605 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -819,6 +819,24 @@ where flake8_pie::rules::non_unique_enums(self, stmt, body); } + if self.settings.rules.any_enabled(&[ + Rule::MutableDataclassDefault, + Rule::FunctionCallInDataclassDefaultArgument, + ]) && ruff::rules::is_dataclass(self, decorator_list) + { + if self.settings.rules.enabled(Rule::MutableDataclassDefault) { + ruff::rules::mutable_dataclass_default(self, body); + } + + if self + .settings + .rules + .enabled(Rule::FunctionCallInDataclassDefaultArgument) + { + ruff::rules::function_call_in_dataclass_defaults(self, body); + } + } + self.check_builtin_shadowing(name, stmt, false); for expr in bases { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 6de83693cec5a..5c50683568569 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -700,6 +700,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Ruff, "005") => Rule::CollectionLiteralConcatenation, (Ruff, "006") => Rule::AsyncioDanglingTask, (Ruff, "007") => Rule::PairwiseOverZipped, + (Ruff, "008") => Rule::MutableDataclassDefault, + (Ruff, "009") => Rule::FunctionCallInDataclassDefaultArgument, (Ruff, "100") => Rule::UnusedNOQA, // flake8-django diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 268ff5d87300f..b585c733782c4 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -644,6 +644,8 @@ ruff_macros::register_rules!( rules::ruff::rules::AsyncioDanglingTask, rules::ruff::rules::UnusedNOQA, rules::ruff::rules::PairwiseOverZipped, + rules::ruff::rules::MutableDataclassDefault, + rules::ruff::rules::FunctionCallInDataclassDefaultArgument, // flake8-django rules::flake8_django::rules::DjangoNullableModelStringField, rules::flake8_django::rules::DjangoLocalsInRenderFunction, diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index 63884d5cf4034..c558ab213b541 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -152,4 +152,16 @@ mod tests { assert_yaml_snapshot!(diagnostics); Ok(()) } + + #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"); "RUF008")] + #[test_case(Rule::FunctionCallInDataclassDefaultArgument, Path::new("RUF009.py"); "RUF009")] + fn mutable_defaults(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("ruff").join(path).as_path(), + &settings::Settings::for_rule(rule_code), + )?; + assert_yaml_snapshot!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index 04841e63dd07e..6a063c18dc1f8 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -1,6 +1,7 @@ mod ambiguous_unicode_character; mod asyncio_dangling_task; mod collection_literal_concatenation; +mod mutable_defaults_in_dataclass_fields; mod pairwise_over_zipped; mod unused_noqa; @@ -12,6 +13,10 @@ pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask}; pub use collection_literal_concatenation::{ collection_literal_concatenation, CollectionLiteralConcatenation, }; +pub use mutable_defaults_in_dataclass_fields::{ + function_call_in_dataclass_defaults, is_dataclass, mutable_dataclass_default, + FunctionCallInDataclassDefaultArgument, MutableDataclassDefault, +}; pub use pairwise_over_zipped::{pairwise_over_zipped, PairwiseOverZipped}; pub use unused_noqa::{UnusedCodes, UnusedNOQA}; diff --git a/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs b/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs new file mode 100644 index 0000000000000..69068eba956c1 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs @@ -0,0 +1,200 @@ +use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{call_path::compose_call_path, helpers::map_callable, types::Range}; +use ruff_python_semantic::context::Context; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for mutable default values in dataclasses without the use 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 bugs when the attributes are +/// changed in one instance, as those changes will unexpectedly affect all +/// other instances. +/// +/// ## 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_ shared behaviour, make it more obvious +/// by assigning 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, at definition time. The returned +/// value is then reused by all 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, +} + +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(context: &Context, func: &Expr) -> bool { + context.resolve_call_path(func).map_or(false, |call_path| { + ALLOWED_FUNCS + .iter() + .any(|target| call_path.as_slice() == *target) + }) +} + +/// RUF009 +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.ctx, func) { + checker.diagnostics.push(Diagnostic::new( + FunctionCallInDataclassDefaultArgument { + name: compose_call_path(func), + }, + Range::from(expr), + )); + } + } + } + } +} + +/// RUF008 +pub fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { + for statement in body { + if let StmtKind::AnnAssign { + value: Some(value), .. + } + | StmtKind::Assign { value, .. } = &statement.node + { + if is_mutable_expr(value) { + checker + .diagnostics + .push(Diagnostic::new(MutableDataclassDefault, Range::from(value))); + } + } + } +} + +pub fn is_dataclass(checker: &Checker, decorator_list: &[Expr]) -> bool { + decorator_list.iter().any(|decorator| { + checker + .ctx + .resolve_call_path(map_callable(decorator)) + .map_or(false, |call_path| { + call_path.as_slice() == ["dataclasses", "dataclass"] + }) + }) +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF008_RUF008.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF008_RUF008.py.snap new file mode 100644 index 0000000000000..29d318d8777fd --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF008_RUF008.py.snap @@ -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: ~ + diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF009_RUF009.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF009_RUF009.py.snap new file mode 100644 index 0000000000000..db545f0e138f5 --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF009_RUF009.py.snap @@ -0,0 +1,61 @@ +--- +source: crates/ruff/src/rules/ruff/mod.rs +expression: diagnostics +--- +- kind: + name: FunctionCallInDataclassDefaultArgument + body: "Do not perform function call `default_function` in dataclass defaults" + suggestion: ~ + fixable: false + location: + row: 15 + column: 40 + end_location: + row: 15 + column: 58 + fix: + edits: [] + parent: ~ +- kind: + name: FunctionCallInDataclassDefaultArgument + body: "Do not perform function call `default_function` in dataclass defaults" + suggestion: ~ + fixable: false + location: + row: 24 + column: 40 + end_location: + row: 24 + column: 58 + fix: + edits: [] + parent: ~ +- kind: + name: FunctionCallInDataclassDefaultArgument + body: "Do not perform function call `A` in dataclass defaults" + suggestion: ~ + fixable: false + location: + row: 25 + column: 27 + end_location: + row: 25 + column: 30 + fix: + edits: [] + parent: ~ +- kind: + name: FunctionCallInDataclassDefaultArgument + body: "Do not perform function call `ImmutableType` in dataclass defaults" + suggestion: ~ + fixable: false + location: + row: 26 + column: 33 + end_location: + row: 26 + column: 50 + fix: + edits: [] + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index aaa04c3f6c008..dd424e6bad665 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2074,6 +2074,8 @@ "RUF005", "RUF006", "RUF007", + "RUF008", + "RUF009", "RUF1", "RUF10", "RUF100",