Repository navigation
Conversation
Pyrefly did not narrow a tuple after a guard such as `if any(v is None for v in values): raise`, so each element stayed optional. Recognize `any(x is None for x in S)` and `all(x is not None for x in S)` and, when no element can be None, strip None from each element of concrete tuples. Fixes meta#5434
This comment has been minimized.
This comment has been minimized.
|
This pull request has been imported. If you are a Meta employee, you can view this in D129043234. (Because this pull request was imported automatically, there will not be any future comments.) |
stroxler
left a comment
There was a problem hiding this comment.
Thanks!
AI review flagged that this looks like it is narrowing a little too aggressively, there are some soundness issues we need to address before we merge. And there's also a suggestion in here to change the structure of the code a bit for maintainability:
Conditional builtin/custom aliases narrow unsoundly
Location: binding/narrow.rs:1260–1267.
The new matcher treats as_special_export as proof that the callee is builtin any or all. Its shared Binding::Phi handling in binding/bindings.rs:1865–1882 ignores branches that return no special export. A callable that is builtin on one reachable branch and an ordinary function on another is therefore incorrectly recognized as definitely builtin.
from typing import Iterable, assert_type
def f(values: tuple[int | None, str | None], use_builtin: bool) -> tuple[int, str]:
if use_builtin:
check = any
else:
def check(it: Iterable[bool]) -> bool:
return False
if check(v is None for v in values):
raise ValueError()
assert_type(values, tuple[int, str])
return valuesObserved: the PR binary, with --preset default, reports 0 errors for this reproducer, including the assert_type. CPython nevertheless returns (None, None) for f((None, None), False). The exact base binary reports the necessary bad-return for the same mixed-callee pattern. A conditional reassignment of an initially builtin alias exhibits the same regression.
The shared alias-resolution defect predates this PR, but registering Any/All exposes it to the new narrowing and suppresses a correct diagnostic.
Fix: in a small prerequisite change, recognize a merged special export only when every reachable branch resolves to the same special export. A non-special/unresolved branch must reject recognition. Add negative tests for mixed builtin/custom aliases for both any and all, as well as direct shadowing.
Impossible tuple union members remain as tuple[Never, …]
Location: alt/narrow.rs:1291–1295.
The helper removes None elementwise, but always reconstructs the tuple. If an element is definitely None, the result is an impossible tuple[Never, …] rather than Never for the entire union member.
def f(values: tuple[None, str] | tuple[int, int]) -> tuple[int, int]:
if any(v is None for v in values):
raise ValueError()
return valuesObserved: the PR binary reports:
Returned type `tuple[int, int] | tuple[Never, str]` is not assignable
to declared return type `tuple[int, int]` [bad-return]
Only tuple[int, int] can reach the return, so the diagnostic is false. The impossible member is not removed downstream; its other element can still fail assignability. Different-length alternatives likewise fail the tuple arity check.
This is incomplete narrowing within the stated concrete-tuple/union scope, not a new diagnostic regression: the base also rejects the example because it does not perform this narrowing at all.
Fix: if any required element narrows to Never, return Never for that concrete tuple member. Preserve the empty tuple. Add tests for both any's false branch and all's true branch, with differing element types and lengths.
Structural improvements we could make
- Check the cheap single-generator argument shape before resolving the special export. The new guard currently adds another scope/alias lookup to ordinary conditions such as
if ordinary(x):; the extra work is visible in the code, but its latency impact was not benchmarked. - Return the iterable together with the operation from the matcher. This avoids re-destructuring the arguments under
unreachable!and keeps ownership of the structural invariant in one place. - The new operations are not accepted by
op_is_still_valid, sook = all(v is not None for v in values); if ok: ...does not narrow. This is a nonblocking integration gap, not a regression; concrete tuple elements are immutable, so the existing reassignment check can support it.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Resolve a special export through a merged name only when every branch resolves to the same export, so a conditionally rebound `any`/`all` is not treated as the builtin. Turn a concrete tuple with an always-None element into Never instead of tuple[Never, ...]. Match the any/all generator shape before resolving the callee, return the iterable with the op, and let the ops survive through a variable (`ok = all(...)`).
mangeshraut712
left a comment
There was a problem hiding this comment.
The handling of Phi branches in special_export_from_binding_idx using ? ensures that any branch not resolving to the expected special export safely fails early rather than being ignored.
Also verified that dropping impossible concrete tuple members in narrow_tuple_elements_not_none (returning mk_never() when any element narrows to Never) correctly eliminates incompatible union variants instead of leaving uninstantiable tuple[Never, ...] types.
A couple of minor edge case observations:
element_none_checkmatches&operands[0]asExpr::Nameand&operands[1]asExpr::NoneLiteral(_). This covers idiomaticv is None/v is not None, though inverted comparisons likeNone is vorNone is not vwill safely fall through un-narrowed.- Invocations using a list comprehension like
any([v is None for v in values])are excluded since onlyExpr::Generatoris matched. Scoping to generator expressions first makes sense to keep the generator AST traversal simple, but list comprehensions are fairly common in Python codebases and could be considered for a follow-up. - Adding a check for
arguments.keywords.is_empty()inelement_none_checkwould ensure any malformed call with unexpected kwargs fails to match.
|
@mangeshraut712 I appreciate the enthusiasm, but I'd ask you to please hold off on unsolicited reviews to avoid causing confusion. |
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
This pull request has been merged in 4ec2a46. |
Fixes #5434
Problem
Pyrefly did not narrow a tuple after a guard like:
Change
Recognize
any(x is None for x in S)andall(x is not None for x in S), where the generator is single, unconditional and synchronous. Two new narrow ops (AnyElementIsNone/NoElementIsNone, each other's negation) are produced in the binding step. When no element can beNone, the solver stripsNonefrom each element of concrete tuples (including within unions). The "some element isNone" branch does not narrow.anyandallare added asSpecialExports so shadowed names are not matched.Review feedback addressed
check = anyvs. a localdef check) was treated as the builtin becauseBinding::Phiignored non-special branches. A merged special export is now recognized only when every branch resolves to the same export. Added tests for mixedany/allaliases and direct shadowing.Nonenow narrows toNeverinstead oftuple[Never, ...]. Added tests forany's false branch andall's true branch with differing element types and lengths.unreachable!.op_is_still_valid, sook = all(v is not None for v in values); if ok: ...narrows.Limitations
tuple[T, ...]).is None/is not Nonechecks are handled.Testing
pyrefly/lib/test/narrow.rs: the original guard, impossible-member pruning, alias/shadowing negatives, and narrowing through a variable.narrowtests pass; the format/lint step oftest.pyis clean.cargo test -p pyrefly --libaborts with a stack overflow aftertest::cycles; this also happens on unmodifiedmain. The wider run excludingcycleswas not confirmed locally; CI is authoritative.Checklist