Implement RUF028 to detect useless formatter suppression comments by snowsignal · Pull Request #9899 · astral-sh/ruff

snowsignal · 2024-02-08T19:25:00Z

Summary

This lint rule spots comments that are intended to suppress or enable the formatter, but will be ignored by the Ruff formatter.

We borrow some functions the formatter uses for determining comment placement / putting them in context within an AST.

The analysis function uses an AST visitor to visit each comment and attach it to the AST. It then uses that context to check:

Is this comment in an expression?
Does this comment have bad placement? (e.g. a # fmt: skip above a function instead of at the end of a line)
Is this comment redundant?
Does this comment actually suppress any code?
Does this comment have ambiguous placement? (e.g. a # fmt: off above an else: block)

If any of these are true, a violation is thrown. The reported reason depends on the order of the above check-list: in other words, a # fmt: skip comment on its own line within a list expression will be reported as being in an expression, since that reason takes priority.

The lint suggests removing the comment as an unsafe fix, regardless of the reason.

Test Plan

A snapshot test has been created.

snowsignal · 2024-02-08T19:28:11Z

Draft Checklist

Snapshot test output isn't totally correct yet. We need to correctly resolving dangling comments at the end of indented bodies.
A few more edge cases need to be added to ensure that all permutations are accounted for.
We need to check if there's any other scenarios to report, and whether those need new error messages.

codspeed-hq · 2024-02-14T20:22:06Z

CodSpeed Performance Report

Merging #9899 will not alter performance

_{Comparing jane/formatter/useless-noqa (8fc186e) with main (36bc725)}

Summary

✅ 30 untouched benchmarks

MichaReiser

Uhh, it seems there are many failing tests. I'm not sure what happened. It doesn't seem like you changed much on the formatter side.

MichaReiser

Nice work. You make it look less complicated than it is. There are some perf opportunities and simplifications that we can do to the code. But there are also still a few cases that are not handled correctly which make it difficult to assess if other cases are handled correctly. We should have a look at those.

I played around with it in the playground and found a few cases that need improvement:

def body():
    # fmt: off
    test
    # fmt: on

    test2

The rule reports that the last fmt: on comment is unnecessary because formatting is already enabled. This is not the case, formatting was disabled

The rule reports that the `fmt: off` in the `if True` body is unused but that's a valid use.

One check the rule doesn't cover today but I'm okay leaving to an extension is missing fmt: on comments to re-enable formatting without relying on the automatic re-enabling at the end of a block.

…esolved, particularly with dangling comments.

…ressed and several new problems are now reported. Edge cases involving dangling comments still need to be handled

…comments when determining suppression state

…le checks that need contextual information

github-actions · 2024-02-23T21:45:14Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

indico/indico (error)

ruff failed
  Cause: Rule `S410` was removed and cannot be selected.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +0 -0 fixes in 1 projects; 1 project error; 41 projects unchanged)

DisnakeDev/disnake (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ disnake/ext/commands/params.py:481:9: RUF028 This suppression comment is invalid because it cannot be in an expression, pattern, argument list, or other non-statement
+ disnake/ext/commands/params.py:498:9: RUF028 This suppression comment is invalid because it cannot be in an expression, pattern, argument list, or other non-statement

indico/indico (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

ruff failed
  Cause: Rule `S410` was removed and cannot be selected.

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF028	2	2	0	0	0

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 2 project errors)

indico/indico (error)

ruff failed
  Cause: Rule `S410` was removed and cannot be selected.

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/How_to_handle_rate_limits.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: trailing comma at line 47 column 4

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 2 project errors)

indico/indico (error)

ruff format --preview

ruff failed
  Cause: Rule `S410` was removed and cannot be selected.

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/How_to_handle_rate_limits.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: trailing comma at line 47 column 4

MichaReiser

I really like how you narrowed down the scope of the rule. This looks good to me. There's one issue around suppression comments in nodes that aren't expressions that we need solving but this is ready otherwise

…mments on function definitions

…tral-sh#9899)  Fixes astral-sh#6611 ## Summary This lint rule spots comments that are _intended_ to suppress or enable the formatter, but will be ignored by the Ruff formatter. We borrow some functions the formatter uses for determining comment placement / putting them in context within an AST. The analysis function uses an AST visitor to visit each comment and attach it to the AST. It then uses that context to check: 1. Is this comment in an expression? 2. Does this comment have bad placement? (e.g. a `# fmt: skip` above a function instead of at the end of a line) 3. Is this comment redundant? 4. Does this comment actually suppress any code? 5. Does this comment have ambiguous placement? (e.g. a `# fmt: off` above an `else:` block) If any of these are true, a violation is thrown. The reported reason depends on the order of the above check-list: in other words, a `# fmt: skip` comment on its own line within a list expression will be reported as being in an expression, since that reason takes priority. The lint suggests removing the comment as an unsafe fix, regardless of the reason. ## Test Plan A snapshot test has been created.

snowsignal requested a review from MichaReiser February 8, 2024 19:25

MichaReiser mentioned this pull request Feb 13, 2024

Add example demonstrating that fmt: skip on expression level is not supported #9973

Merged

snowsignal force-pushed the jane/formatter/useless-noqa branch from 2de4f0a to 4864875 Compare February 14, 2024 20:15

snowsignal marked this pull request as ready for review February 14, 2024 22:53

MichaReiser reviewed Feb 15, 2024

View reviewed changes

MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Feb 15, 2024

MichaReiser requested changes Feb 15, 2024

View reviewed changes

snowsignal force-pushed the jane/formatter/useless-noqa branch from 04deb9a to 9b87600 Compare February 23, 2024 19:21

snowsignal added 9 commits February 23, 2024 11:22

Move SuppressionKind and CommentLinePosition to ruff_python_trivia

d3bc248

Draft implementation of RUF028. Several edge cases still need to be r…

fd65789

…esolved, particularly with dangling comments.

Continuing implementation of RUF028. Several edge cases have been add…

f413bd5

…ressed and several new problems are now reported. Edge cases involving dangling comments still need to be handled

Correctly attach end-of-body comments to the body and ignore invalid …

61d7e10

…comments when determining suppression state

Fix parse error in RUF028 docs

dcd9fef

Update ruff.schema.json

b1539af

Revise the language and messages for RUF028

809d182

Fix formatting

03db750

Address some suggestions

0271500

snowsignal force-pushed the jane/formatter/useless-noqa branch from 9b87600 to dc27643 Compare February 23, 2024 19:24

snowsignal requested a review from MichaReiser February 23, 2024 19:24

Address remaining suggestions and reduce scope of lint by removing ru…

dad765d

…le checks that need contextual information

snowsignal force-pushed the jane/formatter/useless-noqa branch from dc27643 to dad765d Compare February 23, 2024 20:04

Fix inverted logic for is_suppression_on/is_suppression_off

5ab92d8

MichaReiser approved these changes Feb 26, 2024

View reviewed changes

MichaReiser added this to the v0.3.0 milestone Feb 27, 2024

snowsignal added 2 commits February 27, 2024 11:37

Address suggestions

5b80da0

Fix trailing comment visitation and add coverage for trailing skip co…

8fc186e

…mments on function definitions

snowsignal force-pushed the jane/formatter/useless-noqa branch from 5e1ea63 to 8fc186e Compare February 28, 2024 19:14

snowsignal enabled auto-merge (squash) February 28, 2024 19:15

snowsignal merged commit 0293908 into main Feb 28, 2024

snowsignal deleted the jane/formatter/useless-noqa branch February 28, 2024 19:21

MichaReiser mentioned this pull request Feb 29, 2024

Fix ecosystem check for indico #10164

Merged

jmcivor-dk mentioned this pull request Apr 1, 2025

Detect unused # fmt: off & # fmt: on comments #17098

Closed

VascoSch92 mentioned this pull request Apr 19, 2025

Document fix safety #15584

Open

71 tasks

Conversation

snowsignal commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

snowsignal commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Draft Checklist

Uh oh!

codspeed-hq Bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #9899 will not alter performance

Summary

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snowsignal commented Feb 8, 2024 •

edited

Loading

snowsignal commented Feb 8, 2024 •

edited

Loading

codspeed-hq Bot commented Feb 14, 2024 •

edited

Loading

github-actions Bot commented Feb 23, 2024 •

edited

Loading

`ruff-ecosystem` results