From 65312bad01c71cff29b63916e8fc61a285a66ffc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 13 Jun 2023 10:21:14 -0400 Subject: [PATCH] Remove unannotated attributes from RUF008 (#5049) ## Summary In a dataclass: ```py from dataclasses import dataclass @dataclass class X: class_var = {} x: int ``` `class_var` isn't actually a dataclass attribute, since it's unannotated. This PR removes such attributes from RUF008 (`mutable-dataclass-default`), but it does enforce them in RUF012 (`mutable-class-default`), since those should be annotated with `ClassVar` like any other mutable class attribute. Closes #5043. --- .../resources/test/fixtures/ruff/RUF008.py | 4 +- .../resources/test/fixtures/ruff/RUF012.py | 15 ++++- .../function_call_in_dataclass_default.rs | 7 +-- .../rules/ruff/rules/mutable_class_default.rs | 5 +- .../ruff/rules/mutable_dataclass_default.rs | 58 ++++++++++--------- ..._rules__ruff__tests__RUF008_RUF008.py.snap | 34 +++-------- ..._rules__ruff__tests__RUF012_RUF012.py.snap | 36 +++++++----- 7 files changed, 78 insertions(+), 81 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF008.py b/crates/ruff/resources/test/fixtures/ruff/RUF008.py index 3a40f7f094..978b88b0c7 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF008.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF008.py @@ -5,12 +5,11 @@ from typing import ClassVar, Sequence KNOWINGLY_MUTABLE_DEFAULT = [] -@dataclass() +@dataclass class A: mutable_default: list[int] = [] immutable_annotation: typing.Sequence[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) class_variable: typing.ClassVar[list[int]] = [] @@ -21,7 +20,6 @@ class B: mutable_default: list[int] = [] immutable_annotation: Sequence[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) class_variable: ClassVar[list[int]] = [] diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF012.py b/crates/ruff/resources/test/fixtures/ruff/RUF012.py index 4b4c6df0bf..1a51f179cd 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF012.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF012.py @@ -8,7 +8,6 @@ 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]] = [] @@ -17,6 +16,18 @@ 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]] = [] + + +from dataclasses import dataclass, field + + +@dataclass +class C: + mutable_default: list[int] = [] + immutable_annotation: Sequence[int] = [] + without_annotation = [] + correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT + perfectly_fine: list[int] = field(default_factory=list) + class_variable: ClassVar[list[int]] = [] diff --git a/crates/ruff/src/rules/ruff/rules/function_call_in_dataclass_default.rs b/crates/ruff/src/rules/ruff/rules/function_call_in_dataclass_default.rs index 373d37a724..54e0f88488 100644 --- a/crates/ruff/src/rules/ruff/rules/function_call_in_dataclass_default.rs +++ b/crates/ruff/src/rules/ruff/rules/function_call_in_dataclass_default.rs @@ -95,11 +95,8 @@ pub(crate) fn function_call_in_dataclass_default( }) = statement { if let Expr::Call(ast::ExprCall { func, .. }) = expr.as_ref() { - if is_class_var_annotation(checker.semantic_model(), annotation) { - continue; - } - - if !is_immutable_func(checker.semantic_model(), func, &extend_immutable_calls) + if !is_class_var_annotation(checker.semantic_model(), annotation) + && !is_immutable_func(checker.semantic_model(), func, &extend_immutable_calls) && !is_allowed_dataclass_function(checker.semantic_model(), func) { checker.diagnostics.push(Diagnostic::new( diff --git a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs index 9bf066ecad..f126251352 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_class_default.rs @@ -45,10 +45,6 @@ impl Violation for MutableClassDefault { /// RUF012 pub(crate) fn mutable_class_default(checker: &mut Checker, class_def: &ast::StmtClassDef) { - if is_dataclass(checker.semantic_model(), class_def) { - return; - } - for statement in &class_def.body { match statement { Stmt::AnnAssign(ast::StmtAnnAssign { @@ -59,6 +55,7 @@ pub(crate) fn mutable_class_default(checker: &mut Checker, class_def: &ast::Stmt if is_mutable_expr(value) && !is_class_var_annotation(checker.semantic_model(), annotation) && !is_immutable_annotation(checker.semantic_model(), annotation) + && !is_dataclass(checker.semantic_model(), class_def) { checker .diagnostics diff --git a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs index 6c51fc2443..63d4b6d397 100644 --- a/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs +++ b/crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs @@ -8,17 +8,19 @@ use crate::checkers::ast::Checker; use crate::rules::ruff::rules::helpers::{is_class_var_annotation, is_dataclass, is_mutable_expr}; /// ## What it does -/// Checks for mutable default values in dataclasses. +/// Checks for mutable default values in dataclass attributes. /// /// ## Why is this 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. +/// Mutable default values share state across all instances of the dataclass. +/// This can lead to bugs when the attributes are changed in one instance, as +/// those changes will unexpectedly affect all other instances. /// /// Instead of sharing mutable defaults, use the `field(default_factory=...)` /// pattern. /// +/// If the default value is intended to be mutable, it should be annotated with +/// `typing.ClassVar`. +/// /// ## Examples /// ```python /// from dataclasses import dataclass @@ -38,6 +40,17 @@ use crate::rules::ruff::rules::helpers::{is_class_var_annotation, is_dataclass, /// class A: /// mutable_default: list[int] = field(default_factory=list) /// ``` +/// +/// Or: +/// ```python +/// from dataclasses import dataclass, field +/// from typing import ClassVar +/// +/// +/// @dataclass +/// class A: +/// mutable_default: ClassVar[list[int]] = [] +/// ``` #[violation] pub struct MutableDataclassDefault; @@ -55,29 +68,20 @@ pub(crate) fn mutable_dataclass_default(checker: &mut Checker, class_def: &ast:: } for statement in &class_def.body { - match statement { - Stmt::AnnAssign(ast::StmtAnnAssign { - annotation, - value: Some(value), - .. - }) => { - if is_mutable_expr(value) - && !is_class_var_annotation(checker.semantic_model(), annotation) - && !is_immutable_annotation(checker.semantic_model(), annotation) - { - checker - .diagnostics - .push(Diagnostic::new(MutableDataclassDefault, value.range())); - } + if let Stmt::AnnAssign(ast::StmtAnnAssign { + annotation, + value: Some(value), + .. + }) = statement + { + if is_mutable_expr(value) + && !is_class_var_annotation(checker.semantic_model(), annotation) + && !is_immutable_annotation(checker.semantic_model(), annotation) + { + checker + .diagnostics + .push(Diagnostic::new(MutableDataclassDefault, value.range())); } - Stmt::Assign(ast::StmtAssign { value, .. }) => { - if is_mutable_expr(value) { - checker - .diagnostics - .push(Diagnostic::new(MutableDataclassDefault, value.range())); - } - } - _ => (), } } } 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 index 36c054f145..e1d7054a57 100644 --- 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 @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/ruff/mod.rs --- RUF008.py:10:34: RUF008 Do not use mutable default values for dataclass attributes | - 8 | @dataclass() + 8 | @dataclass 9 | class A: 10 | mutable_default: list[int] = [] | ^^ RUF008 @@ -11,34 +11,14 @@ RUF008.py:10:34: RUF008 Do not use mutable default values for dataclass attribut 12 | without_annotation = [] | -RUF008.py:12:26: RUF008 Do not use mutable default values for dataclass attributes +RUF008.py:20:34: RUF008 Do not use mutable default values for dataclass attributes | -10 | mutable_default: list[int] = [] -11 | immutable_annotation: typing.Sequence[int] = [] -12 | without_annotation = [] - | ^^ RUF008 -13 | ignored_via_comment: list[int] = [] # noqa: RUF008 -14 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT - | - -RUF008.py:21:34: RUF008 Do not use mutable default values for dataclass attributes - | -19 | @dataclass -20 | class B: -21 | mutable_default: list[int] = [] +18 | @dataclass +19 | class B: +20 | mutable_default: list[int] = [] | ^^ RUF008 -22 | immutable_annotation: Sequence[int] = [] -23 | without_annotation = [] - | - -RUF008.py:23:26: RUF008 Do not use mutable default values for dataclass attributes - | -21 | mutable_default: list[int] = [] -22 | immutable_annotation: Sequence[int] = [] -23 | without_annotation = [] - | ^^ RUF008 -24 | ignored_via_comment: list[int] = [] # noqa: RUF008 -25 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT +21 | immutable_annotation: Sequence[int] = [] +22 | without_annotation = [] | 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 index 02a09e70f4..55536d8dc2 100644 --- 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 @@ -16,27 +16,37 @@ RUF012.py:10:26: RUF012 Mutable class attributes should be annotated with `typin 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 +11 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT +12 | class_variable: typing.ClassVar[list[int]] = [] | -RUF012.py:17:34: RUF012 Mutable class attributes should be annotated with `typing.ClassVar` +RUF012.py:16:34: RUF012 Mutable class attributes should be annotated with `typing.ClassVar` | -16 | class B: -17 | mutable_default: list[int] = [] +15 | class B: +16 | mutable_default: list[int] = [] | ^^ RUF012 -18 | immutable_annotation: Sequence[int] = [] -19 | without_annotation = [] +17 | immutable_annotation: Sequence[int] = [] +18 | without_annotation = [] | -RUF012.py:19:26: RUF012 Mutable class attributes should be annotated with `typing.ClassVar` +RUF012.py:18:26: RUF012 Mutable class attributes should be annotated with `typing.ClassVar` | -17 | mutable_default: list[int] = [] -18 | immutable_annotation: Sequence[int] = [] -19 | without_annotation = [] +16 | mutable_default: list[int] = [] +17 | immutable_annotation: Sequence[int] = [] +18 | without_annotation = [] | ^^ RUF012 -20 | ignored_via_comment: list[int] = [] # noqa: RUF012 -21 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT +19 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT +20 | class_variable: ClassVar[list[int]] = [] + | + +RUF012.py:30:26: RUF012 Mutable class attributes should be annotated with `typing.ClassVar` + | +28 | mutable_default: list[int] = [] +29 | immutable_annotation: Sequence[int] = [] +30 | without_annotation = [] + | ^^ RUF012 +31 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT +32 | perfectly_fine: list[int] = field(default_factory=list) |