[pyupgrade] Prevent infinite loop with I002 (UP010, UP035) (#19413)

## Summary

Fixes #18729 and fixes #16802

## Test Plan

Manually verified via CLI that Ruff no longer enters an infinite loop by
running:
```sh
echo 1 | ruff --isolated check - --select I002,UP010 --fix
```
with `required-imports = ["from __future__ import generator_stop"]` set
in the config, confirming “All checks passed!” and no snapshots were
generated.

---------

Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
This commit is contained in:
Dan Parizher 2025-07-31 15:17:27 -04:00 committed by GitHub
parent 2ab1502e51
commit b07def07c9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 123 additions and 7 deletions

View file

@ -7,16 +7,18 @@ pub(crate) mod types;
#[cfg(test)]
mod tests {
use std::collections::BTreeSet;
use std::path::Path;
use anyhow::Result;
use ruff_python_ast::PythonVersion;
use ruff_python_semantic::{MemberNameImport, NameImport};
use test_case::test_case;
use crate::registry::Rule;
use crate::rules::pyupgrade;
use crate::rules::{isort, pyupgrade};
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::test::{test_path, test_snippet};
use crate::{assert_diagnostics, settings};
#[test_case(Rule::ConvertNamedTupleFunctionalToClass, Path::new("UP014.py"))]
@ -294,4 +296,63 @@ mod tests {
assert_diagnostics!(diagnostics);
Ok(())
}
#[test]
fn i002_conflict() {
let diagnostics = test_snippet(
"from pipes import quote, Template",
&settings::LinterSettings {
isort: isort::settings::Settings {
required_imports: BTreeSet::from_iter([
// https://github.com/astral-sh/ruff/issues/18729
NameImport::ImportFrom(MemberNameImport::member(
"__future__".to_string(),
"generator_stop".to_string(),
)),
// https://github.com/astral-sh/ruff/issues/16802
NameImport::ImportFrom(MemberNameImport::member(
"collections".to_string(),
"Sequence".to_string(),
)),
// Only bail out if _all_ the names in UP035 are required. `pipes.Template`
// isn't flagged by UP035, so requiring it shouldn't prevent `pipes.quote`
// from getting a diagnostic.
NameImport::ImportFrom(MemberNameImport::member(
"pipes".to_string(),
"Template".to_string(),
)),
]),
..Default::default()
},
..settings::LinterSettings::for_rules([
Rule::MissingRequiredImport,
Rule::UnnecessaryFutureImport,
Rule::DeprecatedImport,
])
},
);
assert_diagnostics!(diagnostics, @r"
<filename>:1:1: UP035 [*] Import from `shlex` instead: `quote`
|
1 | from pipes import quote, Template
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP035
|
= help: Import from `shlex`
Safe fix
1 |-from pipes import quote, Template
1 |+from pipes import Template
2 |+from shlex import quote
<filename>:1:1: I002 [*] Missing required import: `from __future__ import generator_stop`
Safe fix
1 |+from __future__ import generator_stop
1 2 | from pipes import quote, Template
<filename>:1:1: I002 [*] Missing required import: `from collections import Sequence`
Safe fix
1 |+from collections import Sequence
1 2 | from pipes import quote, Template
");
}
}

View file

@ -2,7 +2,7 @@ use itertools::Itertools;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::whitespace::indentation;
use ruff_python_ast::{Alias, StmtImportFrom};
use ruff_python_ast::{Alias, StmtImportFrom, StmtRef};
use ruff_python_codegen::Stylist;
use ruff_python_parser::Tokens;
use ruff_text_size::Ranged;
@ -10,9 +10,12 @@ use ruff_text_size::Ranged;
use crate::Locator;
use crate::checkers::ast::Checker;
use crate::rules::pyupgrade::fixes;
use crate::rules::pyupgrade::rules::unnecessary_future_import::is_import_required_by_isort;
use crate::{Edit, Fix, FixAvailability, Violation};
use ruff_python_ast::PythonVersion;
use super::RequiredImports;
/// An import was moved and renamed as part of a deprecation.
/// For example, `typing.AbstractSet` was moved to `collections.abc.Set`.
#[derive(Debug, PartialEq, Eq)]
@ -410,6 +413,7 @@ struct ImportReplacer<'a> {
stylist: &'a Stylist<'a>,
tokens: &'a Tokens,
version: PythonVersion,
required_imports: &'a RequiredImports,
}
impl<'a> ImportReplacer<'a> {
@ -420,6 +424,7 @@ impl<'a> ImportReplacer<'a> {
stylist: &'a Stylist<'a>,
tokens: &'a Tokens,
version: PythonVersion,
required_imports: &'a RequiredImports,
) -> Self {
Self {
import_from_stmt,
@ -428,6 +433,7 @@ impl<'a> ImportReplacer<'a> {
stylist,
tokens,
version,
required_imports,
}
}
@ -437,6 +443,13 @@ impl<'a> ImportReplacer<'a> {
if self.module == "typing" {
if self.version >= PythonVersion::PY39 {
for member in &self.import_from_stmt.names {
if is_import_required_by_isort(
self.required_imports,
StmtRef::ImportFrom(self.import_from_stmt),
member,
) {
continue;
}
if let Some(target) = TYPING_TO_RENAME_PY39.iter().find_map(|(name, target)| {
if &member.name == *name {
Some(*target)
@ -673,7 +686,13 @@ impl<'a> ImportReplacer<'a> {
let mut matched_names = vec![];
let mut unmatched_names = vec![];
for name in &self.import_from_stmt.names {
if candidates.contains(&name.name.as_str()) {
if is_import_required_by_isort(
self.required_imports,
StmtRef::ImportFrom(self.import_from_stmt),
name,
) {
unmatched_names.push(name);
} else if candidates.contains(&name.name.as_str()) {
matched_names.push(name);
} else {
unmatched_names.push(name);
@ -726,6 +745,7 @@ pub(crate) fn deprecated_import(checker: &Checker, import_from_stmt: &StmtImport
checker.stylist(),
checker.tokens(),
checker.target_version(),
&checker.settings().isort.required_imports,
);
for (operation, fix) in fixer.without_renames() {

View file

@ -1,7 +1,10 @@
use std::collections::BTreeSet;
use itertools::Itertools;
use ruff_python_ast::{Alias, Stmt};
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{self as ast, Alias, Stmt, StmtRef};
use ruff_python_semantic::NameImport;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
@ -84,6 +87,29 @@ const PY37_PLUS_REMOVE_FUTURES: &[&str] = &[
"generator_stop",
];
pub(crate) type RequiredImports = BTreeSet<NameImport>;
pub(crate) fn is_import_required_by_isort(
required_imports: &RequiredImports,
stmt: StmtRef,
alias: &Alias,
) -> bool {
let segments: &[&str] = match stmt {
StmtRef::ImportFrom(ast::StmtImportFrom {
module: Some(module),
..
}) => &[module.as_str(), alias.name.as_str()],
StmtRef::ImportFrom(ast::StmtImportFrom { module: None, .. }) | StmtRef::Import(_) => {
&[alias.name.as_str()]
}
_ => return false,
};
required_imports
.iter()
.any(|required_import| required_import.qualified_name().segments() == segments)
}
/// UP010
pub(crate) fn unnecessary_future_import(checker: &Checker, stmt: &Stmt, names: &[Alias]) {
let mut unused_imports: Vec<&Alias> = vec![];
@ -91,6 +117,15 @@ pub(crate) fn unnecessary_future_import(checker: &Checker, stmt: &Stmt, names: &
if alias.asname.is_some() {
continue;
}
if is_import_required_by_isort(
&checker.settings().isort.required_imports,
stmt.into(),
alias,
) {
continue;
}
if PY33_PLUS_REMOVE_FUTURES.contains(&alias.name.as_str())
|| PY37_PLUS_REMOVE_FUTURES.contains(&alias.name.as_str())
{
@ -119,7 +154,7 @@ pub(crate) fn unnecessary_future_import(checker: &Checker, stmt: &Stmt, names: &
unused_imports
.iter()
.map(|alias| &alias.name)
.map(ruff_python_ast::Identifier::as_str),
.map(ast::Identifier::as_str),
statement,
parent,
checker.locator(),

View file

@ -380,7 +380,7 @@ macro_rules! assert_diagnostics {
}};
($value:expr, @$snapshot:literal) => {{
insta::with_settings!({ omit_expression => true }, {
insta::assert_snapshot!($crate::test::print_messages(&$value), $snapshot);
insta::assert_snapshot!($crate::test::print_messages(&$value), @$snapshot);
});
}};
($name:expr, $value:expr) => {{