Repository navigation
fix: report unreachable match case subsumed by earlier sibling class pattern - #5201
bniladridas wants to merge 2 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D124670346. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
6c269ae to
2333cb5
Compare
|
Hi @samwgoldman (Sam Goldman), I've rebased this onto the latest main and re-run the checks ( |
This comment has been minimized.
This comment has been minimized.
…pattern When the match subject is Any or object, narrowing cannot produce Never, so a class pattern already covered by an earlier unguarded case went unreported. Track preceding unguarded case ops at bind time and, at solve time, report when the current case's class is a subset of their union. Signed-off-by: Niladri Das <125604915+bniladridas@users.noreply.github.com>
pattern_class_operand now rejects classes whose metadata has_base_any() is true, preventing false unreachable-match-case diagnostics when a base class is unresolved and both siblings extend Any. Signed-off-by: Niladri Das <125604915+bniladridas@users.noreply.github.com>
2333cb5 to
4d3aa06
Compare
| let class_ty = classes.pop().expect("checked length above"); | ||
| if let Type::ClassDef(cls) = class_ty | ||
| && self.get_class_tparams(&cls).is_none() | ||
| && !self.get_metadata_for_class(&cls).has_base_any() |
There was a problem hiding this comment.
the added guard skips sibling subsumption for classes that extend Any and fixes the unresolved-base false positive.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Thanks for the PR! I agree with your observation on the issue that unreachability does not need negation types, so we don't need to wait for those. Unfortunately, I don't think this is quite the right fix. It looks like it adds a second type of reachability check rather than extending the existing one. If you'd like to take another stab at this, here's an alternative approach suggested by Claude: Reorder the existing computation: narrow by the current case's pattern first, then apply the earlier cases' negations.
|
|
Thanks for the pointer. Took another pass in a separate worktree: #5515. It narrows against the current case first, then applies earlier unguarded negations through the existing subtract, so the second Tests cover the |
Summary: Fixes #3338. `Any` was short-circuited in match-case reachability, so `case bool()` after `case int()` was never reported. Narrowing by the current class pattern first, then applying earlier unguarded case negations, reuses the existing narrowing and subtraction path: `Any` becomes `bool`, then `bool` minus `int` is `Never`. This supersedes the closed #5201 approach. It removes the second sibling-subsumption check, the separate class extraction, and the `prev_case_ops` vector. The binder passes one earlier negation separately, and reachability applies the current pattern before that negation. # Scope - #3338 is limited to `Any`. `object` keeps the existing early return because structural patterns such as tuples can narrow `object` to `Never` spuriously. - Partial class patterns are covered for the no-false-positive case, for example `case A(x=1)` does not subsume `case B()`. Existing reachability also does not report duplicate partial-class patterns on precise subjects, so positive detection for those remains a separate limitation. - One projected `NarrowOp` is cloned because `Binding::Narrow` and the reachability metadata own separate copies. No vector or class-extraction clone remains. Pull Request resolved: #5515 Test Plan: - `cargo test -p pyrefly pattern_match --quiet`: 113 passed, 0 failed. - `cargo clippy -p pyrefly --lib -- -D warnings`: passed. - `python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema --no-extension`: passed. - Full suite excluding `test::cycles::deep_phi_chain_term_expr`: 10155 passed, 0 failed, 3 ignored. That test overflows the default 10 MiB debug stack on clean current `origin/main` too, so it is unrelated; it passes with `PYREFLY_STACK_SIZE=67108864`. - New regression covers `Any` int then bool, subclass-first order, guarded predecessor, duplicate literal, Or-pattern, object no-report, and partial-class no-false-positive. - [x] I am a human, and I fully understand the code I am submitting. Reviewed By: rchen152 Differential Revision: D129509393 fbshipit-source-id: 4ae4578f76bc1cb26322f3b2c7e6e3e8b7bc573f
Summary
When the match subject is
Anyorobject, narrowing cannot produceNever, so a class pattern already covered by an earlier unguarded sibling case went unreported. This tracks preceding unguarded case ops at bind time and reports at solve time when the current case's class is a subset of their union.The subject-based check stays authoritative for precise subjects. The sibling check runs only when subject narrowing is silent (
Any/objectearly return) or fails to produceNever.A later guard skips classes whose base extends
Any, so unresolved base classes (for example numba types) do not produce false positives.Fixes #3338
Test Plan
cargo test -p pyrefly pattern_match: 112 tests pass, including the newtest_match_case_unreachable_subsumed_by_sibling_any_subjectcovering subsumption onAny, subclass-first ordering, and guarded predecessors.cargo test -p pyrefly: 9068 passed, 0 failed (last full run before the rebase; the change is a one-line guard).cargo clippy -p pyrefly --lib: clean (last run before the rebase).python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema --no-extension: passes.unreachable-match-case.