RUF008/9 for non-dataclasses as well #4096

adampauls · 2023-04-25T15:58:25Z

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.

github-actions · 2023-04-25T16:32:33Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.05     15.5±0.09ms     2.6 MB/sec    1.00     14.8±0.05ms     2.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      3.7±0.01ms     4.5 MB/sec    1.00      3.6±0.01ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.02    388.1±1.39µs     7.6 MB/sec    1.00    378.9±1.70µs     7.8 MB/sec
linter/all-rules/pydantic/types.py         1.03      6.5±0.01ms     3.9 MB/sec    1.00      6.3±0.03ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.10      8.3±0.01ms     4.9 MB/sec    1.00      7.5±0.02ms     5.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.07   1700.6±3.97µs     9.8 MB/sec    1.00   1584.1±2.46µs    10.5 MB/sec
linter/default-rules/numpy/globals.py      1.06    181.9±0.77µs    16.2 MB/sec    1.00    172.2±0.18µs    17.1 MB/sec
linter/default-rules/pydantic/types.py     1.08      3.6±0.00ms     7.0 MB/sec    1.00      3.4±0.01ms     7.6 MB/sec
parser/large/dataset.py                    1.00      5.9±0.00ms     6.9 MB/sec    1.01      5.9±0.01ms     6.9 MB/sec
parser/numpy/ctypeslib.py                  1.00   1145.6±1.01µs    14.5 MB/sec    1.00   1148.8±1.32µs    14.5 MB/sec
parser/numpy/globals.py                    1.00    116.9±0.27µs    25.2 MB/sec    1.00    117.3±1.14µs    25.2 MB/sec
parser/pydantic/types.py                   1.00      2.5±0.00ms    10.2 MB/sec    1.00      2.5±0.00ms    10.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     16.6±0.07ms     2.5 MB/sec    1.00     16.6±0.28ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.3±0.03ms     3.9 MB/sec    1.00      4.2±0.05ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    439.7±8.08µs     6.7 MB/sec    1.02    449.0±6.43µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.1±0.02ms     3.6 MB/sec    1.00      7.1±0.04ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      9.4±0.40ms     4.3 MB/sec    1.01      9.5±0.11ms     4.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00      2.0±0.04ms     8.2 MB/sec    1.04      2.1±0.03ms     7.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    213.8±3.23µs    13.8 MB/sec    1.02    219.0±7.13µs    13.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.4±0.07ms     5.8 MB/sec    1.04      4.6±0.04ms     5.6 MB/sec
parser/large/dataset.py                    1.00      7.0±0.39ms     5.8 MB/sec    1.01      7.1±0.06ms     5.8 MB/sec
parser/numpy/ctypeslib.py                  1.00   1305.9±6.27µs    12.8 MB/sec    1.01  1324.0±12.39µs    12.6 MB/sec
parser/numpy/globals.py                    1.00    134.4±1.07µs    21.9 MB/sec    1.02    137.2±4.75µs    21.5 MB/sec
parser/pydantic/types.py                   1.00      2.9±0.01ms     8.8 MB/sec    1.01      2.9±0.02ms     8.7 MB/sec

crates/ruff/src/checkers/ast/mod.rs

crates/ruff/src/rules/ruff/mod.rs

Co-authored-by: Micha Reiser <micha@reiser.io>

adampauls · 2023-04-26T04:54:43Z

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?

adampauls · 2023-04-28T16:14:17Z

crates/ruff/src/registry/rule_set.rs

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


Is this okay? It fixed a test failure.

Yeah, this is fine. I assume your rule increased ruff's rule count over 576

MichaReiser · 2023-05-01T09:06:51Z

crates/ruff/src/checkers/ast/mod.rs

