Repository navigation
Support functools.Placeholder in partial type inference (Python 3.14+) - #5598
La1itchauhan wants to merge 3 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D130700129. (Because this pull request was imported automatically, there will not be any future comments.) |
ff06121 to
b54fde6
Compare
This comment has been minimized.
This comment has been minimized.
|
✅ This PR meets all requirements from our pull request checklist. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. |
stroxler
left a comment
There was a problem hiding this comment.
Hi @La1itchauhan, thanks for the PR!
Could you rebase, and include the checkbox saying you're a human (sorry about that, but unfortunately we've introduced some friction and automated checks to help us keep the flow of incoming PRs at something we can handle)?
The rebase should trigger an auto-import of the diff, which will make it easier to review and merge.
While you're at it... based on an initial AI-assisted code review I think the overall direction here is pretty good. But it appears that the support is incomplete, there are a number of gaps. For each gap, we have a choice: we could either fix the gap with a unit test covering it, or opt not to fix the gap but include a (passing) unit test with a bug= marker so that we don't lose track of it.
-
False “argument type is unknown” on a known placeholder.
partial(f, Placeholder, 2)producesunknown-argument-typewhen that diagnostic is enabled. This affects the basic supported use case—not an exotic expression—because the implementation substitutes implicitAny. I’d fix this before merging. -
Placeholder-valued expressions aren’t recognized.
Examples:holes[0],(hole := Placeholder), a conditional with placeholder-valued branches, orget_placeholder(). Even when Pyrefly knows the expression has_PlaceholderType, it treats it as a concrete bound argument and subsequently rejects valid calls.
Worth fixing if it stays local; otherwise a documented first-cut limitation withbug=tests is reasonable. Don’t substantially complicate contextual inference to cover every expression. -
A hole plus
**kwargsloses a keyword-collision restriction.def f(x: int, y: int, **kwargs: int) -> int: return x + y p = partial(f, Placeholder, 2) p(1, x=3) # Accepted, but runtime gets multiple values for x.
The distinction matters because an originally positional-only
xlegitimately permits that same-name keyword through**kwargs.
Reasonable to defer with abug=test if fixing it requires new signature machinery. This is an uncommon missed error; don’t broaden the PR into fixing all existing partial/keyword collisions. -
Early fallbacks skip keyword-placeholder rejection.
Examples:partial(f, y=Placeholder, **extra)with an ordinary dictionary splat, andpartial(*targets, y=Placeholder). Both are runtime-invalid, but the new validation is bypassed.
Prefer a small early-validation fix; otherwise track withbug=tests. This does not require precise inference of arbitrary splats. -
Trailing placeholders aren’t rejected.
For example,partial(f, Placeholder, y=2)has a trailing positional placeholder despite the later keyword. The runtime we tested rejects it, while Pyrefly accepts it.
Already an explicit deferred limit in the PR; fine to document and track rather than block on it. -
Some compositions lose precision.
Generic overloaded targets can yieldUnknown; nested partials and ParamSpec cases can fall back to nominalpartial[R].
Fine to retain as explicit inference limits. These are not, by themselves, evidence of incorrect implementation. Use ordinary tests for intentional fallback behavior, andbug=only where the current behavior is considered undesirable.
b54fde6 to
7c73e8e
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Hi @stroxler, thanks for the review! I’ve rebased on main, updated the PR description with the checklist, addressed items 1, 2, and 4 with fixes, and added tests for 3, 5 (tracked with bug=), and 6 |
Summary
Adds type inference support for
functools.Placeholderintroduced in Python 3.14:Placeholderarguments as required positional-only parameters rather than binding them.Anyduring target callability checks, avoiding false positivebad-argument-typeandunknown-argument-typediagnostics.holes[0]), Named ((h := Placeholder)), Call (get_placeholder()), and If (Placeholder if flag else Placeholder). Other expression forms are not recognized.cannot use functools.Placeholder as keyword argument), including calls containing*argsor**kwargssplats (partial(*targets, y=Placeholder)andpartial(f, y=Placeholder, **extra)).bug=tests: trailing positional placeholders accepted statically; hole plus**kwargskeyword collisions.ParamSpectargets.Fixes #5479
Test Plan
pyrefly/lib/test/functools/partial.rscovering supported expressions, early keyword rejection, fallbacks, andbug=markers.python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema --no-extensioncargo fmt -- --checkgit status --shortconfirms no generated files modified.