Implement `Invalid rule provided` as rule RUF102 with `--fix` by maxmynter · Pull Request #17138 · astral-sh/ruff

maxmynter · 2025-04-02T03:20:33Z

Summary

This PR adds a new rule (RUF102) to detect and fix invalid rule codes in noqa comments.
Invalid rule codes in noqa directives serve no purpose and may indicate outdated code suppressions.

This extends the previous behaviour originating from crates/ruff_linter/src/noqa.rs which would only emit a warnigs.
With this rule a --fix is available.

The rule:

Analyzes all noqa directives to identify invalid rule codes
Provides autofix functionality to:
- Remove the entire comment if all codes are invalid
- Remove only the invalid codes when mixed with valid codes
Preserves original comment formatting and whitespace where possible

Example cases:

# noqa: XYZ111 → Remove entire comment (keep empty line)
# noqa: XYZ222, XYZ333 → Remove entire comment (keep empty line)
# noqa: F401, INVALID123 → Keep only valid codes (# noqa: F401)

Test Plan

Added tests in crates/ruff_linter/resources/test/fixtures/ruff/RUF102.py covering different example cases.

Notes

This does not handle cases where parsing fails. E.g. # noqa: NON_EXISTENT, ANOTHER_INVALID causes a LexicalError and the diagnostic is not propagated and we cannot handle the diagnostic. I am also unsure what proper fix handling would be and making the user aware we don't understand the codes is probably the best bet.
The rule is added to the Preview rule group as it's a new addition

Questions

Should we remove the warnings, now that we have a rule?
Is the current fix behavior appropriate for all cases, particularly the handling of whitespace and line deletions?
I'm new to the codebase; let me know if there are rule utilities which could have used but didn't.

github-actions · 2025-04-02T03:27:43Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

apache/airflow (+2 -0 violations, +0 -0 fixes)

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

+ providers/edge/src/airflow/providers/edge/worker_api/auth.py:34:78: RUF102 [*] Invalid rule code in `# noqa`: TCH001
+ providers/edge/src/airflow/providers/edge/worker_api/datamodels.py:28:72: RUF102 [*] Invalid rule code in `# noqa`: TCH001

Changes by rule (1 rules affected)

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

crates/ruff_linter/src/rules/ruff/mod.rs

maxmynter · 2025-04-02T03:46:45Z

Sorry for the CI issues, I thought that clippy runs with checking the pre-commit hooks.

~~I thought I generated the docs with cargo dev generate-all and they are in the comment in the rule file. Am I supposed to do that differently?~~
Edit: I missed a # and had the wrong header level in the docstring. It should work now.

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

MichaReiser · 2025-04-02T06:46:04Z

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

+    );
+
+    let original_text = locator.slice(line.range());
+    if let Some(comment_start) = original_text.find('#') {


Does this work with # test # noqa: invalid? Could we use line.directive.range (assuming directive is the Codes variant) here?

Thanks for the directive suggestion. It's much cleaner (adressed in f7014db).

Regarding multiple comments on a line it removes the invalid parts

# test # noqa: INVALID111 → # test

# test # noqa: INVALID111, VALID111 → # test # noqa: VALID111

I've added tests for this in 5cc8305.

I guess this is the desired behaviour. Having stale comments isn't nice, but auto removing ones that should stay is worse. What do you think.

Note: I'm writing it with uppercase ASCII plus a number because otherwise it's not parsed as a rule code; in this case the comment remains untouched.

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

MichaReiser · 2025-04-03T10:16:31Z

crates/ruff_linter/src/rules/ruff/mod.rs

    }

