-
Notifications
You must be signed in to change notification settings - Fork 244
fix panic: entered unreachable code: Expected a QuantifiedValue, got @_ #1659 #1884
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
Conversation
332d2b2 to
54ad72b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a panic that occurred when a class had type parameters with bounds referencing the class itself (e.g., class C[T: C]). The fix introduces a placeholder mechanism to prevent recursive type resolution during class type parameter computation.
Key changes:
- Added a placeholder guard mechanism that temporarily registers simplified type parameters while computing the actual class tparams, preventing infinite recursion
- Refactored
scoped_type_paramsto read TypeParameter bindings directly instead of resolving them through the solver, avoiding recursiveType::Varresolution - Modified
BindingTParamsto include the owningClassDefIndexand ensured classes with header type parameters consistently emit aKeyTParamsbinding
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/test/generic_basic.rs | Added regression test for class with type parameter bound to the class itself |
| pyrefly/lib/binding/class.rs | Updated class binding logic to always emit KeyTParams when scoped type parameters exist; added def_index to BindingTParams |
| pyrefly/lib/binding/binding.rs | Added def_index field to BindingTParams structure; updated size assertion |
| pyrefly/lib/alt/solve.rs | Refactored type parameter processing: extracted quantified_from_type_parameter helper and updated scoped_type_params to read bindings directly |
| pyrefly/lib/alt/function.rs | Updated function type parameter processing to pass errors to scoped_type_params |
| pyrefly/lib/alt/class/tparams.rs | Implemented placeholder mechanism with guard pattern and updated calculate_class_tparams methods to accept def_index |
| pyrefly/lib/alt/class/classdef.rs | Updated class_definition to pass def_index to calculate_class_tparams_no_legacy |
| pyrefly/lib/alt/answers_solver.rs | Added placeholder_class_tparams HashMap to ThreadState with accessor methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks, I've put this up for internal review. |
stroxler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review automatically exported from Phabricator review in Meta.
facebook#1659 (facebook#1884) Summary: Fixes facebook#1659 scoped_type_params no longer asks the solver to resolve each TypeParameter (which caused the recursive Type::Var). It now reads the TypeParameter binding directly, builds the Quantified via the shared helper quantified_from_type_parameter, and takes the active ErrorCollector so diagnostics still flow to the correct sink. Call sites in class tparams, function tparams, and scoped type aliases were updated to pass errors. Pull Request resolved: facebook#1884 Test Plan: Added a regression test that models class C[T: C] to lock in the crash reported in facebook#1659. Reviewed By: stroxler Differential Revision: D89520905 Pulled By: rchen152 fbshipit-source-id: 7b70551fc29214e4c9cfd26d71cd1d150a71f989
Summary
Fixes #1659
Classes with header type parameters now consistently emit a KeyTParams binding and defer their Class tparams to that binding. BindingTParams holds the owning ClassDefIndex so the solver can identify which class is mid-computation.
While class tparams are being computed we register a temporary placeholder copy so that any recursive reference to the class can safely call get_class_tparams without re-entering the exact computation. The guard automatically removes the placeholder once the real TParams have been validated.
scoped_type_params no longer asks the solver to resolve each TypeParameter (which caused the recursive Type::Var). It now reads the TypeParameter binding directly, builds the Quantified via the shared helper quantified_from_type_parameter, and takes the active ErrorCollector so diagnostics still flow to the correct sink. Call sites in class tparams, function tparams, and scoped type aliases were updated to pass errors.
solve_tparams and class_definition now pass the owning ClassDefIndex through to the calculator so we can key the placeholder map correctly.
Test Plan
Added a regression test that models class C[T: C] to lock in the crash reported in #1659.