From 7b75aee21d45bb84e6bb4d747cd5715dd0d03c95 Mon Sep 17 00:00:00 2001 From: Igor Drokin <41319097+IDrokin117@users.noreply.github.com> Date: Wed, 20 Aug 2025 22:22:03 +0300 Subject: [PATCH] [`pyupgrade`] Avoid reporting `__future__` features as unnecessary when they are used (`UP010`) (#19769) ## Summary Resolves #19561 Fixes the [unnecessary-future-import (UP010)](https://docs.astral.sh/ruff/rules/unnecessary-future-import/) rule to correctly identify when imported __future__ modules are actually used in the code, preventing false positives. I assume there is no way to check usage in `analyze::statements`, because we don't have any usage bindings for imports. To determine unused imports, we have to fully scan the file to create bindings and then check usage, similar to [unused-import (F401)](https://docs.astral.sh/ruff/rules/unused-import/#unused-import-f401). So, `Rule::UnnecessaryFutureImport` was moved from the `analyze::statements` to the `analyze::deferred_scopes` stage. This caused the need to change the logic of future import handling to a bindings-based approach. Also, the diagnostic report was changed. Before ``` | 1 | from __future__ import nested_scopes, generators | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP010 ``` after ``` | 1 | from __future__ import nested_scopes, generators | ^^^^^^^^^^^^^ UP010 ``` I believe this is the correct way, because `generators` may be used, but `nested_scopes` is not. ### Special case I've found out about some specific case. ```python from __future__ import nested_scopes nested_scopes = 1 ``` Here we can treat `nested_scopes` as an unused import because the variable `nested_scopes` shadows it and we can safely remove the future import (my fix does it). But [F401](https://docs.astral.sh/ruff/rules/unused-import/#unused-import-f401) not triggered for such case ([sandbox](https://play.ruff.rs/296d9c7e-0f02-4659-b0c0-78cc21f3de76)) ``` from foo import print_function print_function = 1 ``` In my mind, `print_function` here is an unused import and should be deleted (my IDE highlight it). What do you think? ## Test Plan Added test cases and snapshots: - Split test file into separate _0 and _1 files for appropriate checks. - Added test cases to verify fixes when future module are used. --------- Co-authored-by: Igor Drokin --- .../pyupgrade/{UP010.py => UP010_0.py} | 0 .../test/fixtures/pyupgrade/UP010_1.py | 18 +++ .../checkers/ast/analyze/deferred_scopes.rs | 9 +- .../src/checkers/ast/analyze/statement.rs | 7 - crates/ruff_linter/src/rules/pyupgrade/mod.rs | 3 +- .../rules/unnecessary_future_import.rs | 136 ++++++++++-------- ..._rules__pyupgrade__tests__UP010_0.py.snap} | 22 +-- ...__rules__pyupgrade__tests__UP010_1.py.snap | 99 +++++++++++++ 8 files changed, 213 insertions(+), 81 deletions(-) rename crates/ruff_linter/resources/test/fixtures/pyupgrade/{UP010.py => UP010_0.py} (100%) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_1.py rename crates/ruff_linter/src/rules/pyupgrade/snapshots/{ruff_linter__rules__pyupgrade__tests__UP010.py.snap => ruff_linter__rules__pyupgrade__tests__UP010_0.py.snap} (96%) create mode 100644 crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_1.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_0.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010.py rename to crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_0.py diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_1.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_1.py new file mode 100644 index 0000000000..33f9be894a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP010_1.py @@ -0,0 +1,18 @@ +from __future__ import nested_scopes, generators +from __future__ import with_statement, unicode_literals + +from __future__ import absolute_import, division +from __future__ import generator_stop +from __future__ import print_function, nested_scopes, generator_stop + +print(with_statement) +generators = 1 + + +class Foo(): + + def boo(self): + print(division) + + +__all__ = ["print_function", "generator_stop"] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 4971254acf..2cf6ec15bb 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -1,10 +1,11 @@ +use ruff_python_ast::PythonVersion; use ruff_python_semantic::{Binding, ScopeKind}; use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::rules::{ flake8_builtins, flake8_pyi, flake8_type_checking, flake8_unused_arguments, pep8_naming, - pyflakes, pylint, ruff, + pyflakes, pylint, pyupgrade, ruff, }; /// Run lint rules over all deferred scopes in the [`SemanticModel`]. @@ -45,6 +46,7 @@ pub(crate) fn deferred_scopes(checker: &Checker) { Rule::UnusedStaticMethodArgument, Rule::UnusedUnpackedVariable, Rule::UnusedVariable, + Rule::UnnecessaryFutureImport, ]) { return; } @@ -224,6 +226,11 @@ pub(crate) fn deferred_scopes(checker: &Checker) { if checker.is_rule_enabled(Rule::UnusedImport) { pyflakes::rules::unused_import(checker, scope); } + if checker.is_rule_enabled(Rule::UnnecessaryFutureImport) { + if checker.target_version() >= PythonVersion::PY37 { + pyupgrade::rules::unnecessary_future_import(checker, scope); + } + } if checker.is_rule_enabled(Rule::ImportPrivateName) { pylint::rules::import_private_name(checker, scope); diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 0cfb3a3cc7..164df3846a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -728,13 +728,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::non_ascii_module_import(checker, alias); } } - if checker.is_rule_enabled(Rule::UnnecessaryFutureImport) { - if checker.target_version() >= PythonVersion::PY37 { - if let Some("__future__") = module { - pyupgrade::rules::unnecessary_future_import(checker, stmt, names); - } - } - } if checker.is_rule_enabled(Rule::DeprecatedMockImport) { pyupgrade::rules::deprecated_mock_import(checker, stmt); } diff --git a/crates/ruff_linter/src/rules/pyupgrade/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/mod.rs index afc3f2c049..763e9f34b7 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/mod.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/mod.rs @@ -101,7 +101,8 @@ mod tests { #[test_case(Rule::UnnecessaryClassParentheses, Path::new("UP039.py"))] #[test_case(Rule::UnnecessaryDefaultTypeArgs, Path::new("UP043.py"))] #[test_case(Rule::UnnecessaryEncodeUTF8, Path::new("UP012.py"))] - #[test_case(Rule::UnnecessaryFutureImport, Path::new("UP010.py"))] + #[test_case(Rule::UnnecessaryFutureImport, Path::new("UP010_0.py"))] + #[test_case(Rule::UnnecessaryFutureImport, Path::new("UP010_1.py"))] #[test_case(Rule::UselessMetaclassType, Path::new("UP001.py"))] #[test_case(Rule::UselessObjectInheritance, Path::new("UP004.py"))] #[test_case(Rule::YieldInForLoop, Path::new("UP028_0.py"))] diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs index 1809cc3e16..51b8167172 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs @@ -1,10 +1,11 @@ -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; -use itertools::Itertools; +use itertools::{Itertools, chain}; +use ruff_python_semantic::NodeId; use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::{self as ast, Alias, Stmt, StmtRef}; -use ruff_python_semantic::NameImport; +use ruff_python_semantic::{NameImport, Scope}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -111,68 +112,81 @@ pub(crate) fn is_import_required_by_isort( } /// UP010 -pub(crate) fn unnecessary_future_import(checker: &Checker, stmt: &Stmt, names: &[Alias]) { - let mut unused_imports: Vec<&Alias> = vec![]; - for alias in names { - if alias.asname.is_some() { - continue; - } +pub(crate) fn unnecessary_future_import(checker: &Checker, scope: &Scope) { + let mut unused_imports: HashMap> = 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); + if binding.kind.is_future_import() && binding.is_unused() { + let Some(node_id) = binding.source else { + continue; + }; - if is_import_required_by_isort( - &checker.settings().isort.required_imports, - stmt.into(), - alias, - ) { - continue; - } + let stmt = checker.semantic().statement(node_id); + if let Stmt::ImportFrom(ast::StmtImportFrom { names, .. }) = stmt { + let Some(alias) = names + .iter() + .find(|alias| alias.name.as_str() == binding.name(checker.source())) + else { + continue; + }; - if PY33_PLUS_REMOVE_FUTURES.contains(&alias.name.as_str()) - || PY37_PLUS_REMOVE_FUTURES.contains(&alias.name.as_str()) - { - unused_imports.push(alias); + if alias.asname.is_some() { + continue; + } + + if is_import_required_by_isort( + &checker.settings().isort.required_imports, + stmt.into(), + alias, + ) { + continue; + } + unused_imports.entry(node_id).or_default().push(alias); + } + } } } - if unused_imports.is_empty() { - return; + 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(), + }, + checker.semantic().statement(node_id).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(), + )), + ) + }); } - let mut diagnostic = checker.report_diagnostic( - UnnecessaryFutureImport { - names: unused_imports - .iter() - .map(|alias| alias.name.to_string()) - .sorted() - .collect(), - }, - stmt.range(), - ); - - diagnostic.try_set_fix(|| { - let statement = checker.semantic().current_statement(); - let parent = checker.semantic().current_statement_parent(); - let edit = fix::edits::remove_unused_imports( - unused_imports - .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(), - )), - ) - }); } diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_0.py.snap similarity index 96% rename from crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010.py.snap rename to crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_0.py.snap index 58c064522c..dc038f9c44 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_0.py.snap @@ -2,7 +2,7 @@ source: crates/ruff_linter/src/rules/pyupgrade/mod.rs --- UP010 [*] Unnecessary `__future__` imports `generators`, `nested_scopes` for target Python version - --> UP010.py:1:1 + --> UP010_0.py:1:1 | 1 | from __future__ import nested_scopes, generators | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -18,7 +18,7 @@ help: Remove unnecessary `__future__` import 4 3 | from __future__ import generator_stop UP010 [*] Unnecessary `__future__` imports `unicode_literals`, `with_statement` for target Python version - --> UP010.py:2:1 + --> UP010_0.py:2:1 | 1 | from __future__ import nested_scopes, generators 2 | from __future__ import with_statement, unicode_literals @@ -36,7 +36,7 @@ help: Remove unnecessary `__future__` import 5 4 | from __future__ import print_function, generator_stop UP010 [*] Unnecessary `__future__` imports `absolute_import`, `division` for target Python version - --> UP010.py:3:1 + --> UP010_0.py:3:1 | 1 | from __future__ import nested_scopes, generators 2 | from __future__ import with_statement, unicode_literals @@ -56,7 +56,7 @@ help: Remove unnecessary `__future__` import 6 5 | from __future__ import invalid_module, generators UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version - --> UP010.py:4:1 + --> UP010_0.py:4:1 | 2 | from __future__ import with_statement, unicode_literals 3 | from __future__ import absolute_import, division @@ -77,7 +77,7 @@ help: Remove unnecessary `__future__` import 7 6 | UP010 [*] Unnecessary `__future__` imports `generator_stop`, `print_function` for target Python version - --> UP010.py:5:1 + --> UP010_0.py:5:1 | 3 | from __future__ import absolute_import, division 4 | from __future__ import generator_stop @@ -97,7 +97,7 @@ help: Remove unnecessary `__future__` import 8 7 | if True: UP010 [*] Unnecessary `__future__` import `generators` for target Python version - --> UP010.py:6:1 + --> UP010_0.py:6:1 | 4 | from __future__ import generator_stop 5 | from __future__ import print_function, generator_stop @@ -119,7 +119,7 @@ help: Remove unnecessary `__future__` import 9 9 | from __future__ import generator_stop UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version - --> UP010.py:9:5 + --> UP010_0.py:9:5 | 8 | if True: 9 | from __future__ import generator_stop @@ -138,7 +138,7 @@ help: Remove unnecessary `__future__` import 12 11 | if True: UP010 [*] Unnecessary `__future__` import `generators` for target Python version - --> UP010.py:10:5 + --> UP010_0.py:10:5 | 8 | if True: 9 | from __future__ import generator_stop @@ -159,7 +159,7 @@ help: Remove unnecessary `__future__` import 13 12 | from __future__ import generator_stop UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version - --> UP010.py:13:5 + --> UP010_0.py:13:5 | 12 | if True: 13 | from __future__ import generator_stop @@ -178,7 +178,7 @@ help: Remove unnecessary `__future__` import 15 14 | from __future__ import generators # comment UP010 [*] Unnecessary `__future__` import `generators` for target Python version - --> UP010.py:14:5 + --> UP010_0.py:14:5 | 12 | if True: 13 | from __future__ import generator_stop @@ -197,7 +197,7 @@ help: Remove unnecessary `__future__` import 15 15 | from __future__ import generators # comment UP010 [*] Unnecessary `__future__` import `generators` for target Python version - --> UP010.py:15:5 + --> UP010_0.py:15:5 | 13 | from __future__ import generator_stop 14 | from __future__ import invalid_module, generators diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_1.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_1.py.snap new file mode 100644 index 0000000000..be0f0ce24d --- /dev/null +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP010_1.py.snap @@ -0,0 +1,99 @@ +--- +source: crates/ruff_linter/src/rules/pyupgrade/mod.rs +--- +UP010 [*] Unnecessary `__future__` imports `generators`, `nested_scopes` for target Python version + --> UP010_1.py:1:1 + | +1 | from __future__ import nested_scopes, generators + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +2 | from __future__ import with_statement, unicode_literals + | +help: Remove unnecessary `__future__` import + +ℹ Safe fix +1 |-from __future__ import nested_scopes, generators +2 1 | from __future__ import with_statement, unicode_literals +3 2 | +4 3 | from __future__ import absolute_import, division + +UP010 [*] Unnecessary `__future__` import `unicode_literals` for target Python version + --> UP010_1.py:2:1 + | +1 | from __future__ import nested_scopes, generators +2 | from __future__ import with_statement, unicode_literals + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +3 | +4 | from __future__ import absolute_import, division + | +help: Remove unnecessary `__future__` import + +ℹ Safe fix +1 1 | from __future__ import nested_scopes, generators +2 |-from __future__ import with_statement, unicode_literals + 2 |+from __future__ import with_statement +3 3 | +4 4 | from __future__ import absolute_import, division +5 5 | from __future__ import generator_stop + +UP010 [*] Unnecessary `__future__` import `absolute_import` for target Python version + --> UP010_1.py:4:1 + | +2 | from __future__ import with_statement, unicode_literals +3 | +4 | from __future__ import absolute_import, division + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +5 | from __future__ import generator_stop +6 | from __future__ import print_function, nested_scopes, generator_stop + | +help: Remove unnecessary `__future__` import + +ℹ Safe fix +1 1 | from __future__ import nested_scopes, generators +2 2 | from __future__ import with_statement, unicode_literals +3 3 | +4 |-from __future__ import absolute_import, division + 4 |+from __future__ import division +5 5 | from __future__ import generator_stop +6 6 | from __future__ import print_function, nested_scopes, generator_stop +7 7 | + +UP010 [*] Unnecessary `__future__` import `generator_stop` for target Python version + --> UP010_1.py:5:1 + | +4 | from __future__ import absolute_import, division +5 | from __future__ import generator_stop + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +6 | from __future__ import print_function, nested_scopes, generator_stop + | +help: Remove unnecessary `__future__` import + +ℹ Safe fix +2 2 | from __future__ import with_statement, unicode_literals +3 3 | +4 4 | from __future__ import absolute_import, division +5 |-from __future__ import generator_stop +6 5 | from __future__ import print_function, nested_scopes, generator_stop +7 6 | +8 7 | print(with_statement) + +UP010 [*] Unnecessary `__future__` import `nested_scopes` for target Python version + --> UP010_1.py:6:1 + | +4 | from __future__ import absolute_import, division +5 | from __future__ import generator_stop +6 | from __future__ import print_function, nested_scopes, generator_stop + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +7 | +8 | print(with_statement) + | +help: Remove unnecessary `__future__` import + +ℹ Safe fix +3 3 | +4 4 | from __future__ import absolute_import, division +5 5 | from __future__ import generator_stop +6 |-from __future__ import print_function, nested_scopes, generator_stop + 6 |+from __future__ import print_function, generator_stop +7 7 | +8 8 | print(with_statement) +9 9 | generators = 1