Improve type hint for ImportAttribute#1450
Conversation
|
Hi @Tesla2000! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Summary
Problem
ImportFrom.moduleis typed asAttribute | Name | NoneandImportAlias.nameis typed asAttribute | Name, which correctly reflects Python's grammar. However,Attribute.valueis typed asBaseExpression, which is overly broad for the import context.Python's grammar restricts module paths in import statements to dotted names only (e.g.,
from a.b.c import d,import a.b.c). This means that withinImportFrom.moduleandImportAlias.name, anyAttributenode's value can only ever beNameor anotherAttribute? neverCall,Subscript, or any otherBaseExpression.LibCST's own
scope_provider.pyacknowledges this with awhile isinstance(name_node, cst.Attribute)loop that assumes onlyNameorAttributewill appear, raising an exception if anything else is found.This creates a typing gap: code that processes
ImportFrom.modulerecursively must add unreachable fallback branches to handle the impossible cases, which can never be covered by tests.Example
Proposed Solution
Define
ImportAttributeat module level usingif TYPE_CHECKING:. Under the type checker it is a subclass ofAttributewithvaluenarrowed toName | Self; at runtime it is simplyAttribute, so no behaviour changes whatsoever.Then update both affected fields:
Applied to both:
ImportAlias.name(import a.b.candfrom x import y)ImportFrom.module(from a.b.c import ...)Why This Works
ImportAttribute = Attributein theelsebranch; existing code is entirely unaffected__name__/__qualname__hacks needed ? visitor dispatch is unaffectedSelfcorrectly expresses the recursive dotted-name constraintVerification
With this change, the example from the Problem section becomes:
Test Plan
Not needed. Nothing changes at runtime