[pyupgrade] Gated behind a preview of separate diagnostics (UP010)

[IDrokin117]

Summary

Second part of #19769

Separate diagnostic generation for each unused import and range updating accordingly. Gated behind a preview

Test Plan

Added preview snapshots

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[codspeed-hq]

CodSpeed Performance Report

Merging #20020 will not alter performance

_{Comparing IDrokin117:feat/ruff-19561-preview (affcf04) with main (542f080)}

Summary

✅ 30 untouched
⏩ 13 skipped¹

Footnotes

1. 13 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[IDrokin117]

@ntBre Could you please review PR?

[ntBre]

Yes, sorry for the delay! We've been preparing for the 0.13 release this past week. I'll be catching up on reviews once that goes out.

[MichaReiser]

What's the motivation for splitting the diagnostics? This seems unnecessarily verbose to me. Could we instead use the new sub-diagnostics to highlight the individual ranges?

[ntBre]

What I liked about the split diagnostics in #19769 (review) was mostly just underlining the affected imports instead of the whole import statement, which I think could also be handled by sub-diagnostics.

As one supporting example, we also emit one diagnostic per import for unused-import (F401). Although we could revisit that as well now that we have sub-diagnostics.

I don't feel too strongly either way, though.

playground

[IDrokin117]

@ntBre What should I do here? It seems diagnostics work per import as for F401, doesn't it? Should I use sub-diagnostics instead? What piece of code can I take as a "sub-diagnostic" example?

[ntBre]

I think we just need to reach agreement on how many diagnostics to emit. If you want to try sub-diagnostics to see how they look, here is a previous comment I used on another PR:

The easiest way is with secondary_annotation on the guard returned by report_diagnostic:

ruff/crates/ruff_linter/src/checkers/ast/mod.rs

Lines 3309 to 3311 in 2245355

	/// Add a secondary annotation with the given message and range.
	pub(crate) fn secondary_annotation<'a>(
	&mut self,

Here's the one use of it we've added so far:

ruff/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs

Lines 195 to 198 in 2245355

	diagnostic.secondary_annotation(
	format_args!("previous definition of `{name}` here"),
	shadowed,
	);

And an example of the type of diagnostic it produces (the ----- underlining is used for secondary annotations):

ruff/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_0.py.snap

Lines 4 to 17 in 2245355

	F811 Redefinition of unused `bar` from line 6
	--> F811_0.py:10:5
	|
	10 | def bar():
	| ^^^ `bar` redefined here
	11 | pass
	|
	::: F811_0.py:6:5
	|
	5 | @foo
	6 | def bar():
	| --- previous definition of `bar` here
	7 | pass
	|

There are some other options if that doesn't look good, but that's the easiest place to start.

---

That's not technically a sub-diagnostic, but we've only added a nice helper for secondary annotations rather than SubDiagnostics proper. The helper code would be quite similar to the annotation version, but I'm happy to help with that if we go that route.

[IDrokin117]

@ntBre @MichaReiser I've rewritten the diagnostic using secondary_annotation. Please check it and let me know how it looks now.
I've also handled the edge case when there is only one alias in the import statement, so there is no need to underline that specific alias separately.

[ntBre]

I think if we do go this route, we should narrow the primary diagnostic range to just __future__ (or maybe from __future__ or from __future__ import) to keep the two ranges from overlapping:

Suggested change

	1 | from __future__ import nested_scopes, generators
	| ^^^^^^^^^^^^^^^^^^^^^^^-------------^^----------
	1 | from __future__ import nested_scopes, generators
	| ^^^^^^^^^^ ------------- ----------

I think I'd still prefer either one diagnostic per import, or just leaving the diagnostic as it is on stable, over this, though. But I'm curious to hear what Micha thinks. Thank you for exploring this!

[MichaReiser]

I'd have to double-check but narrowing the range changes the places where suppression comments are allowed.

E.g, the following currently works but I suspect won't work if we narrow the range

from __future__ import (
	nested_scopes, # noqa <RULE>
 	generators
)

[MichaReiser]

Hmm yeah, using multiple annotated ranges and highlighting the entire import statement doesn't help improve readability.

I only found this usage in cargo:

warning: methods `unused1` and `unused2` are never used
  --> src/function/maybe_changed_after.rs:41:19
   |
32 | impl VerifyResult {
   | ----------------- methods in this implementation
...
41 |     pub(crate) fn unused1(&self) {}
   |                   ^^^^^^^
42 |
43 |     pub(crate) fn unused2(&self) {}
   |                   ^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

They use the unused1 as the primary range and mark the impl block as a secondary annotation. But doing the same in Ruff requires figuring out how to make our suppression system understand that this diagnostic represents two separate violations (the current assumption is that diagnostics only represent a single error.

[ntBre]

Ah, would narrowing the noqa range actually be a benefit of separate diagnostics here? Something like this doesn't currently work:

from __future__ import ( 
    nested_scopes,  # noqa: UP010
    generators,
) 

generators

You have to put the noqa here:

from __future__ import ( # noqa: UP010
    nested_scopes,  
    generators,
) 

generators

at least that's the only place I found that works.

But yeah, I think secondary annotations are probably better for highlighting additional context that helps to explain the main diagnostic but likely falls outside the normal snippet context window, like in your Rust example.

[IDrokin117]

It appears that the previous implementation, which uses separate diagnostics, functions well for # noqa: UP010 per import object. Should I revert to that approach?

[MichaReiser]

I'm leaning towards leaving it as is. I don't think that any of the tried approaches greatly improve the rendered diagnostics to justify a breaking change. But maybe Brent thinks differently? I'd otherwise close this PR and suggest that we revisit the rendering once we have figured out how to better handle those cases with our diagnostic system. But thank you a lot for trying out all those different approaches. It was very helpful to drive the decision-making and show where there's some need to improve our diagnostic system.

[ntBre]

Yeah, I think I agree with Micha. Thank you for working on this, I also agree that it was very helpful to see the different options!