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

Split mutable-class-defaults rules into separate modules (#5031)

This commit is contained in:
Charlie Marsh 2023-06-12 13:21:28 -04:00 committed by GitHub
parent 638c18f007
commit a77d2df934
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 342 additions and 305 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -791,21 +791,16 @@ where
flake8_pie::rules::non_unique_enums(self, stmt, body);
}
if self.any_enabled(&[
Rule::MutableDataclassDefault,
Rule::FunctionCallInDataclassDefaultArgument,
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::MutableClassDefault) {
ruff::rules::mutable_class_default(self, class_def);
}
if is_dataclass && self.enabled(Rule::FunctionCallInDataclassDefaultArgument) {
ruff::rules::function_call_in_dataclass_defaults(self, body);
}
if self.enabled(Rule::MutableDataclassDefault) {
ruff::rules::mutable_dataclass_default(self, class_def);
}
if self.enabled(Rule::FunctionCallInDataclassDefaultArgument) {
ruff::rules::function_call_in_dataclass_default(self, class_def);
}
if self.enabled(Rule::FStringDocstring) {

115
crates/ruff/src/rules/ruff/rules/function_call_in_dataclass_default.rs Normal file
View File

@ -0,0 +1,115 @@
use rustpython_parser::ast::{self, Expr, Ranged, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::compose_call_path;
use ruff_python_ast::call_path::{from_qualified_name, CallPath};
use ruff_python_semantic::analyze::typing::is_immutable_func;
use crate::checkers::ast::Checker;
use crate::rules::ruff::rules::helpers::{
is_allowed_dataclass_function, is_class_var_annotation, is_dataclass,
};
/// ## What it does
/// Checks for function calls in dataclass attribute defaults.
///
/// ## Why is this bad?
/// Function calls are only performed once, at definition time. The returned
/// value is then reused by all instances of the dataclass. This can lead to
/// unexpected behavior when the function call returns a mutable object, as
/// changes to the object will be shared across all instances.
///
/// If a field needs to be initialized with a mutable object, use the
/// `field(default_factory=...)` pattern.
///
/// ## Options
/// - `flake8-bugbear.extend-immutable-calls`
///
/// ## Examples
/// ```python
/// from dataclasses import dataclass
///
///
/// def simple_list() -> list[int]:
/// return [1, 2, 3, 4]
///
///
/// @dataclass
/// class A:
/// mutable_default: list[int] = simple_list()
/// ```
///
/// Use instead:
/// ```python
/// from dataclasses import dataclass, field
///
///
/// def creating_list() -> list[int]:
/// return [1, 2, 3, 4]
///
///
/// @dataclass
/// class A:
/// mutable_default: list[int] = field(default_factory=creating_list)
/// ```
#[violation]
pub struct FunctionCallInDataclassDefaultArgument {
pub name: Option<String>,
}
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")
}
}
}
/// RUF009
pub(crate) fn function_call_in_dataclass_default(
checker: &mut Checker,
class_def: &ast::StmtClassDef,
) {
if !is_dataclass(checker.semantic_model(), class_def) {
return;
}
let extend_immutable_calls: Vec<CallPath> = checker
.settings
.flake8_bugbear
.extend_immutable_calls
.iter()
.map(|target| from_qualified_name(target))
.collect();
for statement in &class_def.body {
if let Stmt::AnnAssign(ast::StmtAnnAssign {
annotation,
value: Some(expr),
..
}) = 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)
&& !is_allowed_dataclass_function(checker.semantic_model(), func)
{
checker.diagnostics.push(Diagnostic::new(
FunctionCallInDataclassDefaultArgument {
name: compose_call_path(func),
},
expr.range(),
));
}
}
}
}
}

45
crates/ruff/src/rules/ruff/rules/helpers.rs Normal file
View File

