8000 Lambda-case `cases` syntax by anovstrup · Pull Request #1192 · unisonweb/unison · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Lambda-case cases syntax #1192

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 20, 2020
Merged

Lambda-case cases syntax #1192

merged 3 commits into from
Feb 20, 2020

Conversation

anovstrup
Copy link
Contributor
@anovstrup anovstrup commented Feb 4, 2020 •
edited
Loading

This PR provides support for syntax like the following:

store : a -> Request {Store a} v -> v
store init = cases
  { x }                -> x
  { Store.get   -> k } -> handle k init with store init
  { Store.put v -> k } -> handle !k with store v

Changes:

  • Parse lambda-case syntax (cases keyword)
  • Pretty-print using the new syntax when appropriate
  • Update tests
  • Update editor support

Before the next release:

  • Update docs

@pchiusano
Copy link
Member

@anovstrup Discussion on Slack is for more general lambda case mechanism. Dan suggesed cases to introduce such a lambda:

cases
  { x } -> x
  { Store.get   -> k } -> ...
  { Store.put v -> k } -> ...

-- proposal to have it work for n-ary lambdas too (can be added later)
fix cases
  self { x } -> x
  self { Store.get   -> k } -> ...
  self { Store.put v -> k } -> ...

I like this proposal. You need some keyword or token to introduce such a lambda. Should the keyword be cases? Just case? Something else?

I think it has to be syntactically distinct from the keyword that introduces a regular pattern matching, otherwise ambiguity.

@anovstrup anovstrup force-pushed the topic/handle-with-lambdacase branch from d59134f to afca605 Compare February 6, 2020 16:51
anovstrup
anovstrup commented Feb 6, 2020
View reviewed changes
@anovstrup
Copy link
Contributor Author

@pchiusano Can you recommend someone to review this PR? I'm not sure who's best able to review parser/lexer/pretty-printer changes. It's not ready to merge, yet, but I'd appreciate feedback before going further, when somebody's got time.

@pchiusano
Copy link
Member

@anovstrup - could we actually do cases instead of matches for the lambda case intro? I think matches reads a little weird (I keep reading it as "this matches that" rather than "here's a bunch of cases to match against"). Also just think cases is a bit better since the various cases of a pattern matching expression are usually called "cases" (where each case consists of a pattern on the left side and a right hand side).

I read the match ... with as "match this expression against these cases". So just the cases is introduced via cases.

I'll take a look at the lexer / parser changes.

@atacratic could you review the pretty-printer changes?

@pchiusano
Copy link
Member

