Skip to content

Commit

Permalink
[pyflakes] Fix infinite loop with unused local import in `__init__.…
Browse files Browse the repository at this point in the history
…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 <Alex.Waygood@Gmail.com>
  • Loading branch information
ntBre and AlexWaygood authored Jan 16, 2025
1 parent 6f0b662 commit ca3b210
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ham = 1
29 changes: 29 additions & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
11 changes: 9 additions & 2 deletions crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -460,6 +465,8 @@ struct ImportBinding<'a> {
range: TextRange,
/// The range of the import's parent statement.
parent_range: Option<TextRange>,
/// The [`ScopeId`] of the scope in which the [`ImportBinding`] was defined.
scope: ScopeId,
}

impl ImportBinding<'_> {
Expand Down
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit ca3b210

Please sign in to comment.