Preserve function body braces and distinguish open vs. X.() #1826

jordwalke · 2018-02-16T09:40:41Z

Summary:Created a new place to store some stylistic information in the
AST. [@refmt "foo"] attributes used at parse time, never printed.

Before the following would get reformatted:

let f () => {
  switch(x) {
  | X => 0
  };
};

To this:

let f () =>
  switch(x) {
  | X => 0
  };

That was annoying to many people because you can't easily add a "let" binding above the switch.
After this diff, the original form will be preserved:

let f () => {
  switch(x) {
  | X => 0
  };
};

With this diff you can still have the printer print this form if you prefer: You simply write it in this form and it will be retained.

let f () =>
  switch(x) {
  | X => 0
  };

I consider this a "pretty good" solution to making editing Reason syntax more enjoyable, but is certainly not the best possible solution. Ideally we would automatically insert the braces depending on whether or not the body of the function "breaks". We can also (or even instead) do that when our formatter is more expressive.

Before the following would get reformatted:

let f () => {
  open X;
  switch(x) {
  | X => 0
  };
};

To this:

let f () =>
  X.(
    switch(x) {
    | X => 0
    }
  );

Which is sometimes neat, but sometimes it gets pretty unwieldy especially with nested opens. With this diff, we preserve the original way you wrote the open. You can write it with open X or X.() and the formatter will retain the form you wrote. It's pretty hard to write an algorithm that looks great in all cases.

Summary:Created a new place to store some stylistic information in the AST. `[@refmt "foo"]` attributes used at parse time, never printed. Test Plan: Reviewers: CC:

jordwalke · 2018-02-16T09:42:04Z

Some people that have requested this: @sgrove, @kyldvs

chenglou · 2018-02-16T09:56:55Z

formatTest/typeCheckedTests/expected_output/attributes.re

-let add = a => [@onRet] a;
+let add = a =>
+  [@onRet]
+  {


What's happening here

The input file actually has braces. The old test output was stripping the braces and now this diff preserves them.
This is what the input has:

let add(a) { [@onRet] a };

It doesn't distinguish between [@a] { x } and { [@a] x } as you can see.

chenglou · 2018-02-16T10:35:42Z

formatTest/typeCheckedTests/expected_output/attributes.re

-  [@bs.get null] key2: [@onType2] Js.t(int),
-  [@bs.get null] key3: [@onType2] Js.t(int),
+  [@bs.get
+    {


What's happening here then?

The same thing. The attributes were specified with braces, and before this diff they were getting refmted away.

Does that mean these will always get formatted like this now or something?

If you remove the braces I guess they will get formatted how they were previously.

Exactly. Previously, the braces were getting formatted away. Now, we preserve whether or not you included additional braces. You can have it format whichever way you want - and it will preserve your intent.

TheSpyder · 2018-02-17T03:41:52Z

While I really like the local open change, I’m less sold on retaining optional braces. I usually like the way simplifying a function can lead to the braces becoming unnecessary and collapse the code; often into a single line.

Although I guess it’s easy to argue both sides. This does make me think that reducing how opinionated refmt is means we start needing some options to control behaviour 🤔

jordwalke · 2018-02-17T05:43:28Z

The open brace issue is pretty important because they get removed before you have the opportunity to add a let binding before the return value. Today people already do force the braces to be preserved by adding a dummy let binding to unit. That’s pretty extreme. What this diff does is just makes it more natural to tell the formatter you want them preserved.
However I think that if we had the ability to only print braces when the function body breaks, then this feature would be less useful and at that time perhaps we can revisit.

kyldvs · 2018-02-20T20:33:07Z

I run into the bracket issue all the time, particularly because I'm used to a format-on-save workflow.

let foo = (foo) => {
  /* some placeholder */
};

Will format out the braces and I have to add them back constantly. Typically my workaround is to always add some empty unit statements to the function body like this:

let foo = (foo) => {
  ();
  /* some placeholder */
};

Really looking forward to this change

sgrove · 2018-02-20T20:56:18Z

@kyldvs Literally my exact workflow! Looking forward to it as well.

phated · 2018-07-08T19:30:46Z

@jordwalke are you still planning to land this (or something similar)? I've been encountering both behaviors you outlined originally and it makes editing in vscode with formatOnSave: true very jarring. Often I'll habitually save, get one of those reformats when I planned on editing in the block and have to undo the format (which sometimes ends in a weird result).

jaredly · 2018-07-19T21:27:01Z

This is the main reason I have refmt on save turned off. I might try to get to this to resolve the conflicts if I can in the next couple weeks

jordwalke · 2018-07-20T08:43:21Z

@jaredly Just an observation: I felt that the refactor included in this diff was really necessary to make it easy to retain these "preserved styles". I found the current approach in the printer/parser not really sustainable and kind of fragile. With my refactor in this stack, there's a specific place carved out just for stylistic preservation and it is clearly out of the way because it's its own category that is cleanly filtered out.

facebook-github-bot · 2018-07-25T21:10:01Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

This is an alternative to reasonml#1826 implementing just the brace preservation code, to make it easier to be reviewed. I think this is now in a better place to get into the codebase given that we have `Reason_attributes` outside of `Reason_pprint_ast` and that we now remove our own attributes before printing to OCaml. I think we can probably refactor this code further in future PRs, as well as add other stylistic preservation changes.

* Preserve function body braces This is an alternative to #1826 implementing just the brace preservation code, to make it easier to be reviewed. I think this is now in a better place to get into the codebase given that we have `Reason_attributes` outside of `Reason_pprint_ast` and that we now remove our own attributes before printing to OCaml. I think we can probably refactor this code further in future PRs, as well as add other stylistic preservation changes. * Rebase and fix * address review comments * Adding so many optional laballed to `unparseExprRecurse` can't scale

facebook-github-bot · 2020-10-17T22:42:39Z