diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/suppressions.py b/crates/ruff_linter/resources/test/fixtures/ruff/suppressions.py index 7a70c4d548..f8a3c882aa 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/suppressions.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/suppressions.py @@ -54,3 +54,35 @@ def f(): # ruff:disable[E741,F841] I = 1 # noqa: E741,F841 # ruff:enable[E741,F841] + + +def f(): + # TODO: Duplicate codes should be counted as duplicate, not unused + # ruff: disable[F841, F841] + foo = 0 + + +def f(): + # Overlapping range suppressions, one should be marked as used, + # and the other should trigger an unused suppression diagnostic + # ruff: disable[F841] + # ruff: disable[F841] + foo = 0 + + +def f(): + # Multiple codes but only one is used + # ruff: disable[E741, F401, F841] + foo = 0 + + +def f(): + # Multiple codes but only two are used + # ruff: disable[E741, F401, F841] + I = 0 + + +def f(): + # Multiple codes but none are used + # ruff: disable[E741, F401, F841] + print("hello") diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index 2602adeeee..f984ef3576 100644 --- a/crates/ruff_linter/src/checkers/noqa.rs +++ b/crates/ruff_linter/src/checkers/noqa.rs @@ -119,6 +119,9 @@ pub(crate) fn check_noqa( } } + // Diagnostics for unused/invalid range suppressions + suppressions.check_suppressions(context, locator); + // Enforce that the noqa directive was actually used (RUF100), unless RUF100 was itself // suppressed. if context.is_rule_enabled(Rule::UnusedNOQA) @@ -140,8 +143,13 @@ pub(crate) fn check_noqa( Directive::All(directive) => { if matches.is_empty() { let edit = delete_comment(directive.range(), locator); - let mut diagnostic = context - .report_diagnostic(UnusedNOQA { codes: None }, directive.range()); + let mut diagnostic = context.report_diagnostic( + UnusedNOQA { + codes: None, + kind: ruff::rules::UnusedNOQAKind::Noqa, + }, + directive.range(), + ); diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); diagnostic.set_fix(Fix::safe_edit(edit)); } @@ -236,6 +244,7 @@ pub(crate) fn check_noqa( .map(|code| (*code).to_string()) .collect(), }), + kind: ruff::rules::UnusedNOQAKind::Noqa, }, directive.range(), ); diff --git a/crates/ruff_linter/src/noqa.rs b/crates/ruff_linter/src/noqa.rs index e8c3ada650..a3b5b6133d 100644 --- a/crates/ruff_linter/src/noqa.rs +++ b/crates/ruff_linter/src/noqa.rs @@ -879,7 +879,7 @@ fn find_noqa_comments<'a>( exemption: &'a FileExemption, directives: &'a NoqaDirectives, noqa_line_for: &NoqaMapping, - suppressions: &Suppressions, + suppressions: &'a Suppressions, ) -> Vec>> { // List of noqa comments, ordered to match up with `messages` let mut comments_by_line: Vec>> = vec![]; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_noqa.rs index e4645a5541..d6e4ce94d9 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_noqa.rs @@ -4,7 +4,7 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; use crate::AlwaysFixableViolation; -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Default)] pub(crate) struct UnusedCodes { pub disabled: Vec, pub duplicated: Vec, @@ -12,6 +12,21 @@ pub(crate) struct UnusedCodes { pub unmatched: Vec, } +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum UnusedNOQAKind { + Noqa, + Suppression, +} + +impl UnusedNOQAKind { + fn as_str(&self) -> &str { + match self { + UnusedNOQAKind::Noqa => "`noqa` directive", + UnusedNOQAKind::Suppression => "suppression", + } + } +} + /// ## What it does /// Checks for `noqa` directives that are no longer applicable. /// @@ -46,6 +61,7 @@ pub(crate) struct UnusedCodes { #[violation_metadata(stable_since = "v0.0.155")] pub(crate) struct UnusedNOQA { pub codes: Option, + pub kind: UnusedNOQAKind, } impl AlwaysFixableViolation for UnusedNOQA { @@ -95,16 +111,20 @@ impl AlwaysFixableViolation for UnusedNOQA { )); } if codes_by_reason.is_empty() { - "Unused `noqa` directive".to_string() + format!("Unused {}", self.kind.as_str()) } else { - format!("Unused `noqa` directive ({})", codes_by_reason.join("; ")) + format!( + "Unused {} ({})", + self.kind.as_str(), + codes_by_reason.join("; ") + ) } } - None => "Unused blanket `noqa` directive".to_string(), + None => format!("Unused blanket {}", self.kind.as_str()), } } fn fix_title(&self) -> String { - "Remove unused `noqa` directive".to_string() + format!("Remove unused {}", self.kind.as_str()) } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap index 4e09507482..24a43ade5e 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap @@ -6,8 +6,8 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs +linter.preview = enabled --- Summary --- -Removed: 9 -Added: 1 +Removed: 14 +Added: 11 --- Removed --- E741 Ambiguous variable name: `I` @@ -148,8 +148,136 @@ help: Remove assignment to unused variable `I` 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 +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 +61 | # ruff: disable[F841, F841] + - foo = 0 +62 + pass +63 | +64 | +65 | def f(): +note: This is an unsafe fix and may change runtime behavior + + +F841 [*] Local variable `foo` is assigned to but never used + --> suppressions.py:70:5 + | +68 | # ruff: disable[F841] +69 | # ruff: disable[F841] +70 | foo = 0 + | ^^^ + | +help: Remove assignment to unused variable `foo` +67 | # and the other should trigger an unused suppression diagnostic +68 | # ruff: disable[F841] +69 | # ruff: disable[F841] + - foo = 0 +70 + pass +71 | +72 | +73 | def f(): +note: This is an unsafe fix and may change runtime behavior + + +F841 [*] Local variable `foo` is assigned to but never used + --> suppressions.py:76:5 + | +74 | # Multiple codes but only one is used +75 | # ruff: disable[E741, F401, F841] +76 | foo = 0 + | ^^^ + | +help: Remove assignment to unused variable `foo` +73 | def f(): +74 | # Multiple codes but only one is used +75 | # ruff: disable[E741, F401, F841] + - foo = 0 +76 + pass +77 | +78 | +79 | def f(): +note: This is an unsafe fix and may change runtime behavior + + +E741 Ambiguous variable name: `I` + --> suppressions.py:82:5 + | +80 | # Multiple codes but only two are used +81 | # ruff: disable[E741, F401, F841] +82 | I = 0 + | ^ + | + + +F841 [*] Local variable `I` is assigned to but never used + --> suppressions.py:82:5 + | +80 | # Multiple codes but only two are used +81 | # ruff: disable[E741, F401, F841] +82 | I = 0 + | ^ + | +help: Remove assignment to unused variable `I` +79 | def f(): +80 | # Multiple codes but only two are used +81 | # ruff: disable[E741, F401, F841] + - I = 0 +82 + pass +83 | +84 | +85 | def f(): +note: This is an unsafe fix and may change runtime behavior + + --- Added --- +RUF100 [*] Unused suppression (non-enabled: `E501`) + --> suppressions.py:46:5 + | +44 | # Neither of these are ignored and warnings are +45 | # logged to user +46 | # ruff: disable[E501] + | ^^^^^^^^^^^^^^^^^^^^^ +47 | I = 1 +48 | # ruff: enable[E501] + | +help: Remove unused suppression +43 | def f(): +44 | # Neither of these are ignored and warnings are +45 | # logged to user + - # ruff: disable[E501] +46 | I = 1 +47 | # ruff: enable[E501] +48 | + + +RUF100 [*] Unused suppression (non-enabled: `E501`) + --> suppressions.py:48:5 + | +46 | # ruff: disable[E501] +47 | I = 1 +48 | # ruff: enable[E501] + | ^^^^^^^^^^^^^^^^^^^^ + | +help: Remove unused suppression +45 | # logged to user +46 | # ruff: disable[E501] +47 | I = 1 + - # ruff: enable[E501] +48 | +49 | +50 | def f(): + + RUF100 [*] Unused `noqa` directive (unused: `E741`, `F841`) --> suppressions.py:55:12 | @@ -166,3 +294,158 @@ help: Remove unused `noqa` directive - I = 1 # noqa: E741,F841 55 + I = 1 56 | # ruff:enable[E741,F841] +57 | +58 | + + +RUF100 [*] Unused suppression (unused: `F841`) + --> suppressions.py:61:21 + | +59 | def f(): +60 | # TODO: Duplicate codes should be counted as duplicate, not unused +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 + - # ruff: disable[F841, F841] +61 + # ruff: disable[F841] +62 | foo = 0 +63 | +64 | + + +RUF100 [*] Unused suppression (unused: `F841`) + --> suppressions.py:69:5 + | +67 | # and the other should trigger an unused suppression diagnostic +68 | # ruff: disable[F841] +69 | # ruff: disable[F841] + | ^^^^^^^^^^^^^^^^^^^^^ +70 | foo = 0 + | +help: Remove unused suppression +66 | # Overlapping range suppressions, one should be marked as used, +67 | # and the other should trigger an unused suppression diagnostic +68 | # ruff: disable[F841] + - # ruff: disable[F841] +69 | foo = 0 +70 | +71 | + + +RUF100 [*] Unused suppression (unused: `E741`) + --> suppressions.py:75:21 + | +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[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] +76 | foo = 0 +77 | +78 | + + +RUF100 [*] Unused suppression (non-enabled: `F401`) + --> suppressions.py:81:27 + | +79 | def f(): +80 | # Multiple codes but only two are used +81 | # ruff: disable[E741, F401, F841] + | ^^^^ +82 | I = 0 + | +help: Remove unused suppression +78 | +79 | def f(): +80 | # Multiple codes but only two are used + - # ruff: disable[E741, F401, F841] +81 + # ruff: disable[E741, F841] +82 | I = 0 +83 | +84 | + + +RUF100 [*] Unused suppression (unused: `E741`) + --> suppressions.py:87:21 + | +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[F401, F841] +88 | print("hello") + + +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") + + +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") diff --git a/crates/ruff_linter/src/suppression.rs b/crates/ruff_linter/src/suppression.rs index 3c1a2f57ab..9eb12d1026 100644 --- a/crates/ruff_linter/src/suppression.rs +++ b/crates/ruff_linter/src/suppression.rs @@ -1,8 +1,10 @@ use compact_str::CompactString; use core::fmt; use ruff_db::diagnostic::Diagnostic; +use ruff_diagnostics::{Edit, Fix}; use ruff_python_ast::token::{TokenKind, Tokens}; use ruff_python_ast::whitespace::indentation; +use std::cell::Cell; use std::{error::Error, fmt::Formatter}; use thiserror::Error; @@ -10,10 +12,14 @@ use ruff_python_trivia::Cursor; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize, TextSlice}; use smallvec::{SmallVec, smallvec}; +use crate::Locator; +use crate::checkers::ast::LintContext; +use crate::codes::Rule; +use crate::fix::edits::delete_comment; use crate::preview::is_range_suppressions_enabled; +use crate::rules::ruff::rules::{UnusedCodes, UnusedNOQA, UnusedNOQAKind}; use crate::settings::LinterSettings; -#[allow(unused)] #[derive(Clone, Debug, Eq, PartialEq)] enum SuppressionAction { Disable, @@ -35,7 +41,6 @@ pub(crate) struct SuppressionComment { reason: TextRange, } -#[allow(unused)] impl SuppressionComment { /// Return the suppressed codes as strings fn codes_as_str<'src>(&self, source: &'src str) -> impl Iterator { @@ -52,7 +57,6 @@ pub(crate) struct PendingSuppressionComment<'a> { comment: SuppressionComment, } -#[allow(unused)] impl PendingSuppressionComment<'_> { /// Whether the comment "matches" another comment, based on indentation and suppressed codes /// Expects a "forward search" for matches, ie, will only match if the current comment is a @@ -68,8 +72,7 @@ impl PendingSuppressionComment<'_> { } } -#[allow(unused)] -#[derive(Clone, Debug)] +#[derive(Debug)] pub(crate) struct Suppression { /// The lint code being suppressed code: CompactString, @@ -79,9 +82,11 @@ pub(crate) struct Suppression { /// Any comments associated with the suppression comments: SmallVec<[SuppressionComment; 2]>, + + /// Whether this suppression actually suppressed a diagnostic + used: Cell, } -#[allow(unused)] #[derive(Copy, Clone, Debug)] pub(crate) enum InvalidSuppressionKind { /// Trailing suppression not supported @@ -114,7 +119,6 @@ pub struct Suppressions { errors: Vec, } -#[allow(unused)] impl Suppressions { pub fn from_tokens(settings: &LinterSettings, source: &str, tokens: &Tokens) -> Suppressions { if is_range_suppressions_enabled(settings) { @@ -147,11 +151,90 @@ impl Suppressions { for suppression in &self.valid { if *code == suppression.code.as_str() && suppression.range.contains_range(range) { + suppression.used.set(true); return true; } } false } + + pub(crate) fn check_suppressions(&self, context: &LintContext, locator: &Locator) { + if !context.any_rule_enabled(&[Rule::UnusedNOQA, Rule::InvalidRuleCode]) { + return; + } + + let unused = self + .valid + .iter() + .filter(|suppression| !suppression.used.get()); + + for suppression in unused { + let Ok(rule) = Rule::from_code(&suppression.code) else { + continue; // TODO: invalid code + }; + for comment in &suppression.comments { + let mut range = comment.range; + let edit = if comment.codes.len() == 1 { + delete_comment(comment.range, locator) + } else { + let code_index = comment + .codes + .iter() + .position(|range| locator.slice(range) == suppression.code) + .unwrap(); + range = comment.codes[code_index]; + let code_range = if code_index < (comment.codes.len() - 1) { + TextRange::new( + comment.codes[code_index].start(), + comment.codes[code_index + 1].start(), + ) + } else { + TextRange::new( + comment.codes[code_index - 1].end(), + comment.codes[code_index].end(), + ) + }; + Edit::range_deletion(code_range) + }; + + 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() + } + }; + + let mut diagnostic = context.report_diagnostic( + UnusedNOQA { + codes: Some(codes), + kind: UnusedNOQAKind::Suppression, + }, + range, + ); + diagnostic.set_fix(Fix::safe_edit(edit)); + } + } + + for error in self + .errors + .iter() + .filter(|error| error.kind == ParseErrorKind::MissingCodes) + { + let mut diagnostic = context.report_diagnostic( + UnusedNOQA { + codes: Some(UnusedCodes::default()), + kind: UnusedNOQAKind::Suppression, + }, + error.range, + ); + diagnostic.set_fix(Fix::safe_edit(delete_comment(error.range, locator))); + } + } } #[derive(Default)] @@ -276,6 +359,7 @@ impl<'a> SuppressionsBuilder<'a> { code: code.into(), range: combined_range, comments: smallvec![comment.comment.clone(), other.comment.clone()], + used: false.into(), }); } @@ -292,6 +376,7 @@ impl<'a> SuppressionsBuilder<'a> { code: code.into(), range: implicit_range, comments: smallvec![comment.comment.clone()], + used: false.into(), }); } self.pending.remove(comment_index);