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.

---

<details><summary>Old description of implementation changes</summary>

# 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)

<details>
<summary><h2>Scope cut until a future PR</h2></summary>

* (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`).
 
---

</details>

# 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`.

</details>

---------

Co-authored-by: Micha Reiser <micha@reiser.io>

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24/__init__.py

@@ -0,0 +1,36 @@
+"""__init__.py without __all__
+
+Unused stdlib and third party imports are unsafe removals
+
+Unused first party imports get changed to redundant aliases
+"""
+
+
+# stdlib
+
+import os  # Ok: is used
+
+_ = os
+
+
+import argparse as argparse  # Ok: is redundant alias
+
+
+import sys  # F401: remove unused
+
+
+# first-party
+
+
+from . import used  # Ok: is used
+
+_ = used
+
+
+from . import aliased as aliased  # Ok: is redundant alias
+
+
+from . import unused  # F401: change to redundant alias
+
+
+from . import renamed as bees  # F401: no fix

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24/aliased.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24/renamed.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24/unused.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24/used.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/__init__.py

@@ -0,0 +1,42 @@
+"""__init__.py with __all__
+
+Unused stdlib and third party imports are unsafe removals
+
+Unused first party imports get added to __all__
+"""
+
+
+# stdlib
+
+import os  # Ok: is used
+
+_ = os
+
+
+import argparse  # Ok: is exported in __all__
+
+
+import sys  # F401: remove unused
+
+
+# first-party
+
+
+from . import used  # Ok: is used
+
+_ = used
+
+
+from . import aliased as aliased  # Ok: is redundant alias
+
+
+from . import exported  # Ok: is exported in __all__
+
+
+# from . import unused # F401: add to __all__
+
+
+# from . import renamed as bees # F401: add to __all__
+
+
+__all__ = ["argparse", "exported"]

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/aliased.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/exported.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/renamed.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/unused.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_25__all/used.py

@@ -0,0 +1 @@
+# empty module imported by __init__.py for test fixture

crates/ruff_linter/src/fix/edits.rs

@@ -122,6 +122,28 @@ pub(crate) fn remove_unused_imports<'a>(
     }
 }
 
+/// Edits to make the specified imports explicit, e.g. change `import x` to `import x as x`.
+pub(crate) fn make_redundant_alias<'a>(
+    member_names: impl Iterator<Item = &'a str>,
+    stmt: &Stmt,
+) -> Vec<Edit> {
+    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"
+        );
+    }
 }

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(

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<String> {
         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<Diagnostic>) {
     // Collect all unused imports by statement.
     let mut unused: FxHashMap<(NodeId, Exceptions), Vec<ImportBinding>> = 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 fix = if (!in_init || fix_init) && !in_except_handler {
-            fix_imports(checker, node_id, &imports, in_init).ok()
-        } else {
-            None
+        let multiple = bindings.len() > 1;
+        let level = match checker.semantic().statement(import_statement) {
+            Stmt::Import(_) => 0,
+            Stmt::ImportFrom(StmtImportFrom { level, .. }) => *level,
+            _ => {
+                continue;
+            }
         };
 
-        for ImportBinding {
-            import,
-            range,
-            parent_range,
-        } in imports
-        {
+        // 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 })
+                )
+            });
+
+        // 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)
+        };
+
+        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<Item = &'a ImportBinding<'a>>,
     in_init: bool,
 ) -> Result<Fix> {
     let statement = checker.semantic().statement(node_id);
     let parent = checker.semantic().parent_statement(node_id);
 
     let member_names: Vec<Cow<'_, str>> = 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<Item = &'a ImportBinding<'a>>,
+) -> Result<Fix> {
+    let statement = checker.semantic().statement(node_id);
+
+    let member_names = imports
+        .map(|binding| binding.import.member_name())
+        .collect::<Vec<_>>();
+    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))
+}

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`

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