Skip to content

Commit 98b95c9

Browse files
Implement Invalid rule provided as rule RUF102 with --fix (#17138)
<!-- Thank you for contributing to Ruff! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? --> Closes #17084 ## 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: 1. Analyzes all `noqa` directives to identify invalid rule codes 2. Provides autofix functionality to: - Remove the entire comment if all codes are invalid - Remove only the invalid codes when mixed with valid codes 3. 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. <!-- How was it tested? --> ## 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. --------- Co-authored-by: Micha Reiser <[email protected]>
1 parent a4ba10f commit 98b95c9

File tree

9 files changed

+605
-0
lines changed

9 files changed

+605
-0
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# Invalid code
2+
import os # noqa: INVALID123
3+
# External code
4+
import re # noqa: V123
5+
# Valid noqa
6+
import sys # noqa: E402
7+
from functools import cache # Preceeding comment # noqa: F401, INVALID456
8+
from itertools import product # Preceeding comment # noqa: INVALID789
9+
# Succeeding comment
10+
import math # noqa: INVALID000 # Succeeding comment
11+
# Mixed valid and invalid
12+
from typing import List # noqa: F401, INVALID123
13+
# Test for multiple invalid
14+
from collections import defaultdict # noqa: INVALID100, INVALID200, F401
15+
# Test for preserving valid codes when fixing
16+
from itertools import chain # noqa: E402, INVALID300, F401
17+
# Test for mixed code types
18+
import json # noqa: E402, INVALID400, V100

crates/ruff_linter/src/checkers/noqa.rs

+7
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,13 @@ pub(crate) fn check_noqa(
227227
);
228228
}
229229

230+
if settings.rules.enabled(Rule::InvalidRuleCode)
231+
&& !per_file_ignores.contains(Rule::InvalidRuleCode)
232+
&& !exemption.enumerates(Rule::InvalidRuleCode)
233+
{
234+
ruff::rules::invalid_noqa_code(diagnostics, &noqa_directives, locator, &settings.external);
235+
}
236+
230237
ignored_diagnostics.sort_unstable();
231238
ignored_diagnostics
232239
}

crates/ruff_linter/src/codes.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1016,6 +1016,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
10161016
(Ruff, "059") => (RuleGroup::Preview, rules::ruff::rules::UnusedUnpackedVariable),
10171017
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
10181018
(Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA),
1019+
(Ruff, "102") => (RuleGroup::Preview, rules::ruff::rules::InvalidRuleCode),
10191020

10201021
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
10211022
#[cfg(any(feature = "test-rules", test))]

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

