RUF008/9 for non-dataclasses as well#4096
Conversation
PR Check ResultsBenchmarkLinuxWindows |
Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Micha Reiser <micha@reiser.io>
|
I don't know what's going on with the test failure. It doesn't fail locally on my mac. Did I exceed some constant number of rules or something? |
crates/ruff/src/registry/rule_set.rs
Outdated
| /// 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.
Is this okay? It fixed a test failure.
There was a problem hiding this comment.
Yeah, this is fine. I assume your rule increased ruff's rule count over 576
| ruff::rules::function_call_in_class_defaults( | ||
| self, | ||
| body, | ||
| is_dataclass, |
There was a problem hiding this comment.
Nit: Guaranteed to always be true
| is_dataclass, | |
| true, |
| /// 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.
Nit: It can be difficult to understand the semantic of true in a call of mutable_class_default(checker, true, body) without looking at the method signature.
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
| is_dataclass: bool, | ||
| emit_dataclass_error: bool, |
There was a problem hiding this comment.
Nit: Do we need both booleans. It seems that emit_dataclass_error is always true if is_dataclass is true
There was a problem hiding this comment.
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.
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.
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 field_specifiers in general for @dataclass_transforms, which is not easy to do in general. Then we can sort out whether we want to keep RUF009 scoped to dataclasses or expand it to any class with exemptions for dataclass-like things.
| if self.settings.rules.enabled(Rule::MutableDataclassDefault) { | ||
| ruff::rules::mutable_class_default(self, true, body); | ||
| } |
There was a problem hiding this comment.
Is my understanding correct that ruff now emits two diagnostics for the same locations if both the MutableDataclassDefault and MutableClassDefault 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?
| is_dataclass: bool, | ||
| emit_dataclass_error: bool, |
There was a problem hiding this comment.
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?
|
Closing in favor of #4390. |
Closes #4053.
This is my first PR and I didn't know if discussion in #4053 ended with a tacit approval of this change or not, so I didn't put this up fully polished just yet. I'm hoping for early feedback since this is a pretty simple PR, but happy to go back and polish first if that's preferred.
Makes new rules for non-dataclasses, which might lead to some confusion since there is such high overlap. Still, I didn't want to break backwards compatibility, even for very new rules.