Repository navigation
Let Elements unpack type-level DSL results with an IntTuple domain - #5101
nitishagar wants to merge 1 commit into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D123347321. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
|
@stroxler gets back tomorrow and can take a look after that. thanks for your patience!! |
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
Thanks! This looks good to me |
stroxler
left a comment
There was a problem hiding this comment.
Okay this looks good to me, but can you rebase?
It looks like there are some merge conflicts, and I think an error message relevant to the test changed. I've been landing a lot of minor improvements lately, but I just wrapped up that work so I think things should be more stable now.
`Elements[broadcast(B1, B2)]` in a return annotation was rejected twice over: the Elements argument was parsed under a fixed type-expression context whose rules refuse function calls outright, and its validator knew nothing of type-level DSL calls (issue facebook#4977). A `@type_shape_dsl_function` such as `broadcast` has a declared `IntTuple` result domain, so its call should be legal wherever an `IntTuple` carrier is — exactly how the Tensor shape path already treats it. Elements' argument is now parsed with the ambient annotation context inherited through the type-argument parent (mirroring the Tensor shape argument path), and the carrier validator accepts a type-level DSL call whose result domain is `IntTuple`. Because the allowance is inherited rather than granted, DSL calls inside Elements stay rejected in parameter annotations, aliases, and globals, and calls with a non-`IntTuple` result domain keep failing the existing `requires an IntTuple` check; symbolic-rank and malformed-argument rejections are untouched.
8e49dec to
c8a34d6
Compare
|
@stroxler Thanks for the review — I've rebased onto latest main and fixed the test affected by the changed error message. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Fixes #4977
Problem
Elements[...]could not unpack the result of a type-level DSL function:As stroxler noted in the issue,
broadcastshould be able to expand throughElements— a@type_shape_dsl_functiondeclares its result domain, and anIntTuple-domain call should be legal wherever anIntTuplecarrier is.Change
Two gaps, fixed in
Elements' argument handling (mirroring how the Tensor shape path already treats DSL calls):Elements[...]argument was parsed under a fixedTypeExpression-rooted context whose rules refuse function calls. It now inherits the ambient annotation context through the type-argument parent, so a DSL call parses wherever the surrounding annotation already allows one.Type::TypeLevelDslCallwhoseresult_domain()isIntTuple.Because the allowance is inherited rather than granted, the design intent that DSL calls are return-annotation-only is preserved:
Elements[broadcast(...)]stays rejected inside parameter annotations, aliases, and globals (pinned by a new test), and calls with a non-IntTupleresult domain keep failing the existingrequires an IntTuplecheck (also newly pinned). Symbolic-rank and malformed-argument rejections are untouched.Scope note for reviewers: at a call site, a return type carried by a DSL call reveals
Tensor[IntTuple]— the same result the non-ElementsDSL-call path produces today (def g[B1, B2](x: Tensor[B1], y: Tensor[B2]) -> Tensor[broadcast(B1, B2)]behaves identically). Concrete materialization ofbroadcastshapes at call sites is a pre-existing, separate behavior of the DSL-call evaluation path; this change makes theElementspath consistent with it rather than extending it.Test plan
Elements[broadcast(B1, B2)]in the return annotation → no errors (written failing-first; failed with exactly the issue's twoinvalid-annotationerrors before the fix)IntTuple-domain DSL results still rejected with the existing messagecargo test -p pyrefly --lib test::shape_dsl: 355 passed, 0 failed; full--skip test::lspsuite: 7741 passed, 0 failed (the twomissing-sourceLSP interaction failures also fail on unmodifiedmain)