+                            ruff::rules::function_call_in_class_defaults(
+                                self,
+                                body,
+                                is_dataclass,


Nit: Guaranteed to always be true

Suggested change

is_dataclass,

true,

crates/ruff/src/checkers/ast/mod.rs

MichaReiser · 2023-05-01T09:12:14Z

crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs

-/// 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]) {


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

MichaReiser · 2023-05-01T09:13:00Z

crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs

+    is_dataclass: bool,
+    emit_dataclass_error: bool,


Nit: Do we need both booleans. It seems that emit_dataclass_error is always true if is_dataclass is true

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?

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?

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.

Sounds good to me. @charliermarsh what's your take?

I've added my opinion over in #4390 (comment)

MichaReiser · 2023-05-02T06:57:12Z

crates/ruff/src/checkers/ast/mod.rs

+                        if self.settings.rules.enabled(Rule::MutableDataclassDefault) {
+                            ruff::rules::mutable_class_default(self, true, body);
+                        }


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?

MichaReiser · 2023-05-02T07:01:30Z

crates/ruff/src/rules/ruff/rules/mutable_defaults_in_class_fields.rs

+    is_dataclass: bool,
+    emit_dataclass_error: bool,


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?

AFAIK, there is no reason to limit RUF008 to just dataclasses -- mutable defaults have the same problems for regular classes. Partially addresses #4053 and broken out from #4096. --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>

charliermarsh · 2023-06-12T16:55:40Z

Closing in favor of #4390.

AFAIK, there is no reason to limit RUF008 to just dataclasses -- mutable defaults have the same problems for regular classes. Partially addresses #4053 and broken out from #4096. --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>

RUF008/9 for non-dataclasses as well

43dc99b

MichaReiser approved these changes Apr 26, 2023

View reviewed changes

crates/ruff/src/checkers/ast/mod.rs Outdated Show resolved Hide resolved

crates/ruff/src/rules/ruff/mod.rs Show resolved Hide resolved

adampauls and others added 4 commits April 25, 2023 21:40

Update crates/ruff/src/checkers/ast/mod.rs

941d47a

Co-authored-by: Micha Reiser <micha@reiser.io>

Update crates/ruff/src/rules/ruff/mod.rs

87da6cb

Co-authored-by: Micha Reiser <micha@reiser.io>

fmt

86dfc75

Merge remote-tracking branch 'ruff/main'

261d418

adampauls added 2 commits April 26, 2023 11:30

merge

e7423ad

bump ruleset size

12aff2d

adampauls commented Apr 28, 2023

View reviewed changes

adampauls added 3 commits April 28, 2023 09:16

Merge remote-tracking branch 'ruff/main'

3234dac

fixup

03483eb

that is one opinionated linter

e1a2919

MichaReiser reviewed May 1, 2023

View reviewed changes

MichaReiser reviewed May 2, 2023

View reviewed changes

Merge remote-tracking branch 'ruff/main'

4c451d0

adampauls mentioned this pull request May 12, 2023

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

Merged

charliermarsh closed this Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUF008/9 for non-dataclasses as well #4096

RUF008/9 for non-dataclasses as well #4096

adampauls commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

adampauls commented Apr 26, 2023 •

edited

Loading

adampauls Apr 28, 2023

MichaReiser May 1, 2023

MichaReiser May 1, 2023

MichaReiser May 1, 2023

MichaReiser May 1, 2023

adampauls May 2, 2023

MichaReiser May 2, 2023

adampauls May 9, 2023

MichaReiser May 10, 2023

zanieb May 12, 2023

MichaReiser May 2, 2023

MichaReiser May 2, 2023

charliermarsh commented Jun 12, 2023

RUF008/9 for non-dataclasses as well #4096

RUF008/9 for non-dataclasses as well #4096

Conversation

adampauls commented Apr 25, 2023 • edited Loading

github-actions bot commented Apr 25, 2023 • edited Loading

PR Check Results

Benchmark

Linux

Windows

adampauls commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Jun 12, 2023

adampauls commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

adampauls commented Apr 26, 2023 •

edited

Loading