Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fix to remove unused standard library imports from __init__.py files #10390

Closed
zanieb opened this issue Mar 13, 2024 · 7 comments · Fixed by #11168
Closed

Add fix to remove unused standard library imports from __init__.py files #10390

zanieb opened this issue Mar 13, 2024 · 7 comments · Fixed by #11168
Assignees
Labels
fixes Related to suggested fixes for violations good first issue Good for newcomers

Comments

@zanieb
Copy link
Member

zanieb commented Mar 13, 2024

See #10365 (comment)

We currently do not offer any fixes for unused imports in __init__.py files.

This issue is to fix standard library imports by removing them from the file. We should probably label this fix as unsafe.

@plredmond
Copy link
Contributor

For the purposes of this check, I'm going to consider from __future__ import ... style imports to be "standard library".

@charliermarsh
Copy link
Member

I think from __future__ imports can just be ignored entirely, since they should never be removed as "unused".

@plredmond
Copy link
Contributor

plredmond commented Apr 25, 2024

Considered together with #10391, does this summary of behaviors seem right?

Old table
for some unused import... 

future                            → no diagnostic or fix
stdlib ∧  in_init                 → unsafe, remove
stdlib ∧ ¬in_init                 → safe,   remove
1stpty ∧  in_init                 → safe,   move to __all__ or convert to explicit-import
1stpty ∧ ¬in_init                 → safe,   remove
3rdpty ∧  in_init ∧  ignore_init  → no diagnostic or fix
3rdpty ∧  in_init ∧ ¬ignore_init  → unsafe, remove
3rdpty ∧ ¬in_init                 → safe,   remove

@charliermarsh
Copy link
Member

My impression was that for 3rdpty ∧ in_init, we never offered a fix.

@zanieb
Copy link
Member Author

zanieb commented Apr 25, 2024

That summary looks reasonable to me, with the addition that we should probably deprecate and remove the ignore_init option and remove third-party imports by default?

@charliermarsh
Copy link
Member

Yeah I revise my comment -- what Zanie says sounds right.

@plredmond
Copy link
Contributor

Ok, I've collapsed the table above. Here's an edited version without the ignore_init bit. I'll implement this, and try to make it flexible enough that I can PR #10390 separately from #10391 and so that we can more easily make changes to it in the future if desired.

for some unused import... 

future             → no diagnostic or fix
stdlib ∧  in_init  → unsafe, remove
stdlib ∧ ¬in_init  → safe,   remove
1stpty ∧  in_init  → safe,   move to __all__ or convert to explicit-import
1stpty ∧ ¬in_init  → safe,   remove
3rdpty ∧  in_init  → unsafe, remove
3rdpty ∧ ¬in_init  → safe,   remove

plredmond added a commit that referenced this issue Apr 26, 2024
…ability; remove unnecessary pattern match
plredmond added a commit that referenced this issue Apr 26, 2024
…urn a fix if imports were actually given
plredmond added a commit that referenced this issue Apr 26, 2024
… either moves to __all__ or makes imports explicit
plredmond added a commit that referenced this issue Apr 26, 2024
… two groups of import statement bindings; then iterate over those bindings and the fix which applies to them
plredmond added a commit that referenced this issue Apr 27, 2024
plredmond added a commit that referenced this issue Apr 27, 2024
plredmond added a commit that referenced this issue Apr 30, 2024
plredmond added a commit that referenced this issue Apr 30, 2024
plredmond added a commit that referenced this issue Apr 30, 2024
…sable F401 based on deprecated flag, but use the preview flag; move the testcases down to the preview testcase section; move the insta-snapshots to the preview paths
plredmond added a commit that referenced this issue Apr 30, 2024
… test and add a test-case for the preview linter rules that references that fixture; move its insta-snapshot to the location used by the new test-case
plredmond added a commit that referenced this issue Apr 30, 2024
plredmond added a commit that referenced this issue Apr 30, 2024
plredmond added a commit that referenced this issue May 2, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations good first issue Good for newcomers
Projects
None yet
4 participants