Implement `Invalid rule provided` as rule RUF102 with `--fix` #17138

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.

MichaReiser · 2025-04-02T06:44:48Z

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

+        };
+
+        let mut invalid_code_refs = vec![];
+        let mut valid_codes = vec![];


Having to always collect all codes only to be able to generate the fix isn't ideal for performance (this requires a fair amount of writes).

I'd rewrite this to only track if all codes are invalid and then re-iterate over the noqa codes when generating the fix and skip over the known invalid codes.

Adressed in 52abe14

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.

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

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

+
+        for code in directive.iter() {
+            let code_str = code.as_str();
+            if Rule::from_code(code.as_str()).is_err() {


Can you test that this rule respects https://docs.astral.sh/ruff/settings/#lint_external

It didn't. I added a fix (3f50765) and test (e734d3d).

MichaReiser · 2025-04-02T06:48:28Z

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

+        let prefix = &original_text[..prefix_end];
+        let codes_part = &original_text[prefix_end..];


Nit: I think there's a split_at(prefix_end) method that gives you both the before and after parts

Adressed in 2be49a0

MichaReiser · 2025-04-02T06:49:18Z

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

+        format!(
+            "{}{}{}",
+            prefix,
+            &codes_part[..whitespace_len],


This will panic if there is any non-ASCII whitespace because you pass the character count instead of the length in bytes. You'll have to use find or similar to find the end.

Adressed in e3f4e07

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

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

@@ -314,6 +315,20 @@ mod tests {
        Ok(())
    }

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


NIt:

Suggested change

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

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

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

MichaReiser · 2025-04-03T10:18:22Z

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

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


It would be great to link to the external setting similar to unused-noqa: https://docs.astral.sh/ruff/rules/unused-noqa/

MichaReiser · 2025-04-03T10:19:36Z

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

+            if external.iter().any(|ext| code_str.starts_with(ext))
+                || Rule::from_code(code.as_str()).is_ok()


Suggested change

if external.iter().any(|ext| code_str.starts_with(ext))

|| Rule::from_code(code.as_str()).is_ok()

if Rule::from_code(code.as_str()).is_ok() || external.iter().any(|ext| code_str.starts_with(ext))

This might be faster because it checks the common case first.

MichaReiser · 2025-04-03T10:19:51Z

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

+            if external.iter().any(|ext| code_str.starts_with(ext))
+                || Rule::from_code(code.as_str()).is_ok()


Suggested change

if external.iter().any(|ext| code_str.starts_with(ext))

|| Rule::from_code(code.as_str()).is_ok()

if external.iter().any(|ext| code_str.starts_with(ext))

|| Rule::from_code(code_str)).is_ok()

MichaReiser · 2025-04-03T10:20:28Z

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

+                    if external.iter().any(|ext| code_str.starts_with(ext))
+                        || Rule::from_code(code_str).is_ok()


Nit: Maybe consider moving this to a small helper function and use it here and when checking above.

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?

MichaReiser · 2025-04-03T10:22:11Z

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

+    }
+}
+
+fn handle_all_codes_invalid(diagnostics: &mut Vec<Diagnostic>, directive: &Codes<'_>) {


Nit: To reduce the number of arguments:

Suggested change

fn handle_all_codes_invalid(diagnostics: &mut Vec<Diagnostic>, directive: &Codes<'_>) {

fn create_all_codes_invalid_diagnostic(directive: &Codes<'_>) -> Diagnostic {

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Invalid rule provided` as rule RUF102 with `--fix` #17138

Implement `Invalid rule provided` as rule RUF102 with `--fix` #17138

maxmynter commented Apr 2, 2025

github-actions bot commented Apr 2, 2025 •

edited

Loading

maxmynter commented Apr 2, 2025 •

edited

Loading

MichaReiser Apr 2, 2025

maxmynter Apr 2, 2025

MichaReiser Apr 2, 2025

maxmynter Apr 2, 2025

MichaReiser Apr 2, 2025

maxmynter Apr 2, 2025

MichaReiser Apr 2, 2025

maxmynter Apr 2, 2025

MichaReiser Apr 2, 2025

maxmynter Apr 2, 2025

MichaReiser Apr 3, 2025 •

edited

Loading

MichaReiser left a comment

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

MichaReiser Apr 3, 2025

		let prefix = &original_text[..prefix_end];
		let codes_part = &original_text[prefix_end..];

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

		if external.iter().any(\|ext\| code_str.starts_with(ext))
		\|\| Rule::from_code(code.as_str()).is_ok()

	if external.iter().any(\|ext\| code_str.starts_with(ext))
	\|\| Rule::from_code(code.as_str()).is_ok()
	if Rule::from_code(code.as_str()).is_ok() \|\| external.iter().any(\|ext\| code_str.starts_with(ext))

		if external.iter().any(\|ext\| code_str.starts_with(ext))
		\|\| Rule::from_code(code_str).is_ok()

	fn handle_all_codes_invalid(diagnostics: &mut Vec<Diagnostic>, directive: &Codes<'_>) {
	fn create_all_codes_invalid_diagnostic(directive: &Codes<'_>) -> Diagnostic {

Implement Invalid rule provided as rule RUF102 with --fix #17138

Are you sure you want to change the base?

Implement Invalid rule provided as rule RUF102 with --fix #17138

Conversation

maxmynter commented Apr 2, 2025

Summary

Test Plan

Notes

Questions

github-actions bot commented Apr 2, 2025 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

maxmynter commented Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `Invalid rule provided` as rule RUF102 with `--fix` #17138

Implement `Invalid rule provided` as rule RUF102 with `--fix` #17138

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