apply range suppressions to filter diagnostics #21623

amyreese · 2025-11-25T02:02:01Z

Builds on range suppressions from #21441

Filters diagnostics based on parsed valid range suppressions.

Issue: #3711

astral-sh-bot · 2025-11-25T02:07:00Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

I think we should add some tests now. check_path is called from our testing framework, meaning we should now be able to write a test similar to

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

Lines 294 to 305 in 850398d

    
           #[test] 
        
           fn noqa() -> Result<()> { 
        
               let diagnostics = test_path( 
        
                   Path::new("ruff/noqa.py"), 
        
                   &settings::LinterSettings::for_rules(vec![ 
        
                       Rule::UnusedVariable, 
        
                       Rule::AmbiguousVariableName, 
        
                   ]), 
        
               )?; 
        
               assert_diagnostics!(diagnostics); 
        
               Ok(()) 
        
           }

This could also be used as a replacement for some of the tests I asked for in your previous PR.

I also suggest adding some CLI tests verifying that this PR doesn't break in combination with --add-noqa, --remove-noqa --ignore-noqa etc.

crates/ruff_linter/src/linter.rs

crates/ruff_linter/src/suppression.rs

amyreese · 2025-12-02T01:29:50Z

I also suggest adding some CLI tests verifying that this PR doesn't break in combination with --add-noqa, --remove-noqa --ignore-noqa etc.

Is there an existing set of CLI tests for noqa that I can use as an example here?

MichaReiser · 2025-12-02T08:28:53Z

Is there an existing set of CLI tests for noqa that I can use as an example here?

There are some here

ruff/crates/ruff/tests/cli/lint.rs

Line 1444 in 81f6ec1

fn add_noqa() -> Result<()> {

I couldn't find any for --ignore-noqa, and --remove-noqa doesn't exist, it's just fix and you can find some tests in the above mentioned test file

MichaReiser

This looks good. But we should look into the performance regression before landing this PR (can be fixed as a separate PR but we can't land this until we fixed the regression)

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

amyreese · 2025-12-02T17:00:35Z

Note: add --preview gating so we can land this without waiting for diagnostics etc

MichaReiser · 2025-12-04T07:46:58Z

crates/ruff/tests/cli/lint.rs

+    let test_code =
+        fs::read_to_string(fixture.root().join("noqa.py")).expect("should read test file");


We try to avoid reading fixture files in CLI tests as it makes the tests depend on each other and it can also become harder to reason about what we're testing here. Would it be possible to extract the specific case you want to test from noqa.py?

The tests here also don't need to be exhaustive. We can add more exhaustive tests to add_noqa (I think we have some integration tests, right?)

MichaReiser · 2025-12-04T07:50:22Z

crates/ruff_linter/src/linter.rs

+    let suppressions = if is_range_suppressions_enabled(settings) {
+        Suppressions::from_tokens(locator.contents(), parsed.tokens())
+    } else {
+        Suppressions::default()
+    };
+


We now end up extracting the suppressions twice: Once in check_path and once here. We might need to pass the suppressions as part of check_path

MichaReiser · 2025-12-04T07:56:36Z

crates/ruff_linter/src/noqa.rs

    let exemption = FileExemption::from(&file_directives);
    let directives = NoqaDirectives::from_commented_ranges(comment_ranges, external, path, locator);
-    let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for);
+    // This is called by ruff_server, which already filtered diagnostics via check_path


We also call this function from our tests. Would it make sense to lift Suppressions out of generate_noqa_edits and instead require the caller to pass the suppressions?

MichaReiser · 2025-12-04T07:57:23Z

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

+            ])
+            .with_preview_mode(),
+        )?;
+        assert_diagnostics!(diagnostics);


This might be a good case for assert_diagnostics_diff which snapshots the difference between two settings: E.g. between preview on and off

amyreese changed the base branch from main to suppression-parsing November 25, 2025 02:02

MichaReiser reviewed Nov 25, 2025

View reviewed changes

crates/ruff_linter/src/linter.rs Show resolved Hide resolved

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

MichaReiser added the suppression Related to supression of violations e.g. noqa label Nov 25, 2025

MichaReiser mentioned this pull request Nov 25, 2025

new module for parsing ranged suppressions #21441

Merged

MichaReiser reviewed Nov 25, 2025

View reviewed changes

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

amyreese force-pushed the amy/suppress-diagnostics branch from 850398d to 2ff32de Compare November 26, 2025 01:36

MichaReiser changed the title ~~[ruff] apply range suppressions to filter diagnostics~~ apply range suppressions to filter diagnostics Nov 26, 2025

MichaReiser added the internal An internal refactor or improvement label Nov 26, 2025

amyreese force-pushed the suppression-parsing branch from ce14617 to 607325e Compare December 1, 2025 21:17

amyreese force-pushed the amy/suppress-diagnostics branch from 2ff32de to 1586572 Compare December 2, 2025 01:30

amyreese requested a review from MichaReiser December 2, 2025 01:31

MichaReiser approved these changes Dec 2, 2025

View reviewed changes

amyreese force-pushed the suppression-parsing branch from ea5b48d to 288d431 Compare December 2, 2025 22:46

Base automatically changed from suppression-parsing to main December 2, 2025 23:40

amyreese added 8 commits December 2, 2025 15:41

Prototype filtering diagnostics by range suppressions

ee58f87

Refactor suppressions checking into check_noqa

dd5d45c

Short circuit if no suppressions loaded

df36084

Add test covering expected diagnostics from range suppressions

67be3d9

Gate suppression parsing/checking on --preview

36fbd22

english

7723b63

More integration test cases

2f57e20

Added cli tests (currently failing)

5ebadd4

amyreese force-pushed the amy/suppress-diagnostics branch from 1586572 to 5ebadd4 Compare December 3, 2025 02:03

amyreese added 3 commits December 3, 2025 11:44

Load and pass suppressions through when adding noqa comments

fe83037

Update comment

2100298

Add is_empty to Suppressions, don't abort early in check_noqa

8f104fa

MichaReiser reviewed Dec 4, 2025

View reviewed changes

	#[test]
	fn noqa() -> Result<()> {
	let diagnostics = test_path(
	Path::new("ruff/noqa.py"),
	&settings::LinterSettings::for_rules(vec![
	Rule::UnusedVariable,
	Rule::AmbiguousVariableName,
	]),
	)?;
	assert_diagnostics!(diagnostics);
	Ok(())
	}

		let test_code =
		fs::read_to_string(fixture.root().join("noqa.py")).expect("should read test file");

apply range suppressions to filter diagnostics #21623

Are you sure you want to change the base?

apply range suppressions to filter diagnostics #21623

Conversation

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

Uh oh!

Uh oh!

astral-sh-bot bot commented Nov 25, 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!

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!

Uh oh!

amyreese commented Dec 2, 2025

Uh oh!

MichaReiser commented Dec 2, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amyreese commented Dec 2, 2025

Uh oh!

MichaReiser Dec 4, 2025 • 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!

MichaReiser Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amyreese commented Nov 25, 2025 •

edited

Loading

astral-sh-bot bot commented Nov 25, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser left a comment •

edited

Loading

MichaReiser Dec 4, 2025 •

edited

Loading