@ -0,0 +1,45 @@
use ruff_python_ast::helpers::map_callable;
use rustpython_parser::ast::{self, Expr};
use ruff_python_semantic::model::SemanticModel;
pub(super) fn is_mutable_expr(expr: &Expr) -> bool {
matches!(
expr,
Expr::List(_)
| Expr::Dict(_)
| Expr::Set(_)
| Expr::ListComp(_)
| Expr::DictComp(_)
| Expr::SetComp(_)
)
}
const ALLOWED_DATACLASS_SPECIFIC_FUNCTIONS: &[&[&str]] = &[&["dataclasses", "field"]];
pub(super) fn is_allowed_dataclass_function(model: &SemanticModel, func: &Expr) -> bool {
model.resolve_call_path(func).map_or(false, |call_path| {
ALLOWED_DATACLASS_SPECIFIC_FUNCTIONS
.iter()
.any(|target| call_path.as_slice() == *target)
})
}
/// Returns `true` if the given [`Expr`] is a `typing.ClassVar` annotation.
pub(super) fn is_class_var_annotation(model: &SemanticModel, annotation: &Expr) -> bool {
let Expr::Subscript(ast::ExprSubscript { value, .. }) = &annotation else {
return false;
};
model.match_typing_expr(value, "ClassVar")
}
/// Returns `true` if the given class is a dataclass.
pub(super) fn is_dataclass(model: &SemanticModel, class_def: &ast::StmtClassDef) -> bool {
class_def.decorator_list.iter().any(|decorator| {
model
.resolve_call_path(map_callable(&decorator.expression))
.map_or(false, |call_path| {
call_path.as_slice() == ["dataclasses", "dataclass"]
})
})
}

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

@ -2,20 +2,24 @@ pub(crate) use ambiguous_unicode_character::*;
pub(crate) use asyncio_dangling_task::*;
pub(crate) use collection_literal_concatenation::*;
pub(crate) use explicit_f_string_type_conversion::*;
pub(crate) use function_call_in_dataclass_default::*;
pub(crate) use invalid_pyproject_toml::InvalidPyprojectToml;
pub(crate) use mutable_defaults_in_class_fields::*;
pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*;
pub(crate) use pairwise_over_zipped::*;
pub(crate) use unused_noqa::*;
pub(crate) use static_key_dict_comprehension::*;
pub(crate) use unused_noqa::*;
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod collection_literal_concatenation;
mod confusables;
mod explicit_f_string_type_conversion;
mod function_call_in_dataclass_default;
mod helpers;
mod invalid_pyproject_toml;
mod mutable_defaults_in_class_fields;
mod mutable_class_default;
mod mutable_dataclass_default;
mod pairwise_over_zipped;
mod static_key_dict_comprehension;
mod unused_noqa;

78
crates/ruff/src/rules/ruff/rules/mutable_class_default.rs Normal file
View File

@ -0,0 +1,78 @@
use rustpython_parser::ast::{self, Ranged, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::analyze::typing::is_immutable_annotation;
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 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!("Mutable class attributes should be annotated with `typing.ClassVar`")
}
}
/// 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 {
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(MutableClassDefault, value.range()));
}
}
Stmt::Assign(ast::StmtAssign { value, .. }) => {
if is_mutable_expr(value) {
checker
.diagnostics
.push(Diagnostic::new(MutableClassDefault, value.range()));
}
}
_ => (),
}
}
}

83
crates/ruff/src/rules/ruff/rules/mutable_dataclass_default.rs Normal file
View File

@ -0,0 +1,83 @@
use rustpython_parser::ast::{self, Ranged, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::analyze::typing::is_immutable_annotation;
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.
///
/// ## 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.
///
/// Instead of sharing mutable defaults, use the `field(default_factory=...)`
/// pattern.
///
/// ## 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)
/// ```
#[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")
}
}
/// RUF008
pub(crate) fn mutable_dataclass_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 {
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()));
}
}
_ => (),
}
}
}

283
crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs
View File

