-
Notifications
You must be signed in to change notification settings - Fork 456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Type spreads of regular variants in patterns #6721
Conversation
jscomp/ml/typecore.ml
Outdated
let path, decl = Typetexp.find_type env lid.loc lid.txt in | ||
match decl with | ||
| {type_kind = Type_variant constructors} -> ( | ||
(* TODO: Probably problematic that we don't account for type params here? *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristianoc did you see this here? That we're not caring about type params. Type params aren't allowed in spreads anyway right now so maybe it doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth checking that there are no type params, so it's not overlooked in future.
231f131
to
a77e7b1
Compare
e36991b
to
2f35e3a
Compare
13f89ef
to
a5ad8fc
Compare
a5ad8fc
to
cf8544e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple issues I've found:
- The variant spread pattern does not nest:
let lookup = (b: option<b>) =>
switch b {
| Some(...a as a) => doWithA(a)
| Some(Four) => Js.log("four")
| Some(Five) => Js.log("five")
| None => Js.log("None")
}
yields the syntax error:
35 │ let lookup = (b: option<b>) =>
36 │ switch b {
37 │ | Some(...a as a) => doWithA(a)
38 │ | Some(Four) => Js.log("four")
39 │ | Some(Five) => Js.log("five")
Did you forget a `)` here?
let f = (x: array<a>) => (x :> array<b>)
33 │ }
34 │
35 │ let f = (x: array<a>) => (x :> array<b>)
36 │
Type array<a> is not a subtype of array<b>
Type a is not compatible with type b
This error message is clearly incorrect, and the workaround requires an unnecessary map:
let f = (x: array<a>) => x->Array.map(a => (a :> b))
Edit: Coercing arrays don''t seem to work in general. Coercing option
s do though.
@glennsl could you check:
|
Could be missing the same kind of thing in the parser that was missing for dict parsing. |
Nested polyvariant spread patterns work. Writing the variants out manually does not. That is, |
Also, coercing arrays does not work for polyvariants either. Apparently it doesn't work for arrays at all, maybe it's a variance issue. |
Ah yes, nice catch! It seems to be at least partly the same, forgot about |
Solves part of #6273
Closes #6562
Closes #6277
This PoC adds support for type spreads of regular variants in patterns. This is already a thing with polyvariants, and now it's also a thing for regular variants.
Rationale is we already have variant spreads and coercion, which lets you go from small type to big type. Having this will let you go from big type to small type. This is a missing piece.
Specification:
X
that is a subtype of variantY
can be spread in a pattern matching of a value of typeY
...subtypeVariant as value
will give the typesubTypeVariant
tovalue
#
:| ...subtypeVariant as value => doSomethingWithSubTypeVariant(value)
Should be a well isolated feature, since it's dependent on the parser adding an attribute when the spread is a regular variant (no leading
#
) and no regular pre-existing code path is touched.