New diagnostics for unused range suppressions #21783

amyreese · 2025-12-04T02:21:05Z

Adds new diagnostics for reporting unused range suppressions.

Updates RUF100 to accept an enum to determine whether the diagnostic mentions noqa or "suppression" in the description and fix title.
Updates existing noqa logic to pass the appropriate enum when reporting RUF100 diagnostics.
Adds new logic to the new ranged Suppressions system to track whether individual suppressions/codes have been "used", and then report appropriat RUF100 diagnostics for unused suppressions/codes.
Adds new integration tests exercising a variety of unused suppression cases.

Issue #3711

astral-sh-bot · 2025-12-04T02:28:28Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2025-12-04T07:15:47Z

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

+/// - [Ruff error suppression](https://docs.astral.sh/ruff/linter/#error-suppression)
+#[derive(ViolationMetadata)]
+#[violation_metadata(preview_since = "0.14.9")]
+pub(crate) struct UnusedSuppression;


Did you create a new rule because that was easier or do you forsee use cases where users want to disable unused block level suppression warnings but not warnings about unused noqa comments?

Having two rules might be confusing to users. I can see how the old rule name isn't great, but the name of this rule is generic enough that a user could think that it also handles noqa comments.

Given that our long-term plan is to merge all suppression comments, I'd lean towards reusing the old rule and renaming it later.

MichaReiser · 2025-12-04T07:18:13Z

crates/ruff_linter/src/linter.rs

            parsed.has_valid_syntax(),
            settings,
-            &suppressions,
+            &mut suppressions,


Should we just pass suppressions here (the owned value), given that they aren't used afterwards?

Another alternative would be to store them on LintContext (I don't remember if that would be hard. It's okay to only look at this at the very end. Just floating this as an idea). Ultimately, I think, where we store it probably depends on where we want to use it, e.g. e might need to pass them to a new lint rule that checks for unclosed suppressions)

I went back and forth a couple times on what pieces should own the creation of the Suppressions object. The biggest problem is that the existing APIs are very inconsistent on what's available where, like how far context/locator/tokens get passed through. I like the idea of adding them to LintContext but that context isn't passed to generate_noqa_edits for example.

crates/ruff_linter/src/suppression.rs

amyreese · 2025-12-05T16:22:28Z

Not particularly happy with &mut everywhere, but not sure what other options I have, especially now that we're passing in suppressions objects from further up the stack.

MichaReiser · 2025-12-05T16:26:22Z

Not particularly happy with &mut everywhere, but not sure what other options I have, especially now that we're passing in suppressions objects from further up the stack.

One option you have is to use interior mutability by making Suppression::used a Cell<bool> or AtomicBool.

I don't mind the &mut overly much, but this is also a case where using iterior mutability seems a great fit because the fact that marking suppressions as used requires mutating isn't something that API users should really be aware of and using Cell<bool> allows you to hide that.

MichaReiser

The implementation looks great.

We should add some tests for the new rule that show that the fixes are working as expected. This is the reason why I'm requesting changes.

We should also add some tests demonstrating how you can use ruff:disable to suppress unused-suppression errors within a block.

I'm also leaning towards reusing the existing rules but I'm curious to learn about your motivation for having different rules (and how that expands to the other checks we want to add)

crates/ruff_linter/src/suppression.rs

MichaReiser · 2025-12-05T16:30:53Z

crates/ruff_linter/src/suppression.rs

+                    let code_index = comment
+                        .codes
+                        .iter()
+                        .position(|range| locator.slice(range) == suppression.code)
+                        .unwrap();


I think we want to store the suppression range on Suppression to correctly handle:

# ruff:disable[RUF100, RUF100]

MichaReiser · 2025-12-05T16:34:31Z

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

+/// - [Ruff error suppression](https://docs.astral.sh/ruff/linter/#error-suppression)
+#[derive(ViolationMetadata)]
+#[violation_metadata(preview_since = "0.14.9")]
+pub(crate) struct UnusedSuppression;


Having two rules might be confusing to users. I can see how the old rule name isn't great, but the name of this rule is generic enough that a user could think that it also handles noqa comments.

Given that our long-term plan is to merge all suppression comments, I'd lean towards reusing the old rule and renaming it later.

amyreese · 2025-12-06T01:11:37Z

Updated to use Cell<bool> rather than &mut, to reuse the existing UnusedNOQA rule, as well as to report disabled codes and comments with no codes listed. Duplicate codes are not yet handled.

Currently reports a separate diagnostic for each code due to the way the Suppression struct was designed. I did not realize the current NOQA rules combine everything into a single diagnostic, and I did not like my initial attempt to try and create a single diagnostic from the current data model. That said, ruff check --fix does converge on deleting everything unused, including the entire comment if all of the codes are unused, though presumably requires more iterations which is undesirable.

