Merge multiple possible legacy type parameters in same scope#4134
Merge multiple possible legacy type parameters in same scope#4134defr0std wants to merge 2 commits into
Conversation
This fix resolves a bug where Pyrefly would create separate narrows for modules containing legacy type variables instead of merging them, causing legacy type variables to be lost or shadowed in scope. This behavior causes incorrect type checking failures when multiple TypeVars are imported from a different module. Previously, in scopes referencing multiple potential legacy type variables, one possible-legacy-tparam binding would overwrite/shadow the others, leading to false-positive "not in scope" errors. To fix this, we update `Binding::PossibleLegacyTParam` to hold a collection (`ThinVec`) of key indices instead of a single key index, and modify `Key::PossibleLegacyTParam` to carry both the original range and the scope range in which it occurs. By incorporating the `scope_range` into the key, distinct possible legacy type parameters in different scopes remain separated, while those within the same scope are merged under the same key. When constructing bindings, subsequent potential legacy type variables in the same scope are merged into the existing `Binding::PossibleLegacyTParam` instead of overwriting it. During type solving, the solver iterates over all merged keys to apply narrows for all of them to the module info.
|
Hi @defr0std! 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! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
1 participant
Summary
This fix resolves a bug where Pyrefly would create separate narrows for modules containing legacy type variables instead of merging them, causing legacy type variables to be lost or shadowed in scope. This behavior causes incorrect type checking failures when multiple TypeVars are imported from a different module.
Previously, in scopes referencing multiple potential legacy type variables, one possible-legacy-tparam binding would overwrite/shadow the others, leading to false-positive "not in scope" errors.
To fix this, we update
Binding::PossibleLegacyTParamto hold a collection (ThinVec) of key indices instead of a single key index, and modifyKey::PossibleLegacyTParamto carry both the original range and the scope range in which it occurs.By incorporating the
scope_rangeinto the key, distinct possible legacy type parameters in different scopes remain separated, while those within the same scope are merged under the same key. When constructing bindings, subsequent potential legacy type variables in the same scope are merged into the existingBinding::PossibleLegacyTParaminstead of overwriting it. During type solving, the solver iterates over all merged keys to apply narrows for all of them to the module info.Test Plan
Ran project test cases, fixing a known issue captured in the tests. Also ran the modified type checker on our project where we currently experience these type check failuers.