Add if let syntax

[tomgasson]

if let brings pattern matching to if

if let PAT = EXPR { BODY }

The motivation for having any construct at all for this is to simplify the cases that today call for
a switch statement with a single non-trivial case. This is predominately used for unwrapping
option<'t> values, but can be used elsewhere.

Before:

switch(optVal) {
    | Some(x) => doSomethingWith(x)
    | _ => ()
}

After:

if let Some(x) = optVal {
    doSomethingWith(x)
}

Complex example:

if let Some(x) = optVal when x > 10 {
    doSomethingWith(x)
} else if let Theme({color: Red | Blue, emoji}) = getTheme() {
    doSomething()
} else {
    doSomethingElse()
}

There are >500 occurrences in FB of | _ => () where this form would be more appropriate

This is syntax sugar only, it's implemented similar to ns.ternary so that that it should be stable across parsing and printing

Prior Art

reasonml/reason#1301

• Rust - RFC for if let expression rust-lang/rfcs#160. The same justifications apply here

• Swift if case x {} and if var = foo() (for reference types)

• Clojure https://clojuredocs.org/clojure.core/if-let

[IwanKaramazow]

Good stuff! Looks good at first sight, I'll do a more thorough review during the weekend.

[bloodyowl]

I like the idea, but feel like the legibility is pretty hard.

I'd find it clearer to read something like the following:

if optVal match Some(x) {
  doSomethingWith(x)
}

What do you think?

[tomgasson]

@bloodyowl
Rust also discussed optVal is Some(x) in a similar form to your optVal match Some(x) suggestion, and I think the same reasoning would apply here rust-lang/rfcs#160 (comment)

• let is already a keyword an this is just a new position to use it
• let disambiguates the parsing immediately after if
• The ordering is already equivalent with let bindings let MyVal(x) = foo()

[tomgasson]

Good stuff! Looks good at first sight, I'll do a more thorough review during the weekend.

Thanks - I haven't paid too much attention to breadcrumbs/regions, exception cases or doc printing so could probably use some guidance on how to do them best

[bloodyowl]

@tomgasson Good points yeah. I guess that's where the most sensible tradeoffs are made.

[chenglou]

So to be clear, no when support nor let chaining for now right? These can be postponed

I'm still torn between if let Some(a) = result and if let a = result. I'm not sure the generality of the former is worth it. I'm also not sure the latter covers enough.

[cristianoc]

Wondering how nested optional record fields look like.

Before:

// type point = { x:int, y:int, z:option<int> }
// type nested = { origin:option<point>, name:string }

let getZ = nested =>
  switch nested.origin {
  | None => 0
  | Some(point) =>
    switch point.z {
    | None => 0
    | Some(z) => z
    }
  }

after:

let getZ = nested =>
  if let Some(point) = nested.origin  {
    if let Some(z) = point.z  {
      z
    } else {
      0
    }
  } else {
    0
  }

@tomgasson notices there's an extra space before{ on the second line.

[IwanKaramazow]

Great work. Left a couple of questions w.r.t style/naming, the gist of this PR is actually very good.
I share the same philosophy as Cheng, let's maybe drop the support for when in V1? We can always add it back later.

Can you maybe add some extra test cases with nested if lets (simple and complex) and ternaries a ? b : c intermixed?
Cristiano's example is a good start.

[IwanKaramazow]

Can we rename this into parseIfOrIfLetExpr? Makes it clearer what we're possibly parsing here.

[IwanKaramazow]

I would bring parseIfBody inline here. The general idea is to have something like parseAOrB indicating that there are two possible things: A and B. parseAOrB checks the next token and then delegates to the correct function. Example:

and parseIfOrIfLetExpr p =
  ...
  let expr = match p.token with
    | Let -> parseIfLetExpr …
    | _ -> parseIfExpr …
  in
  ...

[IwanKaramazow]

See comments, we can bring this inline.

[IwanKaramazow]

For consistency I would rename this to parseIfLetExpr to indicate that we're going to parse something of the expression flavour.

[IwanKaramazow]

Should we rename this to parseThenBranch for clarity?

[IwanKaramazow]

"Core" is a bit opaque, would it be clearer if we named this just parseIfExpr?

[IwanKaramazow]

Can be removed as we're not supporting guards in v1?

[yawaramin]

Does this PR suppress the fragile pattern match warning (4)? I guess it wouldn't, correct?

[tomgasson]

@yawaramin: If there's an else { expr } it's encoded as | _ => expr and if not there's an implicit | _ => () used, so this will never trigger that warning

[yawaramin]

@tomgasson thanks. What I'm thinking of is something like:

let x = if let Ok(_) = result { 1 } else { 0 }

...desugaring to:

let x = switch result {
  | Ok(_) => 1
  | _ => 0
}

...which indeed seems to trigger the warning.

[tomgasson]

@yawaramin: Ah, I see. Yeah I think we should then suppress it then, since the intention here is to allow that

but you've raised a few more cases of interest to me

let x = if let Ok(_) = result {
  1
} else if let Error(_) = result {
  2
} else {
  0
}

...the else will never be taken. else if becomes a nested switch rather than lifting the conditions up to the same level because that's better in the general case (if it was getResult() and side-effecting or if they were different result and other)

let x = if let Ok(_) | Error(_) = result {
  1
} else {
  0
}

...the else will never be take. The existing warning should let us know

[yawaramin]

@tomgasson yeah, in that case we would hit warning 11 (this match case is unused). But just to clarify, you do want to suppress warning 4 for this construct.

Another question, will warning 11 wording also be adjusted to reflect the sugar syntax? I.e. it would be confusing to see this match case is unused for an unused else clause. Although to be somewhat fair, the wording is used for Reason's current switch anyway.

[tomgasson]

Updated to suppress warning 4.

For the wording of 11, I think we need the capability to rewrite warnings as mentioned in #1

[IwanKaramazow]

@tomgasson Merging, thanks for the implementation. Nice work!