Cleanup fast pipe implementation

[IwanKaramazow]

-> should just be |., a regular infix function with interesting printing/precedence properties as if it was a ., and then ppx does all the real logic. Function application on the right side of fast pipe is a must if you want the regular infix behaviour. The following expressions should be all valid without requiring weird parens:

e->ReactEvent.Form.target##value
a->f(c)->Js.log;
a->f(~named, ~bar)->g(~z, ())->Js.log

The current implementation is based on the fact that it needs to be easy to stub:
let (|.) = (a, f) => f(a);
This results into the rhs of -> not taking function application to make this work:
a->f(b) is parsed as (a->f)(b).
The fact that there isn't function application on the rhs, has resulted into a wide range of problems with fastpipe. In printing you actually want the layout to break as if there was a function application on the rhs! The printing problems were fixed at the cost of a tremendous amount of complexity in the printer.

The following question springs to mind: are the semantics (evaluation order etc) identical when defined in user space like this, as opposed to ppx?

No, they aren't. Example:

let fn1 = (~foo=?, ()) => 1;
let fn2 = (~bar=?, x) => 2;

/* Ok */
fn1(~foo=1, ())->(fn2(~bar=2))

/* Ok */
fn1(~foo=1,()) |. fn2(~bar=2)

/* Error: This expression has type int. It is not a function. */
fn1(~foo=1, ())->fn2(~bar=2)

/* Explanation */
fn1(~foo=1, ())->fn2(~bar=2)

/* this translates to: */
fn2(1)(~bar=2)

/* however, because `~bar` is optional, `fn2(1)` already produced a value. Therefore: */
2(~bar=2)

In retrospect, not having function application on the rhs was a terrible mistake.
Fast pipe is all about function application in its current form.
If it was always done via ppx, then you wouldn't get the error in the example
above. Having -> behave as regular infix operator with better precedence, also strokes better
with the mental model of |. for function/variant piping.

TODO:

• improve printing of function application with single fast pipe
• add extra tests
• take care of a ppx for rtop/native usage

[jordwalke]

I'm really glad you are cleaning up the code. This is how we can keep the printer sustainable and move towards a better long term printing framework (by making the code as simple as we can).

Let's make sure we also include in this PR, a summary of how current users of fast-pipe in BS will be impacted by this (what will be necessary to mitigate that impact).

[jaredly]

I'm confused at what this changes. Could you clarify in the description, what the transformation ends up being?

I expect this to transform to

e->ReactEvent.Form.target##value
a->f(c)->Js.log;
a->f(~named, ~bar)->g(~z, ())->Js.log

this

ReactEvent.Form.target(e)##value
Js.log(f(a, c));
Js.log(g(f(a, ~named, ~bar), ~z, ()))

Is that still the case?

[IwanKaramazow]

It will still be the case. The current behaviour is actually different to provide easy stubs on native:
let (|.) = (a, f) => f(a).

ReactEvent.Form.target(e)##value
Js.log(f(a)(c));
Js.log(g(f(a)(~named, ~bar))(~z, ()))

[jaredly]

Ah ok, that makes sense :) Sounds great!

[sikanhe]

What is the status on this PR? I have stumbled a couple times when using fastPipe with labeled argument result in weird type errors that require me to wrap the rhs of fast pipe with parens