Skip to content

Commit f1d4ba3

Browse files
authored
Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061)
Closes #17042 ## Summary This PR fixes the issue outlined in #17042 where RUF100 (unused-noqa) fails to detect unused file-level noqa directives (`# ruff: noqa` or `# ruff: noqa: {code}`). The issue stems from two underlying causes: 1. For blanket file-level directives (`# ruff: noqa`), there's a circular dependency: the directive exempts all rules including RUF100 itself, which prevents checking for usage. This isn't changed by this PR. I would argue it is intendend behavior - a blanket `# ruff: noqa` directive should exempt all rules including RUF100 itself. 2. For code-specific file-level directives (e.g. `# ruff: noqa: F841`), the handling was missing in the `check_noqa` function. This is added in this PR. ## Notes - For file-level directives, the `matches` array is pre-populated with the specified codes during parsing, unlike line-level directives which only populate their `matches` array when actually suppressing diagnostics. This difference requires the somewhat clunky handling of both cases. I would appreciate guidance on a cleaner design :) - A more fundamental solution would be to change how file-level directives initialize the `matches` array in `FileNoqaDirectives::extract()`, but that requires more substantial changes as it breaks existing functionality. I suspect discussions in #16483 are relevant for this. ## Test Plan - Local verification - Added a test case and fixture
1 parent 83a9723 commit f1d4ba3

File tree

6 files changed

+134
-14
lines changed

6 files changed

+134
-14
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
# ruff: noqa: F841 -- intentional unused file directive; will be removed
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# flake8: noqa: F841, E501 -- used followed by unused code
2+
# ruff: noqa: E701, F541 -- unused followed by used code
3+
4+
5+
def a():
6+
# Violating noqa'd F841 (unused) and F541 (no-placeholder f-string)
7+
x = f"Hello, world"
8+
return 6

