Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Uni pipe chaining for imperative APIs #2748

Closed
chenglou opened this issue Apr 17, 2018 · 19 comments
Closed

Uni pipe chaining for imperative APIs #2748

chenglou opened this issue Apr 17, 2018 · 19 comments
Labels
stale Old issues that went stale

Comments

@chenglou
Copy link
Member

let mySet = Set.make();
mySet |. [
  Set.add(1),
  Set.add(2),
  Set.remove(1),
];

This feature was held back due to being overly magical and due to |. still being new. Upvote this issue if you'd like to see it re-released.

@chenglou chenglou changed the title Uni pipe Uni pipe chaining for imperative APIs Apr 17, 2018
@bobzhang
Copy link
Member

note [ ] would cause some ambiguity, [| |] is preferred implementation wise. It would be simplified as:

mySet |. Set.[|
   add 1 ; 
   add 2 ;
   remove 1
|]  

@chenglou
Copy link
Member Author

What's the ambiguity of []?

@bobzhang
Copy link
Member

bobzhang commented Apr 18, 2018

$ bsc -dparsetree -bs-syntax-only -bs-eval '[1;3]'
[
  structure_item (//toplevel//[1,0+0]..[1,0+5])
    Pstr_eval
    expression (//toplevel//[1,0+0]..[1,0+5])
      Pexp_construct "::" (//toplevel//[1,0+1]..[1,0+5]) ghost
      Some
        expression (//toplevel//[1,0+1]..[1,0+5]) ghost
          Pexp_tuple
          [
            expression (//toplevel//[1,0+1]..[1,0+2])
              Pexp_constant Const_int 1
            expression (//toplevel//[1,0+3]..[1,0+5]) ghost
              Pexp_construct "::" (//toplevel//[1,0+3]..[1,0+5]) ghost
              Some
                expression (//toplevel//[1,0+3]..[1,0+5]) ghost
                  Pexp_tuple
                  [
                    expression (//toplevel//[1,0+3]..[1,0+4])
                      Pexp_constant Const_int 3
                    expression (//toplevel//[1,0+4]..[1,0+5]) ghost
                      Pexp_construct "[]" (//toplevel//[1,0+4]..[1,0+5]) ghost
                      None
                  ]
          ]
]

the [ ] is already de-sugared in the parsetree

@chenglou
Copy link
Member Author

chenglou commented May 17, 2018

cc @jaredly this was the feature I was talking about

@justgage
Copy link

I really like this! Admittedly I think people should just you immutable data structures but when that's not an option then this is sweet.

@chenglou
Copy link
Member Author

Yeah, the goal is not to make the performant paradigm unnecessarily harder.

@jaredly
Copy link
Contributor

jaredly commented May 22, 2018

so fancy

@justgage
Copy link

Good point @chenglou.

I thought about this a little more and it would be easy enough to just define this in plain code:

let (|..) = (obj: 'a, lst: list('a => unit)) : unit =>
  Belt.List.forEach(lst, fn => fn(obj));

let a = Stack.create();

a
|.. Stack.[
      push(1),
      push(2),
      s => pop(s) |> ignore,
      push(3),
      iter(Js.log),
    ];

Runnable example: https://reasonml.github.io/en/try.html?rrjsx=true&reason=DYUwLgBAFAPgdHAlBAvNA9gIwFYC4IDkAhgDQTADOY+wAllVMagHwQCuAdrWIsvp9xYAoCBABCIYGDgAZetIBm6AE4BRIgGMAFlEpgyCjiwiGoWbLwDcQoaEhFUEAMphNAazgblIImBBREayEiIXg4Z1cNDwBtEVF4gAc2Ch0ARkQSOPiIJJSoACYMrPiKYwT0BKgKZBhWWgBzDhUQTOzRXJ0AZiK2iG4QZSgAKQo4YHR6nviAXWsgA

Pros:

  1. No compiler differences
  2. No giving people false hope for writing their own polymorphic operators (can be run on tuples or on arrays or etc..)

Cons:

  1. There's a different operator for each thing (tuples, list, etc...)

Another option might be to just define a function on Belt rather than an operator. Not sure where it would live.

@chenglou
Copy link
Member Author

@justgage this undoes the performance benefits. See the generated code. This pipe operator is specifically syntax and not just a function. Additionally, function doesn't compose with other non-function things. A syntax-level pipe does: a |. Some.

@justgage
Copy link

Ah very good point @chenglou. I forgot about that benefit!

@SllyQ
Copy link

SllyQ commented May 28, 2018

Correct me if I'm wrong, but given that this is a thing.
let (left, middle, right) = myData |. (getLeft, getMiddle, getRight)
I think you could just do

let mySet = Set.make();
let _ = mySet |. (
  Set.add(1),
  Set.add(2),
  Set.remove(1),
);

Maybe that could be good enough?

@chenglou
Copy link
Member Author

Yeah, though that leaves a tuple in the output

@SllyQ
Copy link

SllyQ commented May 29, 2018

Yeah, it does, that's why I ignored it. I guess it introduces some room for mistakes. Maybe it would be possible to do so that if you use fast pipe with a tuple of functions and they all return units you don't have to ignore them? I think it would be a win in terms of consistency and less syntax to learn.

@SllyQ
Copy link

SllyQ commented Jun 4, 2018

@bobzhang
Copy link
Member

bobzhang commented Jun 5, 2018

this could be optimized, but when I read tuple, in general, I don't assume evaluation order, it is not the case for semicolons though

@SllyQ
Copy link

SllyQ commented Jun 5, 2018

Well the semicolon argument applies only to ocaml so I'm not sure if we want to get too attached to that. And in general I think the expectation is that code executes from left to right and top to bottom (at least for me) so I think it should be ok. Or am I incorrect in assuming such evaluation order in ocaml / reason?

@ELLIOTTCABLE
Copy link
Contributor

I'm confused as to why this is necessary — isn't this what the chained imperative statements already do? i.e. the semicolon in OCaml-syntax?

mrmurphy added a commit to mrmurphy/bucklescript that referenced this issue Dec 14, 2019
This is obviously not ready to be merged yet. But my hope is that, once reasonml/reason#2487 is merged, we can get default support for monadic sugar with JS promises in Reason. Expected usage would be like this:

```reason
open Js.Promise;

{
    let.async name = resolve("amazeface");
    Js.log2("The user's name  is " ++ name); 
}
```

Except, I just realized that I don't think this will work, because the source file is in OCaml syntax, and (let.async) isn't valid OCaml syntax. So this PR may not even be possible.

Would you consider the possibility of changing the js_promise bindings source file to be a Reason file after rescript-lang#2748 is merged? I can see that you might be trying to keep Reason out of the source so that pure OCaml users of Bucklescript don't have to mess with Reason. But according to my understanding bsb should be able to compile both Reason and OCaml files in the same project without an issue, right?
@stale
Copy link

stale bot commented Jan 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Old issues that went stale label Jan 15, 2021
@stale stale bot closed this as completed Jan 22, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
stale Old issues that went stale
Projects
None yet
Development

No branches or pull requests

6 participants