@ -1,283 +0,0 @@
use rustpython_parser::ast::{self, Decorator, Expr, Ranged, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::{from_qualified_name, CallPath};
use ruff_python_ast::{call_path::compose_call_path, helpers::map_callable};
use ruff_python_semantic::{
analyze::typing::{is_immutable_annotation, is_immutable_func},
model::SemanticModel,
};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for mutable default values in dataclasses.
///
/// ## 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.
///
/// Instead of sharing mutable defaults, use the `field(default_factory=...)`
/// pattern.
///
/// ## 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)
/// ```
#[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 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.
///
/// ## Why is this bad?
/// Function calls are only performed once, at definition time. The returned
/// value is then reused by all instances of the dataclass.
///
/// ## Options
/// - `flake8-bugbear.extend-immutable-calls`
///
/// ## Examples
/// ```python
/// from dataclasses import dataclass
///
///
/// def creating_list() -> list[int]:
/// 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[int]:
/// 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[int]:
/// 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<String>,
}
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,
Expr::List(_)
| Expr::Dict(_)
| Expr::Set(_)
| Expr::ListComp(_)
| Expr::DictComp(_)
| Expr::SetComp(_)
)
}
const ALLOWED_DATACLASS_SPECIFIC_FUNCTIONS: &[&[&str]] = &[&["dataclasses", "field"]];
fn is_allowed_dataclass_function(model: &SemanticModel, func: &Expr) -> bool {
model.resolve_call_path(func).map_or(false, |call_path| {
ALLOWED_DATACLASS_SPECIFIC_FUNCTIONS
.iter()
.any(|target| call_path.as_slice() == *target)
})
}
/// Returns `true` if the given [`Expr`] is a `typing.ClassVar` annotation.
fn is_class_var_annotation(model: &SemanticModel, annotation: &Expr) -> bool {
let Expr::Subscript(ast::ExprSubscript { value, .. }) = &annotation else {
return false;
};
model.match_typing_expr(value, "ClassVar")
}
/// RUF009
pub(crate) fn function_call_in_dataclass_defaults(checker: &mut Checker, body: &[Stmt]) {
let extend_immutable_calls: Vec<CallPath> = checker
.settings
.flake8_bugbear
.extend_immutable_calls
.iter()
.map(|target| from_qualified_name(target))
.collect();
for statement in body {
if let Stmt::AnnAssign(ast::StmtAnnAssign {
annotation,
value: Some(expr),
..
}) = statement
{
if is_class_var_annotation(checker.semantic_model(), annotation) {
continue;
}
if let Expr::Call(ast::ExprCall { func, .. }) = expr.as_ref() {
if !is_immutable_func(checker.semantic_model(), func, &extend_immutable_calls)
&& !is_allowed_dataclass_function(checker.semantic_model(), func)
{
checker.diagnostics.push(Diagnostic::new(
FunctionCallInDataclassDefaultArgument {
name: compose_call_path(func),
},
expr.range(),
));
}
}
}
}
}
/// 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 {
annotation,
value: Some(value),
..
}) => {
if !is_class_var_annotation(checker.semantic_model(), annotation)
&& !is_immutable_annotation(checker.semantic_model(), annotation)
&& is_mutable_expr(value)
{
checker.diagnostics.push(diagnostic(is_dataclass, value));
}
}
Stmt::Assign(ast::StmtAssign { value, .. }) => {
if is_mutable_expr(value) {
checker.diagnostics.push(diagnostic(is_dataclass, value));
}
}
_ => (),
}
}
}
pub(crate) fn is_dataclass(model: &SemanticModel, decorator_list: &[Decorator]) -> bool {
decorator_list.iter().any(|decorator| {
model
.resolve_call_path(map_callable(&decorator.expression))
.map_or(false, |call_path| {
call_path.as_slice() == ["dataclasses", "dataclass"]
})
})
}

8
crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF012_RUF012.py.snap
View File

@ -1,7 +1,7 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
---
RUF012.py:8:34: RUF012 Do not use mutable default values for class attributes
RUF012.py:8:34: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
|
7 | class A:
8 | mutable_default: list[int] = []
@ -10,7 +10,7 @@ RUF012.py:8:34: RUF012 Do not use mutable default values for class attributes
10 | without_annotation = []
|
RUF012.py:10:26: RUF012 Do not use mutable default values for class attributes
RUF012.py:10:26: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
|
8 | mutable_default: list[int] = []
9 | immutable_annotation: typing.Sequence[int] = []
@ -20,7 +20,7 @@ RUF012.py:10:26: RUF012 Do not use mutable default values for class attributes
12 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
|
RUF012.py:17:34: RUF012 Do not use mutable default values for class attributes
RUF012.py:17:34: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
|
16 | class B:
17 | mutable_default: list[int] = []
@ -29,7 +29,7 @@ RUF012.py:17:34: RUF012 Do not use mutable default values for class attributes
19 | without_annotation = []
|
RUF012.py:19:26: RUF012 Do not use mutable default values for class attributes
RUF012.py:19:26: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
|
17 | mutable_default: list[int] = []
18 | immutable_annotation: Sequence[int] = []