New diagnostics for unused range suppressions (#21783)

Issue #3711

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")

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
+                        let mut diagnostic = context.report_diagnostic(
-                            .report_diagnostic(UnusedNOQA { codes: None }, directive.range());
+                            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(),
                         );

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<Option<NoqaComment<'a>>> {
     // List of noqa comments, ordered to match up with `messages`
     let mut comments_by_line: Vec<Option<NoqaComment<'a>>> = vec![];

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<String>,
     pub duplicated: Vec<String>,
@@ -12,6 +12,21 @@ pub(crate) struct UnusedCodes {
     pub unmatched: Vec<String>,
 }
 
+#[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<UnusedCodes>,
+    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())
     }
 }

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
+Removed: 14
-Added: 1
+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")

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<Item = &'src str> {
@@ -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(Debug)]
-#[derive(Clone, 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<bool>,
 }
 
-#[allow(unused)]
 #[derive(Copy, Clone, Debug)]
 pub(crate) enum InvalidSuppressionKind {
     /// Trailing suppression not supported
@@ -114,7 +119,6 @@ pub struct Suppressions {
     errors: Vec<ParseError>,
 }
 
-#[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);