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

F401 - Distinguish between imports we wish to remove and those we wish to make explicit-exports #11168

Merged
merged 36 commits into from
May 2, 2024

Conversation

plredmond
Copy link
Contributor

@plredmond plredmond commented Apr 26, 2024

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:
    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:
    (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.
  • // FIXME: rename "imports" to "bindings" if reviewer agrees (see code)
  • // 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

  • crates/ruff_linter/resources/test/fixtures/pyflakes/F401_24 contains an __init__.py without __all__ that exercises the features in this PR, but it doesn't pass.
  • 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.
  • Write unit tests for the new edit functions in fix::edits::make_redundant_alias.

@plredmond plredmond force-pushed the ruff.F401 branch 2 times, most recently from d9d863e to 0baf16e Compare April 29, 2024 20:59
@plredmond plredmond marked this pull request as ready for review April 29, 2024 21:12
Copy link
Contributor

github-actions bot commented Apr 29, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +104 -0 fixes in 2 projects; 42 projects unchanged)

PostHog/HouseWatch (+0 -0 violations, +4 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ housewatch/models/__init__.py:2:21: F401 [*] `.backup.ScheduledBackup` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- housewatch/models/__init__.py:2:21: F401 `.backup.ScheduledBackup` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ housewatch/models/__init__.py:2:38: F401 [*] `.backup.ScheduledBackupRun` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- housewatch/models/__init__.py:2:38: F401 `.backup.ScheduledBackupRun` imported but unused; consider removing, adding to `__all__`, or using a redundant alias

latchbio/latch (+0 -0 violations, +100 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ latch/__init__.py:10:5: F401 [*] `latch.functions.operators.group_tuple` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:10:5: F401 `latch.functions.operators.group_tuple` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:11:5: F401 [*] `latch.functions.operators.inner_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:11:5: F401 `latch.functions.operators.inner_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:12:5: F401 [*] `latch.functions.operators.latch_filter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:12:5: F401 `latch.functions.operators.latch_filter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:13:5: F401 [*] `latch.functions.operators.left_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:13:5: F401 `latch.functions.operators.left_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:14:5: F401 [*] `latch.functions.operators.outer_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:14:5: F401 `latch.functions.operators.outer_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:15:5: F401 [*] `latch.functions.operators.right_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:15:5: F401 `latch.functions.operators.right_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:17:41: F401 [*] `latch.resources.conditional.create_conditional_section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:17:41: F401 `latch.resources.conditional.create_conditional_section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:18:39: F401 [*] `latch.resources.map_tasks.map_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:18:39: F401 `latch.resources.map_tasks.map_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:19:48: F401 [*] `latch.resources.reference_workflow.workflow_reference` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:19:48: F401 `latch.resources.reference_workflow.workflow_reference` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:21:5: F401 [*] `latch.resources.tasks.custom_memory_optimized_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:21:5: F401 `latch.resources.tasks.custom_memory_optimized_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:22:5: F401 [*] `latch.resources.tasks.custom_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:22:5: F401 `latch.resources.tasks.custom_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:23:5: F401 [*] `latch.resources.tasks.large_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:23:5: F401 `latch.resources.tasks.large_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:24:5: F401 [*] `latch.resources.tasks.large_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:24:5: F401 `latch.resources.tasks.large_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:25:5: F401 [*] `latch.resources.tasks.medium_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:25:5: F401 `latch.resources.tasks.medium_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:26:5: F401 [*] `latch.resources.tasks.small_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:26:5: F401 `latch.resources.tasks.small_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:27:5: F401 [*] `latch.resources.tasks.small_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:27:5: F401 `latch.resources.tasks.small_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:29:38: F401 [*] `latch.resources.workflow.workflow` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:29:38: F401 `latch.resources.workflow.workflow` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:7:38: F401 [*] `latch.functions.messages.message` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:7:38: F401 `latch.functions.messages.message` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/__init__.py:9:5: F401 [*] `latch.functions.operators.combine` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/__init__.py:9:5: F401 `latch.functions.operators.combine` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/types/__init__.py:10:5: F401 [*] `latch.types.metadata.LatchMetadata` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/types/__init__.py:10:5: F401 `latch.types.metadata.LatchMetadata` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/types/__init__.py:11:5: F401 [*] `latch.types.metadata.LatchParameter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/types/__init__.py:11:5: F401 `latch.types.metadata.LatchParameter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/types/__init__.py:12:5: F401 [*] `latch.types.metadata.LatchRule` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/types/__init__.py:12:5: F401 `latch.types.metadata.LatchRule` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/types/__init__.py:13:5: F401 [*] `latch.types.metadata.Params` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/types/__init__.py:13:5: F401 `latch.types.metadata.Params` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
+ latch/types/__init__.py:14:5: F401 [*] `latch.types.metadata.Section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
- latch/types/__init__.py:14:5: F401 `latch.types.metadata.Section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
... 52 additional changes omitted for project

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
F401 104 0 0 104 0

@zanieb
Copy link
Member

zanieb commented Apr 29, 2024

The ecosystem checks look concerning. We're losing fixes in preview? I think there should be no changes in stable?

@plredmond
Copy link
Contributor Author

Yes, yes those do look concerning. I'll look into it.

@zanieb zanieb requested a review from MichaReiser April 30, 2024 15:31
…ability; remove unnecessary pattern match
…urn a fix if imports were actually given
… either moves to __all__ or makes imports explicit
… two groups of import statement bindings; then iterate over those bindings and the fix which applies to them
@charliermarsh
Copy link
Member

Not sure what's up with the Latch changes. I ran it locally and they are indeed classified as first-party (at least by the isort rule).

Some(UnusedImportContext::Init {
first_party: is_first_party(
checker,
&binding.import.qualified_name().to_string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to understand what's happening in Latch, at least one thing is that I think isort::categorize expects .module_name() rather than .qualified_name(), so this should probably be binding.import.module_name() (although I wouldn't expect it to change the outcome -- the module name is generally a prefix of the qualified name, and the import categorization just operates on the first segment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, but crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs uses the .qualified_name with level=0. Interesting... I mean, on further review, it's possible that level=0 should work here, because the binding name is meant to be the fully-resolved name.

Like, if the user does this from a module named foo:

from . import bar

Then the qualified name on the binding should be ["foo", "bar"] -- and so we should be able to resolve it without taking into account the dots. I'm surprised it's not working for those projects. It's probably worth checking them out, running them locally, and looking at what binding.import.qualified_name is here.

@plredmond
Copy link
Contributor Author

Done with this round of review. I think the outstanding issues (in order of importance) are:

(1) Incorrectly categorizing many imports as not first-party in ecosystem results.

  • Latest change might address this by setting level correctly
(2) Dangling `__all__` code
  1. `let dunder_all = None;`
  2. `dunder_all: bool` field of `UnusedImportContext::init`
  3. `dunder_all: Option` argument of `fix_by_reexporting`
  4. `match dunder_all` in `fix_by_reexporting` which has a "not implemented" branch
  5. branch in `fix_title` of `impl Violation for UnusedImport` which handles the case for no2
  • To complete this feature requires adding a new edit function and calling it at no4.
(3) Some functions still take `impl Iterator<...>` but might be better to take `&'a [...]`
  1. `fix::edits::make_redundant_alias`
  2. `fix_by_reexporting`
  3. `fix_by_removing_imports`

@plredmond
Copy link
Contributor Author

latch/latchbio is still getting many imports incorrectly categorized, it seems

@plredmond
Copy link
Contributor Author

I think there might just be a bug in my logic somewhere, since it seems that the only things being miscategorized are bindings in the syntax

from foo.bar.baz import (bind1,bind2)

So I'll write a test for that syntax and see what happens with it.

@charliermarsh
Copy link
Member

Cool, I can help take a look at the remaining Latch categorizations tomorrow if it’s helpful!

@charliermarsh
Copy link
Member

On second glance, aren't the ecosystem changes correct? We're now offering fixes for first-party imports in __init__.py files, whereas we weren't before?

charliermarsh

This comment was marked as duplicate.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior here looks correct to me. Would you mind adding a brief summary of the behavior changes to the PR body, so that users that click through in the changelog can understand what changed, rather than being limited to the implementation details?

@plredmond plredmond merged commit 59afff0 into main May 2, 2024
19 checks passed
@plredmond plredmond deleted the ruff.F401 branch May 2, 2024 23:10
@charliermarsh
Copy link
Member

Congrats on the merge :)

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label May 3, 2024
plredmond added a commit that referenced this pull request May 15, 2024
Followup on #11168 and resolve #10391

# User facing changes

* F401 now recommends a fix to add unused import bindings to to
`__all__` if a single `__all__` list or tuple is found in `__init__.py`.
* If there are no `__all__` found in the file, fall back to recommending
redundant-aliases.
* If there are multiple `__all__` or only one but of the wrong type (non
list or tuple) then diagnostics are generated without fixes.
* `fix_title` is updated to reflect what the fix/recommendation is.

Subtlety: For a renamed import such as `import foo as bees`, we can
generate a fix to add `bees` to `__all__` but cannot generate a fix to
produce a redundant import (because that would break uses of the binding
`bees`).

# Implementation changes

* Add `name` field to `ImportBinding` to contain the name of the
_binding_ we want to add to `__all__` (important for the `import foo as
bees` case). It previously only contained the `AnyImport` which can give
us information about the import but not the binding.
* Add `binding` field to `UnusedImport` to contain the same. (Naming
note: the field `name` field already existed on `UnusedImport` and
contains the qualified name of the imported symbol/module)
* Change `fix_by_reexporting` to branch on the size of `dunder_all:
Vec<&Expr>`
* For length 0 call the edit-producing function `make_redundant_alias`.
  * For length 1 call edit-producing function `add_to_dunder_all`.
  * Otherwise, produce no fix.
* Implement the edit-producing function `add_to_dunder_all` and add unit
tests.
* Implement several fixture tests: empty `__all__ = []`, nonempty
`__all__ = ["foo"]`, mis-typed `__all__ = None`, plus-eq `__all__ +=
["foo"]`
* `UnusedImportContext::Init` variant now has two fields: whether the
fix is in `__init__.py` and how many `__all__` were found.

# Other changes

* Remove a spurious pattern match and instead use field lookups b/c the
addition of a field would have required changing the unrelated pattern.
* Tweak input type of `make_redundant_alias`

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
plredmond added a commit that referenced this pull request May 15, 2024
plredmond added a commit that referenced this pull request May 15, 2024
plredmond added a commit that referenced this pull request May 21, 2024
#11436)

## Summary

* Update documentation for F401 following recent PRs
  * #11168
  * #11314
* Deprecate `ignore_init_module_imports`
* Add a deprecation pragma to the option and a "warn user once" message
when the option is used.
* Restore the old behavior for stable (non-preview) mode:
* When `ignore_init_module_imports` is set to `true` (default) there are
no `__init_.py` fixes (but we get nice fix titles!).
* When `ignore_init_module_imports` is set to `false` there are unsafe
`__init__.py` fixes to remove unused imports.
* When preview mode is enabled, it overrides
`ignore_init_module_imports`.
* Fixed a bug in fix titles where `import foo as bar` would recommend
reexporting `bar as bar`. It now says to reexport `foo as foo`. (In this
case we don't issue a fix, fwiw; it was just a fix title bug.)

## Test plan

Added new fixture tests that reuse the existing fixtures for
`__init__.py` files. Each of the three situations listed above has
fixture tests. The F401 "stable" tests cover:

> * When `ignore_init_module_imports` is set to `true` (default) there
are no `__init_.py` fixes (but we get nice fix titles!).

The F401 "deprecated option" tests cover:

> * When `ignore_init_module_imports` is set to `false` there are unsafe
`__init__.py` fixes to remove unused imports.

These complement existing "preview" tests that show the new behavior
which recommends fixes in `__init__.py` according to whether the import
is 1st party and other circumstances (for more on that behavior see:
#11314).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add fix to remove unused standard library imports from __init__.py files
5 participants