Skip to content

Commit 5bdf049

Browse files
amyreeseamyreese
committed
Reorganize check_suppressions to iterate over valid/errors only once
1 parent b90f418 commit 5bdf049

File tree

2 files changed

+80
-121
lines changed

2 files changed

+80
-121
lines changed

‎crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap‎

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs
77

88
--- Summary ---
99
Removed: 15
10-
Added: 23
10+
Added: 21
1111

1212
--- Removed ---
1313
E741 Ambiguous variable name: `I`
@@ -393,17 +393,6 @@ RUF104 Suppression comment without matching `#ruff:enable` comment
393393
|
394394

395395

396-
RUF104 Suppression comment without matching `#ruff:enable` comment
397-
--> suppressions.py:69:5
398-
|
399-
67 | # and the other should trigger an unused suppression diagnostic
400-
68 | # ruff: disable[F841]
401-
69 | # ruff: disable[F841]
402-
| ^^^^^^^^^^^^^^^^^^^^^
403-
70 | foo = 0
404-
|
405-
406-
407396
RUF100 [*] Unused suppression (unused: `F841`)
408397
--> suppressions.py:69:5
409398
|
@@ -505,17 +494,6 @@ help: Remove unused suppression
505494
84 |
506495

507496

508-
RUF104 Suppression comment without matching `#ruff:enable` comment
509-
--> suppressions.py:87:5
510-
|
511-
85 | def f():
512-
86 | # Multiple codes but none are used
513-
87 | # ruff: disable[E741, F401, F841]
514-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
515-
88 | print("hello")
516-
|
517-
518-
519497
RUF100 [*] Unused suppression (unused: `E741`)
520498
--> suppressions.py:87:21
521499
|

‎crates/ruff_linter/src/suppression.rs‎

Lines changed: 79 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use ruff_db::diagnostic::Diagnostic;
55
use ruff_diagnostics::{Edit, Fix};
66
use ruff_python_ast::token::{TokenKind, Tokens};
77
use ruff_python_ast::whitespace::indentation;
8+
use rustc_hash::FxHashSet;
89
use std::cell::Cell;
910
use std::{error::Error, fmt::Formatter};
1011
use thiserror::Error;
@@ -166,99 +167,94 @@ impl Suppressions {
166167
}
167168

