From ca3b210f2e0839c00ace4d2762092301f92c32f6 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 16 Jan 2025 10:43:32 -0500 Subject: [PATCH] [`pyflakes`] Fix infinite loop with unused local import in `__init__.py` (`F401`) (#15517) ## Summary This fixes the infinite loop reported in #12897, where an `unused-import` that is undefined at the scope of `__all__` is "fixed" by adding it to `__all__` repeatedly. These changes make it so that only imports in the global scope will be suggested to add to `__all__` and the unused local import is simply removed. ## Test Plan Added a CLI integration test that sets up the same module structure as the original report Closes #12897 --------- Co-authored-by: Alex Waygood --- .../fixtures/pyflakes/F401_33/__init__.py | 8 +++++ .../test/fixtures/pyflakes/F401_33/other.py | 1 + crates/ruff_linter/src/rules/pyflakes/mod.rs | 29 +++++++++++++++++++ .../src/rules/pyflakes/rules/unused_import.rs | 11 +++++-- ...s__preview__F401_F401_33____init__.py.snap | 19 ++++++++++++ 5 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/__init__.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/other.py create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_33____init__.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/__init__.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/__init__.py new file mode 100644 index 0000000000000..4375c1931ef5b --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/__init__.py @@ -0,0 +1,8 @@ +"""Regression test for https://github.com/astral-sh/ruff/issues/12897""" + +__all__ = ('Spam',) + + +class Spam: + def __init__(self) -> None: + from F401_33.other import Ham diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/other.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/other.py new file mode 100644 index 0000000000000..364ca1d04652c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F401_33/other.py @@ -0,0 +1 @@ +Ham = 1 diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index 66ae601b2548c..f20dd088c74f0 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -286,6 +286,35 @@ mod tests { assert_messages!(snapshot, diagnostics); } + // Regression test for https://github.com/astral-sh/ruff/issues/12897 + #[test_case(Rule::UnusedImport, Path::new("F401_33/__init__.py"))] + fn f401_preview_local_init_import(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let settings = LinterSettings { + preview: PreviewMode::Enabled, + isort: isort::settings::Settings { + // Like `f401_preview_first_party_submodule`, this test requires the input module to + // be first-party + known_modules: isort::categorize::KnownModules::new( + vec!["F401_*".parse()?], + vec![], + vec![], + vec![], + FxHashMap::default(), + ), + ..isort::settings::Settings::default() + }, + ..LinterSettings::for_rule(rule_code) + }; + let diagnostics = test_path(Path::new("pyflakes").join(path).as_path(), &settings)?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Rule::UnusedImport, Path::new("F401_24/__init__.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_25__all_nonempty/__init__.py"))] #[test_case(Rule::UnusedImport, Path::new("F401_26__all_empty/__init__.py"))] diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index 04acc5a30eaab..487b190f0ad49 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs @@ -9,7 +9,8 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::name::QualifiedName; use ruff_python_ast::{self as ast, Stmt}; use ruff_python_semantic::{ - AnyImport, BindingKind, Exceptions, Imported, NodeId, Scope, SemanticModel, SubmoduleImport, + AnyImport, BindingKind, Exceptions, Imported, NodeId, Scope, ScopeId, SemanticModel, + SubmoduleImport, }; use ruff_text_size::{Ranged, TextRange}; @@ -329,6 +330,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut import, range: binding.range(), parent_range: binding.parent_range(checker.semantic()), + scope: binding.scope, }; if checker.rule_is_ignored(Rule::UnusedImport, import.start()) @@ -366,7 +368,10 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut .map(|binding| { let context = if in_except_handler { UnusedImportContext::ExceptHandler - } else if in_init && is_first_party(&binding.import, checker) { + } else if in_init + && binding.scope.is_global() + && is_first_party(&binding.import, checker) + { UnusedImportContext::DunderInitFirstParty { dunder_all_count: DunderAllCount::from(dunder_all_exprs.len()), submodule_import: binding.import.is_submodule_import(), @@ -460,6 +465,8 @@ struct ImportBinding<'a> { range: TextRange, /// The range of the import's parent statement. parent_range: Option, + /// The [`ScopeId`] of the scope in which the [`ImportBinding`] was defined. + scope: ScopeId, } impl ImportBinding<'_> { diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_33____init__.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_33____init__.py.snap new file mode 100644 index 0000000000000..d17deaa8387c3 --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_33____init__.py.snap @@ -0,0 +1,19 @@ +--- +source: crates/ruff_linter/src/rules/pyflakes/mod.rs +snapshot_kind: text +--- +__init__.py:8:35: F401 [*] `F401_33.other.Ham` imported but unused + | +6 | class Spam: +7 | def __init__(self) -> None: +8 | from F401_33.other import Ham + | ^^^ F401 + | + = help: Remove unused import: `F401_33.other.Ham` + +ℹ Unsafe fix +5 5 | +6 6 | class Spam: +7 7 | def __init__(self) -> None: +8 |- from F401_33.other import Ham + 8 |+ pass