Preserve tuple match narrows across subsequent cases#3224
Preserve tuple match narrows across subsequent cases#3224Arths17 wants to merge 6 commits intofacebook:mainfrom
Conversation
Code action save hooks currently require enabling generic source.fixAll, which can trigger unrelated servers. This change advertises and emits source.fixAll.pyrefly so editors can target pyrefly-only fix-all behavior.\n\nThe code action filter now accepts both source.fixAll and source.fixAll.* requests so parent-kind clients remain compatible while pyrefly-specific requests are supported.
When a client requests source.fixAll.*, we should not run pyrefly fix-all for sibling providers. Restrict matching to either the generic source.fixAll ancestor or source.fixAll.pyrefly descendants.
Reviewer feedback pointed out that prefix matching for source.fixAll.pyrefly would accept unrelated kinds such as source.fixAll.pyrefly.foo. This makes fix-all behavior broader than intended. Use exact matching for source.fixAll.pyrefly (while still accepting source.fixAll), and add regression tests that prove accepted and rejected kind values. This keeps code-action filtering aligned with LSP expectations and prevents false-positive fix-all triggers.
Keep tuple-element narrows alive when a match case falls through to later cases, so `None` cases on multi-subject matches do not lose the surviving element types.
There was a problem hiding this comment.
Pull request overview
This PR aims to preserve type narrowing information across subsequent match cases when the match subject is a tuple (e.g. match a, b:), addressing false positives like those described in #3213. It also updates the LSP “fix all” code action kind to a Pyrefly-specific kind.
Changes:
- Add a regression test covering tuple-subject narrowing across later
matchcases after aNone, Nonecase. - Update LSP server capabilities and emitted code actions to use
source.fixAll.pyrefly, with new unit tests around kind filtering. - Adjust pattern binding logic to retain and propagate narrowing ops for tuple match subjects across cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pyrefly/lib/test/pattern_match.rs |
Adds a regression test for tuple-subject narrowing after a None, None match case. |
pyrefly/lib/binding/pattern.rs |
Changes how tuple-subject match narrows are accumulated/retained across cases. |
pyrefly/lib/lsp/non_wasm/server.rs |
Switches fix-all code action kind to source.fixAll.pyrefly and adds filtering tests. |
pyrefly/lib/test/lsp/lsp_interaction/basic.rs |
Updates LSP initialization capability expectations for the new fix-all kind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .0 | ||
| { | ||
| let subject = NarrowingSubject::Name(name); | ||
| narrow_ops.and_for_subject(&subject, op, range); | ||
| } |
There was a problem hiding this comment.
In tuple/sequence pattern handling, this change switches from and_all to per-entry and_for_subject, which avoids Placeholder insertion. That means a multi-element pattern like case None, None will produce independent narrows for both names, and when stmt_match negates them for fallthrough it becomes a is not None AND b is not None (conjunction), which is not the logical negation of (a is None) AND (b is None). This can make later cases like case _, None incorrectly treat b as Never and can hide type errors. Consider representing fallthrough constraints at the tuple-pattern level (or otherwise avoid per-name negation for multi-name patterns) so the complement is modeled correctly.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| .0 | ||
| { | ||
| let subject = NarrowingSubject::Name(name); | ||
| narrow_ops.and_for_subject(&subject, op, range); | ||
| } |
There was a problem hiding this comment.
Same issue as above: collecting subpattern narrows without Placeholder insertion makes subsequent fallthrough narrowing treat per-element negations as conjunctive constraints. For tuple subjects this is not a sound representation of case exclusion and can lead to contradictory narrows (e.g., b != None carried into a case _, None).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| MatchSubject::Single(subject) => name == subject.name(), | ||
| MatchSubject::Tuple(subjects) => subjects | ||
| .iter() | ||
| .flatten() | ||
| .any(|subject| name == subject.name()), |
There was a problem hiding this comment.
Retaining per-element narrows for MatchSubject::Tuple means negated_prev_ops will accumulate per-name negations across cases, but those negations are applied conjunctively (bind_narrow_ops loops over names). For a prior case that narrowed multiple tuple elements (e.g. None, None), the correct fallthrough condition is a disjunction across elements, not a != None && b != None. This can make later cases unreachable or produce Never types. A different representation of tuple-case fallthrough (not per-name conjunction) is needed for sound narrowing across cases.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This keeps per-element narrows from tuple match cases alive when control falls through to later cases, so a
case None, Noneno longer drops the surviving type information for the remaining branches.Fixes #3213.