[ruff] Fix for RUF102 should delete entire comment

[amyreese]

Prevents generating invalid syntax for noqa comments with trailing text

Fixes #23349

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[dylwil3]

I'm not sure about this... could we delete just the noqa: INVALID123 but not the leading # if there is text after?

At the very least it feels like the fix should be unsafe in this case?

(sort of surprised it isn't always unsafe, to be honest)

[amyreese]

FWIW, this matches the behavior of the RUF100 fix when all codes are unused, which also deletes the entire comment.

[ntBre]

If I remember correctly, we assume the rest of the text is the "reason" for the noqa, which is why we delete it too. There's a (possibly closed) issue about this around somewhere. #20762 and #12251, for example.

[amyreese]

Also worth noting that currently this fix is only relevant if the user has RUF100 disabled, because otherwise that rule would also flag this line and offer a fix to remove the entire comment:

RUF100:

amethyst@lunatone ~/workspace/ruff amy/ruf102-reasons » ruff-check --isolated --select RUF100 --diff ../test.py
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s
     Running `target/debug/ruff check --no-cache --isolated --select RUF100 --diff ../test.py`
warning: Invalid rule code provided to `# noqa` at /Users/amethyst/workspace/test.py:3: TK412
--- /Users/amethyst/workspace/test.py
+++ /Users/amethyst/workspace/test.py
@@ -1,3 +1,3 @@
-pass  # noqa: E501 some reason
+pass

-pass  # noqa: TK412 some reason
+pass

Would fix 2 errors.
> [1]

RUF102:

amethyst@lunatone ~/workspace/ruff amy/ruf102-reasons » ruff-check --isolated --select RUF102 --diff ../test.py
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/ruff check --no-cache --isolated --select RUF102 --diff ../test.py`
warning: Invalid rule code provided to `# noqa` at /Users/amethyst/workspace/test.py:3: TK412
--- /Users/amethyst/workspace/test.py
+++ /Users/amethyst/workspace/test.py
@@ -1,3 +1,3 @@
 pass  # noqa: E501 some reason

-pass  # noqa: TK412 some reason
+pass

Would fix 1 error.
> [1]

[dylwil3]

Somehow the situation of RUF102 feels slightly different than that of RUF100. In RUF100, we're pretty confident it's one of our rules being suppressed, and we helpfully see it's not doing anything. In this case we think we've identified some wrong code, but it may be serving some purpose unknown to us.

But I am happy to be overruled!

Maybe we should separately put something like this https://docs.astral.sh/ruff/rules/unused-noqa/#conflict-with-other-linters in the docs for RUF102 as well? (can be a separate PR)

[amyreese]

In RUF100, we're pretty confident it's one of our rules being suppressed, and we helpfully see it's not doing anything.

Take a look at my examples above. RUF100 will fire for a noqa with an "unknown" code, and suggest to fix it by removing the code and/or comment. This is a behavior overlap with RUF102, to be clear.

[dylwil3]

Hmm... well that seems weird. Should we remove that behavior from RUF100? Sorry this discussion keeps sprawling in scope.

[amyreese]

Also considered that this matches the behavior for how the new range suppressions fix RUF100/102/103 errors, where they also assume anything up to # (the next comment node) is a "reason" and remove it as part of the fix.

[ntBre]

Thanks! This makes sense to me, and it's nice to reuse delete_comment and clean up some of the whitespace issues in the previous fix. Should we add a test case specifically for the syntax error?

We may want to open follow-up issues for some of the other discussion here, but I think this is fine to land to resolve the syntax error.