Check the bound name (alias) per import in Python RemoveImport - #8407
Merged
Conversation
RemoveImport decided removability from the imported name, but an aliased import such as `from typing import List as L` binds `L`, not `List`, so an import whose alias was still referenced could be removed, leaving dangling references. The unused check now runs per import entry against the name that entry actually binds (alias if present), so a MultiImport mixing aliased and unaliased names only drops the unreferenced bindings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The Python
RemoveImportvisitor decided whether an import was still referenced using the imported name, but an aliased import such asfrom typing import List as LbindsL, notList. Removingtyping.Listfrom a file whose code referencesLtherefore found no references toList, deemed the import unused, and removed it — leaving theLusages dangling. This was reproducible end-to-end: running Java'sChangeType(typing.List -> list)over RPC on a file using the alias deleted the import and left broken code behind. With this fix, that flow becomes a conservative no-op until alias-aware re-adding lands on the JavaChangeType/ImportServiceside.Summary
_is_referencedpre-check is replaced by a per-import-entry_is_removablecheck that judges removability by the name the entry actually binds: its alias if present, else the imported name.MultiImportname lists are filtered entry by entry, so a statement mixing aliased and unaliased names (e.g.from typing import List, List as L) only drops the bindings that are unreferenced.from X import a, bstatement now also checks each entry's bound name instead of the module's last segment, so entries that are still in use are kept rather than deleted with the statement._binds_otheradditionally excludes the entry under evaluation by identity, so the "another import already binds this name" escape hatch (kept for whatChangeTypeleaves behind) cannot count an import against itself; the existing module/name comparison still covers duplicate statements.MultiImportrebuild blocks are consolidated into a_prune_nameshelper.Test plan
tests/python/test_remove_import.py: aliased from-import kept while its alias is used and removed when unused, removal proceeding when only the bare imported name appears, per-entry pruning of a mixedList, List as Lstatement, and the aliased plain-import (import numpy as np) equivalents. Four failed before the fix; two guard the removal direction.python -m pytest tests/python/test_remove_import.py --timeout=60— 16 passed.python -m pytest tests/python tests/recipes --timeout=60— 1634 passed, 6 skipped.