@anovstrup honestly, seeing some of the complexity in the lexer makes me wonder if we should punt on the match ... with change until we can rewrite the lexer (I'd like lexer to just be Megaparsec) to make these sorts of changes easier.

Like just do the lambda case cases syntax. That should be easy to get working and merged and would be pretty nice to have!!

WDYT?

@anovstrup
Copy link
Contributor Author

@pchiusano 😬 You're killing me! :)

Although the lexer changes did end up being more of a bear than desirable, they're already done. If review is too difficult, my preference would be to add code/review comments rather than throw the changes away. That said, I could cherry-pick the match .. with changes into a separate PR so that lambda-case can be finished up sooner. No reason to couple these changes if the lambda-case doesn't use a similar keyword, and I agree that cases reads better than matches (I kept going back and forth on that).

@pchiusano
Copy link
Member

@anovstrup lol, okay, sorry! I misinterpreted your "It's not ready to merge, yet, but I'd appreciate feedback before going further, when somebody's got time." thinking you still had stuff that needed working out.

If you were doing this from scratch it would have been better to separate the lambda cases into a separate PR but I think it's okay not to split them up now and I'm guessing it'd be a hassle.

I'll review the parser and lexer changes, hopefully this weekend sometime.

@anovstrup
Copy link
Contributor Author

@pchiusano Splitting off a PR would be easy, actually. The commits for lambda-case are separate from ones for match-with, so it's a simple cherry-pick. There's still stuff to do (for both parts; see the checkboxes in the description), but the hardest stuff is done (I think).

@anovstrup anovstrup force-pushed the topic/handle-with-lambdacase branch from 1b1a2fc to 46c312b Compare February 8, 2020 03:09
@anovstrup
Copy link
Contributor Author

@pchiusano @atacratic I split off the match .. with changes into a separate PR (#1214). Those are the trickier changes, but I still wouldn't mind feedback on this PR.

@anovstrup anovstrup force-pushed the topic/handle-with-lambdacase branch from 88d23fb to 894f2fb Compare February 14, 2020 16:43
@pchiusano
Copy link
Member

@anovstrup for the pretty-printer, I think you could check here inside TermPrinter.prettyBinding0. You're looking for f v1 v2 ... vN = match vN with ... where vN is not a free variable in the match (you can use Term.freeVars to get the set of free variables of a term).

    LamsNamedOrDelay' vs body ->
      let (im', uses) = calcImports im body'
          -- In the case where we're being called from inside `pretty0`, this
          -- call to printAnnotate is unfortunately repeating work we've already
          -- done.
          body'       = printAnnotate env body
      in  PP.group
            $         PP.group (defnLhs v vs <> fmt S.BindingEquals " =")
            `PP.hang` uses [pretty0 env (ac (-1) Block im' doc) body']

You can use the Term.Match' pattern to detect if the body is a match expression.

Let me know how it goes!

@anovstrup
Copy link
Contributor Author

@pchiusano Thanks for the tip. I've starting looking at it -- hopefully I'll have something working this weekend.

@anovstrup
Copy link
Contributor Author
anovstrup commented Feb 15, 2020 •
edited
Loading

@pchiusano Turns out it's a bit more complicated, I think. In the cases we're interested in, vN is (of course) a free variable in the match term, since it's the scrutinee. We need to check that it's not free in the cases themselves, but the MatchCases are not themselves terms (nor are the patterns, which may need to be checked, too, in case they reference the variable?). 8000 So I think I need to check each guard and case RHS term with freeVars, but I'm not sure what to do with the Patterns.

I'll also need to handle cases that are not in let bindings, but it should be straightforward once I figure out the ones in bindings.

@pchiusano
Copy link
Member
pchiusano commented Feb 15, 2020 •
edited
Loading

@anovstrup - cool, yeah that makes sense. You don’t need to do anything for the patterns, those don’t contain variable references, just the RHS and guards is sufficient. You can take my word for this for now, but I’ll dig up link re: how pattern matching is encoded with ABTs if you are curious.

@anovstrup
Copy link
Contributor Author

@pchiusano Progress! I'm probably not dealing with imports correctly, though — I'd appreciate if you or @atacratic could review my WIP when you get a chance. In the meantime, I'm going to work on adding more test cases and pretty-printing cases outside of let bindings.

@anovstrup
Copy link
Contributor Author

Any idea why this would fail as a test case in the TermParser tests, while a similar form parses and typechecks just fine in ucm?

"handle foo with cases\n" ++
" { a -> k } -> b\n" ++ 
" { x } -> x"

It fails with:

🦄  termparser.handle foo with cases
2:6:
unexpected ->
    2 |  { a -> k } -> b

💥  termparser.handle foo with cases
termparser.handle foo with cases FAILURE 2:6:
unexpected ->
    2 |  { a -> k } -> b
 CallStack (from HasCallStack):
  crash, called at tests/Unison/Test/TermParser.hs:225:7 in main:Unison.Test.TermParser

This, on the other hand, parses, typechecks, and evaluates correctly in ucm:

ability Abort where abort : a

> handle
    Abort.abort
    1
  with cases
    { Abort.abort -> k } -> 0
    { x } -> x

@anovstrup
Copy link
Contributor Author

I made the Abort example a test case, and it fails with this:

I don't know about any ability constructor named
Abort.abort. Maybe make sure it's correctly spelled and
that you've imported it:

    5 |   { Abort.abort -> k } -> 0

So I guess that explains the other failure... I didn't realize that the parser knew about existing definitions/names.

@pchiusano
Copy link
Member

@anovstrup yes pattern constructor references are resolved at parse time. This was a mistake but isn’t a slam dunk to fix.

@anovstrup
Copy link
Contributor Author

This is ready for review, but I'm not that confident about the pretty-printing code.

@anovstrup anovstrup marked this pull request as ready for review February 18, 2020 00:01
@atacratic
Copy link
Contributor

Sorry for not taking a look at this yet - I'll review the pretty-printer changes on Wednesday night.

Great that you've done this! 👍

atacratic
atacratic approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor
@atacratic atacratic left a comment

Choose a reason for hiding this comment

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

TermPrinter changes look good!

@anovstrup anovstrup changed the title Allow request patterns after with Lambda-case cases syntax Feb 20, 2020
pchiusano
pchiusano approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member
@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

Looks great @anovstrup am stoked to try this out!!

It is pretty cool how easy it is to make changes to the syntax, and how existing code will render using cases when possible, just via that tweak to the pretty printer.

unison-core/src/Unison/Term.hs
Comment on lines +792 to +794
Just (reverse -> (v1:vs), Match' (Var' v1') branches) |
(v1 == v1') && not (Set.member v1' (Set.unions $ freeVars <$> branches)) ->
Just (reverse vs, branches)
Copy link
Member

Choose a reason for hiding this comment

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

These three lines are pretty epic and are doing a lot, but pretty easy to follow.

I might have written as not (any (Set.member v1') (freeVars <$> branches)) to avoid needing to build up the union'd Set (which does a bunch of tree rebalancing stuff) but I think it's totally fine as is.

@pchiusano pchiusano merged commit 652ae4a into unisonweb:master Feb 20, 2020
@anovstrup anovstrup deleted the topic/handle-with-lambdacase branch February 20, 2020 16:43
@anovstrup anovstrup mentioned this pull request Mar 1, 2020
Merged
@aryairani aryairani mentioned this pull request Mar 12, 2020
Merged
@pchiusano pchiusano mentioned this pull request May 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchiusano pchiusano pchiusano approved these changes

@atacratic atacratic atacratic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anovstrup @pchiusano @atacratic
0