-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[pyupgrade
] Prevent infinite loop with I002
(UP010
, UP035
)
#19413
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.
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
Conversation
|
There was a problem hiding this 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.
crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs
Outdated
Show resolved
Hide resolved
Thanks, I extracted that as a function and also added a check into |
There was a problem hiding this 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.
crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_future_import.rs
Outdated
Show resolved
Hide resolved
pyupgrade
] Prevent infinite loop with I002 (UP010
, UP035
)
pyupgrade
] Prevent infinite loop with I002 (UP010
, UP035
)pyupgrade
] Prevent infinite loop with I002
(UP010
, UP035
)
crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs
Outdated
Show resolved
Hide resolved
I pushed a few changes:
I'm not sure if the simplification actually paid off. It seems nice not to allocate strings for the |
* 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) ...
Summary
Fixes #18729 and fixes #16802
Test Plan
Manually verified via CLI that Ruff no longer enters an infinite loop by running:
with
required-imports = ["from __future__ import generator_stop"]
set in the config, confirming “All checks passed!” and no snapshots were generated.