refmt long chain of simple |> doesn't break at all

[kyldvs]

formatTest/customMLFiles/kad.re:

let x = foo |> somelongfunctionname "foo" |> anotherlongfunctionname "bar" 1 |> somelongfunction |> bazasdasdad;

_build/src/refmt_impl.native formatTest/customMLFiles/kad.re --print-width 80 results in on master:

let x =
  foo |> somelongfunctionname "foo" |> anotherlongfunctionname "bar" 1 |> somelongfunction |> bazasdasdad;

[bobzhang]

I would like it to see it formatted as:

let x = 
    foo
    |> f 
    |> g 
    |> x;

[jordwalke]

I like that breaking too. It might be easier to just do this for all infix, if it looks okay.

[hcarty]

@jordwalke What would that mean for 1 + 1?

[jordwalke]

@hcarty There are two issues right now with |> that I can see. The first is that it's not breaking at the correct width, and then (as Bob suggested) when it does break, we want it to break differently (aligned to a consistent vertical column). So for addition, the formatting could be just like |>, or at least configurable to be like |>.

[hcarty]

@jordwalke I see, I read the suggestion as 'break at every |> which makes a lot of sense for |> and maybe the monadic infix operators but wouldn't play well with most others.

[IwanKaramazow]

I'm taking a look atm

[kyldvs]

@IwanKaramazow depending on how deep you get into it #1045 also seems related.

[jordwalke]

@IwanKaramazow What did you find with this formatting challenge? Bob's example of formatting would be great for all infix identifiers:

let x = 
    foo
    |> f 
    |> g 
    |> x;

Did you see a specific challenge that I can help with?

[kyldvs]

@IwanKaramazow thanks for taking a look! Do you have any updates, this is my single biggest issue with refmt at the moment.

This was my attempt at a fix in #1045 but couldn't quite get it right: https://github.com/facebook/reason/compare/master...kyldvs:pipe-chaining?expand=1

[IwanKaramazow]

The challenge is mainly in generalizing to all infix operators. >>=& family is parsed differently than |>. The first is a 'right-fold', while the latter is a 'left-fold' in terms of how the ast looks.
I'll go ahead and submit one for |> only later today, it will unblock you.
There are a couple of things I needed to do to make this work.

let x = foo |> z;

let x =
  foo
  |> z
  |> g;

In the first we don't want breaking, while in the last we want breaking.
So I have to look to in the next part in the ast, to determine if we are chaining |> (then break).
Since the inner most |> ast node (foo |> z) is equal to the first case where we don't want to break, I needed some kind of way to remember if we we're chaining and than explicitly let the first case break. This mostly means dragging something like ~infixChain through the unparsing.
I'll clean out my code & submit a PR which does that.

If we would always break, this is basically a one-liner though 😄
But both chenglou & SanderSpies mentioned it would kind of weird to break the first

[jordwalke]

Happy to help. I do know that part of the code fairly well, and I know it's complicated because of the fact that we need to recover the parsing precedence table at print time to ensure that we print faithfully.

[IwanKaramazow]

#1259 contains a PR which unblocks the |> case.
I'm looking to improve the printing of all infix operators, but that'll take more time.

@jordwalke can you take a look, does it make sense what I'm doing?
Can you maybe also elaborate on why we need closer integration with Menhir? I found some comments about >>= fun s => return s requiring parens, because there are some problems with the current approach?

[jordwalke]

Regarding:
>>= fun s => return s

What I discovered is that the problem of printing the absolute minimum number of parens requires that deep parts of the AST know about the higher parts of the AST that they are embedded within - nonlocal knowledge. I haven't thought about it in a while but I believed that having Menhir generate some of the printing scaffolding would help. For now, what we have is decent except in a couple of cases where we print too many parens.

[chenglou]

Fixed by #1259