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

feat: let ... else binding #3817

Merged
merged 86 commits into from
Feb 23, 2023
Merged

feat: let ... else binding #3817

merged 86 commits into from
Feb 23, 2023

Conversation

kentosugama
Copy link
Contributor

@kentosugama kentosugama commented Feb 17, 2023

Original discussions in #3588.

New let binding that allows a failure block in case of a pattern-match failure. The main motivation for this feature is to avoid deeply nested switch statements that lead to less readable code.

See forum discussion here.

Simple example:

let ?x = foo() else { Debug.print "foo failed"; return };

The failure block must always have type None to ensure that execution does not continue beyond the failed binding. Failure blocks may thus trap, return out of the function, or break / continue if in the context of a loop.

Failure blocks may also perform asynchronous effects, if the binding itself is in an asynchronous context.

let (#ok x) = foo() else { await logError(); return; };

More complex example:
Without this binding, you might have to either do the following

switch (foo()) {
  case (?x) {
    switch (bar()) {
      case (?y) {
        switch (baz()) {
          case (?z) {
            return #ok x + y + z;
          }
          case null {
            return #err "baz failed";
          }
        }
      };
      case null {
        return #err "bar failed";
      }
    }
  };
  case null {
    return #err "foo failed with";
  }
}

or

switch (foo(), bar(), baz()) {
  case (?x, ?y, ?z) {
    return #ok x + y + z;
  }
  case (null, _, _) {
    return #err "foo failed";
  }
  case (_, null, _) {
    return #err "bar failed";
  }
  case (_, _, null) {
    return #err "baz failed";
  }
}

however this second version does not execute examples sequentially (i.e. even if foo() fails, bar() and baz() will execute).

With let-else bindings:

let ?x = foo() else { return #err "foo failed" };
let ?y = bar() else { return #err "bar failed" };
let ?z = baz() else { return #err "baz failed" };
   
return #ok x + y + x

Inspired by the same feature in rust


Items left to do:

  • Change parser to parse the body of else as a block instead of a value
  • Have return at the end of else block return out of the function
  • Finish the compilation with desugaring
  • Come up with lots of positive (run) and negative (fail) tests
    • return, continue, break in failure part (also with 0, 1 and more return values)
    • return in monadic context (async)
    • await in failure part
  • Assign error code and explanation (M0500 is provisional) — not needed
  • Suppress coverage warning when fail-expression is present
  • Let-bound terminal expressions (in compUnit.ml)?
  • Double check all the None pattern matches
  • Add documentation
  • remove trailing whitespace from PR
  • add trailing newlines to files that miss it
  • Fix repl tests
  • Fix async effects
  • Fix null pattern matches
  • Find more complex example

Other PR:

Potential further optimisations:

  • let (b_1, b_2, ..., b_n) = switch { case <pat> (b_1, b_2, ..., b_n); ... };

@mergify mergify bot mentioned this pull request Feb 17, 2023
19 tasks
@ggreif ggreif changed the title feat: let ... else binding feat: let ... else binding Feb 17, 2023
@github-actions
Copy link

github-actions bot commented Feb 17, 2023

Comparing from 8b92bee to 684fe68:
In terms of gas, no changes are observed in 4 tests.
In terms of size, no changes are observed in 4 tests.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some interim comments on doc. Now looking at code.

doc/md/language-manual.md Show resolved Hide resolved
doc/md/language-manual.md Outdated Show resolved Hide resolved
src/mo_frontend/typing.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. I'm still worried by some of those pattern matches on None.

src/mo_frontend/typing.ml Outdated Show resolved Hide resolved
src/mo_frontend/typing.ml Outdated Show resolved Hide resolved
src/mo_frontend/typing.ml Show resolved Hide resolved
src/mo_frontend/typing.ml Outdated Show resolved Hide resolved
@@ -771,35 +771,35 @@ and define_id env id v =
Lib.Promise.fulfill (find id.it env.vals) v

and define_pat env pat v =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a bool is nicer and more efficient I expect. Nice!

src/mo_frontend/typing.ml Outdated Show resolved Hide resolved
src/mo_frontend/static.ml Show resolved Hide resolved
src/mo_def/compUnit.ml Outdated Show resolved Hide resolved
src/lowering/desugar.ml Outdated Show resolved Hide resolved
src/lowering/desugar.ml Outdated Show resolved Hide resolved
@crusso crusso self-requested a review February 20, 2023 22:52
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Generally looks good but I'm still worried by some of those pattern matches on None. The problem is that alot of our actor/module/object sugar desugars to let expressions (to support recursion) but the code to spot these may now fail or behave strangely differently when that desugaring suddenly contains a fail clause (because the author didn't use the sugar but programmed directly in the target language. I guess the problem wouldn't exist if people could not both write, e.g.

module X = {}

and

let X = module {}

but now also

let X = module {} else {...}

(and similarly for objects and actors).

The first two are treated the same by the compiler, but the last winds up being treated differently in some cases the programmer might not be expecting.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@crusso crusso added the automerge-squash When ready, merge (using squash) label Feb 23, 2023
@mergify mergify bot merged commit 93bafab into master Feb 23, 2023
@mergify mergify bot deleted the gabor/let-else branch February 23, 2023 21:54
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 23, 2023
@kentosugama kentosugama mentioned this pull request Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants