Skip to content

Commit

Permalink
Remove unannotated attributes from RUF008 (#5049)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
charliermarsh authored Jun 13, 2023
1 parent 7b4dde0 commit 65312ba
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 81 deletions.
4 changes: 1 addition & 3 deletions crates/ruff/resources/test/fixtures/ruff/RUF008.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
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]] = []
Expand All @@ -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]] = []
15 changes: 13 additions & 2 deletions crates/ruff/resources/test/fixtures/ruff/RUF012.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]] = []

Expand All @@ -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]] = []
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 1 addition & 4 deletions crates/ruff/src/rules/ruff/rules/mutable_class_default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
58 changes: 31 additions & 27 deletions crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;

Expand All @@ -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()));
}
}
Stmt::Assign(ast::StmtAssign { value, .. }) => {
if is_mutable_expr(value) {
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()));
}
_ => (),
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,22 @@ 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 | immutable_annotation: typing.Sequence[int] = []
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 = []
|


Original file line number Diff line number Diff line change
Expand Up @@ -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)
|


0 comments on commit 65312ba

Please sign in to comment.