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

Pipe operator support #91

Closed
wants to merge 4 commits into from
Closed

Conversation

ayshiff
Copy link

@ayshiff ayshiff commented Mar 29, 2021

Closes #72

This PR adds pipe operator support (|>).
It is in Draft because we have to wait for #44 to be resolved first. (As mentionned here)

Copy link
Contributor

@michallepicki michallepicki left a comment

Choose a reason for hiding this comment

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

I think this is a good step forward to get things working, great job!

@@ -16,3 +17,5 @@ recv(Timeout) ->

binary_concat(A, B) when is_binary(A) and is_binary(B) ->
<< (A)/binary, (B)/binary >>.

pipe(A, B) -> B A.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pipe(A, B) -> B A.
pipe(A, B) -> B(A).

I think Erlang needs the parentheses here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

$ erl
1> F = fun (X) -> X + 1 end.
#Fun<erl_eval.44.79398840>
2> F(1).                    
2
3> F 1.                     
* 1: syntax error before: 1
3> F 1 . 
* 1: syntax error before: 1
3> 

Copy link
Owner

@leostera leostera left a comment

Choose a reason for hiding this comment

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

Good work! 🤩

Looks good to me. We should be able to optimize the pipe away later on.

@@ -3,3 +3,5 @@ external recv_with_timeout : int -> 'a = "recv"
external recv_and_wait : unit -> 'a = "recv"

external ( ^ ) : string -> string -> string = "binary_concat"

external ( |> ) : 'a -> 'b -> 'c = "pipe"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
external ( |> ) : 'a -> 'b -> 'c = "pipe"
external ( |> ) : 'a -> ('a -> 'b) -> 'b = "pipe"

This should be a stricter signature that matches what we want out of this operator.

@leostera
Copy link
Owner

leostera commented Apr 1, 2021

Another thought I had is that we may want to just rewrite the pipes into SSA so we minimize the amount of fun allocations too. So we'd go from this:

let f x = x |> add 1 |> div 2 |> mult 3

To this:

f(X) ->
  Caramel@Tmp1 = add(1, X),
  Caramel@Tmp2 = div(2, Caramel@Tmp1),
  Caramel@Tmp3 = mult(3, Caramel@Tmp2).

This rewrite would be an optimization during translation that happens the moment we find a |> in there. We may need to just leave a mark and have a second compilation pass do this optimization. We'd still need the external to be able to type-check correctly, but we won't actually end up creating a caramel_runtime:pipe/2.

It also means we can take our time to work out the quirks of partial application in the presence of optional and named arguments as pointed out here: https://github.com/AbstractMachinesLab/caramel/pull/92/files#r605967813

@ayshiff would you be interested in trying this approach? 🙌🏽

@ayshiff
Copy link
Author

ayshiff commented Apr 1, 2021

Another thought I had is that we may want to just rewrite the pipes into SSA so we minimize the amount of fun allocations too. So we'd go from this:

let f x = x |> add 1 |> div 2 |> mult 3

To this:

f(X) ->
  Caramel@Tmp1 = add(1, X),
  Caramel@Tmp2 = div(2, Caramel@Tmp1),
  Caramel@Tmp3 = mult(3, Caramel@Tmp2).

This rewrite would be an optimization during translation that happens the moment we find a |> in there. We may need to just leave a mark and have a second compilation pass do this optimization. We'd still need the external to be able to type-check correctly, but we won't actually end up creating a caramel_runtime:pipe/2.

It also means we can take our time to work out the quirks of partial application in the presence of optional and named arguments as pointed out here: https://github.com/AbstractMachinesLab/caramel/pull/92/files#r605967813

@ayshiff would you be interested in trying this approach? 🙌🏽

Yes, absolutely! I am interested in trying this 👍

@ayshiff
Copy link
Author

ayshiff commented Apr 16, 2021

Hey @Ostera @michallepicki 👋
I'm here to ask for some help. I tried to follow Leandro's recommendations but I'm a bit lost.
I made a branch with my progress if you want to have a look 👍

https://github.com/ayshiff/caramel/tree/feature/pipe-operator-test

I also have a question about testing.
To generate a unique variable name in the transformation phase, I wanted to rely on random hash generation. The issue is that I don't know how we could test this behavior.

@leostera
Copy link
Owner

leostera commented Jan 8, 2022

Hi! 👋🏽 since I'm currently not working on and have no plans to continue working on this version of the compiler, I'll close this PR. Thanks a lot for your contribution 🙏🏽

@leostera leostera closed this Jan 8, 2022
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.

Pipe operator support
3 participants