Allow rules without codes - #28049
Conversation
ntBre
added
preview
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 97.79%. The percentage of expected errors that received a diagnostic held steady at 94.33%. The number of fully passing files held steady at 112/136. |
Memory usage reportMemory usage unchanged ✅ |
| (Flake8UsePathlib, "124") => rules::flake8_use_pathlib::violations::PyPath, | ||
| (Flake8UsePathlib, "201") => rules::flake8_use_pathlib::rules::PathConstructorCurrentDirectory, | ||
| (Flake8UsePathlib, "202") => rules::flake8_use_pathlib::rules::OsPathGetsize, | ||
| (Flake8UsePathlib, "202") => rules::flake8_use_pathlib::rules::OsPathGetsize, |
There was a problem hiding this comment.
Oops. Unrelated but noticed while I was here.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| pytest-fixture-autouse | 740 | 740 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
b36a3c4 to
8e75e0c
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you.
I think I'd prefer if we used rule.identifier in fewer places.
- Let's move existing tests. While a little more work, it's not that involved and it simplifies the tests, they can simply call name. This also makes the test future prove for when we remove or deprecate codes. It also expresses that names are the preferred default now (I can't wait for when I don't need to remember both)
- Let's review the uses of
identifierinSupport lint rules without legacy codes. Are there places where we can just usenamebecause it isn't user facing? Are some of the uses user facing and need preview gating...
| pub fn identifier(&self) -> LintIdentifier { | ||
| match self.noqa_code() { | ||
| Some(code) => LintIdentifier::Code(code), | ||
| None => LintIdentifier::Name(self.name()), | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the reason that we continue preferring the code over its name? Why can't we always use the name as identifier? Do we need to preview gate this?
Overall, the name identifier seems a bit confusing to me. A rule's unique identifier now and going forward is always its name. This seems to be mainly a compatibility layer, but it can't be for anything user facing, because it would otherwise have to be preview gated?
There was a problem hiding this comment.
Codex is telling me that this breaks the ruff server.
With preview enabled and
output-prefer-rule-codes = true, the server requests Noqa-style suppression edits, so this branch drops the edit forpytest-fixture-autouse. I reproduced that its “Disable for this line” action disappears; setting the option to false restores it. Fall back to a name-basedruff: ignoreedit for codeless diagnostics in the server, regardless of the output preference.
There was a problem hiding this comment.
My Codex told me this as well, but I don't think this is actually a problem. This requires:
- preview enabled
- output-prefer-rule-codes = true
- activating the preview, category-only rule
pytest-fixture-autousethat is also quite pedantic - trying to quick-fix the rule in an editor
and the only "breakage" is that we don't offer the ruff: ignore quickfix when you've expressed a preference for codes. My codex also tried to fix this and it seemed more trouble than it was worth to allow changing the suppression style per diagnostic (it's currently selected for the whole batch of diagnostics).
I also wanted to fix it initially but was put off at least by Codex's initial approach, and it's quite a niche use case anyway. I can take a closer look if you want, though.
| let mut rule_names: Vec<_> = incompatible_rules | ||
| .into_iter() | ||
| .map(|rule| format!("`{}`", rule.noqa_code())) | ||
| .map(|rule| format!("`{}`", rule.identifier())) |
There was a problem hiding this comment.
This use seems user visible. Do we need to preview gate it?
There was a problem hiding this comment.
This is already preview-gated by virtue of the fact that the name-only rules are going to be in preview. There's no other option for rules without codes than to show the name, so I don't think there's any additional level of preview-gating we can apply. That's why identifier prefers codes when they're available, to preserve compatibility.
There was a problem hiding this comment.
Actually, we could just unwrap here for now. This is only used to warn for two rules that definitely have noqa codes.
Alternatively, are warnings part of our stable interface? It doesn't seem totally crazy to me just to switch this always to use names.
This also applies to this warning in the server:
ruff/crates/ruff_server/src/server/api/requests/hover.rs
Lines 147 to 148 in 26f4f73
We're down to only 3 uses of identifier including these two warnings.
There was a problem hiding this comment.
Curious what you'll think about this, but I went ahead and unwrapped noqa_code here, used name_and_code in the server warning, and inlined identifier into RUF105, which allowed me to delete identifier completely.
There was a problem hiding this comment.
Alternatively, are warnings part of our stable interface? It doesn't seem totally crazy to me just to switch this always to use names.
They're part of what I consider Ruff's user interface. Which is why I'd expect it to follow my configuration. But I admit, it's probably not worth bothering too much about it, but we should use codes or names based on the user's configuration if we can.
| let mut rule_names: Vec<_> = incompatible_rules | ||
| .into_iter() | ||
| .map(|rule| format!("`{}`", rule.noqa_code())) | ||
| .map(|rule| format!("`{}`", rule.identifier())) |
There was a problem hiding this comment.
Isn't this user facing? I think we need to use names here in preview
| .inner | ||
| .iter() | ||
| .sorted_by_key(|diagnostic| diagnostic.secondary_code()) | ||
| .sorted_by_key(|diagnostic| diagnostic.secondary_code_or_id()) |
There was a problem hiding this comment.
Why do we group by secondary_code_or_id and not by id and only store the secondary code if needed for display?
There was a problem hiding this comment.
Ah good catch, I didn't realize we were calling secondary_code again down below for the output. We can use name here too. I always find this whole fold pretty complicated to follow.
There was a problem hiding this comment.
Well, codex pointed out that using name here has a very small impact on the stable output because the second sort below this won't reorder ties with the same count. So "F401 previously appeared before F821 [when they had the same count]; now F821 appears first." I think this is probably fine?
There was a problem hiding this comment.
I'd have to take a closer look, but if it's user facing, I'd expect it to follow my setting. Again, I agree, that's a detail, so probably not worth spending much time on, but it degrades polish
| for rule in rules { | ||
| let name = rule.name(); | ||
| let code = rule.noqa_code(); | ||
| let code = rule.identifier(); |
There was a problem hiding this comment.
Why not use name_and_code here (and above)?
There was a problem hiding this comment.
name_and_code formats exactly as name (code), but this formats them differently:
let _ = writeln!(output, "- [`{name}`](rules/{name}.md) (`{code}`)");This code is actually problematic for a different reason since it loops over linters, disregarding rules that don't belong to a linter group. I think for now I'll just use noqa_code().unwrap() in light of that.
We'll have to stabilize both the categories themselves and a rule without a code for this to be a real problem, and I'm planning to include a secondary, category-focused rule page in my migration guide PR anyway.
There was a problem hiding this comment.
We should probably search by both here?
There was a problem hiding this comment.
I think we do already? This has both encoded_name and rule_code, producing example links like this:
(%27unused-import%27%20OR%20F401)
https://docs.astral.sh/ruff/rules/unused-import/
Ah but I guess we should drop the rule_code if it ends up being the name again.
There was a problem hiding this comment.
We could also have codex retitle all issues
There was a problem hiding this comment.
We could also have codex retitle all issues
That's a bit scary 😆 I'll start with the rule_code change.
8e75e0c to
8f81cca
Compare
Summary -- This PR allows adding rules that only belong to categories and only have names rather than requiring a linter group and code. The first non-empty commit introduces a `Rule::identifier` shim and uses it to replace `Rule::noqa_code` calls mechanically in all the test snapshots. The second commit updates `FixTable` from a mapping of `SecondaryCode -> (name, fix_count)` to `DiagnosticId -> (Option<SecondaryCode>, fix_count)`. This doesn't really change its usage but involves minor diffs to a substantial number of lines. It also allows us to drop some of the special `hashbrown` handling and the `hashbrown` dependency in `ruff_linter`, which is nice. The third commit contains the bulk of the change, including the macro changes needed to support `map_codes` entries like that for the `pytest-fixture-autouse` rule in the fourth commit: ```rust () => rules::ruff::rules::PytestFixtureAutouse, ``` These changes are pretty mechanical overall, just handling cases where the linter or noqa code could be missing, which fortunately seemed pretty straightforward in every case. The fourth commit takes advantage of these changes to reinstate `RUF076` as our first purely `pedantic` rule. I'm happy to drop that if it's too controversial or opens too many questions about the previously-removed rule and its code, but I thought it was nice to have a motivating example. We moved this rule from `preview` to `removed` because it was too pedantic for the `RUF` category, even in preview, so it's exactly the kind of rule we can support now with the categories. Finally, the fifth commit updates the contributing docs to reflect that linters and codes are optional. This may need to be adjusted if we back out the `RUF076` change. Test Plan -- Existing tests, plus a few new ones for `pytest-fixture-autouse`
8f81cca to
2941dfc
Compare
There was a problem hiding this comment.
wow, this touched more snapshots than I expected 😆
Waiting for the day where we run into Windows path-length issues
|
Can you squash some of your commit? It's otherwise very hard to review this PR because I can't review all changes because of the million snapshot changes, but going commit by commit only gives me partial changes, that I then need to keep in mind |
2 participants
Summary
This PR allows adding rules that only belong to categories and only have names rather than requiring
a linter group and code.
The first non-empty commit introduces a
Rule::identifiershim and uses it to replaceRule::noqa_codecalls mechanically in all the test snapshots.The second commit updates
FixTablefrom a mapping ofSecondaryCode -> (name, fix_count)toDiagnosticId -> (Option<SecondaryCode>, fix_count). This doesn't really change its usage butinvolves minor diffs to a substantial number of lines. It also allows us to drop some of the special
hashbrownhandling and thehashbrowndependency inruff_linter, which is nice.The third commit contains the bulk of the change, including the macro changes needed to support
map_codesentries like that for thepytest-fixture-autouserule in the fourth commit:These changes are pretty mechanical overall, just handling cases where the linter or noqa code could
be missing, which fortunately seemed pretty straightforward in every case.
The fourth commit takes advantage of these changes to reinstate
RUF076as our first purelypedanticrule. I'm happy to drop that if it's too controversial or opens too many questions aboutthe previously-removed rule and its code, but I thought it was nice to have a motivating example. We
moved this rule from
previewtoremovedbecause it was too pedantic for theRUFcategory, evenin preview, so it's exactly the kind of rule we can support now with the categories.
Finally, the fifth commit updates the contributing docs to reflect that linters and codes are
optional. This may need to be adjusted if we back out the
RUF076change.Test Plan
Existing tests, plus a few new ones for
pytest-fixture-autouse