-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUF008/9 for non-dataclasses as well #4096
Changes from 10 commits
43dc99b
941d47a
87da6cb
86dfc75
261d418
e7423ad
12aff2d
3234dac
03483eb
e1a2919
4c451d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import typing | ||
from typing import Sequence | ||
|
||
KNOWINGLY_MUTABLE_DEFAULT = [] | ||
|
||
|
||
class A: | ||
mutable_default: list[int] = [] | ||
immutable_annotation: typing.Sequence[int] = [] | ||
without_annotation = [] | ||
ignored_via_comment: list[int] = [] # noqa: RUF010 | ||
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT | ||
|
||
|
||
class B: | ||
mutable_default: list[int] = [] | ||
immutable_annotation: Sequence[int] = [] | ||
without_annotation = [] | ||
ignored_via_comment: list[int] = [] # noqa: RUF010 | ||
correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import datetime | ||
import typing | ||
from typing import NamedTuple | ||
from dataclasses import field | ||
|
||
def default_function() -> list[int]: | ||
return [] | ||
|
||
|
||
class ImmutableType(NamedTuple): | ||
something: int = 8 | ||
|
||
|
||
class A: | ||
hidden_mutable_default: list[int] = default_function() | ||
class_variable: typing.ClassVar[list[int]] = default_function() | ||
fine_date: datetime.date = datetime.date(2042, 1, 1) | ||
|
||
|
||
DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES = ImmutableType(40) | ||
DEFAULT_A_FOR_ALL_DATACLASSES = A([1, 2, 3]) | ||
|
||
|
||
class B: | ||
hidden_mutable_default: list[int] = default_function() | ||
another_class: A = A() | ||
not_optimal: ImmutableType = ImmutableType(20) | ||
good_variant: ImmutableType = DEFAULT_IMMUTABLETYPE_FOR_ALL_DATACLASSES | ||
okay_variant: A = DEFAULT_A_FOR_ALL_DATACLASSES | ||
not_fine_field: list[int] = field(default_vactory=list) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -843,19 +843,45 @@ where | |||||
|
||||||
if self.settings.rules.any_enabled(&[ | ||||||
Rule::MutableDataclassDefault, | ||||||
Rule::MutableClassDefault, | ||||||
Rule::FunctionCallInDataclassDefaultArgument, | ||||||
]) && ruff::rules::is_dataclass(self, decorator_list) | ||||||
{ | ||||||
if self.settings.rules.enabled(Rule::MutableDataclassDefault) { | ||||||
ruff::rules::mutable_dataclass_default(self, body); | ||||||
Rule::FunctionCallInClassDefaultArgument, | ||||||
]) { | ||||||
let is_dataclass = ruff::rules::is_dataclass(self, decorator_list); | ||||||
|
||||||
if is_dataclass { | ||||||
if self.settings.rules.enabled(Rule::MutableDataclassDefault) { | ||||||
ruff::rules::mutable_class_default(self, true, body); | ||||||
} | ||||||
|
||||||
if self | ||||||
.settings | ||||||
.rules | ||||||
.enabled(Rule::FunctionCallInDataclassDefaultArgument) | ||||||
{ | ||||||
ruff::rules::function_call_in_class_defaults( | ||||||
self, | ||||||
body, | ||||||
is_dataclass, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Guaranteed to always be
Suggested change
|
||||||
true, | ||||||
); | ||||||
} | ||||||
} | ||||||
if self.settings.rules.enabled(Rule::MutableClassDefault) { | ||||||
ruff::rules::mutable_class_default(self, false, body); | ||||||
} | ||||||
|
||||||
if self | ||||||
.settings | ||||||
.rules | ||||||
.enabled(Rule::FunctionCallInDataclassDefaultArgument) | ||||||
.enabled(Rule::FunctionCallInClassDefaultArgument) | ||||||
{ | ||||||
ruff::rules::function_call_in_dataclass_defaults(self, body); | ||||||
ruff::rules::function_call_in_class_defaults( | ||||||
self, | ||||||
body, | ||||||
is_dataclass, | ||||||
MichaReiser marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
false, | ||||||
); | ||||||
} | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,10 @@ use std::iter::FusedIterator; | |
/// | ||
/// Uses a bitset where a bit of one signals that the Rule with that [u16] is in this set. | ||
#[derive(Clone, Default, CacheKey, PartialEq, Eq)] | ||
pub struct RuleSet([u64; 9]); | ||
pub struct RuleSet([u64; 10]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this okay? It fixed a test failure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is fine. I assume your rule increased ruff's rule count over 576 |
||
|
||
impl RuleSet { | ||
const EMPTY: [u64; 9] = [0; 9]; | ||
const EMPTY: [u64; 10] = [0; 10]; | ||
|
||
// 64 fits into a u16 without truncation | ||
#[allow(clippy::cast_possible_truncation)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,17 @@ impl Violation for MutableDataclassDefault { | |
} | ||
} | ||
|
||
/// This rule is same as MutableDataclassDefault, but for any class. The same arguments apply. | ||
#[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. | ||
/// | ||
|
@@ -135,6 +146,25 @@ impl Violation for FunctionCallInDataclassDefaultArgument { | |
} | ||
} | ||
|
||
/// Same as FunctionCallInDataclassDefaultArgument, but for any class. | ||
/// Importantly, this error will be issued on calls to dataclasses.field | ||
#[violation] | ||
pub struct FunctionCallInClassDefaultArgument { | ||
pub name: Option<String>, | ||
} | ||
|
||
impl Violation for FunctionCallInClassDefaultArgument { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let FunctionCallInClassDefaultArgument { name } = self; | ||
if let Some(name) = name { | ||
format!("Do not perform function call `{name}` in non-dataclass attribute defaults") | ||
} else { | ||
format!("Do not perform function call in non-dataclass attribute defaults") | ||
} | ||
} | ||
} | ||
|
||
fn is_mutable_expr(expr: &Expr) -> bool { | ||
matches!( | ||
&expr.node, | ||
|
@@ -165,8 +195,13 @@ fn is_class_var_annotation(context: &Context, annotation: &Expr) -> bool { | |
context.match_typing_expr(value, "ClassVar") | ||
} | ||
|
||
/// RUF009 | ||
pub fn function_call_in_dataclass_defaults(checker: &mut Checker, body: &[Stmt]) { | ||
/// RUF009/RUF011 | ||
pub fn function_call_in_class_defaults( | ||
checker: &mut Checker, | ||
body: &[Stmt], | ||
is_dataclass: bool, | ||
emit_dataclass_error: bool, | ||
Comment on lines
+215
to
+216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Do we need both booleans. It seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it depends on if you think RUF008/9 are distinct from RUF010/11. Should the latter issue the same error as the former if the former are not enabled? Probably not right? I would personally rather just collapse the error but I wasn't sure if it was acceptable to break backwards compatibility like that. That said, RUF008/9 are quite new so maybe it's okay? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see, the nesting in the call-site is different than I thought. But does that mean that Ruff now emits two errors when seeing a non-immutable function call: Once the dataclass error and once the normal error if both rules are enabled? @charliermarsh what's your opinion on merging the rules and/or parametrizing the diagnostics with whether it is a dataclass or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about this: I'll put up a PR that expands RUF008 to all classes, not just dataclasses. I think that's uncontroversial, since mutable defaults are never a good idea. There are some issues with RUF009 because we need exemptions for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. @charliermarsh what's your take? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added my opinion over in #4390 (comment) |
||
) { | ||
let extend_immutable_calls: Vec<CallPath> = checker | ||
.settings | ||
.flake8_bugbear | ||
|
@@ -186,23 +221,41 @@ pub fn function_call_in_dataclass_defaults(checker: &mut Checker, body: &[Stmt]) | |
continue; | ||
} | ||
if let ExprKind::Call { func, .. } = &expr.node { | ||
if !is_immutable_func(&checker.ctx, func, &extend_immutable_calls) | ||
&& !is_allowed_dataclass_function(&checker.ctx, func) | ||
if !(is_immutable_func(&checker.ctx, func, &extend_immutable_calls) | ||
|| is_dataclass && is_allowed_dataclass_function(&checker.ctx, func)) | ||
{ | ||
checker.diagnostics.push(Diagnostic::new( | ||
FunctionCallInDataclassDefaultArgument { | ||
name: compose_call_path(func), | ||
}, | ||
expr.range(), | ||
)); | ||
let diagnostic: Diagnostic = if emit_dataclass_error { | ||
Diagnostic::new( | ||
FunctionCallInDataclassDefaultArgument { | ||
name: compose_call_path(func), | ||
}, | ||
expr.range(), | ||
) | ||
} else { | ||
Diagnostic::new( | ||
FunctionCallInClassDefaultArgument { | ||
name: compose_call_path(func), | ||
}, | ||
expr.range(), | ||
) | ||
}; | ||
checker.diagnostics.push(diagnostic); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// RUF008 | ||
pub fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { | ||
/// RUF008/RUF010 | ||
pub fn mutable_class_default(checker: &mut Checker, emit_dataclass_error: bool, body: &[Stmt]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: It can be difficult to understand the semantic of Introducing an enum can improve readability: #[derive(Copy, Clone, Debug)]
enum ClassKind {
Class,
Dataclass
} It has the added benefit that you can implement methods on the kind |
||
fn diagnostic(emit_dataclass_error: bool, value: &Expr) -> Diagnostic { | ||
if emit_dataclass_error { | ||
Diagnostic::new(MutableDataclassDefault, value.range()) | ||
} else { | ||
Diagnostic::new(MutableClassDefault, value.range()) | ||
} | ||
} | ||
|
||
for statement in body { | ||
match &statement.node { | ||
StmtKind::AnnAssign { | ||
|
@@ -216,14 +269,14 @@ pub fn mutable_dataclass_default(checker: &mut Checker, body: &[Stmt]) { | |
{ | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(MutableDataclassDefault, value.range())); | ||
.push(diagnostic(emit_dataclass_error, value)); | ||
} | ||
} | ||
StmtKind::Assign { value, .. } => { | ||
if is_mutable_expr(value) { | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(MutableDataclassDefault, value.range())); | ||
.push(diagnostic(emit_dataclass_error, value)); | ||
} | ||
} | ||
_ => (), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
--- | ||
source: crates/ruff/src/rules/ruff/mod.rs | ||
--- | ||
RUF010.py:8:34: RUF010 Do not use mutable default values for class attributes | ||
| | ||
8 | class A: | ||
9 | mutable_default: list[int] = [] | ||
| ^^ RUF010 | ||
10 | immutable_annotation: typing.Sequence[int] = [] | ||
11 | without_annotation = [] | ||
| | ||
|
||
RUF010.py:10:26: RUF010 Do not use mutable default values for class attributes | ||
| | ||
10 | mutable_default: list[int] = [] | ||
11 | immutable_annotation: typing.Sequence[int] = [] | ||
12 | without_annotation = [] | ||
| ^^ RUF010 | ||
13 | ignored_via_comment: list[int] = [] # noqa: RUF010 | ||
14 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT | ||
| | ||
|
||
RUF010.py:16:34: RUF010 Do not use mutable default values for class attributes | ||
| | ||
16 | class B: | ||
17 | mutable_default: list[int] = [] | ||
| ^^ RUF010 | ||
18 | immutable_annotation: Sequence[int] = [] | ||
19 | without_annotation = [] | ||
| | ||
|
||
RUF010.py:18:26: RUF010 Do not use mutable default values for class attributes | ||
| | ||
18 | mutable_default: list[int] = [] | ||
19 | immutable_annotation: Sequence[int] = [] | ||
20 | without_annotation = [] | ||
| ^^ RUF010 | ||
21 | ignored_via_comment: list[int] = [] # noqa: RUF010 | ||
22 | correct_code: list[int] = KNOWINGLY_MUTABLE_DEFAULT | ||
| | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is my understanding correct that ruff now emits two diagnostics for the same locations if both the
MutableDataclassDefault
andMutableClassDefault
rules are enabled?I think we should avoid this when possible. Is the motivation of having two different rules so that we can show two different messages? We could e.g. parameterize the diagnostic with the class kind to emit two different messages but implement it as a single rule. @charliermarsh any thoughts on this?