crates/ruff_linter/src/checkers/noqa.rs

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,29 @@ pub(crate) fn check_noqa(
110110
&& !exemption.includes(Rule::UnusedNOQA)
111111
&& !per_file_ignores.contains(Rule::UnusedNOQA)
112112
{
113-
for line in noqa_directives.lines() {
114-
match &line.directive {
113+
let directives: Vec<_> = if settings.preview.is_enabled() {
114+
noqa_directives
115+
.lines()
116+
.iter()
117+
.map(|line| (&line.directive, &line.matches, false))
118+
.chain(
119+
file_noqa_directives
120+
.lines()
121+
.iter()
122+
.map(|line| (&line.parsed_file_exemption, &line.matches, true)),
123+
)
124+
.collect()
125+
} else {
126+
noqa_directives
127+
.lines()
128+
.iter()
129+
.map(|line| (&line.directive, &line.matches, false))
130+
.collect()
131+
};
132+
for (directive, matches, is_file_level) in directives {
133+
match directive {
115134
Directive::All(directive) => {
116-
if line.matches.is_empty() {
135+
if matches.is_empty() {
117136
let edit = delete_comment(directive.range(), locator);
118137
let mut diagnostic =
119138
Diagnostic::new(UnusedNOQA { codes: None }, directive.range());
@@ -137,17 +156,21 @@ pub(crate) fn check_noqa(
137156
break;
138157
}
139158

140-
if !seen_codes.insert(original_code) {
141-
duplicated_codes.push(original_code);
142-
} else if line.matches.iter().any(|match_| *match_ == code)
143-
|| settings
159+
if seen_codes.insert(original_code) {
160+
let is_code_used = if is_file_level {
161+
diagnostics
162+
.iter()
163+
.any(|diag| diag.kind.rule().noqa_code() == code)
164+
} else {
165+
matches.iter().any(|match_| *match_ == code)
166+
} || settings
144167
.external
145168
.iter()
146-
.any(|external| code.starts_with(external))
147-
{
148-
valid_codes.push(original_code);
149-
} else {
150-
if let Ok(rule) = Rule::from_code(code) {
169+
.any(|external| code.starts_with(external));
170+
171+
if is_code_used {
172+
valid_codes.push(original_code);
173+
} else if let Ok(rule) = Rule::from_code(code) {
151174
if settings.rules.enabled(rule) {
152175
unmatched_codes.push(original_code);
153176
} else {
@@ -156,6 +179,8 @@ pub(crate) fn check_noqa(
156179
} else {
157180
unknown_codes.push(original_code);
158181
}
182+
} else {
183+
duplicated_codes.push(original_code);
159184
}
160185
}
161186

@@ -171,8 +196,18 @@ pub(crate) fn check_noqa(
171196
let edit = if valid_codes.is_empty() {
172197
delete_comment(directive.range(), locator)
173198
} else {
199+
let original_text = locator.slice(directive.range());
200+
let prefix = if is_file_level {
201+
if original_text.contains("flake8") {
202+
"# flake8: noqa: "
203+
} else {
204+
"# ruff: noqa: "
205+
}
206+
} else {
207+
"# noqa: "
208+
};
174209
Edit::range_replacement(
175-
format!("# noqa: {}", valid_codes.join(", ")),
210+
format!("{}{}", prefix, valid_codes.join(", ")),
176211
directive.range(),
177212
)
178213
};

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

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,37 @@ mod tests {
315315
Ok(())
316316
}
317317

318+
#[test]
319+
fn ruff_noqa_filedirective_unused() -> Result<()> {
320+
let diagnostics = test_path(
321+
Path::new("ruff/RUF100_6.py"),
322+
&settings::LinterSettings {
323+
preview: PreviewMode::Enabled,
324+
..settings::LinterSettings::for_rules(vec![Rule::UnusedNOQA])
325+
},
326+
)?;
327+
assert_messages!(diagnostics);
328+
Ok(())
329+
}
330+
331+
#[test]
332+
fn ruff_noqa_filedirective_unused_last_of_many() -> Result<()> {
333+
let diagnostics = test_path(
334+
Path::new("ruff/RUF100_7.py"),
335+
&settings::LinterSettings {
336+
preview: PreviewMode::Enabled,
337+
..settings::LinterSettings::for_rules(vec![
338+
Rule::UnusedNOQA,
339+
Rule::FStringMissingPlaceholders,
340+
Rule::LineTooLong,
341+
Rule::UnusedVariable,
342+
])
343+
},
344+
)?;
345+
assert_messages!(diagnostics);
346+
Ok(())
347+
}
348+
318349
#[test]
319350
fn invalid_rule_code_external_rules() -> Result<()> {
320351
let diagnostics = test_path(
@@ -324,7 +355,6 @@ mod tests {
324355
..settings::LinterSettings::for_rule(Rule::InvalidRuleCode)
325356
},
326357
)?;
327-
328358
assert_messages!(diagnostics);
329359
Ok(())
330360
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
source: crates/ruff_linter/src/rules/ruff/mod.rs
3+
---
4+
RUF100_6.py:1:1: RUF100 [*] Unused `noqa` directive (non-enabled: `F841`)
5+
|
6+
1 | # ruff: noqa: F841 -- intentional unused file directive; will be removed
7+
| ^^^^^^^^^^^^^^^^^^ RUF100
8+
|
9+
= help: Remove unused `noqa` directive
10+
11+
Safe fix
12+
1 |-# ruff: noqa: F841 -- intentional unused file directive; will be removed
13+
1 |+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
source: crates/ruff_linter/src/rules/ruff/mod.rs
3+
---
4+
RUF100_7.py:1:1: RUF100 [*] Unused `noqa` directive (unused: `E501`)
5+
|
6+
1 | # flake8: noqa: F841, E501 -- used followed by unused code
7+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF100
8+
2 | # ruff: noqa: E701, F541 -- unused followed by used code
9+
|
10+
= help: Remove unused `noqa` directive
11+
12+
Safe fix
13+
1 |-# flake8: noqa: F841, E501 -- used followed by unused code
14+
1 |+# flake8: noqa: F841 -- used followed by unused code
15+
2 2 | # ruff: noqa: E701, F541 -- unused followed by used code
16+
3 3 |
17+
4 4 |
18+
19+
RUF100_7.py:2:1: RUF100 [*] Unused `noqa` directive (non-enabled: `E701`)
20+
|
21+
1 | # flake8: noqa: F841, E501 -- used followed by unused code
22+
2 | # ruff: noqa: E701, F541 -- unused followed by used code
23+
| ^^^^^^^^^^^^^^^^^^^^^^^^ RUF100
24+
|
25+
= help: Remove unused `noqa` directive
26+
27+
Safe fix
28+
1 1 | # flake8: noqa: F841, E501 -- used followed by unused code
29+
2 |-# ruff: noqa: E701, F541 -- unused followed by used code
30+
2 |+# ruff: noqa: F541 -- unused followed by used code
31+
3 3 |
32+
4 4 |
33+
5 5 | def a():

0 commit comments

Comments
 (0)