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.
This commit is contained in:
Charlie Marsh 2023-06-13 10:21:14 -04:00 committed by GitHub
parent 7b4dde0c6c
commit 65312bad01
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 78 additions and 81 deletions

View File

@ -5,12 +5,11 @@ from typing import ClassVar, Sequence
KNOWINGLY_MUTABLE_DEFAULT = [] KNOWINGLY_MUTABLE_DEFAULT = []
@dataclass() @dataclass
class A: class A:
mutable_default: list[int] = [] mutable_default: list[int] = []
immutable_annotation: typing.Sequence[int] = [] immutable_annotation: typing.Sequence[int] = []
without_annotation = [] without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF008
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
perfectly_fine: list[int] = field(default_factory=list) perfectly_fine: list[int] = field(default_factory=list)
class_variable: typing.ClassVar[list[int]] = [] class_variable: typing.ClassVar[list[int]] = []
@ -21,7 +20,6 @@ class B:
mutable_default: list[int] = [] mutable_default: list[int] = []
immutable_annotation: Sequence[int] = [] immutable_annotation: Sequence[int] = []
without_annotation = [] without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF008
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
perfectly_fine: list[int] = field(default_factory=list) perfectly_fine: list[int] = field(default_factory=list)
class_variable: ClassVar[list[int]] = [] class_variable: ClassVar[list[int]] = []

View File

@ -8,7 +8,6 @@ class A:
mutable_default: list[int] = [] mutable_default: list[int] = []
immutable_annotation: typing.Sequence[int] = [] immutable_annotation: typing.Sequence[int] = []
without_annotation = [] without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF012
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
class_variable: typing.ClassVar[list[int]] = [] class_variable: typing.ClassVar[list[int]] = []
@ -17,6 +16,18 @@ class B:
mutable_default: list[int] = [] mutable_default: list[int] = []
immutable_annotation: Sequence[int] = [] immutable_annotation: Sequence[int] = []
without_annotation = [] without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF012
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
class_variable: ClassVar[list[int]] = [] 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]] = []

View File

@ -95,11 +95,8 @@ pub(crate) fn function_call_in_dataclass_default(
}) = statement }) = statement
{ {
if let Expr::Call(ast::ExprCall { func, .. }) = expr.as_ref() { if let Expr::Call(ast::ExprCall { func, .. }) = expr.as_ref() {
if is_class_var_annotation(checker.semantic_model(), annotation) { if !is_class_var_annotation(checker.semantic_model(), annotation)
continue; && !is_immutable_func(checker.semantic_model(), func, &extend_immutable_calls)
}
if !is_immutable_func(checker.semantic_model(), func, &extend_immutable_calls)
&& !is_allowed_dataclass_function(checker.semantic_model(), func) && !is_allowed_dataclass_function(checker.semantic_model(), func)
{ {
checker.diagnostics.push(Diagnostic::new( checker.diagnostics.push(Diagnostic::new(

View File

@ -45,10 +45,6 @@ impl Violation for MutableClassDefault {
/// RUF012 /// RUF012
pub(crate) fn mutable_class_default(checker: &mut Checker, class_def: &ast::StmtClassDef) { 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 { for statement in &class_def.body {
match statement { match statement {
Stmt::AnnAssign(ast::StmtAnnAssign { 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) if is_mutable_expr(value)
&& !is_class_var_annotation(checker.semantic_model(), annotation) && !is_class_var_annotation(checker.semantic_model(), annotation)
&& !is_immutable_annotation(checker.semantic_model(), annotation) && !is_immutable_annotation(checker.semantic_model(), annotation)
&& !is_dataclass(checker.semantic_model(), class_def)
{ {
checker checker
.diagnostics .diagnostics

View File

@ -8,17 +8,19 @@ use crate::checkers::ast::Checker;
use crate::rules::ruff::rules::helpers::{is_class_var_annotation, is_dataclass, is_mutable_expr}; use crate::rules::ruff::rules::helpers::{is_class_var_annotation, is_dataclass, is_mutable_expr};
/// ## What it does /// ## What it does
/// Checks for mutable default values in dataclasses. /// Checks for mutable default values in dataclass attributes.
/// ///
/// ## Why is this bad? /// ## Why is this bad?
/// Mutable default values share state across all instances of the dataclass, /// Mutable default values share state across all instances of the dataclass.
/// while not being obvious. This can lead to bugs when the attributes are /// This can lead to bugs when the attributes are changed in one instance, as
/// changed in one instance, as those changes will unexpectedly affect all /// those changes will unexpectedly affect all other instances.
/// other instances.
/// ///
/// Instead of sharing mutable defaults, use the `field(default_factory=...)` /// Instead of sharing mutable defaults, use the `field(default_factory=...)`
/// pattern. /// pattern.
/// ///
/// If the default value is intended to be mutable, it should be annotated with
/// `typing.ClassVar`.
///
/// ## Examples /// ## Examples
/// ```python /// ```python
/// from dataclasses import dataclass /// from dataclasses import dataclass
@ -38,6 +40,17 @@ use crate::rules::ruff::rules::helpers::{is_class_var_annotation, is_dataclass,
/// class A: /// class A:
/// mutable_default: list[int] = field(default_factory=list) /// 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] #[violation]
pub struct MutableDataclassDefault; 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 { for statement in &class_def.body {
match statement { if let Stmt::AnnAssign(ast::StmtAnnAssign {
Stmt::AnnAssign(ast::StmtAnnAssign { annotation,
annotation, value: Some(value),
value: Some(value), ..
.. }) = statement
}) => { {
if is_mutable_expr(value) if is_mutable_expr(value)
&& !is_class_var_annotation(checker.semantic_model(), annotation) && !is_class_var_annotation(checker.semantic_model(), annotation)
&& !is_immutable_annotation(checker.semantic_model(), annotation) && !is_immutable_annotation(checker.semantic_model(), annotation)
{ {
checker checker
.diagnostics .diagnostics
.push(Diagnostic::new(MutableDataclassDefault, value.range())); .push(Diagnostic::new(MutableDataclassDefault, value.range()));
}
} }
Stmt::Assign(ast::StmtAssign { value, .. }) => {
if is_mutable_expr(value) {
checker
.diagnostics
.push(Diagnostic::new(MutableDataclassDefault, value.range()));
}
}
_ => (),
} }
} }
} }

View File

@ -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 RUF008.py:10:34: RUF008 Do not use mutable default values for dataclass attributes
| |
8 | @dataclass() 8 | @dataclass
9 | class A: 9 | class A:
10 | mutable_default: list[int] = [] 10 | mutable_default: list[int] = []
| ^^ RUF008 | ^^ RUF008
@ -11,34 +11,14 @@ RUF008.py:10:34: RUF008 Do not use mutable default values for dataclass attribut
12 | without_annotation = [] 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] = [] 18 | @dataclass
11 | immutable_annotation: typing.Sequence[int] = [] 19 | class B:
12 | without_annotation = [] 20 | mutable_default: list[int] = []
| ^^ 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] = []
| ^^ RUF008 | ^^ RUF008
22 | immutable_annotation: Sequence[int] = [] 21 | immutable_annotation: Sequence[int] = []
23 | without_annotation = [] 22 | 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
| |

View File

@ -16,27 +16,37 @@ RUF012.py:10:26: RUF012 Mutable class attributes should be annotated with `typin
9 | immutable_annotation: typing.Sequence[int] = [] 9 | immutable_annotation: typing.Sequence[int] = []
10 | without_annotation = [] 10 | without_annotation = []
| ^^ RUF012 | ^^ RUF012
11 | ignored_via_comment: list[int] = [] # noqa: RUF012 11 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
12 | 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: 15 | class B:
17 | mutable_default: list[int] = [] 16 | mutable_default: list[int] = []
| ^^ RUF012 | ^^ RUF012
18 | immutable_annotation: Sequence[int] = [] 17 | immutable_annotation: Sequence[int] = []
19 | without_annotation = [] 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] = [] 16 | mutable_default: list[int] = []
18 | immutable_annotation: Sequence[int] = [] 17 | immutable_annotation: Sequence[int] = []
19 | without_annotation = [] 18 | without_annotation = []
| ^^ RUF012 | ^^ RUF012
20 | ignored_via_comment: list[int] = [] # noqa: RUF012 19 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
21 | 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)
| |