+    #[test]
+    fn ruf102() -> Result<()> {


NIt:

Suggested change

fn ruf102() -> Result<()> {

fn invalid_rule_code_external_rules() -> Result<()> {

Adressed in 3050304

MichaReiser

Thanks for incorporating external. This mostly looks good. The main thing we've to figure out is how to handle noqa comments where more than one code is invalid. Maybe @dylwil3 has an opinion on this?

MichaReiser · 2025-04-03T10:17:04Z

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

+/// ```python
+/// import os  # noqa: E402
+/// ```
+


Suggested change

Adressed in 2fe49fb

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

MichaReiser · 2025-04-03T10:21:39Z

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

+    let invalid_codes = directive
+        .iter()
+        .map(crate::noqa::Code::as_str)
+        .collect::<Vec<_>>()


Could we reuse the collected invalid_code_refs and pass them to this function instead of collecting them again?

We need to loop over the directive if we want to preserve the total order rule with only filtering codes for single code fixes.

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

MichaReiser · 2025-04-03T10:27:30Z

crates/ruff_linter/src/rules/ruff/rules/invalid_rule_code.rs

+    for invalid_code in invalid_codes {
+        let mut diagnostic = Diagnostic::new(
+            InvalidRuleCode {
+                rule_code: invalid_code.as_str().to_string(),
+            },
+            invalid_code.range(),
+        );
+
+        diagnostic.set_fix(fix.clone());
+        diagnostics.push(diagnostic);
+    }


And another case for multispan diagnostics...

I think I'd be surprised that applying the fix for a specific code replaces all invalid codes and not just the one where I positioned my cursor on.

That's why I think we should either:

Only create one diagnostic that fixes all invalid codes at once (multispan diagnostics would be great)

Create a diagnostic for each invalid code, the fix only removes that one code.

I'm unsure what you mean by multispan diagnostics or what the exact requirements are. Is that already established in Ruff or something you plan on integrating.

In 921f066 i fixed the handling to create one diagnostic for each invalid code.

I'm unsure what you mean by multispan diagnostics or what the exact requirements are. Is that already established in Ruff or something you plan on integrating.

Sorry, it's something we plan on implementing. This was mainly directed at @ntBre

Yeah sorry I should have acknowledged and resolved this. I'm keeping a list of places to use multispan diagnostics once they're added to Ruff!

We could create a tracking issue for it which can then be referenced by the reviewers and the references itself becomes a list of known places which can be improved.

Oh yes, of course!

after changing the test name

We previously accepted reordering of the codes which is not desirable. also added a test for a succeeding comment.

* main: [red-knot] Empty tuple is always-falsy (#17213) Run fuzzer with `--preview` (#17210) Bump 0.11.4 (#17212) [syntax-errors] Allow `yield` in base classes and annotations (#17206) Don't skip visiting non-tuple slice in `typing.Annotated` subscripts (#17201) [red-knot] mypy_primer: do not specify Python version (#17200) [red-knot] Add `Type.definition` method (#17153) Implement `Invalid rule provided` as rule RUF102 with `--fix` (#17138) [red-knot] Add basic on-hover to playground and LSP (#17057) [red-knot] don't remove negations when simplifying constrained typevars (#17189) [minor] Fix extra semicolon for clippy (#17188) [syntax-errors] Invalid syntax in annotations (#17101) [syntax-errors] Duplicate attributes in match class pattern (#17186) [syntax-errors] Fix multiple assignment for class keyword argument (#17184) use astral-sh/cargo-dist instead (#17187) Enable overindented docs lint (#17182)

maxmynter · 2025-04-04T21:16:25Z

Thanks for merging @MichaReiser !

If you have the bandwith, I would love a short explanation on your rationale in e1998a1 (no worries if not).

I agree that it reads much nicer, so I'm curious if you have any tips / resources on how you think about intended changes so i can deliver more quality out of the gate :)

The warning that `--output-format` for the formatter requires preview mode is no longer needed since astral-sh#17138 stabilized formatter output format support. Closes astral-sh#23267 https://claude.ai/code/session_0151C636aixLhqvzG7BURQ7P

This warning is now obsolete since RUF102 was added as a proper lint rule in astral-sh#17138 to detect invalid rule codes in `# noqa` comments. Having both a warning and a lint rule for the same issue is redundant, and the warning cannot be suppressed via rule configuration. Also removes the now-unused `external` parameter from `NoqaDirectives::from_commented_ranges`, since it was only used by the removed warning logic. Closes astral-sh#23267 https://claude.ai/code/session_0151C636aixLhqvzG7BURQ7P

maxmynter added 5 commits April 1, 2025 22:59

Create rule RUF102 -- Invalid Rule Code -- in preview

203a514

Add tests for RUF102

437680d

Update Schema

d0eadbe

Integrate RUF102 into ruleset

223ae54

Apply RUF102 check in noqa handling

7a15b89

ntBre added the rule Implementing or modifying a lint rule label Apr 2, 2025

ntBre reviewed Apr 2, 2025

View reviewed changes

crates/ruff_linter/src/rules/ruff/mod.rs Outdated Show resolved Hide resolved

Satisfy Clippy

1ed6d76

MichaReiser added the preview Related to preview mode features label Apr 2, 2025

MichaReiser requested changes Apr 2, 2025

View reviewed changes

maxmynter added 11 commits April 2, 2025 11:54

(refactor) Simplify all_codes_invalid handling

f7014db

(tests) Test comment preceeding codes

5cc8305

(fixup) Respect external config in invalid rule codes rule

3f50765

(test) Respect external config in invalid rule codes

e734d3d

(tests) Add snapshot for External code in ignore invalid codes

c8a17fd

(refactor) Use 'split_at' for inline slicing

2be49a0

(fixup) Use find, not takewhile, to handle multibyte chars

e3f4e07

Implement two passes and only collect valid codes first pass

52abe14

Move applicable parts RUF102 to general rules test

d34476f

Remove stale snapshot

e044bf9

(fixup) Fix heading level in invalid noqa docs

22b774b

maxmynter requested a review from MichaReiser April 2, 2025 21:03

MichaReiser reviewed Apr 3, 2025

View reviewed changes

maxmynter requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners April 3, 2025 22:01

maxmynter added 3 commits April 3, 2025 18:04

Make test name more expressive

3050304

Add external reference to docstring

2fe49fb

Check common case first in external rule check

e9de061

maxmynter force-pushed the rule-noqa branch from 31bfc6c to e9de061 Compare April 3, 2025 22:04

This comment was marked as resolved.

Sign in to view

Use code_str variable

979cf98

carljm removed request for AlexWaygood, BurntSushi, carljm, dcreager, dhruvmanila and sharkdp April 3, 2025 22:43

maxmynter added 5 commits April 3, 2025 20:40

Update cago insta snapshot

ba56357

after changing the test name

Refactor rule fix handling

921f066

(chore) Wrestling with Clippy

f9d4cf3

Update / fix test cases

9397c2b

We previously accepted reordering of the codes which is not desirable. also added a test for a succeeding comment.

(chore) Remove unneccessary ;

c8a9fe7

maxmynter requested a review from MichaReiser April 4, 2025 03:59

MichaReiser added 2 commits April 4, 2025 10:00

Simplifications

e1998a1

Delete commented out code

636a364

MichaReiser approved these changes Apr 4, 2025

View reviewed changes

MichaReiser enabled auto-merge (squash) April 4, 2025 08:03

MichaReiser merged commit 98b95c9 into astral-sh:main Apr 4, 2025
21 checks passed

ntBre mentioned this pull request Apr 4, 2025

Use new diagnostic functionality #17203

Open

7 tasks

BrewTestBot mentioned this pull request Apr 4, 2025

ruff 0.11.4 Homebrew/homebrew-core#218282

Merged

maxmynter mentioned this pull request Apr 4, 2025

Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) #17061

Merged

	fn ruf102() -> Result<()> {
	fn invalid_rule_code_external_rules() -> Result<()> {

Comments

Conversation

maxmynter commented Apr 2, 2025

Summary

Test Plan

Notes

Questions

Uh oh!

github-actions bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Uh oh!

maxmynter commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

maxmynter commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Apr 2, 2025 •

edited

Loading

`ruff-ecosystem` results

maxmynter commented Apr 2, 2025 •

edited

Loading

MichaReiser Apr 3, 2025 •

edited

Loading