Skip to content

Conversation

danparizher
Copy link
Contributor

@danparizher danparizher commented Jul 17, 2025

Summary

Fixes #18729 and fixes #16802

Test Plan

Manually verified via CLI that Ruff no longer enters an infinite loop by running:

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.

Copy link
Contributor

github-actions bot commented Jul 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre self-requested a review July 24, 2025 13:08
@ntBre ntBre added the bug Something isn't working label Jul 25, 2025
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks! I had one suggestion to refactor the fix here, and we should also add a test. Since the fix is on the UP010 side, I think it would make sense to add a unit test in the pyupgrade module too. You should be able to select both rules and use the normal snapshot infrastructure otherwise.

@danparizher
Copy link
Contributor Author

Thanks, I extracted that as a function and also added a check into crates\ruff_linter\src\rules\pyupgrade\rules\deprecated_import.rs. Let me know if that's correct. For adding tests for the whole pyupgrade module, what would that look like? Would appreciate recommendations

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

I found one new suggestion for the implementation, but this looks good otherwise.

For the test, I wrote up this patch:

Patch
diff --git a/crates/ruff_linter/src/rules/pyupgrade/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/mod.rs
index 3f64cb323f..919958e48e 100644
--- a/crates/ruff_linter/src/rules/pyupgrade/mod.rs
+++ b/crates/ruff_linter/src/rules/pyupgrade/mod.rs
@@ -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,44 @@ mod tests {
         assert_diagnostics!(diagnostics);
         Ok(())
     }
+
+    #[test]
+    fn i002_conflict() {
+        let diagnostics = test_snippet(
+            "1",
+            &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(),
+                        )),
+                    ]),
+                    ..Default::default()
+                },
+                ..settings::LinterSettings::for_rules([
+                    Rule::MissingRequiredImport,
+                    Rule::UnnecessaryFutureImport,
+                    Rule::DeprecatedImport,
+                ])
+            },
+        );
+        assert_diagnostics!(diagnostics, @r"
+        <filename>:1:1: I002 [*] Missing required import: `from __future__ import generator_stop`
+        ℹ Safe fix
+          1 |+from __future__ import generator_stop
+        1 2 | 1
+
+        <filename>:1:1: I002 [*] Missing required import: `from collections import Sequence`
+        ℹ Safe fix
+          1 |+from collections import Sequence
+        1 2 | 1
+        ");
+    }
 }
diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs
index cf63762b8a..02eca4ca05 100644
--- a/crates/ruff_linter/src/test.rs
+++ b/crates/ruff_linter/src/test.rs
@@ -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) => {{

I guess nobody had ever tried to use the inline snapshot version of assert_diagnostics! because the @ wasn't being passed along.

This test fails on main with the expected failure to converge, but with your fixes it yields the expected snapshot. We fix the I002 issues and don't raise UP010 or UP035. Nice work.

@danparizher danparizher requested a review from ntBre July 30, 2025 15:34
@ntBre ntBre changed the title Prevent infinite fix loop between I002 and UP010 when required import is also unnecessary [pyupgrade] Prevent infinite loop with I002 (UP010, UP035) Jul 30, 2025
@ntBre ntBre changed the title [pyupgrade] Prevent infinite loop with I002 (UP010, UP035) [pyupgrade] Prevent infinite loop with I002 (UP010, UP035) Jul 30, 2025
@ntBre
Copy link
Contributor

ntBre commented Jul 31, 2025

I pushed a few changes:

  • Stored the required imports on ImportReplacer instead of a HashSet so we can avoid allocating that up front
  • Pushed to unmatched_names instead of continuing in partition_imports (this was causing the from pipes import Template import to be dropped when removing the quote import)
  • Combined i002_conflict and up035_partial_required_import, that's what I had in mind initially. I don't think we need both tests separately
  • Tried to simplify is_import_required_by_isort a bit

I'm not sure if the simplification actually paid off. It seems nice not to allocate strings for the NameImports, but now we have to call qualified_name repeatedly in the any check. Your version wasn't showing any performance regression, so if this regresses performance at all, I'll just put it back.

@ntBre ntBre merged commit b07def0 into astral-sh:main Jul 31, 2025
35 checks passed
@danparizher danparizher deleted the fix-18729 branch July 31, 2025 20:44
dcreager added a commit that referenced this pull request Aug 1, 2025
* main: (39 commits)
  [ty] Initial test suite for `TypedDict` (#19686)
  [ty] Improve debuggability of protocol types (#19662)
  [ty] Simplify lifetime requirements for `PySlice` trait (#19687)
  [ty] Improve `isinstance()` truthiness analysis for generic types (#19668)
  [`refurb`] Make example error out-of-the-box (`FURB164`) (#19673)
  Fix link: unused_import.rs (#19648)
  [ty] Remove `Specialization::display` (full) (#19682)
  [ty] Remove `KnownModule::is_enum` (#19681)
  [ty] Support `__setitem__` and improve `__getitem__` related diagnostics (#19578)
  [ty] Sync vendored typeshed stubs (#19676)
  [`flake8-use-pathlib`] Expand `PTH201` to check all `PurePath` subclasses (#19440)
  [`refurb`] Make example error out-of-the-box (`FURB180`) (#19672)
  [`pyupgrade`] Prevent infinite loop with `I002` (`UP010`, `UP035`) (#19413)
  [ty] Improve the `Display` for generic `type[]` types (#19667)
  [ty] Refactor `TypeInferenceBuilder::infer_subscript_expression_types` (#19658)
  Fix tests on 32-bit architectures (#19652)
  [ty] Move `pandas-stubs` to bad.txt (#19659)
  [ty] Remove special casing for string-literal-in-tuple `__contains__` (#19642)
  Update pre-commit's `ruff` id (#19654)
  Update salsa (#19449)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infinite loop] I002 and UP010 [Infinite loop] I002 and UP035
2 participants