Skip to content
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

Merged
merged 18 commits into from
Sep 26, 2024

Conversation

zth
Copy link
Collaborator

@zth zth commented Apr 10, 2024

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:

  • Any variant X that is a subtype of variant Y can be spread in a pattern matching of a value of type Y
  • Using an alias ...subtypeVariant as value will give the type subTypeVariant to value
  • Syntax: Spreads of regular variants are done like spreads of polyvariants, but without the leading #: | ...subtypeVariant as value => doSomethingWithSubTypeVariant(value)
type a = One | Two | Three
type b = | ...a | Four | Five

let doWithA = (a: a) => {
  switch a {
  | One => Js.log("aaa")
  | Two => Js.log("twwwoooo")
  | Three => Js.log("threeeee")
  }
}

let lookup = (b: b) =>
  switch b {
  | ...a as a => doWithA(a)
  | Four => Js.log("four")
  | Five => Js.log("five")
  }

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.

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? *)
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

@zth zth force-pushed the variant-pattern-matching-type-spread branch from 231f131 to a77e7b1 Compare April 11, 2024 06:03
@zth zth force-pushed the variant-pattern-matching-type-spread branch from e36991b to 2f35e3a Compare September 11, 2024 19:55
@zth zth changed the base branch from 11.0_release to master September 11, 2024 19:56
@zth zth force-pushed the variant-pattern-matching-type-spread branch from 13f89ef to a5ad8fc Compare September 17, 2024 17:36
@zth zth force-pushed the variant-pattern-matching-type-spread branch from a5ad8fc to cf8544e Compare September 24, 2024 16:58
@zth zth merged commit 4ab9a5f into master Sep 26, 2024
20 checks passed
@zth zth deleted the variant-pattern-matching-type-spread branch September 26, 2024 09:01
@zth zth added this to the v12 milestone Sep 26, 2024
Copy link
Contributor

@glennsl glennsl left a 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:

  1. 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?
2. Not directly related to this PR but to variant spread as a whole, the subtype relationship does not nest either.
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 options do though.

@zth
Copy link
Collaborator Author

zth commented Oct 5, 2024

@glennsl could you check:

  1. Whether the same holds for polyvariant spreads
  2. Whether the same holds for a non spread variant, so the spread variant but written out manually

@tsnobip
Copy link
Contributor

tsnobip commented Oct 5, 2024

Could be missing the same kind of thing in the parser that was missing for dict parsing.

@glennsl
Copy link
Contributor

glennsl commented Oct 5, 2024

@glennsl could you check:

1. Whether the same holds for polyvariant spreads

2. Whether the same holds for a non spread variant, so the spread variant but written out manually

Nested polyvariant spread patterns work. Writing the variants out manually does not. That is, Some((One | Two | Three) as a) is syntactically valid, but does not infer a to be of type a.

@glennsl
Copy link
Contributor

glennsl commented Oct 5, 2024

Also, coercing arrays does not work for polyvariants either. Apparently it doesn't work for arrays at all, maybe it's a variance issue. options work in both cases though. So scratch that.

@zth
Copy link
Collaborator Author

zth commented Oct 5, 2024

Could be missing the same kind of thing in the parser that was missing for dict parsing.

Ah yes, nice catch! It seems to be at least partly the same, forgot about is_pattern_start.

@zth
Copy link
Collaborator Author

zth commented Oct 5, 2024

@glennsl got it thanks. Yeah, arrays is because of variance. We did do a sort of fix for that at some point: #6518

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Match on variant spread in switch Issues modeling GeoJSON spec with rescript@11 types
4 participants