New diagnostics for unused range suppressions

[amyreese]

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]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

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?

[amyreese]

Both 😅

[MichaReiser]

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]

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)

[amyreese]

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.

[amyreese]

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]

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)

[MichaReiser]

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

# ruff:disable[RUF100, RUF100]

[MichaReiser]

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]

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]

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)

[MichaReiser]

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

[MichaReiser]

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