Not sure if it's worth reworking the data model before going further on this, or if I should worry about that later, or if you have a better idea of which direction to go.

MichaReiser · 2025-12-06T12:19:36Z

Not sure if it's worth reworking the data model before going further on this, or if I should worry about that later, or if you have a better idea of which direction to go.

That shouldn't be necessary (other than storing the range of the codes?). We use a similar data structure in ty, and we group consecutive unused codes together, which is perfectly fine (it can result in multiple diagnostics if most codes are unused but only creates one diagnostic if all codes are unused)

See

ruff/crates/ty_python_semantic/src/suppression.rs

Lines 260 to 289 in adf468b

    
           // Is this the first code directly after the `[`? 
        
           let includes_first_code = source[..suppression.range.start().to_usize()] 
        
               .trim_end() 
        
               .ends_with('['); 
        
           let mut current = suppression; 
        
           let mut unused_codes = Vec::new(); 
        
           // Group successive codes together into a single diagnostic, 
        
           // or report the entire directive if all codes are unused. 
        
           while let Some(next) = unused_iter.peek() { 
        
               if let SuppressionTarget::Lint(next_lint) = next.target 
        
                   && next.comment_range == current.comment_range 
        
                   && source[TextRange::new(current.range.end(), next.range.start())] 
        
                       .chars() 
        
                       .all(|c| c.is_whitespace() || c == ',') 
        
               { 
        
                   unused_codes.push(next_lint); 
        
                   current = *next; 
        
                   unused_iter.next(); 
        
               } else { 
        
                   break; 
        
               } 
        
           } 
        
           // Is the last suppression code the last code before the closing `]`. 
        
           let includes_last_code = source[current.range.end().to_usize()..] 
        
               .trim_start() 
        
               .starts_with(']');

