mirror of
https://github.com/astral-sh/ruff
synced 2026-01-20 21:10:48 -05:00
Combine suppression code diagnostics (#22613)
# Summary - Report a single combined UnusedNOQA diagnostic when any range suppression has one or more unused, disabled, or duplicate lint codes. - Report a single combined InvalidRuleCode diagnostic for any range suppression with one or more invalid lint codes. # Test Plan Updated fixtures and snapshots Fixes #22429, #21873
This commit is contained in:
Amethyst Reese
GitHub
@@ -57,7 +57,7 @@ def f():
|
||||
|
||||
|
||||
def f():
|
||||
# TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
# Duplicate codes that are actually used.
|
||||
# ruff: disable[F841, F841]
|
||||
foo = 0
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs
|
||||
|
||||
--- Summary ---
|
||||
Removed: 15
|
||||
Added: 20
|
||||
Added: 17
|
||||
|
||||
--- Removed ---
|
||||
E741 Ambiguous variable name: `I`
|
||||
@@ -151,14 +151,14 @@ note: This is an unsafe fix and may change runtime behavior
|
||||
F841 [*] Local variable `foo` is assigned to but never used
|
||||
--> suppressions.py:62:5
|
||||
|
|
||||
60 | # TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
60 | # Duplicate codes that are actually used.
|
||||
61 | # ruff: disable[F841, F841]
|
||||
62 | foo = 0
|
||||
| ^^^
|
||||
|
|
||||
help: Remove assignment to unused variable `foo`
|
||||
59 | def f():
|
||||
60 | # TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
60 | # Duplicate codes that are actually used.
|
||||
61 | # ruff: disable[F841, F841]
|
||||
- foo = 0
|
||||
62 + pass
|
||||
@@ -339,26 +339,26 @@ RUF104 Suppression comment without matching `#ruff:enable` comment
|
||||
--> suppressions.py:61:5
|
||||
|
|
||||
59 | def f():
|
||||
60 | # TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
60 | # Duplicate codes that are actually used.
|
||||
61 | # ruff: disable[F841, F841]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
62 | foo = 0
|
||||
|
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (unused: `F841`)
|
||||
--> suppressions.py:61:21
|
||||
RUF100 [*] Unused suppression (duplicated: `F841`)
|
||||
--> suppressions.py:61:5
|
||||
|
|
||||
59 | def f():
|
||||
60 | # TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
60 | # Duplicate codes that are actually used.
|
||||
61 | # ruff: disable[F841, F841]
|
||||
| ^^^^
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
62 | foo = 0
|
||||
|
|
||||
help: Remove unused suppression
|
||||
58 |
|
||||
59 | def f():
|
||||
60 | # TODO: Duplicate codes should be counted as duplicate, not unused
|
||||
60 | # Duplicate codes that are actually used.
|
||||
- # ruff: disable[F841, F841]
|
||||
61 + # ruff: disable[F841]
|
||||
62 | foo = 0
|
||||
@@ -408,13 +408,13 @@ RUF104 Suppression comment without matching `#ruff:enable` comment
|
||||
|
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (unused: `E741`)
|
||||
--> suppressions.py:75:21
|
||||
RUF100 [*] Unused suppression (unused: `E741`; non-enabled: `F401`)
|
||||
--> suppressions.py:75:5
|
||||
|
|
||||
73 | def f():
|
||||
74 | # Multiple codes but only one is used
|
||||
75 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
76 | foo = 0
|
||||
|
|
||||
help: Remove unused suppression
|
||||
@@ -422,27 +422,7 @@ help: Remove unused suppression
|
||||
73 | def f():
|
||||
74 | # Multiple codes but only one is used
|
||||
- # ruff: disable[E741, F401, F841]
|
||||
75 + # ruff: disable[F401, F841]
|
||||
76 | foo = 0
|
||||
77 |
|
||||
78 |
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (non-enabled: `F401`)
|
||||
--> suppressions.py:75:27
|
||||
|
|
||||
73 | def f():
|
||||
74 | # Multiple codes but only one is used
|
||||
75 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
76 | foo = 0
|
||||
|
|
||||
help: Remove unused suppression
|
||||
72 |
|
||||
73 | def f():
|
||||
74 | # Multiple codes but only one is used
|
||||
- # ruff: disable[E741, F401, F841]
|
||||
75 + # ruff: disable[E741, F841]
|
||||
75 + # ruff: disable[F841]
|
||||
76 | foo = 0
|
||||
77 |
|
||||
78 |
|
||||
@@ -460,12 +440,12 @@ RUF104 Suppression comment without matching `#ruff:enable` comment
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (non-enabled: `F401`)
|
||||
--> suppressions.py:81:27
|
||||
--> suppressions.py:81:5
|
||||
|
|
||||
79 | def f():
|
||||
80 | # Multiple codes but only two are used
|
||||
81 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
82 | I = 0
|
||||
|
|
||||
help: Remove unused suppression
|
||||
@@ -479,13 +459,13 @@ help: Remove unused suppression
|
||||
84 |
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (unused: `E741`)
|
||||
--> suppressions.py:87:21
|
||||
RUF100 [*] Unused suppression (unused: `E741`, `F841`; non-enabled: `F401`)
|
||||
--> suppressions.py:87:5
|
||||
|
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
87 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
88 | print("hello")
|
||||
|
|
||||
help: Remove unused suppression
|
||||
@@ -493,50 +473,9 @@ help: Remove unused suppression
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
- # ruff: disable[E741, F401, F841]
|
||||
87 + # ruff: disable[F401, F841]
|
||||
88 | print("hello")
|
||||
87 | print("hello")
|
||||
88 |
|
||||
89 |
|
||||
90 |
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (non-enabled: `F401`)
|
||||
--> suppressions.py:87:27
|
||||
|
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
87 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
88 | print("hello")
|
||||
|
|
||||
help: Remove unused suppression
|
||||
84 |
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
- # ruff: disable[E741, F401, F841]
|
||||
87 + # ruff: disable[E741, F841]
|
||||
88 | print("hello")
|
||||
89 |
|
||||
90 |
|
||||
|
||||
|
||||
RUF100 [*] Unused suppression (unused: `F841`)
|
||||
--> suppressions.py:87:33
|
||||
|
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
87 | # ruff: disable[E741, F401, F841]
|
||||
| ^^^^
|
||||
88 | print("hello")
|
||||
|
|
||||
help: Remove unused suppression
|
||||
84 |
|
||||
85 | def f():
|
||||
86 | # Multiple codes but none are used
|
||||
- # ruff: disable[E741, F401, F841]
|
||||
87 + # ruff: disable[E741, F401]
|
||||
88 | print("hello")
|
||||
89 |
|
||||
90 |
|
||||
|
||||
|
||||
RUF102 [*] Invalid rule code in suppression: YF829
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use compact_str::CompactString;
|
||||
use core::fmt;
|
||||
use itertools::Itertools;
|
||||
use ruff_db::diagnostic::Diagnostic;
|
||||
use ruff_diagnostics::{Edit, Fix};
|
||||
use ruff_python_ast::token::{TokenKind, Tokens};
|
||||
@@ -152,6 +153,37 @@ pub struct Suppressions {
|
||||
errors: Vec<ParseError>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
struct SuppressionDiagnostic<'a> {
|
||||
suppression: &'a Suppression,
|
||||
invalid_codes: Vec<&'a str>,
|
||||
duplicated_codes: Vec<&'a str>,
|
||||
disabled_codes: Vec<&'a str>,
|
||||
unused_codes: Vec<&'a str>,
|
||||
}
|
||||
|
||||
impl<'a> SuppressionDiagnostic<'a> {
|
||||
fn new(suppression: &'a Suppression) -> Self {
|
||||
Self {
|
||||
suppression,
|
||||
invalid_codes: Vec::new(),
|
||||
duplicated_codes: Vec::new(),
|
||||
disabled_codes: Vec::new(),
|
||||
unused_codes: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn any_invalid(&self) -> bool {
|
||||
!self.invalid_codes.is_empty()
|
||||
}
|
||||
|
||||
fn any_unused(&self) -> bool {
|
||||
!self.disabled_codes.is_empty()
|
||||
|| !self.duplicated_codes.is_empty()
|
||||
|| !self.unused_codes.is_empty()
|
||||
}
|
||||
}
|
||||
|
||||
impl Suppressions {
|
||||
pub fn from_tokens(
|
||||
settings: &LinterSettings,
|
||||
@@ -199,55 +231,99 @@ impl Suppressions {
|
||||
}
|
||||
|
||||
pub(crate) fn check_suppressions(&self, context: &LintContext, locator: &Locator) {
|
||||
let mut grouped_diagnostic: Option<(TextRange, SuppressionDiagnostic)> = None;
|
||||
let mut unmatched_ranges = FxHashSet::default();
|
||||
for suppression in &self.valid {
|
||||
if !code_is_valid(&suppression.code, &context.settings().external) {
|
||||
// InvalidRuleCode
|
||||
if context.is_rule_enabled(Rule::InvalidRuleCode) {
|
||||
Suppressions::report_suppression(
|
||||
let key = suppression.comments.disable_comment().range;
|
||||
|
||||
// Process any pending grouped diagnostics
|
||||
if let Some((group_key, ref group)) = grouped_diagnostic
|
||||
&& key != group_key
|
||||
{
|
||||
if group.any_invalid() {
|
||||
Suppressions::report_suppression_codes(
|
||||
context,
|
||||
locator,
|
||||
suppression,
|
||||
group.suppression,
|
||||
&group.invalid_codes,
|
||||
true,
|
||||
InvalidRuleCode {
|
||||
rule_code: suppression.code.to_string(),
|
||||
rule_code: group.invalid_codes.iter().join(", "),
|
||||
kind: InvalidRuleCodeKind::Suppression,
|
||||
whole_comment: suppression.codes().len() == 1,
|
||||
whole_comment: group.suppression.codes().len()
|
||||
== group.invalid_codes.len(),
|
||||
},
|
||||
);
|
||||
}
|
||||
} else if !suppression.used.get() {
|
||||
// UnusedNOQA
|
||||
if context.is_rule_enabled(Rule::UnusedNOQA) {
|
||||
let Ok(rule) = Rule::from_code(
|
||||
get_redirect_target(&suppression.code).unwrap_or(&suppression.code),
|
||||
) else {
|
||||
continue; // "external" lint code, don't treat it as unused
|
||||
};
|
||||
|
||||
let codes = if context.is_rule_enabled(rule) {
|
||||
UnusedCodes {
|
||||
unmatched: vec![suppression.code.to_string()],
|
||||
..Default::default()
|
||||
}
|
||||
} else {
|
||||
UnusedCodes {
|
||||
disabled: vec![suppression.code.to_string()],
|
||||
..Default::default()
|
||||
}
|
||||
};
|
||||
|
||||
Suppressions::report_suppression(
|
||||
if group.any_unused() {
|
||||
let mut codes = group.disabled_codes.clone();
|
||||
codes.extend(group.unused_codes.clone());
|
||||
Suppressions::report_suppression_codes(
|
||||
context,
|
||||
locator,
|
||||
suppression,
|
||||
group.suppression,
|
||||
&codes,
|
||||
false,
|
||||
UnusedNOQA {
|
||||
codes: Some(codes),
|
||||
codes: Some(UnusedCodes {
|
||||
disabled: group
|
||||
.disabled_codes
|
||||
.iter()
|
||||
.map(ToString::to_string)
|
||||
.collect_vec(),
|
||||
duplicated: group
|
||||
.duplicated_codes
|
||||
.iter()
|
||||
.map(ToString::to_string)
|
||||
.collect_vec(),
|
||||
unmatched: group
|
||||
.unused_codes
|
||||
.iter()
|
||||
.map(ToString::to_string)
|
||||
.collect_vec(),
|
||||
..Default::default()
|
||||
}),
|
||||
kind: UnusedNOQAKind::Suppression,
|
||||
},
|
||||
);
|
||||
}
|
||||
grouped_diagnostic = None;
|
||||
}
|
||||
|
||||
let code_str = suppression.code.as_str();
|
||||
|
||||
if !code_is_valid(&suppression.code, &context.settings().external) {
|
||||
// InvalidRuleCode
|
||||
let (_key, group) = grouped_diagnostic
|
||||
.get_or_insert_with(|| (key, SuppressionDiagnostic::new(suppression)));
|
||||
group.invalid_codes.push(code_str);
|
||||
} else if !suppression.used.get() {
|
||||
// UnusedNOQA
|
||||
let Ok(rule) = Rule::from_code(
|
||||
get_redirect_target(&suppression.code).unwrap_or(&suppression.code),
|
||||
) else {
|
||||
continue; // "external" lint code, don't treat it as unused
|
||||
};
|
||||
|
||||
let (_key, group) = grouped_diagnostic
|
||||
.get_or_insert_with(|| (key, SuppressionDiagnostic::new(suppression)));
|
||||
|
||||
if context.is_rule_enabled(rule) {
|
||||
if suppression
|
||||
.comments
|
||||
.disable_comment()
|
||||
.codes_as_str(locator.contents())
|
||||
.filter(|code| *code == code_str)
|
||||
.count()
|
||||
> 1
|
||||
{
|
||||
group.duplicated_codes.push(code_str);
|
||||
} else {
|
||||
group.unused_codes.push(code_str);
|
||||
}
|
||||
} else {
|
||||
group.disabled_codes.push(code_str);
|
||||
}
|
||||
} else if let DisableEnableComments::Disable(comment) = &suppression.comments {
|
||||
// UnmatchedSuppressionComment
|
||||
if unmatched_ranges.insert(comment.range) {
|
||||
@@ -289,39 +365,41 @@ impl Suppressions {
|
||||
}
|
||||
}
|
||||
|
||||
fn report_suppression<T: Violation>(
|
||||
fn report_suppression_codes<T: Violation>(
|
||||
context: &LintContext,
|
||||
locator: &Locator,
|
||||
suppression: &Suppression,
|
||||
remove_codes: &[&str],
|
||||
highlight_only_code: bool,
|
||||
kind: T,
|
||||
) {
|
||||
let disable_comment = suppression.comments.disable_comment();
|
||||
let (range, edit) = Suppressions::delete_code_or_comment(
|
||||
let (range, edit) = Suppressions::delete_codes_or_comment(
|
||||
locator,
|
||||
suppression,
|
||||
disable_comment,
|
||||
remove_codes,
|
||||
highlight_only_code,
|
||||
);
|
||||
let mut diagnostic = context.report_diagnostic(kind, range);
|
||||
if let Some(enable_comment) = suppression.comments.enable_comment() {
|
||||
let (enable_range, enable_range_edit) = Suppressions::delete_code_or_comment(
|
||||
locator,
|
||||
suppression,
|
||||
enable_comment,
|
||||
highlight_only_code,
|
||||
);
|
||||
diagnostic.secondary_annotation("", enable_range);
|
||||
diagnostic.set_fix(Fix::safe_edits(edit, [enable_range_edit]));
|
||||
} else {
|
||||
diagnostic.set_fix(Fix::safe_edit(edit));
|
||||
if let Some(mut diagnostic) = context.report_diagnostic_if_enabled(kind, range) {
|
||||
if let Some(enable_comment) = suppression.comments.enable_comment() {
|
||||
let (enable_range, enable_range_edit) = Suppressions::delete_codes_or_comment(
|
||||
locator,
|
||||
enable_comment,
|
||||
remove_codes,
|
||||
highlight_only_code,
|
||||
);
|
||||
diagnostic.secondary_annotation("", enable_range);
|
||||
diagnostic.set_fix(Fix::safe_edits(edit, [enable_range_edit]));
|
||||
} else {
|
||||
diagnostic.set_fix(Fix::safe_edit(edit));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn delete_code_or_comment(
|
||||
fn delete_codes_or_comment(
|
||||
locator: &Locator<'_>,
|
||||
suppression: &Suppression,
|
||||
comment: &SuppressionComment,
|
||||
remove_codes: &[&str],
|
||||
highlight_only_code: bool,
|
||||
) -> (TextRange, Edit) {
|
||||
let mut range = comment.range;
|
||||
@@ -330,13 +408,15 @@ impl Suppressions {
|
||||
range = comment.codes[0];
|
||||
}
|
||||
delete_comment(comment.range, locator)
|
||||
} else {
|
||||
} else if remove_codes.len() == 1 {
|
||||
let code_index = comment
|
||||
.codes
|
||||
.iter()
|
||||
.position(|range| locator.slice(range) == suppression.code)
|
||||
.position(|range| locator.slice(range) == remove_codes[0])
|
||||
.unwrap();
|
||||
range = comment.codes[code_index];
|
||||
if highlight_only_code {
|
||||
range = comment.codes[code_index];
|
||||
}
|
||||
let code_range = if code_index < (comment.codes.len() - 1) {
|
||||
TextRange::new(
|
||||
comment.codes[code_index].start(),
|
||||
@@ -349,6 +429,27 @@ impl Suppressions {
|
||||
)
|
||||
};
|
||||
Edit::range_deletion(code_range)
|
||||
} else {
|
||||
let first = comment
|
||||
.codes
|
||||
.first()
|
||||
.expect("suppression comment without codes");
|
||||
let last = comment
|
||||
.codes
|
||||
.last()
|
||||
.expect("suppression comment without codes");
|
||||
let code_range = TextRange::new(first.start(), last.end());
|
||||
let remaining = comment
|
||||
.codes_as_str(locator.contents())
|
||||
.filter(|code| !remove_codes.contains(code))
|
||||
.dedup()
|
||||
.join(", ");
|
||||
|
||||
if remaining.is_empty() {
|
||||
delete_comment(comment.range, locator)
|
||||
} else {
|
||||
Edit::range_replacement(remaining, code_range)
|
||||
}
|
||||
};
|
||||
(range, edit)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user