QPQ-AG/sophia
10
0
Fork 0
Code Issues 41 Pull Requests 5 Actions Packages Projects Releases 16 Wiki Activity

Pattern guards for functions and switch statements #830

Merged
ghallak merged 23 commits from ghallak/232 into master 2021-10-20 17:04:01 +09:00
Conversation 38 Commits 23 Files Changed 17 +245 -74
ghallak commented 2021-09-02 23:20:25 +09:00 (Migrated from gitlab.com)
Copy Link

Fixes: #232

Fixes: #232
zxq9 commented 2021-09-14 21:46:41 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

I'd elaborate a little bit on that. Note that many smart contract devs aren't familiar with functional programming. Also, mention that these expressions cannot be stateful

*Created by: radrow* I'd elaborate a little bit on that. Note that many smart contract devs aren't familiar with functional programming. Also, mention that these expressions cannot be stateful
zxq9 commented 2021-09-14 21:48:56 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Looks good from a quick look, are there any (actually running) tests over in aeternity repo?

*Created by: hanssv* Looks good from a quick look, are there any (actually running) tests over in `aeternity` repo?
zxq9 commented 2021-09-14 21:52:25 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

For the user facing syntax I think it would be better to have just one variant for simplicity. If there are no guards, then the expr could be true which can be optimized out later on. Also, I would rather opt for comma separated multiple guards -- this would ease implementing assigning values in guards like you can do in Haskell (but that's for another task). Then, no guards would be just an empty list here. @UlfNorell what do you think?

*Created by: radrow* For the user facing syntax I think it would be better to have just one variant for simplicity. If there are no guards, then the expr could be `true` which can be optimized out later on. Also, I would rather opt for comma separated multiple guards -- this would ease implementing assigning values in guards like you can do in Haskell (but that's for another task). Then, no guards would be just an empty list here. @UlfNorell what do you think?
zxq9 commented 2021-09-14 21:53:33 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Remove stateful context so there are no surprises

*Created by: radrow* Remove stateful context so there are no surprises
zxq9 commented 2021-09-14 21:54:08 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Add some negative test maybe?

*Created by: radrow* Add some negative test maybe?
zxq9 commented 2021-09-14 22:24:35 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Not introducing redundant forms in the AST is good, and comma separated guards make sense. Especially if we want to add support for what is called pattern guards in Haskell land later. (What you have here is referred to as just guards.)

*Created by: UlfNorell* Not introducing redundant forms in the AST is good, and comma separated guards make sense. Especially if we want to add support for what is called pattern guards in Haskell land later. (What you have here is referred to as just *guards*.)
ghallak commented 2021-09-19 00:10:32 +09:00 (Migrated from gitlab.com)
Copy Link

Done here 756c05c60038d774ce1156c7b65896ea9741036e Had to change this in 6b9bbfa in order to get a more relevant error message, and I've added a failing test for stateful guards as well.

~~Done here 756c05c60038d774ce1156c7b65896ea9741036e~~ Had to change this in 6b9bbfa in order to get a more relevant error message, and I've added a failing test for stateful guards as well.
ghallak commented 2021-09-19 00:33:56 +09:00 (Migrated from gitlab.com)
Copy Link

Let me know if you think this makes it clearer 2deb568be3081db48f2ea0ddf77337aa1bde88ba

Let me know if you think this makes it clearer 2deb568be3081db48f2ea0ddf77337aa1bde88ba
zxq9 commented 2021-09-21 18:03:48 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

-type letfun()  :: {letfun, ann(), id(), [pat()], type(), [guard()], expr()}.
*Created by: radrow* ```suggestion:-0+0 -type letfun() :: {letfun, ann(), id(), [pat()], type(), [guard()], expr()}. ```
zxq9 commented 2021-09-21 18:05:23 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

-type alt() :: {'case', ann(), pat(), [guard()], expr()}.

-type guard() :: expr().

So we may add assign-guards later on here

*Created by: radrow* ```suggestion:-0+0 -type alt() :: {'case', ann(), pat(), [guard()], expr()}. -type guard() :: expr(). ``` So we may add assign-guards later on here
zxq9 commented 2021-09-21 18:09:21 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

    [ ?RULE(id(), args(),                   tok('|'), comma_sep(expr()), tok('='), body(), {letfun, get_ann(_1), _1, _2, type_wildcard(get_ann(_1)), _4, _6})
    , ?RULE(id(), args(), tok(':'), type(), tok('|'), comma_sep(expr()), tok('='), body(), {letfun, get_ann(_1), _1, _2, _4, _6, _8})
*Created by: radrow* ```suggestion:-0+0 [ ?RULE(id(), args(), tok('|'), comma_sep(expr()), tok('='), body(), {letfun, get_ann(_1), _1, _2, type_wildcard(get_ann(_1)), _4, _6}) , ?RULE(id(), args(), tok(':'), type(), tok('|'), comma_sep(expr()), tok('='), body(), {letfun, get_ann(_1), _1, _2, _4, _6, _8}) ```
zxq9 commented 2021-09-21 18:11:09 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Might be also good to make guard/0 a separate parser

*Created by: radrow* Might be also good to make `guard/0` a separate parser
zxq9 commented 2021-09-21 18:12:10 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Isn't there some combinator for optional parsing?

*Created by: radrow* Isn't there some combinator for optional parsing?
zxq9 commented 2021-09-21 18:13:41 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Review: Commented

You can compile a list of guards as a big conjunction.

*Created by: radrow* **Review:** Commented You can compile a list of guards as a big conjunction.
zxq9 commented 2021-09-22 01:36:17 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

I'd split the sentence before "if" and "otherwise". Otherwise good

*Created by: radrow* I'd split the sentence before "if" and "otherwise". Otherwise good
zxq9 commented 2021-10-03 23:09:59 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

why list of expr?

*Created by: radrow* why list of expr?
ghallak commented 2021-10-03 23:13:57 +09:00 (Migrated from gitlab.com)
Copy Link

multiple guards, each guard corresponds to one body.

multiple guards, each guard corresponds to one body.
ghallak commented 2021-10-03 23:14:21 +09:00 (Migrated from gitlab.com)
Copy Link

multiple guards, each guard corresponds to one body.

multiple guards, each guard corresponds to one body.
zxq9 commented 2021-10-03 23:23:40 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

I don't get it, how do you represent

switch(x)
  (a, b) | a > 0, b > 0 => a + b
         | true => 0

?

*Created by: radrow* I don't get it, how do you represent ``` switch(x) (a, b) | a > 0, b > 0 => a + b | true => 0 ``` ?
zxq9 commented 2021-10-03 23:24:44 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Or if it is invalid

switch(x)
  (a, b) | a > 0, b > 0 => a + b
  _  => 0
*Created by: radrow* Or if it is invalid ``` switch(x) (a, b) | a > 0, b > 0 => a + b _ => 0 ```
ghallak commented 2021-10-04 14:52:15 +09:00 (Migrated from gitlab.com)
Copy Link

The above does is not valid since the guard should be a single expr, so if you write a > 0 && b > 0 instead of a > 0, b > 0 in the first example it would be parsed like this:

[{switch,                                                                                     [3/4542]
     [{file,no_file},{line,4},{col,5}],
     {id,[{file,no_file},{line,4},{col,12}],"x"},
     [{'case',
          [{file,no_file},{line,6},{col,26}],
          {tuple,
              [{file,no_file},{line,5},{col,7}],
              [{id,[{file,no_file},{line,5},{col,8}],"a"},
               {id,[{file,no_file},{line,5},{col,11}],"b"}]},
          [{app,
               [{file,no_file},{line,6},{col,11},{format,infix}],
               {'&&',[{file,no_file},{line,6},{col,17}]},
               [{app,
                    [{file,no_file},{line,6},{col,11},{format,infix}],
                    {'>',[{file,no_file},{line,6},{col,13}]},
                    [{id,[{file,no_file},{line,6},{col,11}],"a"},
                     {int,[{file,no_file},{line,6},{col,15}],0}]},
                {app,
                    [{file,no_file},{line,6},{col,20},{format,infix}],
                    {'>',[{file,no_file},{line,6},{col,22}]},
                    [{id,[{file,no_file},{line,6},{col,20}],"b"},
                     {int,[{file,no_file},{line,6},{col,24}],0}]}]},
           {bool,[{file,no_file},{line,7},{col,11}],true}],
          [{app,
               [{file,no_file},{line,6},{col,29},{format,infix}],
               {'+',[{file,no_file},{line,6},{col,31}]},
               [{id,[{file,no_file},{line,6},{col,29}],"a"},
                {id,[{file,no_file},{line,6},{col,33}],"b"}]},
           {int,[{file,no_file},{line,7},{col,19}],0}]}]}]}]}]