+15
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ mod tests {
9999
#[test_case(Rule::UnusedUnpackedVariable, Path::new("RUF059_3.py"))]
100100
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101_0.py"))]
101101
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101_1.py"))]
102+
#[test_case(Rule::InvalidRuleCode, Path::new("RUF102.py"))]
102103
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
103104
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
104105
let diagnostics = test_path(
@@ -314,6 +315,20 @@ mod tests {
314315
Ok(())
315316
}
316317

318+
#[test]
319+
fn invalid_rule_code_external_rules() -> Result<()> {
320+
let diagnostics = test_path(
321+
Path::new("ruff/RUF102.py"),
322+
&settings::LinterSettings {
323+
external: vec!["V".to_string()],
324+
..settings::LinterSettings::for_rule(Rule::InvalidRuleCode)
325+
},
326+
)?;
327+
328+
assert_messages!(diagnostics);
329+
Ok(())
330+
}
331+
317332
#[test]
318333
fn ruff_per_file_ignores() -> Result<()> {
319334
let diagnostics = test_path(
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
use crate::noqa::{Code, Directive};
2+
use crate::registry::Rule;
3+
use crate::Locator;
4+
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
5+
use ruff_macros::{derive_message_formats, ViolationMetadata};
6+
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
7+
8+
use crate::noqa::{Codes, NoqaDirectives};
9+
10+
/// ## What it does
11+
/// Checks for `noqa` codes that are invalid.
12+
///
13+
/// ## Why is this bad?
14+
/// Invalid rule codes serve no purpose and may indicate outdated code suppressions.
15+
///
16+
/// ## Example
17+
/// ```python
18+
/// import os # noqa: XYZ999
19+
/// ```
20+
///
21+
/// Use instead:
22+
/// ```python
23+
/// import os
24+
/// ```
25+
///
26+
/// Or if there are still valid codes needed:
27+
/// ```python
28+
/// import os # noqa: E402
29+
/// ```
30+
///
31+
/// ## Options
32+
/// - `lint.external`
33+
#[derive(ViolationMetadata)]
34+
pub(crate) struct InvalidRuleCode {
35+
pub(crate) rule_code: String,
36+
}
37+
38+
impl AlwaysFixableViolation for InvalidRuleCode {
39+
#[derive_message_formats]
40+
fn message(&self) -> String {
41+
format!("Invalid rule code in `# noqa`: {}", self.rule_code)
42+
}
43+
44+
fn fix_title(&self) -> String {
45+
"Remove the rule code".to_string()
46+
}
47+
}
48+
49+
/// RUF102 for invalid noqa codes
50+
pub(crate) fn invalid_noqa_code(
51+
diagnostics: &mut Vec<Diagnostic>,
52+
noqa_directives: &NoqaDirectives,
53+
locator: &Locator,
54+
external: &[String],
55+
) {
56+
for line in noqa_directives.lines() {
57+
let Directive::Codes(directive) = &line.directive else {
58+
continue;
59+
};
60+
61+
let all_valid = directive.iter().all(|code| code_is_valid(code, external));
62+
63+
if all_valid {
64+
continue;
65+
}
66+
67+
let (valid_codes, invalid_codes): (Vec<_>, Vec<_>) = directive
68+
.iter()
69+
.partition(|&code| code_is_valid(code, external));
70+
71+
if valid_codes.is_empty() {
72+
diagnostics.push(all_codes_invalid_diagnostic(directive, invalid_codes));
73+
} else {
74+
diagnostics.extend(invalid_codes.into_iter().map(|invalid_code| {
75+
some_codes_are_invalid_diagnostic(directive, invalid_code, locator)
76+
}));
77+
}
78+
}
79+
}
80+
81+
fn code_is_valid(code: &Code, external: &[String]) -> bool {
82+
let code_str = code.as_str();
83+
Rule::from_code(code_str).is_ok() || external.iter().any(|ext| code_str.starts_with(ext))
84+
}
85+
86+
fn all_codes_invalid_diagnostic(
87+
directive: &Codes<'_>,
88+
invalid_codes: Vec<&Code<'_>>,
89+
) -> Diagnostic {
90+
Diagnostic::new(
91+
InvalidRuleCode {
92+
rule_code: invalid_codes
93+
.into_iter()
94+
.map(Code::as_str)
95+
.collect::<Vec<_>>()
96+
.join(", "),
97+
},
98+
directive.range(),
99+
)
100+
.with_fix(Fix::safe_edit(Edit::range_deletion(directive.range())))
101+
}
102+
103+
fn some_codes_are_invalid_diagnostic(
104+
codes: &Codes,
105+
invalid_code: &Code,
106+
locator: &Locator,
107+
) -> Diagnostic {
108+
let diagnostic = Diagnostic::new(
109+
InvalidRuleCode {
110+
rule_code: invalid_code.to_string(),
111+
},
112+
invalid_code.range(),
113+
);
114+
diagnostic.with_fix(Fix::safe_edit(remove_invalid_noqa(
115+
codes,
116+
invalid_code,
117+
locator,
118+
)))
119+
}
120+
121+
fn remove_invalid_noqa(codes: &Codes, invalid_code: &Code, locator: &Locator) -> Edit {
122+
// Is this the first code after the `:` that needs to get deleted
123+
// For the first element, delete from after the `:` to the next comma (including)
124+
// For any other element, delete from the previous comma (including) to the next comma (excluding)
125+
let mut first = false;
126+
127+
// Find the index of the previous comma or colon.
128+
let start = locator
129+
.slice(TextRange::new(codes.start(), invalid_code.start()))
130+
.rmatch_indices([',', ':'])
131+
.next()
132+
.map(|(offset, text)| {
133+
let offset = codes.start() + TextSize::try_from(offset).unwrap();
134+
if text == ":" {
135+
first = true;
136+
// Don't include the colon in the deletion range, or the noqa comment becomes invalid
137+
offset + ':'.text_len()
138+
} else {
139+
offset
140+
}
141+
})
142+
.unwrap_or(invalid_code.start());
143+
144+
// Find the index of the trailing comma (if any)
145+
let end = locator
146+
.slice(TextRange::new(invalid_code.end(), codes.end()))
147+
.find(',')
148+
.map(|offset| {
149+
let offset = invalid_code.end() + TextSize::try_from(offset).unwrap();
150+
151+
if first {
152+
offset + ','.text_len()
153+
} else {
154+
offset
155+
}
156+
})
157+
.unwrap_or(invalid_code.end());
158+
159+
Edit::range_deletion(TextRange::new(start, end))
160+
}

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

+2
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ pub(crate) use invalid_assert_message_literal_argument::*;
1919
pub(crate) use invalid_formatter_suppression_comment::*;
2020
pub(crate) use invalid_index_type::*;
2121
pub(crate) use invalid_pyproject_toml::*;
22+
pub(crate) use invalid_rule_code::*;
2223
pub(crate) use map_int_version_parsing::*;
2324
pub(crate) use missing_fstring_syntax::*;
2425
pub(crate) use mutable_class_default::*;
@@ -78,6 +79,7 @@ mod invalid_assert_message_literal_argument;
7879
mod invalid_formatter_suppression_comment;
7980
mod invalid_index_type;
8081
mod invalid_pyproject_toml;
82+
mod invalid_rule_code;
8183
mod map_int_version_parsing;
8284
mod missing_fstring_syntax;
8385
mod mutable_class_default;

0 commit comments

Comments
 (0)