[ruff] Add rule to replace noqa comments with ruff:ignore (RUF105) - #26423
Conversation
ntBre
added
rule
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| noqa-comments | 18503 | 18503 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
…105`) Summary -- This is the first in a series of migration rules to help users move from `noqa` comments with rule codes to `ruff:ignore` comments with names. This rule simply replaces `noqa: codes` with `ruff:ignore[codes]` (and the file-level variants), deferring the transformation of codes to names to a follow-up rule. This rule may be useful on its own without the codes -> names rule, assuming we stabilize `ruff:ignore` in the next minor release without stabilizing human-readable names. The rule skips any comment with an unknown selector, so it doesn't have to inspect `lint.external` like many of the other `noqa` rules. We also skip file-level `flake8: noqa` directives assuming they are intended to be shared with flake8. Codex pointed out one remaining issue with suppressing RUF105, but I think I'm convinced that this is a non-issue. You can't suppress `RUF105` with a `ruff:ignore` comment because it only runs after `Suppressions::check_suppressions`. This seems okay to me because it's pretty contradictory to try to add `ruff:ignore[RUF105]` to prevent an existing `noqa` comment from being converted to a `ruff:ignore` comment. I initially wrote a small, standalone implementation in the rule file itself, but it had to be combined with the `RUF100` implementation to leave unused `noqa` codes out of `RUF105` to be cleaned up by `RUF100` instead. Test Plan -- New mdtests I also expect a huge number of ecosystem results on this PR, possibly so many that the check times out.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Codex pointed out one remaining issue with suppressing RUF105, but I think I'm convinced that this
is a non-issue. Namely, you can't suppress RUF105 with a ruff:ignore comment because it only runs after
Suppressions::check_suppressions. This seems okay to me because it's pretty contradictory to try
to add ruff:ignore[RUF105] to prevent an existing noqa comment from being converted to a
ruff:ignore comment.
I'd find this surprising as a user, and the reason why I can't suppress this lint wouldn't be obvious to me. I don't know if it introduces much complexity, but it seems worth fixing.
I'd also assume that, in this case, --add-ignore doesn't work with RUF105 because it will add a ruff:ignore that is then simply ignored.
Codex also found two other issues:
- RUF101 does not apply to
ruff:ignore. This is not specific to this PR but it surfaces it more often, because RUF105 can rewrite a suppression before the redirect was applied. We can fix this as a separate issue but we should address it. - Then there's another
ruff:ignore/noqadifference. Maybe you're already aware of it. Butruff:file-ignoredoes not support nested pragma comments like# explanation # ruff:file-ignore[F401]but file-level noqa comments do. That means, rewriting them breaks the suppression. Again, this is moreruff:ignorespecific but something we should look into
| This leaves an unused `noqa` comment to be cleaned up by `RUF100` instead, which can be especially | ||
| important in the case of a standalone `noqa` comment, which has no effect (in almost all cases), but | ||
| could become an effectful own-line `ruff:ignore` comment if `RUF105` applied. |
There was a problem hiding this comment.
What about invalid rule codes? Does Ruff treat them as unused and removes them?
There was a problem hiding this comment.
This seems related to my response above, but yes, invalid-rule-code flags and offers to remove any invalid codes.
There was a problem hiding this comment.
We had a very long discussion around this behavior on the ty side #26426 for a similar rule
The conclusion was that the rule should also flag unused ignore comments. We may want to do the same here.
There was a problem hiding this comment.
I feel like it's a little different here since changing the comment type can change it from an inactive noqa comment to an active ruff:ignore comment, but after reading through #26426 again, I also don't want to debate it too much :)
I'm fine with emitting a diagnostic here and maybe making the fix unsafe or omitting it altogether.
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks for the review! I think I've addressed all of your comments. I realized that there is still an issue with
Basically, I think we should restructure |
| ``` | ||
|
|
||
| ```py | ||
| import math # noqa: F401, EXT001 |
There was a problem hiding this comment.
I think there's an argument that we should still flag F401. I'd be fine if we skip the autofix in this case.
There was a problem hiding this comment.
Updated to avoid the diagnostic if all codes are external and skip the fix if only some of them are external, as is the case here.
| | | ||
| ``` | ||
|
|
||
| ### External codes disable the rule |
There was a problem hiding this comment.
I find the word disable the rule a bit misleading. It's not that we disable the rule. It's more that using noqa remains allowed in combination with external codes.
| This leaves an unused `noqa` comment to be cleaned up by `RUF100` instead, which can be especially | ||
| important in the case of a standalone `noqa` comment, which has no effect (in almost all cases), but | ||
| could become an effectful own-line `ruff:ignore` comment if `RUF105` applied. |
There was a problem hiding this comment.
We had a very long discussion around this behavior on the ty side #26426 for a similar rule
The conclusion was that the rule should also flag unused ignore comments. We may want to do the same here.
| | | ||
| ``` | ||
|
|
||
| ### Nested pragma comment before the directive |
There was a problem hiding this comment.
Ah, you added the test here. The distance between the two tests is a bit long :)
There was a problem hiding this comment.
Yeah sorry, I put this in the ## Inline comments section to test both the nesting and inline comment part.
| RUF105 flags blanket comments but does not offer a fix because `ruff:ignore` requires at least one | ||
| rule selector. |
There was a problem hiding this comment.
and I think the complexity is that we don't track which rules were suppressed by a noqa comment?
There was a problem hiding this comment.
It turns out that we actually do track this in matches:
ruff/crates/ruff_linter/src/noqa.rs
Lines 1150 to 1156 in c36f662
I wonder why we don't use this in PGH004.
There was a problem hiding this comment.
Happy to revert this if I went too far, but I got this working with matches without too much code. My only hesitation is whether this logic belongs exclusively in PGH004 or if it's okay to use it here too.
|
I asked codex whether this new rule name follows clippys (and our own) naming convention and it does not. It should be |
Co-authored-by: Micha Reiser <micha@reiser.io>
2 participants
Summary
This is the first in a series of migration rules to help users move from
noqacomments with rulecodes to
ruff:ignorecomments with names. This rule simply replacesnoqa: codeswithruff:ignore[codes](and the file-level variants), deferring the transformation of codes to namesto a follow-up rule. This rule may be useful on its own without the codes -> names rule, assuming we
stabilize
ruff:ignorein the next minor release without stabilizing human-readable names.The rule skips any comment consisting entirely of known
externalselectors, while still emitting diagnostics for totally unknown codes. We also skip file-levelflake8: noqadirectives assuming they are intended to be shared with flake8.I initially wrote a small, standalone implementation in the rule file itself, but it had to be
combined with the
RUF100implementation to leave unusednoqacodes out ofRUF105to be cleanedup by
RUF100instead, as I note in the mdtest for this case.Test Plan
New mdtests
I also expect a huge number of ecosystem results on this PR, possibly so many that the check times out.