Notice that there are 2 guards and 2 bodies.

Do you think there's something wrong with that?

The above does is not valid since the guard should be a single expr, so if you write `a > 0 && b > 0` instead of `a > 0, b > 0` in the first example it would be parsed like this: ``` [{switch, [3/4542] [{file,no_file},{line,4},{col,5}], {id,[{file,no_file},{line,4},{col,12}],"x"}, [{'case', [{file,no_file},{line,6},{col,26}], {tuple, [{file,no_file},{line,5},{col,7}], [{id,[{file,no_file},{line,5},{col,8}],"a"}, {id,[{file,no_file},{line,5},{col,11}],"b"}]}, [{app, [{file,no_file},{line,6},{col,11},{format,infix}], {'&&',[{file,no_file},{line,6},{col,17}]}, [{app, [{file,no_file},{line,6},{col,11},{format,infix}], {'>',[{file,no_file},{line,6},{col,13}]}, [{id,[{file,no_file},{line,6},{col,11}],"a"}, {int,[{file,no_file},{line,6},{col,15}],0}]}, {app, [{file,no_file},{line,6},{col,20},{format,infix}], {'>',[{file,no_file},{line,6},{col,22}]}, [{id,[{file,no_file},{line,6},{col,20}],"b"}, {int,[{file,no_file},{line,6},{col,24}],0}]}]}, {bool,[{file,no_file},{line,7},{col,11}],true}], [{app, [{file,no_file},{line,6},{col,29},{format,infix}], {'+',[{file,no_file},{line,6},{col,31}]}, [{id,[{file,no_file},{line,6},{col,29}],"a"}, {id,[{file,no_file},{line,6},{col,33}],"b"}]}, {int,[{file,no_file},{line,7},{col,19}],0}]}]}]}]}] ``` Notice that there are 2 guards and 2 bodies. Do you think there's something wrong with that?
ghallak commented 2021-10-04 14:56:23 +09:00 (Migrated from gitlab.com)
Copy Link

