Skip to content

Commit

Permalink
feat: Add dataclass checks
Browse files Browse the repository at this point in the history
  • Loading branch information
mosauter committed Apr 6, 2023
1 parent 34e9786 commit 6fa00e6
Show file tree
Hide file tree
Showing 11 changed files with 417 additions and 0 deletions.
21 changes: 21 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF200.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from dataclasses import dataclass, field

KNOWINGLY_MUTABLE_DEFAULT = []


@dataclass()
class A:
mutable_default: list[int] = []
without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF200
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
perfectly_fine: list[int] = field(default_factory=list)


@dataclass
class B:
mutable_default: list[int] = []
without_annotation = []
ignored_via_comment: list[int] = [] # noqa: RUF200
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT
perfectly_fine: list[int] = field(default_factory=list)
28 changes: 28 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF201.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from dataclasses import dataclass
from typing import NamedTuple


def default_function() -> list[int]:
return []


class ImmutableType(NamedTuple):
something: int = 8


@dataclass()
class A:
hidden_mutable_default: list[int] = default_function()


DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES = ImmutableType(40)
DEFAULT_A_FOR_ALL_DATACLASSES = A([1, 2, 3])


@dataclass
class B:
hidden_mutable_default: list[int] = default_function()
another_dataclass: A = A()
not_optimal: ImmutableType = ImmutableType(20)
good_variant: ImmutableType = DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES
okay_variant: A = DEFAULT_A_FOR_ALL_DATACLASSES
14 changes: 14 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,20 @@ where
flake8_pie::rules::non_unique_enums(self, stmt, body);
}

if ruff::rules::is_dataclass(self, decorator_list) {
if self.settings.rules.enabled(Rule::MutableDataclassDefault) {
ruff::rules::mutable_dataclass_default(self, body);
}

if self
.settings
.rules
.enabled(Rule::FunctionCallInDataclassDefaultArgument)
{
ruff::rules::function_call_in_dataclass_defaults(self, body);
}
}

self.check_builtin_shadowing(name, stmt, false);

for expr in bases {
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Ruff, "006") => Rule::AsyncioDanglingTask,
(Ruff, "007") => Rule::PairwiseOverZipped,
(Ruff, "100") => Rule::UnusedNOQA,
(Ruff, "200") => Rule::MutableDataclassDefault,
(Ruff, "201") => Rule::FunctionCallInDataclassDefaultArgument,

// flake8-django
(Flake8Django, "001") => Rule::DjangoNullableModelStringField,
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,8 @@ ruff_macros::register_rules!(
rules::ruff::rules::AsyncioDanglingTask,
rules::ruff::rules::UnusedNOQA,
rules::ruff::rules::PairwiseOverZipped,
rules::ruff::rules::MutableDataclassDefault,
rules::ruff::rules::FunctionCallInDataclassDefaultArgument,
// flake8-django
rules::flake8_django::rules::DjangoNullableModelStringField,
rules::flake8_django::rules::DjangoLocalsInRenderFunction,
Expand Down
12 changes: 12 additions & 0 deletions crates/ruff/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,16 @@ mod tests {
assert_yaml_snapshot!(diagnostics);
Ok(())
}

#[test_case(Rule::MutableDataclassDefault, Path::new("RUF200.py"); "RUF200")]
#[test_case(Rule::FunctionCallInDataclassDefaultArgument, Path::new("RUF201.py"); "RUF201")]
fn mutable_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_yaml_snapshot!(snapshot, diagnostics);
Ok(())
}
}
5 changes: 5 additions & 0 deletions crates/ruff/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod collection_literal_concatenation;
mod mutable_defaults_in_dataclass_fields;
mod pairwise_over_zipped;
mod unused_noqa;

