Skip to content

Commit

Permalink
F401 - Distinguish between imports we wish to remove and those we wis…
Browse files Browse the repository at this point in the history
…h 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>
  • Loading branch information
plredmond and MichaReiser authored May 2, 2024
1 parent 7cec3b2 commit 59afff0
Show file tree
Hide file tree
Showing 17 changed files with 343 additions and 46 deletions.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -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"]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# empty module imported by __init__.py for test fixture
58 changes: 56 additions & 2 deletions crates/ruff_linter/src/fix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<()> {
Expand Down Expand Up @@ -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"
);
}
}
16 changes: 3 additions & 13 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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__{}_{}",
Expand Down Expand Up @@ -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(
Expand Down
Loading

0 comments on commit 59afff0

Please sign in to comment.