Done that here 70af4828483be4a8ac93a9b123411e2e0b946fef

Done that here 70af4828483be4a8ac93a9b123411e2e0b946fef
ghallak commented 2021-10-04 15:03:18 +09:00 (Migrated from gitlab.com)
Copy Link

Why do you think that guards should have comma separated exprs? Isn't it better if a single expr is used and boolean operators are used when needed?

Why do you think that guards should have comma separated exprs? Isn't it better if a single expr is used and boolean operators are used when needed?
ghallak commented 2021-10-04 15:07:12 +09:00 (Migrated from gitlab.com)
Copy Link

If there are multiple guards and a single body, how would you parse something like

switch(x)
  (a, b) | a > 0, b > 0 => a + b
         | true => 0
If there are multiple guards and a single body, how would you parse something like ``` switch(x) (a, b) | a > 0, b > 0 => a + b | true => 0 ```
zxq9 commented 2021-10-04 21:25:04 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Uh, when talking about multiple guards we were talking about | a > 0, b > 0, so in the future we might have | a > 0, b > 0, let x = a + b, x > 10. That's why there would be a list of guards.

Multiple guard alternatives for a single pattern are great too, but then why not pack them into a list of tuples? They are conceptually together, so imo they should stay side by side.

My idea for the typing:

-type alt() :: {'case', ann(), pat(), [guard_alt()]}.
-type guard_alt() :: {guarded, ann(), [guard()], expr()}.
-type guard() :: expr()
  % | letval()  % add in the future
   .

This way this:

switch(x)
  (a, b) | a > 0, b > 0 => a + b
         | true => 0

can be expressed as (simplified for readability)

{switch, _, "x", [
  {case, _, {tuple, ["a", "b"]}, [
    {guarded, _, [a > 0, b > 0], a + b},
    {guarded, _, true, 0}
  ]}
]}
*Created by: radrow* Uh, when talking about multiple guards we were talking about `| a > 0, b > 0`, so in the future we might have `| a > 0, b > 0, let x = a + b, x > 10`. That's why there would be a list of guards. Multiple guard alternatives for a single pattern are great too, but then why not pack them into a list of tuples? They are conceptually together, so imo they should stay side by side. My idea for the typing: ```erlang -type alt() :: {'case', ann(), pat(), [guard_alt()]}. -type guard_alt() :: {guarded, ann(), [guard()], expr()}. -type guard() :: expr() % | letval() % add in the future . ``` This way this: ``` switch(x) (a, b) | a > 0, b > 0 => a + b | true => 0 ``` can be expressed as (simplified for readability) ``` {switch, _, "x", [ {case, _, {tuple, ["a", "b"]}, [ {guarded, _, [a > 0, b > 0], a + b}, {guarded, _, true, 0} ]} ]} ```
zxq9 commented 2021-10-05 18:05:08 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Because later we might want to add support for introducing variables in guards. Just like in Haskell you can do

