Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
IDrokin117 wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e04c737
feat(UP010): separate diagnostics for each unused __future__ import
Aug 21, 2025
355e753
feat(up010): update snapshots
Sep 8, 2025
2f8a912
feat(UP010): use secondary annotation to underlying certain unused im…
Sep 30, 2025
affcf04
Merge branch 'astral-sh:main' into feat/ruff-19561-preview
IDrokin117 Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/ruff_linter/src/preview.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,8 @@ pub(crate) const fn is_a003_class_scope_shadowing_expansion_enabled(
pub(crate) const fn is_refined_submodule_import_match_enabled(settings: &LinterSettings) -> bool {
settings.preview.is_enabled()
}

// https://github.com/astral-sh/ruff/pull/20020
pub(crate) const fn is_separate_unused_import_diag_enabled(settings: &LinterSettings) -> bool {
settings.preview.is_enabled()
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ mod tests {
}

#[test_case(Rule::SuperCallWithParameters, Path::new("UP008.py"))]
#[test_case(Rule::UnnecessaryFutureImport, Path::new("UP010_0.py"))]
#[test_case(Rule::UnnecessaryFutureImport, Path::new("UP010_1.py"))]
fn rules_preview(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}__preview", path.to_string_lossy());
let diagnostics = test_path(
Expand Down
120 changes: 77 additions & 43 deletions crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
use std::collections::{BTreeSet, HashMap};

use itertools::{Itertools, chain};
use ruff_python_semantic::NodeId;

use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::name::{QualifiedName, QualifiedNameBuilder};
use ruff_python_ast::{self as ast, Alias, Stmt, StmtRef};
use ruff_python_semantic::NodeId;
use ruff_python_semantic::{NameImport, Scope};
use ruff_text_size::Ranged;
use ruff_text_size::{Ranged, TextRange};
use std::collections::{BTreeSet, HashMap};

use crate::checkers::ast::Checker;
use crate::fix;
use crate::preview::is_separate_unused_import_diag_enabled;
use crate::{AlwaysFixableViolation, Applicability, Fix};

/// ## What it does
Expand All @@ -35,6 +34,11 @@ use crate::{AlwaysFixableViolation, Applicability, Fix};
/// print("Hello, world!")
/// ```
///
/// ## Preview
///
/// When [preview] is enabled, this rule underlines each unused import individually
/// instead of grouping them together if there are two or more within an import statement.
///
/// ## Fix safety
/// This fix is marked unsafe if applying it would delete a comment.
///
Expand All @@ -43,6 +47,8 @@ use crate::{AlwaysFixableViolation, Applicability, Fix};
///
/// ## References
/// - [Python documentation: `__future__` — Future statement definitions](https://docs.python.org/3/library/__future__.html)
///
/// [preview]: https://docs.astral.sh/ruff/preview/
#[derive(ViolationMetadata)]
pub(crate) struct UnnecessaryFutureImport {
pub names: Vec<String>,
Expand Down Expand Up @@ -130,6 +136,7 @@ pub(crate) fn is_import_required_by_isort(
/// UP010
pub(crate) fn unnecessary_future_import(checker: &Checker, scope: &Scope) {
let mut unused_imports: HashMap<NodeId, Vec<&Alias>> = HashMap::new();
let mut import_counts: HashMap<NodeId, usize> = HashMap::new();
for future_name in chain(PY33_PLUS_REMOVE_FUTURES, PY37_PLUS_REMOVE_FUTURES).unique() {
for binding_id in scope.get_all(future_name) {
let binding = checker.semantic().binding(binding_id);
Expand All @@ -140,6 +147,10 @@ pub(crate) fn unnecessary_future_import(checker: &Checker, scope: &Scope) {

let stmt = checker.semantic().statement(node_id);
if let Stmt::ImportFrom(ast::StmtImportFrom { names, .. }) = stmt {
import_counts
.entry(node_id)
.and_modify(|c| *c += names.len())
.or_insert(names.len());
let Some(alias) = names
.iter()
.find(|alias| alias.name.as_str() == binding.name(checker.source()))
Expand All @@ -163,46 +174,69 @@ pub(crate) fn unnecessary_future_import(checker: &Checker, scope: &Scope) {
}
}
}

for (node_id, unused_aliases) in unused_imports {
let mut diagnostic = checker.report_diagnostic(
UnnecessaryFutureImport {
names: unused_aliases
.iter()
.map(|alias| alias.name.to_string())
.sorted()
.collect(),
},
create_diagnostic(
checker,
unused_aliases.as_slice(),
*import_counts.get(&node_id).unwrap_or(&unused_aliases.len()),
checker.semantic().statement(node_id).range(),
node_id,
);

diagnostic.try_set_fix(|| {
let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);
let edit = fix::edits::remove_unused_imports(
unused_aliases
.iter()
.map(|alias| &alias.name)
.map(ast::Identifier::as_str),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
)?;

let range = edit.range();
let applicability = if checker.comment_ranges().intersects(range) {
Applicability::Unsafe
} else {
Applicability::Safe
};

Ok(
Fix::applicable_edit(edit, applicability).isolate(Checker::isolation(
checker.semantic().current_statement_parent_id(),
)),
)
});
}
}
fn create_diagnostic(
checker: &Checker,
unused_aliases: &[&Alias],
import_counts: usize,
range: TextRange,
node_id: NodeId,
) {
let mut diagnostic = checker.report_diagnostic(
UnnecessaryFutureImport {
names: unused_aliases
.iter()
.map(|alias| alias.name.to_string())
.sorted()
.collect(),
},
range,
);

if is_separate_unused_import_diag_enabled(checker.settings()) && import_counts > 1 {
for unused_alias in unused_aliases {
diagnostic.secondary_annotation(
format!("Unused import `{}`", unused_alias.name),
unused_alias.range(),
);
}
}

diagnostic.try_set_fix(|| {
let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);
let edit = fix::edits::remove_unused_imports(
unused_aliases
.iter()
.map(|alias| &alias.name)
.map(ast::Identifier::as_str),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
)?;

let range = edit.range();
let applicability = if checker.comment_ranges().intersects(range) {
Applicability::Unsafe
} else {
Applicability::Safe
};

Ok(
Fix::applicable_edit(edit, applicability).isolate(Checker::isolation(
checker.semantic().current_statement_parent_id(),
)),
)
});
}
208 changes: 208 additions & 0 deletions .../rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_0.py__preview.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
---
source: crates/ruff_linter/src/rules/pyupgrade/mod.rs
---
UP010 [*] Unnecessary `__future__` imports `generators`, `nested_scopes` for target Python version
--> UP010_0.py:1:1
|
1 | from __future__ import nested_scopes, generators
| ^^^^^^^^^^^^^^^^^^^^^^^-------------^^----------
Comment on lines +7 to +8
Copy link
Contributor

@ntBre ntBre Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@MichaReiser MichaReiser Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
)

Copy link
Member

@MichaReiser MichaReiser Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ntBre ntBre Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@IDrokin117 IDrokin117 Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@MichaReiser MichaReiser Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ntBre ntBre Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

| | |
| | Unused import `generators`
| Unused import `nested_scopes`
2 | from __future__ import with_statement, unicode_literals
3 | from __future__ import absolute_import, division
|
help: Remove unnecessary `__future__` import
- from __future__ import nested_scopes, generators
1 | from __future__ import with_statement, unicode_literals
2 | from __future__ import absolute_import, division
3 | from __future__ import generator_stop

UP010 [*] Unnecessary `__future__` imports `unicode_literals`, `with_statement` for target Python version
--> UP010_0.py:2:1
|
1 | from __future__ import nested_scopes, generators
2 | from __future__ import with_statement, unicode_literals
| ^^^^^^^^^^^^^^^^^^^^^^^--------------^^----------------
| | |
| | Unused import `unicode_literals`
| Unused import `with_statement`
3 | from __future__ import absolute_import, division
4 | from __future__ import generator_stop
|
help: Remove unnecessary `__future__` import
1 | from __future__ import nested_scopes, generators
- from __future__ import with_statement, unicode_literals
2 | from __future__ import absolute_import, division
3 | from __future__ import generator_stop
4 | from __future__ import print_function, generator_stop

UP010 [*] Unnecessary `__future__` imports `absolute_import`, `division` for target Python version
--> UP010_0.py:3:1
|
1 | from __future__ import nested_scopes, generators
2 | from __future__ import with_statement, unicode_literals
3 | from __future__ import absolute_import, division
| ^^^^^^^^^^^^^^^^^^^^^^^---------------^^--------
| | |
| | Unused import `division`
| Unused import `absolute_import`
4 | from __future__ import generator_stop
5 | from __future__ import print_function, generator_stop
|
help: Remove unnecessary `__future__` import
1 | from __future__ import nested_scopes, generators
2 | from __future__ import with_statement, unicode_literals
- from __future__ import absolute_import, division
3 | from __future__ import generator_stop
4 | from __future__ import print_function, generator_stop
5 | from __future__ import invalid_module, generators

UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version
--> UP010_0.py:4:1
|
2 | from __future__ import with_statement, unicode_literals
3 | from __future__ import absolute_import, division
4 | from __future__ import generator_stop
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5 | from __future__ import print_function, generator_stop
6 | from __future__ import invalid_module, generators
|
help: Remove unnecessary `__future__` import
1 | from __future__ import nested_scopes, generators
2 | from __future__ import with_statement, unicode_literals
3 | from __future__ import absolute_import, division
- from __future__ import generator_stop
4 | from __future__ import print_function, generator_stop
5 | from __future__ import invalid_module, generators
6 |

UP010 [*] Unnecessary `__future__` imports `generator_stop`, `print_function` for target Python version
--> UP010_0.py:5:1
|
3 | from __future__ import absolute_import, division
4 | from __future__ import generator_stop
5 | from __future__ import print_function, generator_stop
| ^^^^^^^^^^^^^^^^^^^^^^^--------------^^--------------
| | |
| | Unused import `generator_stop`
| Unused import `print_function`
6 | from __future__ import invalid_module, generators
|
help: Remove unnecessary `__future__` import
2 | from __future__ import with_statement, unicode_literals
3 | from __future__ import absolute_import, division
4 | from __future__ import generator_stop
- from __future__ import print_function, generator_stop
5 | from __future__ import invalid_module, generators
6 |
7 | if True:

UP010 [*] Unnecessary `__future__` import `generators` for target Python version
--> UP010_0.py:6:1
|
4 | from __future__ import generator_stop
5 | from __future__ import print_function, generator_stop
6 | from __future__ import invalid_module, generators
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------
| |
| Unused import `generators`
7 |
8 | if True:
|
help: Remove unnecessary `__future__` import
3 | from __future__ import absolute_import, division
4 | from __future__ import generator_stop
5 | from __future__ import print_function, generator_stop
- from __future__ import invalid_module, generators
6 + from __future__ import invalid_module
7 |
8 | if True:
9 | from __future__ import generator_stop

UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version
--> UP010_0.py:9:5
|
8 | if True:
9 | from __future__ import generator_stop
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10 | from __future__ import generators
|
help: Remove unnecessary `__future__` import
6 | from __future__ import invalid_module, generators
7 |
8 | if True:
- from __future__ import generator_stop
9 | from __future__ import generators
10 |
11 | if True:

UP010 [*] Unnecessary `__future__` import `generators` for target Python version
--> UP010_0.py:10:5
|
8 | if True:
9 | from __future__ import generator_stop
10 | from __future__ import generators
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11 |
12 | if True:
|
help: Remove unnecessary `__future__` import
7 |
8 | if True:
9 | from __future__ import generator_stop
- from __future__ import generators
10 |
11 | if True:
12 | from __future__ import generator_stop

UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version
--> UP010_0.py:13:5
|
12 | if True:
13 | from __future__ import generator_stop
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14 | from __future__ import invalid_module, generators
15 | from __future__ import generators # comment
|
help: Remove unnecessary `__future__` import
10 | from __future__ import generators
11 |
12 | if True:
- from __future__ import generator_stop
13 | from __future__ import invalid_module, generators
14 | from __future__ import generators # comment

UP010 [*] Unnecessary `__future__` import `generators` for target Python version
--> UP010_0.py:14:5
|
12 | if True:
13 | from __future__ import generator_stop
14 | from __future__ import invalid_module, generators
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------
| |
| Unused import `generators`
15 | from __future__ import generators # comment
|
help: Remove unnecessary `__future__` import
11 |
12 | if True:
13 | from __future__ import generator_stop
- from __future__ import invalid_module, generators
14 + from __future__ import invalid_module
15 | from __future__ import generators # comment

UP010 [*] Unnecessary `__future__` import `generators` for target Python version
--> UP010_0.py:15:5
|
13 | from __future__ import generator_stop
14 | from __future__ import invalid_module, generators
15 | from __future__ import generators # comment
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: Remove unnecessary `__future__` import
12 | if True:
13 | from __future__ import generator_stop
14 | from __future__ import invalid_module, generators
- from __future__ import generators # comment
note: This is an unsafe fix and may change runtime behavior
Loading
Loading