Expand All @@ -12,6 +13,10 @@ pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask};
pub use collection_literal_concatenation::{
collection_literal_concatenation, CollectionLiteralConcatenation,
};
pub use mutable_defaults_in_dataclass_fields::{
function_call_in_dataclass_defaults, is_dataclass, mutable_dataclass_default,
FunctionCallInDataclassDefaultArgument, MutableDataclassDefault,
};
pub use pairwise_over_zipped::{pairwise_over_zipped, PairwiseOverZipped};
pub use unused_noqa::{UnusedCodes, UnusedNOQA};

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
use crate::checkers::ast::Checker;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{call_path::compose_call_path, types::Range};
use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind};

/// ## What it does
/// Checks for mutable default values in dataclasses without the usage of
/// `dataclasses.field`.
///
/// ## Why is it bad?
/// Mutable default values share state across all instances of the dataclass,
/// while not being obvious. This can lead to nasty bugs when the attributes
/// are changed, but expected not to in another instance.
///
/// ## 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)
/// ```
///
/// Alternatively, if you __want__ the shared behaviour, make it more obvious
/// by assigning it 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;

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 function calls in dataclass defaults.
///
/// ## Why is it bad?
/// Function calls are only performed once during definition time. The result
/// gets reused in all created instances of the dataclass.
///
/// ## Examples:
/// ```python
/// from dataclasses import dataclass
///
/// def creating_list() -> list[]:
/// 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[]:
/// 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[]:
/// 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.node,
ExprKind::List { .. }
| ExprKind::Dict { .. }
| ExprKind::Set { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::SetComp { .. }
)
}

const ALLOWED_FUNCS: &[&[&str]] = &[&["dataclasses", "field"]];

fn is_allowed_func(checker: &Checker, func: &Expr) -> bool {
checker
.ctx
.resolve_call_path(func)
.map_or(false, |call_path| {
ALLOWED_FUNCS
.iter()
.any(|target| call_path.as_slice() == *target)
})
}

pub fn function_call_in_dataclass_defaults(checker: &mut Checker, body: &[Stmt]) {
for statement in body {
if let StmtKind::AnnAssign {
value: Some(expr), ..
} = &statement.node
{
if let ExprKind::Call { func, .. } = &expr.node {
if !is_allowed_func(checker, func) {
checker.diagnostics.push(Diagnostic::new(
FunctionCallInDataclassDefaultArgument {
name: compose_call_path(func),
},
Range::from(expr),
));
}
}
}
}
}

pub fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) {
for statement in body {
match &statement.node {
StmtKind::AnnAssign {
value: Some(value), ..
}
| StmtKind::Assign { value, .. } => {
if is_mutable_expr(value) {
checker
.diagnostics
.push(Diagnostic::new(MutableDataclassDefault, Range::from(value)));
}
}
_ => (),
}
}
}

pub fn is_dataclass(checker: &Checker, decorator_list: &[Expr]) -> bool {
for decorator in decorator_list.iter() {
if checker
.ctx
.resolve_call_path(if let ExprKind::Call { func, .. } = &decorator.node {
func
} else {
decorator
})
.map_or(false, |call_path| {
call_path.as_slice() == ["dataclasses", "dataclass"]
})
{
return true;
}
}
false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
expression: diagnostics
---
- kind:
name: MutableDataclassDefault
body: Do not use mutable default values for dataclass attributes
suggestion: ~
fixable: false
location:
row: 8
column: 33
end_location:
row: 8
column: 35
fix:
edits: []
parent: ~
- kind:
name: MutableDataclassDefault
body: Do not use mutable default values for dataclass attributes
suggestion: ~
fixable: false
location:
row: 9
column: 25
end_location:
row: 9
column: 27
fix:
edits: []
parent: ~
- kind:
name: MutableDataclassDefault
body: Do not use mutable default values for dataclass attributes
suggestion: ~
fixable: false
location:
row: 17
column: 33
end_location:
row: 17
column: 35
fix:
edits: []
parent: ~
- kind:
name: MutableDataclassDefault
body: Do not use mutable default values for dataclass attributes
suggestion: ~
fixable: false
location:
row: 18
column: 25
end_location:
row: 18
column: 27
fix:
edits: []
parent: ~

Loading

0 comments on commit 6fa00e6

Please sign in to comment.