Reason: Refmt printer error when running on a large codebase

[thangngoc89]

I ran refmt over my repo and here are some bugs I found (which totally prevent me from using [email protected])

1.

          <span className="EditorNav__button--saveIndicator ">
            (
                (switch (saveStatus) {
                | Pristine => ""
                | Saved => "Saved"
                | Saving => "Saving"
                | Unsaved => "Unsaved"
                })
              ->str
            )
          </span>

Get formatted into this

<span className="EditorNav__button--saveIndicator ">
            switch (saveStatus) {
            | Pristine => ""
            | Saved => "Saved"
            | Saving => "Saving"
            | Unsaved => "Unsaved"
            }
            ->str
          </span>

2.

  ->(blocks => {"blocks": blocks})

to

  ->blocks => {"blocks": blocks}

3.

<title> ((state.title == "" ? "untitled" : state.title)->str) </title>

to

<title> state.title == "" ? "untitled" : state.title->str </title>

1. Refmt doesn't understand this (the self.send part)

ReasonReact.Router.watchUrl(url =>
        Route.urlToRoute(url)->ChangeView->self.send
      );

Changed it into this as a work around

ReasonReact.Router.watchUrl(url =>
        Route.urlToRoute(url)->ChangeView->send
      );

[IwanKaramazow]

Thanks for the bug report, there's already a PR open with fixes, we're working on it.

[cknitt]

I ran into similar issues, e.g.

let twitterUrl = Social.profiles |. Map.String.getExn("twitter") |. (p => p.Social.url);

becomes

let twitterUrl = Social.profiles->(Map.String.getExn("twitter"))->p => p.Social.url;

Also, there are many cases where superfluous () are added, e.g.

locale->(Js.String.substring(~from=0, ~to_=2))

or

db
->(version(1))
->(stores(Js.Dict.fromList([("installation", ""), ("settings", ""), ("incidents", "incidentId")])))

or

        LicenseList.licenses
        ->(
            mapListToReactElements(item =>
              <div key=item.name>
                /* ... */
              </div>
            )
          )

[anmonteiro]

@cknitt My open PR (#2111) fixes your issues.

Furthermore, the parens are not superfluous. -> has different precedence from |..

So these are different:

/* these are the same */ 
locale->(Js.String.substring(~from=0, ~to_=2));
Js.String.substring(locale, ~from=0, ~to_=2);


/* these are the same, but different from the examples above */ 
locale->Js.String.substring(~from=0, ~to_=2)
(locale->Js.String.substring)(~from=0, ~to_=2)

[jaredly]

Wait, so
locale |. Js.String.substring(~from=0, ~to_=2) (which is equivalent to Js.String.substring(locale, ~from=0, ~to_=2)
gets refmt'd to
locale->(Js.String.substring(~from=0, ~to_=2)) (which is equivalent to Js.String.substring(~from=0, ~to_=2, locale))?
That sounds like a bug.
I would expect local |. Js.String.substring(~from=0, ~to_=2) to be refmt'd to locale->Js.String.substring(~from=0, ~to_=2).

[anmonteiro]

@jaredly sorry my example was misleading. it gets put as the first arg.

Edited for consistency.

[cknitt]

My expectation would have been the same as @jaredly's.

I.e. I would have expected something like

a |. b(c) |. d;

to reformat to

a->b(c)->d;

but it reformats to

a->(b(c))->d;

which feels weird.

[anmonteiro]

I'm starting to think the parens are actually not needed, given that a->b(c) is effectively equivalent to a->(b(c)) because of currying.

But they're needed in this case: a->(b->c)

[IwanKaramazow]

The Reason fast pipe -> is actually different than the |. infix operator from Bucklescript.
There's a range of things -> can do, that |. can't. For example precedence like foo->bar##baz.
You can think of -> as an "access"-thing like the . infoo.bar or ## in foo##baz.
If you write a.b.c[0], everyone reads (((a).b).c)[0].
-> behaves exactly the same, a->b->c[0] is ((a->b)->c)[0].
a->b(c) is parsed as (a->b)(c), after the bs ppx transform it'll be (b(a))(c). Since Ocaml is a curried language this is actually the same as b(a, c).
Because of the differences between |. as infix operator and -> being "access"-like (think .), we can't just drop the parens when converting |. in ->. Reason operates on the ast level, we can't make any assumptions other than the ast we just parsed.
However, if you're compiling Reason with Bucklescript, you can drop the parens yourself safely. We don't have the same luxury on native.

[cknitt]

@IwanKaramazow Thanks for the explanation. I wasn't aware I could remove the superfluous () manually (and refmt would keep it that way). That is of course fine with me! Maybe this information could be added to the release notes / instructions for the migration script.

[IwanKaramazow]

All bugs have been fixed in master. Ping me if there are any new issues surfacing.