diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF012.py b/crates/ruff/resources/test/fixtures/ruff/RUF012.py new file mode 100644 index 0000000000000..4b4c6df0bf0ac --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF012.py @@ -0,0 +1,22 @@ +import typing +from typing import ClassVar, Sequence + +KNOWINGLY_MUTABLE_DEFAULT = [] + + +class A: + mutable_default: list[int] = [] + immutable_annotation: typing.Sequence[int] = [] + without_annotation = [] + ignored_via_comment: list[int] = [] # noqa: RUF012 + correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + class_variable: typing.ClassVar[list[int]] = [] + + +class B: + mutable_default: list[int] = [] + immutable_annotation: Sequence[int] = [] + without_annotation = [] + ignored_via_comment: list[int] = [] # noqa: RUF012 + correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + class_variable: ClassVar[list[int]] = [] diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 7381a5f7b7a84..40263c7f08e6f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -794,13 +794,16 @@ where if self.any_enabled(&[ Rule::MutableDataclassDefault, Rule::FunctionCallInDataclassDefaultArgument, - ]) && ruff::rules::is_dataclass(&self.semantic_model, decorator_list) - { - if self.enabled(Rule::MutableDataclassDefault) { - ruff::rules::mutable_dataclass_default(self, body); + Rule::MutableClassDefault, + ]) { + let is_dataclass = + ruff::rules::is_dataclass(&self.semantic_model, decorator_list); + if self.any_enabled(&[Rule::MutableDataclassDefault, Rule::MutableClassDefault]) + { + ruff::rules::mutable_class_default(self, body, is_dataclass); } - if self.enabled(Rule::FunctionCallInDataclassDefaultArgument) { + if is_dataclass && self.enabled(Rule::FunctionCallInDataclassDefaultArgument) { ruff::rules::function_call_in_dataclass_defaults(self, body); } } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 962190fbbde29..07a8826f1b442 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -752,6 +752,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "009") => (RuleGroup::Unspecified, rules::ruff::rules::FunctionCallInDataclassDefaultArgument), (Ruff, "010") => (RuleGroup::Unspecified, rules::ruff::rules::ExplicitFStringTypeConversion), (Ruff, "011") => (RuleGroup::Unspecified, rules::ruff::rules::StaticKeyDictComprehension), + (Ruff, "012") => (RuleGroup::Unspecified, rules::ruff::rules::MutableClassDefault), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index 66948c9d3eeb4..646842686fcfa 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -170,7 +170,18 @@ mod tests { #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"))] #[test_case(Rule::FunctionCallInDataclassDefaultArgument, Path::new("RUF009.py"))] - fn mutable_defaults(rule_code: Rule, path: &Path) -> Result<()> { + fn mutable_dataclass_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_messages!(snapshot, diagnostics); + Ok(()) + } + + #[test_case(Rule::MutableClassDefault, Path::new("RUF012.py"))] + fn mutable_class_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(), diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index 9835966ece9fc..368570cd208f2 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -3,7 +3,7 @@ pub(crate) use asyncio_dangling_task::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use invalid_pyproject_toml::InvalidPyprojectToml; -pub(crate) use mutable_defaults_in_dataclass_fields::*; +pub(crate) use mutable_defaults_in_class_fields::*; pub(crate) use pairwise_over_zipped::*; pub(crate) use unused_noqa::*; @@ -15,7 +15,7 @@ mod collection_literal_concatenation; mod confusables; mod explicit_f_string_type_conversion; mod invalid_pyproject_toml; -mod mutable_defaults_in_dataclass_fields; +mod mutable_defaults_in_class_fields; mod pairwise_over_zipped; mod static_key_dict_comprehension; mod unused_noqa; 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_class_fields.rs similarity index 80% rename from crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs rename to crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs index 1f300b303b32d..ea1ab50cd18aa 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs @@ -12,8 +12,7 @@ use ruff_python_semantic::{ use crate::checkers::ast::Checker; /// ## What it does -/// Checks for mutable default values in dataclasses without the use of -/// `dataclasses.field`. +/// Checks for mutable default values in dataclasses. /// /// ## Why is this bad? /// Mutable default values share state across all instances of the dataclass, @@ -21,7 +20,10 @@ use crate::checkers::ast::Checker; /// changed in one instance, as those changes will unexpectedly affect all /// other instances. /// -/// ## Examples: +/// Instead of sharing mutable defaults, use the `field(default_factory=...)` +/// pattern. +/// +/// ## Examples /// ```python /// from dataclasses import dataclass /// @@ -40,26 +42,49 @@ use crate::checkers::ast::Checker; /// class A: /// mutable_default: list[int] = field(default_factory=list) /// ``` +#[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 mutable default values in class attributes. +/// +/// ## Why is this bad? +/// Mutable default values share state across all instances of the class, +/// 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. +/// +/// When mutable value are intended, they should be annotated with +/// `typing.ClassVar`. /// -/// Alternatively, if you _want_ shared behaviour, make it more obvious -/// by assigning to a module-level variable: +/// ## Examples /// ```python -/// from dataclasses import dataclass +/// class A: +/// mutable_default: list[int] = [] +/// ``` /// -/// I_KNOW_THIS_IS_SHARED_STATE = [1, 2, 3, 4] +/// Use instead: +/// ```python +/// from typing import ClassVar /// /// -/// @dataclass /// class A: -/// mutable_default: list[int] = I_KNOW_THIS_IS_SHARED_STATE +/// mutable_default: ClassVar[list[int]] = [] /// ``` #[violation] -pub struct MutableDataclassDefault; +pub struct MutableClassDefault; -impl Violation for MutableDataclassDefault { +impl Violation for MutableClassDefault { #[derive_message_formats] fn message(&self) -> String { - format!("Do not use mutable default values for dataclass attributes") + format!("Do not use mutable default values for class attributes") } } @@ -73,7 +98,7 @@ impl Violation for MutableDataclassDefault { /// ## Options /// - `flake8-bugbear.extend-immutable-calls` /// -/// ## Examples: +/// ## Examples /// ```python /// from dataclasses import dataclass /// @@ -214,8 +239,15 @@ pub(crate) fn function_call_in_dataclass_defaults(checker: &mut Checker, body: & } } -/// RUF008 -pub(crate) fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { +/// RUF008/RUF012 +pub(crate) fn mutable_class_default(checker: &mut Checker, body: &[Stmt], is_dataclass: bool) { + fn diagnostic(is_dataclass: bool, value: &Expr) -> Diagnostic { + if is_dataclass { + Diagnostic::new(MutableDataclassDefault, value.range()) + } else { + Diagnostic::new(MutableClassDefault, value.range()) + } + } for statement in body { match statement { Stmt::AnnAssign(ast::StmtAnnAssign { @@ -227,16 +259,12 @@ pub(crate) fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { && !is_immutable_annotation(checker.semantic_model(), annotation) && is_mutable_expr(value) { - checker - .diagnostics - .push(Diagnostic::new(MutableDataclassDefault, value.range())); + checker.diagnostics.push(diagnostic(is_dataclass, value)); } } Stmt::Assign(ast::StmtAssign { value, .. }) => { if is_mutable_expr(value) { - checker - .diagnostics - .push(Diagnostic::new(MutableDataclassDefault, value.range())); + checker.diagnostics.push(diagnostic(is_dataclass, value)); } } _ => (), diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF012_RUF012.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF012_RUF012.py.snap new file mode 100644 index 0000000000000..4f12483f56c2e --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF012_RUF012.py.snap @@ -0,0 +1,42 @@ +--- +source: crates/ruff/src/rules/ruff/mod.rs +--- +RUF012.py:8:34: RUF012 Do not use mutable default values for class attributes + | + 7 | class A: + 8 | mutable_default: list[int] = [] + | ^^ RUF012 + 9 | immutable_annotation: typing.Sequence[int] = [] +10 | without_annotation = [] + | + +RUF012.py:10:26: RUF012 Do not use mutable default values for class attributes + | + 8 | mutable_default: list[int] = [] + 9 | immutable_annotation: typing.Sequence[int] = [] +10 | without_annotation = [] + | ^^ RUF012 +11 | ignored_via_comment: list[int] = [] # noqa: RUF012 +12 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + | + +RUF012.py:17:34: RUF012 Do not use mutable default values for class attributes + | +16 | class B: +17 | mutable_default: list[int] = [] + | ^^ RUF012 +18 | immutable_annotation: Sequence[int] = [] +19 | without_annotation = [] + | + +RUF012.py:19:26: RUF012 Do not use mutable default values for class attributes + | +17 | mutable_default: list[int] = [] +18 | immutable_annotation: Sequence[int] = [] +19 | without_annotation = [] + | ^^ RUF012 +20 | ignored_via_comment: list[int] = [] # noqa: RUF012 +21 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + | + + diff --git a/ruff.schema.json b/ruff.schema.json index f2190aecf46f5..ba2220c10cd46 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2348,6 +2348,7 @@ "RUF01", "RUF010", "RUF011", + "RUF012", "RUF1", "RUF10", "RUF100",