168169
pub(crate) fn check_suppressions(&self, context: &LintContext, locator: &Locator) {
169-
if context.is_rule_enabled(Rule::UnusedNOQA) {
170-
let unused = self
171-
.valid
172-
.iter()
173-
.filter(|suppression| !suppression.used.get());
174-
175-
for suppression in unused {
176-
let Ok(rule) = Rule::from_code(&suppression.code) else {
177-
continue; // TODO: invalid code
178-
};
179-
for comment in &suppression.comments {
180-
let (range, edit) =
181-
Suppressions::delete_code_or_comment(locator, suppression, comment);
182-
183-
let codes = if context.is_rule_enabled(rule) {
184-
UnusedCodes {
185-
unmatched: vec![suppression.code.to_string()],
186-
..Default::default()
187-
}
188-
} else {
189-
UnusedCodes {
190-
disabled: vec![suppression.code.to_string()],
191-
..Default::default()
192-
}
170+
let mut unmatched_ranges = FxHashSet::default();
171+
for suppression in &self.valid {
172+
if !code_is_valid(&suppression.code, &context.settings().external) {
173+
// InvalidRuleCode
174+
if context.is_rule_enabled(Rule::InvalidRuleCode) {
175+
for comment in &suppression.comments {
176+
let (range, edit) =
177+
Suppressions::delete_code_or_comment(locator, suppression, comment);
178+
context
179+
.report_diagnostic(
180+
InvalidRuleCode {
181+
rule_code: suppression.code.to_string(),
182+
kind: InvalidRuleCodeKind::Suppression,
183+
},
184+
range,
185+
)
186+
.set_fix(Fix::safe_edit(edit));
187+
}
188+
}
189+
} else if !suppression.used.get() {
190+
// UnusedNOQA
191+
if context.is_rule_enabled(Rule::UnusedNOQA) {
192+
let Ok(rule) = Rule::from_code(
193+
get_redirect_target(&suppression.code).unwrap_or(&suppression.code),
194+
) else {
195+
continue; // should trigger InvalidRuleCode above
193196
};
194-
195-
context
196-
.report_diagnostic(
197-
UnusedNOQA {
198-
codes: Some(codes),
199-
kind: UnusedNOQAKind::Suppression,
200-
},
201-
range,
202-
)
203-
.set_fix(Fix::safe_edit(edit));
197+
for comment in &suppression.comments {
198+
let (range, edit) =
199+
Suppressions::delete_code_or_comment(locator, suppression, comment);
200+
201+
let codes = if context.is_rule_enabled(rule) {
202+
UnusedCodes {
203+
unmatched: vec![suppression.code.to_string()],
204+
..Default::default()
205+
}
206+
} else {
207+
UnusedCodes {
208+
disabled: vec![suppression.code.to_string()],
209+
..Default::default()
210+
}
211+
};
212+
213+
context
214+
.report_diagnostic(
215+
UnusedNOQA {
216+
codes: Some(codes),
217+
kind: UnusedNOQAKind::Suppression,
218+
},
219+
range,
220+
)
221+
.set_fix(Fix::safe_edit(edit));
222+
}
223+
}
224+
} else if suppression.comments.len() == 1 {
225+
// UnmatchedSuppressionComment
226+
let range = suppression.comments[0].range;
227+
if unmatched_ranges.insert(range) {
228+
context.report_diagnostic_if_enabled(UnmatchedSuppressionComment {}, range);
204229
}
205-
}
206-
207-
// treat comments with no codes as unused suppression
208-
for error in self
209-
.errors
210-
.iter()
211-
.filter(|error| error.kind == ParseErrorKind::MissingCodes)
212-
{
213-
context
214-
.report_diagnostic(
215-
UnusedNOQA {
216-
codes: Some(UnusedCodes::default()),
217-
kind: UnusedNOQAKind::Suppression,
218-
},
219-
error.range,
220-
)
221-
.set_fix(Fix::safe_edit(delete_comment(error.range, locator)));
222230
}
223231
}
224232

225-
if context.is_rule_enabled(Rule::InvalidRuleCode) {
226-
for suppression in self.valid.iter().filter(|suppression| {
227-
!code_is_valid(&suppression.code, &context.settings().external)
228-
}) {
229-
for comment in &suppression.comments {
230-
let (range, edit) =
231-
Suppressions::delete_code_or_comment(locator, suppression, comment);
232-
context
233-
.report_diagnostic(
234-
InvalidRuleCode {
235-
rule_code: suppression.code.to_string(),
236-
kind: InvalidRuleCodeKind::Suppression,
237-
},
238-
range,
239-
)
240-
.set_fix(Fix::safe_edit(edit));
233+
for error in &self.errors {
234+
// treat comments with no codes as unused suppression
235+
if error.kind == ParseErrorKind::MissingCodes {
236+
if let Some(mut diagnostic) = context.report_diagnostic_if_enabled(
237+
UnusedNOQA {
238+
codes: Some(UnusedCodes::default()),
239+
kind: UnusedNOQAKind::Suppression,
240+
},
241+
error.range,
242+
) {
243+
diagnostic.set_fix(Fix::safe_edit(delete_comment(error.range, locator)));
244+
}
245+
} else {
246+
if let Some(mut diagnostic) = context.report_diagnostic_if_enabled(
247+
InvalidSuppressionComment {
248+
kind: InvalidSuppressionCommentKind::Error(error.kind),
249+
},
250+
error.range,
251+
) {
252+
diagnostic.set_fix(Fix::unsafe_edit(delete_comment(error.range, locator)));
241253
}
242254
}
243255
}
244256

245257
if context.is_rule_enabled(Rule::InvalidSuppressionComment) {
246-
// missing codes already handled above, report the rest as invalid comments
247-
for error in self
248-
.errors
249-
.iter()
250-
.filter(|error| error.kind != ParseErrorKind::MissingCodes)
251-
{
252-
context
253-
.report_diagnostic(
254-
InvalidSuppressionComment {
255-
kind: InvalidSuppressionCommentKind::Error(error.kind),
256-
},
257-
error.range,
258-
)
259-
.set_fix(Fix::unsafe_edit(delete_comment(error.range, locator)));
260-
}
261-
262258
for invalid in &self.invalid {
263259
context
264260
.report_diagnostic(
@@ -273,21 +269,6 @@ impl Suppressions {
273269
)));
274270
}
275271
}
276-
277-
if context.is_rule_enabled(Rule::UnmatchedSuppressionComment) {
278-
for range in self
279-
.valid
280-
.iter()
281-
.filter(|suppression| {
282-
suppression.comments.len() == 1
283-
&& suppression.comments[0].action == SuppressionAction::Disable
284-
})
285-
.map(|suppression| suppression.comments[0].range)
286-
.unique()
287-
{
288-
context.report_diagnostic(UnmatchedSuppressionComment {}, range);
289-
}
290-
}
291272
}
292273

293274
fn delete_code_or_comment(

0 commit comments

Comments
 (0)