From 6fa00e6bf8592a9710347b76dfa350f2cc9db7b5 Mon Sep 17 00:00:00 2001 From: Moritz Sauter Date: Tue, 4 Apr 2023 18:11:09 +0200 Subject: [PATCH] feat: Add dataclass checks --- .../resources/test/fixtures/ruff/RUF200.py | 21 ++ .../resources/test/fixtures/ruff/RUF201.py | 28 +++ crates/ruff/src/checkers/ast/mod.rs | 14 ++ crates/ruff/src/codes.rs | 2 + crates/ruff/src/registry.rs | 2 + crates/ruff/src/rules/ruff/mod.rs | 12 + crates/ruff/src/rules/ruff/rules/mod.rs | 5 + .../mutable_defaults_in_dataclass_fields.rs | 207 ++++++++++++++++++ ..._rules__ruff__tests__RUF200_RUF200.py.snap | 61 ++++++ ..._rules__ruff__tests__RUF201_RUF201.py.snap | 61 ++++++ ruff.schema.json | 4 + 11 files changed, 417 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/ruff/RUF200.py create mode 100644 crates/ruff/resources/test/fixtures/ruff/RUF201.py create mode 100644 crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs create mode 100644 crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF200_RUF200.py.snap create mode 100644 crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF201_RUF201.py.snap diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF200.py b/crates/ruff/resources/test/fixtures/ruff/RUF200.py new file mode 100644 index 00000000000000..bfb5a9b446327b --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF200.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: 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) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF201.py b/crates/ruff/resources/test/fixtures/ruff/RUF201.py new file mode 100644 index 00000000000000..cfe4e6b871b021 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF201.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 ff33cbbbb434eb..a207154143f06e 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -816,6 +816,20 @@ where flake8_pie::rules::non_unique_enums(self, stmt, body); } + if 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 6de83693cec5a2..81616529796c07 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -701,6 +701,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Ruff, "006") => Rule::AsyncioDanglingTask, (Ruff, "007") => Rule::PairwiseOverZipped, (Ruff, "100") => Rule::UnusedNOQA, + (Ruff, "200") => Rule::MutableDataclassDefault, + (Ruff, "201") => Rule::FunctionCallInDataclassDefaultArgument, // flake8-django (Flake8Django, "001") => Rule::DjangoNullableModelStringField, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 268ff5d87300fd..b585c733782c41 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 63884d5cf4034c..b2f79a92d5b561 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("RUF200.py"); "RUF200")] + #[test_case(Rule::FunctionCallInDataclassDefaultArgument, Path::new("RUF201.py"); "RUF201")] + 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 04841e63dd07e0..6a063c18dc1f8e 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 00000000000000..21c6572719bbdf --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs @@ -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, +} + +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 +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF200_RUF200.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF200_RUF200.py.snap new file mode 100644 index 00000000000000..29d318d8777fdb --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF200_RUF200.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__RUF201_RUF201.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF201_RUF201.py.snap new file mode 100644 index 00000000000000..db545f0e138f51 --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF201_RUF201.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 aaa04c3f6c008c..ef91acc88678b8 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2077,6 +2077,10 @@ "RUF1", "RUF10", "RUF100", + "RUF2", + "RUF20", + "RUF200", + "RUF201", "S", "S1", "S10",