We can address this in a follow-up PR (which is also what I did in ty, the initial version didn't support grouping)

crates/ruff_linter/src/suppression.rs

MichaReiser · 2025-12-06T12:26:54Z

...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap

+help: Remove unused suppression
+45 |     # logged to user
+46 |     # ruff: disable[E501]
+47 |     I = 1
+   -     # ruff: enable[E501]
+48 | 
+49 | 
+50 | def f():
+


Let's add some more tests demonstrating some edge cases:

Duplicate rule codes (it's okay to add a todo)

Disabling RUF100

Overlapping ranges (ruff: disable[E501 followed by another ruff: disable[E501])

Multiple codes where only one code is unused

Multiple codes where only some codes are unused

Multiple codes were all codes are unused

…ules

MichaReiser · 2025-12-09T15:40:48Z

crates/ruff_linter/resources/test/fixtures/ruff/suppressions.py

+
+
+def f():
+    # TODO: Duplicate codes should be counted as duplicate, not unused


It's fine if one of them counts as unused, for as long as we remove the right one.

…return * dcreager/die-die-intersections: (29 commits) simpler bounds [`pylint`] Detect subclasses of builtin exceptions (`PLW0133`) (#21382) Fix stack overflow with recursive generic protocols (depth limit) (#21858) New diagnostics for unused range suppressions (#21783) [ty] Use default settings in completion tests [ty] Infer type variables within generic unions (#21862) [ty] Fix overload filtering to prefer more "precise" match (#21859) [ty] Stabilize auto-import [ty] Fix reveal-type E2E test (#21865) [ty] Use concise message for LSP clients not supporting related diagnostic information (#21850) Include more details in Tokens 'offset is inside token' panic message (#21860) apply range suppressions to filter diagnostics (#21623) [ty] followup: add-import action for `reveal_type` too (#21668) [ty] Enrich function argument auto-complete suggestions with annotated types [ty] Add autocomplete suggestions for function arguments [`flake8-bugbear`] Accept immutable slice default arguments (`B008`) (#21823) [`pydocstyle`] Suppress `D417` for parameters with `Unpack` annotations (#21816) [ty] Remove legacy `concise_message` fallback behavior (#21847) [ty] Make Python-version subdiagnostics less verbose (#21849) [ty] Supress inlay hints when assigning a trivial initializer call (#21848) ...

…return * dcreager/die-die-intersections: (31 commits) clippy reword comment simpler bounds [`pylint`] Detect subclasses of builtin exceptions (`PLW0133`) (#21382) Fix stack overflow with recursive generic protocols (depth limit) (#21858) New diagnostics for unused range suppressions (#21783) [ty] Use default settings in completion tests [ty] Infer type variables within generic unions (#21862) [ty] Fix overload filtering to prefer more "precise" match (#21859) [ty] Stabilize auto-import [ty] Fix reveal-type E2E test (#21865) [ty] Use concise message for LSP clients not supporting related diagnostic information (#21850) Include more details in Tokens 'offset is inside token' panic message (#21860) apply range suppressions to filter diagnostics (#21623) [ty] followup: add-import action for `reveal_type` too (#21668) [ty] Enrich function argument auto-complete suggestions with annotated types [ty] Add autocomplete suggestions for function arguments [`flake8-bugbear`] Accept immutable slice default arguments (`B008`) (#21823) [`pydocstyle`] Suppress `D417` for parameters with `Unpack` annotations (#21816) [ty] Remove legacy `concise_message` fallback behavior (#21847) ...

* origin/main: (33 commits) [ty] Simplify union lower bounds and intersection upper bounds in constraint sets (#21871) [ty] Collapse `never` paths in constraint set BDDs (#21880) Fix leading comment formatting for lambdas with multiple parameters (#21879) [ty] Type inference for `@asynccontextmanager` (#21876) Fix comment placement in lambda parameters (#21868) [`pylint`] Detect subclasses of builtin exceptions (`PLW0133`) (#21382) Fix stack overflow with recursive generic protocols (depth limit) (#21858) New diagnostics for unused range suppressions (#21783) [ty] Use default settings in completion tests [ty] Infer type variables within generic unions (#21862) [ty] Fix overload filtering to prefer more "precise" match (#21859) [ty] Stabilize auto-import [ty] Fix reveal-type E2E test (#21865) [ty] Use concise message for LSP clients not supporting related diagnostic information (#21850) Include more details in Tokens 'offset is inside token' panic message (#21860) apply range suppressions to filter diagnostics (#21623) [ty] followup: add-import action for `reveal_type` too (#21668) [ty] Enrich function argument auto-complete suggestions with annotated types [ty] Add autocomplete suggestions for function arguments [`flake8-bugbear`] Accept immutable slice default arguments (`B008`) (#21823) ...

amyreese changed the base branch from main to amy/suppress-diagnostics December 4, 2025 02:21

MichaReiser reviewed Dec 4, 2025

View reviewed changes

crates/ruff_linter/src/suppression.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Dec 4, 2025

View reviewed changes

crates/ruff_linter/src/suppression.rs Outdated Show resolved Hide resolved

amyreese force-pushed the amy/unused-suppressions branch from 14b1a4d to bc990f1 Compare December 5, 2025 02:23

amyreese marked this pull request as ready for review December 5, 2025 16:20

MichaReiser requested changes Dec 5, 2025

View reviewed changes

amyreese force-pushed the amy/unused-suppressions branch from bc990f1 to 646ad15 Compare December 6, 2025 01:05

MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 6, 2025

MichaReiser reviewed Dec 6, 2025

View reviewed changes

Base automatically changed from amy/suppress-diagnostics to main December 9, 2025 00:12

amyreese added 12 commits December 8, 2025 16:17

Track which suppressions get used when filtering diagnostics

f69de03

Generate diagnostics for unused suppressions

8c7578e

Fix panic with delete_comment

ea5618a

Merge changes from base branch

b35918d

Update schema for new rule

7a8adcc

Suggested changes, improve target ranges and fixes, handle disabled r…

7fc52f2

…ules

Use cell for tracking used suppressions

3de50d0

rename function

226eee2

Resue existing UnusedNOQA rule

90ea030

Report disabled codes, as well as comments with empty code lists

d208d68

Remove unused lint allows

c12f4a0

Use correct unusedcodes object

7c88b49

amyreese added 2 commits December 8, 2025 16:52

NOQA -> Noqa to make lint happy

2f9042c

More test cases covering unused suppressions

2f8d4a4

amyreese force-pushed the amy/unused-suppressions branch from 646ad15 to 2f8d4a4 Compare December 9, 2025 02:12

MichaReiser reviewed Dec 9, 2025

View reviewed changes

MichaReiser approved these changes Dec 9, 2025

View reviewed changes

amyreese merged commit 4e4d018 into main Dec 9, 2025
37 checks passed

amyreese deleted the amy/unused-suppressions branch December 9, 2025 16:30

BrewTestBot mentioned this pull request Dec 11, 2025

ruff 0.14.9 Homebrew/homebrew-core#258289

Merged



		def f():
		# TODO: Duplicate codes should be counted as duplicate, not unused

New diagnostics for unused range suppressions #21783

New diagnostics for unused range suppressions #21783

Uh oh!

Conversation

amyreese commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astral-sh-bot bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amyreese commented Dec 5, 2025

Uh oh!

MichaReiser commented Dec 5, 2025

Uh oh!

MichaReiser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amyreese commented Dec 6, 2025

Uh oh!

MichaReiser commented Dec 6, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amyreese commented Dec 4, 2025 •

edited

Loading

astral-sh-bot bot commented Dec 4, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser left a comment •

edited

Loading