[ruff] Unformatted special comments (RUF037)
#14111
Conversation
|
I'm not sure why the snapshot is empty. Debugging showed that new diagnostics are correctly added, yet it's almost as if the rule never ran. |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF037 | 1074 | 1074 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
InSyncWithFoo
changed the title
ruff] Unformatted special comments (RUF102)ruff] Unformatted special comments (RUF104)
InSyncWithFoo
force-pushed
the
RUF102
branch
3 times, most recently
from
e54e9a0 to
7e9c370
Compare
crates/ruff_linter/src/rules/ruff/rules/unformatted_special_comment.rs
Outdated
Show resolved
Hide resolved
dhruvmanila
added
rule
There was a problem hiding this comment.
Thanks for working on this rule.
I think we need to define the scope of this rule more carefully, especially when it comes to non-ruff suppression comments.
- All other
RUF1XXrules are specific to ruff pragma comments - I don't see us as the authority defining how
pyrightandmypycomments should be formatted.
I see two possible versions of this rule:
- One specific to ruff's suppression comments that enforces consistent formatting, including how rule codes are formatted
- A general purpose rule that handles all kinds of pragma comments. It only enforces a single space between
#and the keyword and, optionally, that the colon is followed by a space. We can add a configuration option to support custom pragma codes. This is not aRUF1XXrule.
| // If the file is exempted, ignore all diagnostics, | ||
| // save for RUF104, which operates on `# noqa` comments | ||
| // and thus needs to be suppressed explicitly. | ||
| if !matches!(diagnostic.kind.rule(), Rule::UnformattedSpecialComment) |
There was a problem hiding this comment.
Can you explain the reasoning for this special handling? I think I would find this behavior surprising and I'm not sure if it justifies the added complexity
There was a problem hiding this comment.
blanket-noqa (PGH004) gets similar treatment (right above, line 48), and must either not be enabled or explicitly disabled using per-file-ignores/# ruff: noqa: PGH004 (line 223):
if matches!(diagnostic.kind.rule(), Rule::BlanketNOQA) {
continue;
}if settings.rules.enabled(Rule::BlanketNOQA)
&& !per_file_ignores.contains(Rule::BlanketNOQA)
&& !exemption.enumerates(Rule::BlanketNOQA)RUF104 isn't a noqa-only rule, so unformatted_special_comment() shouldn't be called from check_noqa(). However, exemption is computed within check_noqa() using a function that might emit a warning if a # noqa: comment does not have any codes. This means the necessary information cannot be retrieved from functions other than check_noqa() without undesirable side-effects.
crates/ruff_linter/src/rules/ruff/rules/unformatted_special_comment.rs
Outdated
Show resolved
Hide resolved
|
@AlexWaygood do you have any recommendations on the rule's scope? |
|
I agree that we're getting into very hairy territory if we start formatting other tools' pragma comments by default. However, it does seem useful to have a rule that does generalised formatting of pragma comments. I like @MichaReiser's second suggestion in #14111 (review) -- I would suggest a rule
|
|
They are! I wasn't necessarily criticising anything you were doing -- was just giving my response to Micha's request in #14111 (comment) on what the scope of the rule should be :-)
Feel free to keep this PR small and not implement any configurability now. Small, incremental PRs are definitely encouraged! However, if you want to open a followup PR, please make this rule configurable in the followup PR rather than creating a separate rule just for custom comments. I think Micha and I are aligned that a single, configurable rule would be preferable to two rules that do very similar things. |
InSyncWithFoo
changed the title
ruff] Unformatted special comments (RUF104)ruff] Unformatted special comments (RUF037)
|
I do think that we can drastically simplify the implementation because we no longer need to special case different comments. All we need is to match for
Technically, there's also |
Special casing @AlexWaygood If we were to add a "custom" formatter, that would need to be generic enough to apply to most, if not all, pragma comments. The only way to do so (that I can see) is to use regex search-and-replace. Rust's regex engine guarantees linear time matching, so there is no risk of ReDoS. However, there are also disadvantages: No advanced features. Having no lookarounds, no conditional replacements, no multicaptures and no backreferences means that some comments would require multiple runs to be formatted correctly. For example: [tool.ruff.lint.format-special-comments]
# `#pyright:ignore[reportFoo,reportBar,reportQux]` -> `# pyright: ignore[reportFoo,reportBar,reportQux]`
# https://regex101.com/r/zd9esV/1
'(?i)#\s*pyright\s*:\s*' = '# pyright: '
# `# pyright: ignore[reportFoo,reportBar,reportQux]` -> `# pyright: ignore[reportFoo, reportBar,reportQux]`
# https://regex101.com/r/zd9esV/2
'(# pyright: ignore\[[^\s,]+(?:, [^\s,]+)*?),(?:\s{2,})?(\S)' = '$1, $2'
# `# pyright: ignore[reportFoo, reportBar,reportQux]` -> `# pyright: ignore[reportFoo, reportBar, reportQux]`
# https://regex101.com/r/zd9esV/3
'(# pyright: ignore\[[^\s,]+(?:, [^\s,]+)*?),(?:\s{2,})?(\S)' = '$1, $2'
# And so on.With PCRE features, it's much easier (less readable too, though): # https://regex101.com/r/URCpwl/1
'(?i)(?:(?:(\#\s*pyright\s*:\s*)|\G(?:ignore\[)?[^\s,]+\K,\s*))' = '${1:+# pyright\: :, }'We could also consider |
|
The problem I see with special casing is that it isn't a good fit for a general-purpose, extensible rule; it should instead be a specific rule for The question is if we can come up with some general rules that work for all suppression comments without being specific to any pragma comment and that allows easy configuration:
The rule doesn't require that any suppression comment is valid. E.g. it accepts Note: What's interesting is that e.g. pyright's documentation uses a space before |
|
I would prefer it if we could have two rules, one for Ruff-specific comments and one customizable, somewhat similar to |
The rule would handle this, when using the rules that I outlined above. What the rule wouldn't handle is that e.g. all rule codes are upper-case (although I'm not sure if ruff even accepts lower case rule codes?) I'm open to having a noqa specific rule, but I don't think it should handle other ruff suppression comments. |
Making this rule extensible is also what I've suggesting in #10160 (comment) . Even if it comes as a follow-up PR, I'd like if it could handle comments that Ruff doesn't need to be aware of (like pyright) And if it supports regex somehow, this would superseed TD007 (https://docs.astral.sh/ruff/rules/missing-space-after-todo-colon/) entirely |
|
I'll close this PR because there's no agreement on the rule's semantics. We should define and align on that first before working on a new PR. Thank you @InSyncWithFoo for initiating the discussion and creating this pr. |
Summary
Resolves #10160.
Test Plan
cargo nextest runandcargo insta test.