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

Proposal 869: pattern-matching syntax #908

Merged
merged 53 commits into from
Sep 4, 2020
Merged

Conversation

Emanon42
Copy link
Contributor

@Emanon42 Emanon42 commented Aug 4, 2020

This pull request implements #869

To enable this syntax sugar, set pattern_matching_sugar=true in config

Syntax

It adds syntax of SML-style pattern-matching like that:

fun ack(_,_) switch {
  case (0, n) ->  n + 1
  case (m, 0) -> ack(m - 1, 1)
  case (m,n) ->  ack(m - 1, ack(m, n - 1)) 
}

Semantics

Desugaring the cases defined in pattern-matching function to a Switch phrase and adding a wild card case to raise an error to handle the non-exhaustive case (with position of wrong code).

@dhil
Copy link
Member

dhil commented Aug 26, 2020

@Emanon42 while I remember it, for consistency we should probably change all occurrences of MatchFunlit to SwitchFunlit and the match_ prefixed type aliases to have switch_ as a prefix instead.

core/desugarMatching.ml Outdated Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/desugarMatching.ml Outdated Show resolved Hide resolved
core/frontend.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@jamescheney jamescheney left a comment

Choose a reason for hiding this comment

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

This looks good now. My main suggestion besides whitespace nitpicking is to make get_normal_funlit into a library function and use it in more places to avoid repetition (and save effort if we ever later refactor the funlit cases). Otherwise looks ready to merge.

core/parser.mly Outdated Show resolved Hide resolved
core/transformSugar.ml Outdated Show resolved Hide resolved
core/typeSugar.ml Outdated Show resolved Hide resolved
core/typeSugar.ml Outdated Show resolved Hide resolved
@dhil dhil requested a review from SimonJF August 28, 2020 18:28
@Emanon42
Copy link
Contributor Author

This looks good now. My main suggestion besides whitespace nitpicking is to make get_normal_funlit into a library function and use it in more places to avoid repetition (and save effort if we ever later refactor the funlit cases). Otherwise looks ready to merge.

Looks like put get_normal_funlit in utility.ml could have the problem of the dependency cycle, so I just put it in sugartypes.

Copy link
Member

@SimonJF SimonJF left a comment

Choose a reason for hiding this comment

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

Thanks for doing all these revisions, @Emanon42! It was good working with you during your internship.

I've got a few final comments, but all of the major things have been addressed now. I'd be happy for this to go in.

core/desugarInners.ml Outdated Show resolved Hide resolved
core/desugarSwitchFuns.ml Outdated Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/sugarTraversals.ml Outdated Show resolved Hide resolved
core/sugarTraversals.ml Outdated Show resolved Hide resolved
core/sugarTraversals.ml Outdated Show resolved Hide resolved
core/sugartypes.ml Show resolved Hide resolved
core/transformSugar.ml Outdated Show resolved Hide resolved
core/typeSugar.ml Outdated Show resolved Hide resolved
core/typeSugar.ml Outdated Show resolved Hide resolved
@jamescheney
Copy link
Contributor

Hmm, the CI is failing due to the documentation not building for some reason - maybe a transient problem? (this only happens on Travis so it might be a Travis problem).

@dhil, any further remarks before this gets merged? It seems that your most recent review comments have been addressed.

@dhil
Copy link
Member

dhil commented Sep 1, 2020

It seems that anonymous switch functions are not being desugared properly:

links> fun(s) switch { case x -> x };
***: Error: "Assert_failure core/desugarModules.ml:322:15"

core/desugarSwitchFuns.ml Outdated Show resolved Hide resolved
Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

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

Looks good to me! I will merge this in shortly.

let name_list = List.map (fun pat -> (pat, Utility.gensym())) patterns in
let switch_tuple = List.map (fun (_, name) -> with_pos (Var name)) name_list in
(* assemble exhaustive handler *)
let exhaustive_patterns = with_pos (Pattern.Any) in
Copy link
Member

Choose a reason for hiding this comment

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

Note: This pattern/case may be redundant, though, currently we do not check for redundancy or exhaustiveness of patterns.

@dhil dhil merged commit f978d45 into links-lang:master Sep 4, 2020
@SimonJF
Copy link
Member

SimonJF commented Sep 4, 2020

Congratulations, @Emanon42!

@Emanon42
Copy link
Contributor Author

Emanon42 commented Sep 4, 2020

Congratulations, @Emanon42!

Thanks again for mentoring me! It is really happy to work with you all.
and new avatar is cool

frank-emrich pushed a commit to frank-emrich/links that referenced this pull request Sep 16, 2020
This patch implements named and anonymous switch functions, which provide convenient syntax for defining functions on a case-by-case basis on their input.

Closes links-lang#869.
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.

Proposal: SML-style function definition groups
4 participants