Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Expand RUF008 to all classes, but to a new code (RUF012) (#4390) 

AFAIK, there is no reason to limit RUF008 to just dataclasses -- mutable
defaults have the same problems for regular classes.

Partially addresses https://github.com/charliermarsh/ruff/issues/4053
and broken out from https://github.com/charliermarsh/ruff/pull/4096.

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
This commit is contained in:
Adam Pauls 2023-06-12 09:54:27 -07:00 committed by GitHub
parent 70e6c212d9
commit 638c18f007
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 141 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
crates/ruff/resources/test/fixtures/ruff/RUF012.py vendored Normal file
View File

@ -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]] = []

13
crates/ruff/src/checkers/ast/mod.rs
View File

@ -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);
}
}

1
crates/ruff/src/codes.rs
View File

@ -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),

13
crates/ruff/src/rules/ruff/mod.rs
View File

@ -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(),

4
crates/ruff/src/rules/ruff/rules/mod.rs
View File

@ -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;

78
crates/ruff/src/rules/ruff/rules/mutable_defaults_in_dataclass_fields.rs → crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs
View File

@ -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,19 +42,6 @@ use crate::checkers::ast::Checker;
/// class A:
/// mutable_default: list[int] = field(default_factory=list)
/// ```
///
/// Alternatively, if you _want_ shared behaviour, make it more obvious
/// by assigning 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;
@ -63,6 +52,42 @@ impl Violation for MutableDataclassDefault {
}
}
/// ## 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`.
///
/// ## Examples
/// ```python
/// class A:
/// mutable_default: list[int] = []
/// ```
///
/// Use instead:
/// ```python
/// from typing import ClassVar
///
///
/// class A:
/// mutable_default: ClassVar[list[int]] = []
/// ```
#[violation]
pub struct MutableClassDefault;
impl Violation for MutableClassDefault {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use mutable default values for class attributes")
}
}
/// ## What it does
/// Checks for function calls in dataclass defaults.
///
@ -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));
}
}
_ => (),

42
crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF012_RUF012.py.snap Normal file
View File

@ -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
|

1
ruff.schema.json generated
View File

@ -2348,6 +2348,7 @@
"RUF01",
"RUF010",
"RUF011",
"RUF012",
"RUF1",
"RUF10",
"RUF100",