-
Notifications
You must be signed in to change notification settings - Fork 239
fix Match case on type don't exaust on types #598 #1994
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
This comment has been minimized.
This comment has been minimized.
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 issue #598 where match case patterns with type guards and aliases (using as) were not properly narrowing the original match subject. The fix ensures that when a case introduces an alias, type narrowing operations are mirrored back onto the original subject, allowing subsequent branches and exhaustiveness checks to work correctly.
Key Changes:
- Added logic to rebase narrowing operations from alias names back to original match subjects
- Implemented facet subject merging for proper handling of nested property access patterns
- Updated type narrowing to properly track and apply constraints across aliased patterns
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pyrefly/lib/test/pattern_match.rs |
Added test case demonstrating that match aliases now correctly narrow the original subject variable |
pyrefly/lib/test/flow_branching.rs |
Updated existing test assertions to reflect correct narrowing behavior (e.g., x now narrows to Literal[2] and B instead of broader types) |
pyrefly/lib/binding/pattern.rs |
Modified Pattern::MatchAs handling to rebase alias narrowing operations onto the original subject when they differ |
pyrefly/lib/binding/narrow.rs |
Added rebase_subject, rebase_facet_subject, and merge_facet_subjects functions plus and_for_subject method to support narrowing operation rebasing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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.
This looks reasonable to me, thanks for the PR! I'll aim to get a second review and merge it
rchen152
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.
rchen152
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.
Summary
Fixes #598
Updated Pattern::MatchAs so that, when a case introduces an alias, the resultant narrows are mirrored back onto the original match subject (without rebinding names twice), ensuring later branches see the refined type.
Introduced NarrowOp::rebase_subject plus facet-merging helpers and reworked NarrowOps::and_for_subject in to re-target alias narrows, compose facet chains/origins, and insert the rebased operations without placeholder pollution so negated flows correctly eliminate matched variants.
Test Plan
Added test_match_alias_narrows_subject