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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation55 Commits13 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

snowsignal

Fixes #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

Draft Checklist

@codspeed-hq CodSpeed HQ

MichaReiser

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@snowsignal

@snowsignal

…esolved, particularly with dangling comments.

@snowsignal

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

@snowsignal

…comments when determining suppression state

@snowsignal

@snowsignal

@snowsignal

@snowsignal

@snowsignal

@snowsignal

…le checks that need contextual information

@snowsignal

@github-actions GitHub Actions

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@snowsignal

@snowsignal

…mments on function definitions

nkxxll pushed a commit to nkxxll/ruff that referenced this pull request

Mar 10, 2024

@snowsignal @nkxxll

…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.

Labels

preview

Related to preview mode features

rule

Implementing or modifying a lint rule

2 participants

@snowsignal @MichaReiser