From 59afff0e6a3aabc1bdbda5e6d333529911a8a101 Mon Sep 17 00:00:00 2001
From: plredmond <51248199+plredmond@users.noreply.github.com>
Date: Thu, 2 May 2024 16:10:32 -0700
Subject: [PATCH] F401 - Distinguish between imports we wish to remove and
those we wish to make explicit-exports (#11168)
Resolves #10390 and starts to address #10391
# Changes to behavior
* In `__init__.py` we now offer some fixes for unused imports.
* If the import binding is first-party this PR suggests a fix to turn it
into a redundant alias.
* If the import binding is not first-party, this PR suggests a fix to
remove it from the `__init__.py`.
* The fix-titles are specific to these new suggested fixes.
* `checker.settings.ignore_init_module_imports` setting is
deprecated/ignored. There is probably a documentation change to make
that complete which I haven't done.
---
Old description of implementation changes
# Changes to the implementation
* In the body of the loop over import statements that contain unused
bindings, the bindings are partitioned into `to_reexport` and
`to_remove` (according to how we want to resolve the fact they're
unused) with the following predicate:
```rust
in_init && is_first_party(checker, &import.qualified_name().to_string())
// true means make it a reexport
```
* Instead of generating a single fix per import statement, we now
generate up to two fixes per import statement:
```rust
(fix_by_removing_imports(checker, node_id, &to_remove, in_init).ok(),
fix_by_reexporting(checker, node_id, &to_reexport, dunder_all).ok())
```
* The `to_remove` fixes are unsafe when `in_init`.
* The `to_explicit` fixes are safe. Currently, until a future PR, we
make them redundant aliases (e.g. `import a` would become `import a as
a`).
## Other changes
* `checker.settings.ignore_init_module_imports` is deprecated/ignored.
Instead, all fixes are gated on `checker.settings.preview.is_enabled()`.
* Got rid of the pattern match on the import-binding bound by the inner
loop because it seemed less readable than referencing fields on the
binding.
* [x] `// FIXME: rename "imports" to "bindings"` if reviewer agrees (see
code)
* [x] `// FIXME: rename "node_id" to "import_statement"` if reviewer
agrees (see code)
Scope cut until a future PR
* (Not implemented) The `to_explicit` fixes will be added to `__all__`
unless it doesn't exist. When `__all__` doesn't exist they're resolved
by converting to redundant aliases (e.g. `import a` would become `import
a as a`).
---
# Test plan
* [x] `crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24`
contains an `__init__.py` with*out* `__all__` that exercises the
features in this PR, but it doesn't pass.
* [x]
`crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25_dunder_all`
contains an `__init__.py` *with* `__all__` that exercises the features
in this PR, but it doesn't pass.
* [x] Write unit tests for the new edit functions in
`fix::edits::make_redundant_alias`.
- ,
+ stmt: &Stmt,
+) -> Vec {
+ let aliases = match stmt {
+ Stmt::Import(ast::StmtImport { names, .. }) => names,
+ Stmt::ImportFrom(ast::StmtImportFrom { names, .. }) => names,
+ _ => {
+ return Vec::new();
+ }
+ };
+ member_names
+ .filter_map(|name| {
+ aliases
+ .iter()
+ .find(|alias| alias.asname.is_none() && name == alias.name.id)
+ .map(|alias| Edit::range_replacement(format!("{name} as {name}"), alias.range))
+ })
+ .collect()
+}
+
#[derive(Debug, Copy, Clone)]
pub(crate) enum Parentheses {
/// Remove parentheses, if the removed argument is the only argument left.
@@ -457,11 +479,12 @@ fn all_lines_fit(
mod tests {
use anyhow::Result;
+ use ruff_diagnostics::Edit;
use ruff_python_parser::parse_suite;
use ruff_source_file::Locator;
- use ruff_text_size::{Ranged, TextSize};
+ use ruff_text_size::{Ranged, TextRange, TextSize};
- use crate::fix::edits::{next_stmt_break, trailing_semicolon};
+ use crate::fix::edits::{make_redundant_alias, next_stmt_break, trailing_semicolon};
#[test]
fn find_semicolon() -> Result<()> {
@@ -532,4 +555,35 @@ x = 1 \
TextSize::from(12)
);
}
+
+ #[test]
+ fn redundant_alias() {
+ let contents = "import x, y as y, z as bees";
+ let program = parse_suite(contents).unwrap();
+ let stmt = program.first().unwrap();
+ assert_eq!(
+ make_redundant_alias(["x"].into_iter(), stmt),
+ vec![Edit::range_replacement(
+ String::from("x as x"),
+ TextRange::new(TextSize::new(7), TextSize::new(8)),
+ )],
+ "make just one item redundant"
+ );
+ assert_eq!(
+ make_redundant_alias(vec!["x", "y"].into_iter(), stmt),
+ vec![Edit::range_replacement(
+ String::from("x as x"),
+ TextRange::new(TextSize::new(7), TextSize::new(8)),
+ )],
+ "the second item is already a redundant alias"
+ );
+ assert_eq!(
+ make_redundant_alias(vec!["x", "z"].into_iter(), stmt),
+ vec![Edit::range_replacement(
+ String::from("x as x"),
+ TextRange::new(TextSize::new(7), TextSize::new(8)),
+ )],
+ "the third item is already aliased to something else"
+ );
+ }
}
diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs
index a7736614012f5..6212c18c585ee 100644
--- a/crates/ruff_linter/src/rules/pyflakes/mod.rs
+++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs
@@ -205,6 +205,9 @@ mod tests {
}
#[test_case(Rule::UnusedVariable, Path::new("F841_4.py"))]
+ #[test_case(Rule::UnusedImport, Path::new("__init__.py"))]
+ #[test_case(Rule::UnusedImport, Path::new("F401_24/__init__.py"))]
+ #[test_case(Rule::UnusedImport, Path::new("F401_25__all/__init__.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
@@ -249,19 +252,6 @@ mod tests {
Ok(())
}
- #[test]
- fn init_unused_import_opt_in_to_fix() -> Result<()> {
- let diagnostics = test_path(
- Path::new("pyflakes/__init__.py"),
- &LinterSettings {
- ignore_init_module_imports: false,
- ..LinterSettings::for_rules(vec![Rule::UnusedImport])
- },
- )?;
- assert_messages!(diagnostics);
- Ok(())
- }
-
#[test]
fn default_builtins() -> Result<()> {
let diagnostics = test_path(
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 df446ff608e0c..963bc8dd2e19b 100644
--- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
+++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
@@ -1,21 +1,24 @@
use std::borrow::Cow;
+use std::iter;
-use anyhow::Result;
+use anyhow::{anyhow, bail, Result};
use rustc_hash::FxHashMap;
use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{Stmt, StmtImportFrom};
use ruff_python_semantic::{AnyImport, Exceptions, Imported, NodeId, Scope};
use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker;
use crate::fix;
use crate::registry::Rule;
+use crate::rules::{isort, isort::ImportSection, isort::ImportType};
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
enum UnusedImportContext {
ExceptHandler,
- Init,
+ Init { first_party: bool },
}
/// ## What it does
@@ -93,7 +96,7 @@ impl Violation for UnusedImport {
"`{name}` imported but unused; consider using `importlib.util.find_spec` to test for availability"
)
}
- Some(UnusedImportContext::Init) => {
+ Some(UnusedImportContext::Init { .. }) => {
format!(
"`{name}` imported but unused; consider removing, adding to `__all__`, or using a redundant alias"
)
@@ -104,14 +107,47 @@ impl Violation for UnusedImport {
fn fix_title(&self) -> Option {
let UnusedImport { name, multiple, .. } = self;
+ let resolution = match self.context {
+ Some(UnusedImportContext::Init { first_party: true }) => "Use a redundant alias",
+ _ => "Remove unused import",
+ };
Some(if *multiple {
- "Remove unused import".to_string()
+ resolution.to_string()
} else {
- format!("Remove unused import: `{name}`")
+ format!("{resolution}: `{name}`")
})
}
}
+fn is_first_party(qualified_name: &str, level: u32, checker: &Checker) -> bool {
+ let category = isort::categorize(
+ qualified_name,
+ level,
+ &checker.settings.src,
+ checker.package(),
+ checker.settings.isort.detect_same_package,
+ &checker.settings.isort.known_modules,
+ checker.settings.target_version,
+ checker.settings.isort.no_sections,
+ &checker.settings.isort.section_order,
+ &checker.settings.isort.default_section,
+ );
+ matches! {
+ category,
+ ImportSection::Known(ImportType::FirstParty | ImportType::LocalFolder)
+ }
+}
+
+/// For some unused binding in an import statement...
+///
+/// __init__.py ∧ 1stpty → safe, convert to redundant-alias
+/// __init__.py ∧ stdlib → unsafe, remove
+/// __init__.py ∧ 3rdpty → unsafe, remove
+///
+/// ¬__init__.py ∧ 1stpty → safe, remove
+/// ¬__init__.py ∧ stdlib → safe, remove
+/// ¬__init__.py ∧ 3rdpty → safe, remove
+///
pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) {
// Collect all unused imports by statement.
let mut unused: FxHashMap<(NodeId, Exceptions), Vec> = FxHashMap::default();
@@ -160,42 +196,82 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}
let in_init = checker.path().ends_with("__init__.py");
- let fix_init = !checker.settings.ignore_init_module_imports;
+ let fix_init = checker.settings.preview.is_enabled();
- // Generate a diagnostic for every import, but share a fix across all imports within the same
+ // Generate a diagnostic for every import, but share fixes across all imports within the same
// statement (excluding those that are ignored).
- for ((node_id, exceptions), imports) in unused {
+ for ((import_statement, exceptions), bindings) in unused {
let in_except_handler =
exceptions.intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR);
- let multiple = imports.len() > 1;
+ let multiple = bindings.len() > 1;
+ let level = match checker.semantic().statement(import_statement) {
+ Stmt::Import(_) => 0,
+ Stmt::ImportFrom(StmtImportFrom { level, .. }) => *level,
+ _ => {
+ continue;
+ }
+ };
+
+ // pair each binding with context; divide them by how we want to fix them
+ let (to_reexport, to_remove): (Vec<_>, Vec<_>) = bindings
+ .into_iter()
+ .map(|binding| {
+ let context = if in_except_handler {
+ Some(UnusedImportContext::ExceptHandler)
+ } else if in_init {
+ Some(UnusedImportContext::Init {
+ first_party: is_first_party(
+ &binding.import.qualified_name().to_string(),
+ level,
+ checker,
+ ),
+ })
+ } else {
+ None
+ };
+ (binding, context)
+ })
+ .partition(|(_, context)| {
+ matches!(
+ context,
+ Some(UnusedImportContext::Init { first_party: true })
+ )
+ });
- let fix = if (!in_init || fix_init) && !in_except_handler {
- fix_imports(checker, node_id, &imports, in_init).ok()
+ // generate fixes that are shared across bindings in the statement
+ let (fix_remove, fix_reexport) = if (!in_init || fix_init) && !in_except_handler {
+ (
+ fix_by_removing_imports(
+ checker,
+ import_statement,
+ to_remove.iter().map(|(binding, _)| binding),
+ in_init,
+ )
+ .ok(),
+ fix_by_reexporting(
+ checker,
+ import_statement,
+ to_reexport.iter().map(|(binding, _)| binding),
+ )
+ .ok(),
+ )
} else {
- None
+ (None, None)
};
- for ImportBinding {
- import,
- range,
- parent_range,
- } in imports
- {
+ for ((binding, context), fix) in iter::Iterator::chain(
+ iter::zip(to_remove, iter::repeat(fix_remove)),
+ iter::zip(to_reexport, iter::repeat(fix_reexport)),
+ ) {
let mut diagnostic = Diagnostic::new(
UnusedImport {
- name: import.qualified_name().to_string(),
- context: if in_except_handler {
- Some(UnusedImportContext::ExceptHandler)
- } else if in_init {
- Some(UnusedImportContext::Init)
- } else {
- None
- },
+ name: binding.import.qualified_name().to_string(),
+ context,
multiple,
},
- range,
+ binding.range,
);
- if let Some(range) = parent_range {
+ if let Some(range) = binding.parent_range {
diagnostic.set_parent(range.start());
}
if !in_except_handler {
@@ -248,20 +324,22 @@ impl Ranged for ImportBinding<'_> {
}
/// Generate a [`Fix`] to remove unused imports from a statement.
-fn fix_imports(
+fn fix_by_removing_imports<'a>(
checker: &Checker,
node_id: NodeId,
- imports: &[ImportBinding],
+ imports: impl Iterator
- >,
in_init: bool,
) -> Result {
let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);
let member_names: Vec> = imports
- .iter()
.map(|ImportBinding { import, .. }| import)
.map(Imported::member_name)
.collect();
+ if member_names.is_empty() {
+ bail!("Expected import bindings");
+ }
let edit = fix::edits::remove_unused_imports(
member_names.iter().map(AsRef::as_ref),
@@ -271,15 +349,43 @@ fn fix_imports(
checker.stylist(),
checker.indexer(),
)?;
+
// It's unsafe to remove things from `__init__.py` because it can break public interfaces
let applicability = if in_init {
Applicability::Unsafe
} else {
Applicability::Safe
};
+
Ok(
Fix::applicable_edit(edit, applicability).isolate(Checker::isolation(
checker.semantic().parent_statement_id(node_id),
)),
)
}
+
+/// Generate a [`Fix`] to make bindings in a statement explicit, by changing from `import a` to
+/// `import a as a`.
+fn fix_by_reexporting<'a>(
+ checker: &Checker,
+ node_id: NodeId,
+ imports: impl Iterator
- >,
+) -> Result {
+ let statement = checker.semantic().statement(node_id);
+
+ let member_names = imports
+ .map(|binding| binding.import.member_name())
+ .collect::>();
+ if member_names.is_empty() {
+ bail!("Expected import bindings");
+ }
+
+ let edits = fix::edits::make_redundant_alias(member_names.iter().map(AsRef::as_ref), statement);
+
+ // Only emit a fix if there are edits
+ let mut tail = edits.into_iter();
+ let head = tail.next().ok_or(anyhow!("No edits to make"))?;
+
+ let isolation = Checker::isolation(checker.semantic().parent_statement_id(node_id));
+ Ok(Fix::safe_edits(head, tail).isolate(isolation))
+}
diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap
new file mode 100644
index 0000000000000..2a5f8071045ea
--- /dev/null
+++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap
@@ -0,0 +1,42 @@
+---
+source: crates/ruff_linter/src/rules/pyflakes/mod.rs
+---
+__init__.py:19:8: F401 [*] `sys` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ |
+19 | import sys # F401: remove unused
+ | ^^^ F401
+ |
+ = help: Remove unused import: `sys`
+
+ℹ Unsafe fix
+16 16 | import argparse as argparse # Ok: is redundant alias
+17 17 |
+18 18 |
+19 |-import sys # F401: remove unused
+20 19 |
+21 20 |
+22 21 | # first-party
+
+__init__.py:33:15: F401 [*] `.unused` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ |
+33 | from . import unused # F401: change to redundant alias
+ | ^^^^^^ F401
+ |
+ = help: Use a redundant alias: `.unused`
+
+ℹ Safe fix
+30 30 | from . import aliased as aliased # Ok: is redundant alias
+31 31 |
+32 32 |
+33 |-from . import unused # F401: change to redundant alias
+ 33 |+from . import unused as unused # F401: change to redundant alias
+34 34 |
+35 35 |
+36 36 | from . import renamed as bees # F401: no fix
+
+__init__.py:36:26: F401 `.renamed` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ |
+36 | from . import renamed as bees # F401: no fix
+ | ^^^^ F401
+ |
+ = help: Use a redundant alias: `.renamed`
diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_25__all____init__.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_25__all____init__.py.snap
new file mode 100644
index 0000000000000..43c550ba55f7d
--- /dev/null
+++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_25__all____init__.py.snap
@@ -0,0 +1,18 @@
+---
+source: crates/ruff_linter/src/rules/pyflakes/mod.rs
+---
+__init__.py:19:8: F401 [*] `sys` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ |
+19 | import sys # F401: remove unused
+ | ^^^ F401
+ |
+ = help: Remove unused import: `sys`
+
+ℹ Unsafe fix
+16 16 | import argparse # Ok: is exported in __all__
+17 17 |
+18 18 |
+19 |-import sys # F401: remove unused
+20 19 |
+21 20 |
+22 21 | # first-party
diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401___init__.py.snap
similarity index 100%
rename from crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap
rename to crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401___init__.py.snap