f x | y <- x + x, y > 10 = 10
*Created by: radrow* Because later we might want to add support for introducing variables in guards. Just like in Haskell you can do ``` f x | y <- x + x, y > 10 = 10 ```
ghallak commented 2021-10-12 18:30:13 +09:00 (Migrated from gitlab.com)
Copy Link

Looks good from a quick look, are there any (actually running) tests over in aeternity repo?

There are tests here now (and they're passing): 4683da5b50

> Looks good from a quick look, are there any (actually running) tests over in `aeternity` repo? There are tests here now (and they're passing): https://github.com/ghallak/aeternity/commit/4683da5b500dd764f62d4c8b4f3f1e83e9723fd7
zxq9 commented 2021-10-18 16:57:59 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

This example won't compile 😛

*Created by: hanssv* This example won't compile 😛
zxq9 commented 2021-10-18 17:32:03 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

I'm not entirely convinced the new test is better 🤔

*Created by: hanssv* I'm not entirely convinced the new test is better 🤔
ghallak commented 2021-10-18 18:39:29 +09:00 (Migrated from gitlab.com)
Copy Link

Now it does 91002c1f77fc164484f05c1ab82708805c5f35bb

Now it does 91002c1f77fc164484f05c1ab82708805c5f35bb
zxq9 commented 2021-10-18 18:42:56 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

👍

*Created by: hanssv* 👍
ghallak commented 2021-10-18 19:13:19 +09:00 (Migrated from gitlab.com)
Copy Link

In this commit e828099bd97dffe11438f2e48f3a92ce3641e85b I have disabled multiple tests that are related to aevm, including this one.

To make these tests work, changes are required to be made to icode generation which is deprecated as I understood.

Should I make these changes to icode generation? or is there another way to re-enable these tests?

In this commit e828099bd97dffe11438f2e48f3a92ce3641e85b I have disabled multiple tests that are related to aevm, including this one. To make these tests work, changes are required to be made to icode generation which is deprecated as I understood. Should I make these changes to icode generation? or is there another way to re-enable these tests?
zxq9 commented 2021-10-18 21:27:00 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Ah, I had forgotten these tests were AEVM only... What makes them fail? I don't think we should just break AEVM compilation we should rather remove it properly then?

*Created by: hanssv* Ah, I had forgotten these tests were AEVM only... What makes them fail? I don't think we should just break AEVM compilation we should rather remove it properly then?
ghallak commented 2021-10-19 00:15:16 +09:00 (Migrated from gitlab.com)
Copy Link

I have changed the icode generation so that aevm compilation won't break when no guards are used, but it will break when guards are used here 96dc007f00c37368b3c5603d04ab64244e9d70ac.

And I have reverted the commit that disables the aevm tests here 1525e80992f562dedfaacc61e61486291ef51675.

I have changed the icode generation so that aevm compilation won't break when no guards are used, but it will break when guards are used here 96dc007f00c37368b3c5603d04ab64244e9d70ac. And I have reverted the commit that disables the aevm tests here 1525e80992f562dedfaacc61e61486291ef51675.
zxq9 commented 2021-10-20 00:15:00 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Great!

*Created by: hanssv* Great!
zxq9 commented 2021-10-20 00:15:11 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Review: Approved

*Created by: hanssv* **Review:** Approved
zxq9 commented 2021-10-20 02:58:10 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Review: Approved

*Created by: radrow* **Review:** Approved
zxq9 commented 2021-10-20 17:04:01 +09:00 (Migrated from gitlab.com)
Copy Link

Merged by: ghallak at 2021-10-20 08:04:01 UTC

*Merged by: ghallak at 2021-10-20 08:04:01 UTC*
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
WIP
bug
consensus-breaking
dependencies
documentation
duplicate
effort: high
effort: low
effort: medium
effort: trivial
enhancement
good first issue
help wanted
invalid
maintenance
question
task/feature
todo-in-rewrite
wontfix
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
pig lipstick
Muggle-facing enhancements
question
More information is needed
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: QPQ-AG/sophia#830
No description provided.
Delete Branch "ghallak/232"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?