erlfmt: experiment by formatting 9 "random" files in otp #2756

awalterschulze · 2020-09-18T13:13:44Z

We are looking for feedback on erlfmt from the erlang community.
To make this easy, we thought to create some draft pull requests on some projects.
This allows you to easily comment and provide feedback, instead of having to learn how to use the tool before being able to provide feedback.

Note: This pull request is not intended to be merged

If you would like me to format some other files for you to also comment on, please let me know.
Thank you for your time and I hope this is appropriate.

CLAassistant · 2020-09-18T13:13:49Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

okeuday · 2020-09-18T17:23:54Z

It would be nice if the formatting aided your view of the Erlang data as your eye scans the file, starting from the upper-left and moving down, gradually to the right. To do that, it should be best to make the indentation level clear with the left-most character, so lib/crypto/src/crypto.app.src would look like (possibly with the other lists starting on the next line too):

{application, crypto,
 [{description, "CRYPTO"},
  {vsn, "%VSN%"},
  {modules,
   [crypto,
    crypto_ec_curves]},
  {registered, []},
  {applications, [kernel, stdlib]},
  {env, [{fips_mode, false}, {rand_cache_size, 896}]},
  {runtime_dependencies, ["erts-9.0", "stdlib-3.4", "kernel-5.3"]}]}.

lib/crypto/src/crypto.app.src

lib/crypto/src/crypto.erl

paulo-ferraz-oliveira · 2020-09-18T21:40:11Z

lib/crypto/src/crypto.erl

-                             Macs :: [hmac | cmac | poly1305],
-                             Curves :: [ec_named_curve() | edwards_curve_dh() | edwards_curve_ed()],
-                             RSAopts :: [rsa_sign_verify_opt() | rsa_opt()] .
+-spec supports() -> [Support] when


I do prefer stuff like

-spec supports(Input) -> Result when Input :: binary(), Result :: ok.

but maybe it's not "norm".

That is very interesting. Do you think you can find more examples, this could be worth exploring.

I don't think I'll find many examples. This was the result I reached after a few iterations of working with 4-space tabs, tabs v. spaces, same line variables, etc., over the years.

This is similar, for example: https://github.com/erlang/otp/blob/master/lib/stdlib/src/erl_eval.erl#L100, but not an exact match.

The way @paulo-ferraz-oliveira prefer also gives nicer documentation if you use the feature to generate the xml from type specs.

This is very interesting, but I am struggling to understand how the placement of when impacts the documentation. Could you provide an example, maybe as a screen shot here or whatever is easiest? Sorry for the inconvenience.

So I tried to generate edocs using two different placements of when

-spec supports(Input) -> Result when Input :: binary(), Result :: ok. supports(_) -> ok. -spec supports1(Input) -> Result when Input :: binary(), Result :: ok. supports1(_) -> ok.

But the edoc looks the same

Were you using some other tool to generate docs?

We are nearing finalizing our format, so it would be really helpful to hear what I am missing sooner rather than later. Sorry for the short timeline, I know I have expected much more leniency for myself. @HansN @paulo-ferraz-oliveira

@HansN @paulo-ferraz-oliveira am I using the wrong tool to generate these docs, what tool were you referring to?

@richcarl do you maybe have an idea at the top of your head what is being referred to, to generate xml from type specs?

lib/crypto/src/crypto.erl

HansN · 2020-09-21T08:01:18Z

lib/crypto/src/crypto.erl

@@ -779,21 +843,18 @@ enable_fips_mode_nif(_) -> ?nif_stub.
 equal_const_time(X1, X2) ->
    equal_const_time(X1, X2, true).

-
-equal_const_time(<<B1,R1/binary>>, <<B2,R2/binary>>, Truth) ->
+equal_const_time(<<B1, R1/binary>>, <<B2, R2/binary>>, Truth) ->


A b i t l i k e n o t s e p r a t i n g w o r d s i n t e x t.
I like the grouping of the arguments in the original line 783:)

Another interesting case :) What would you propose as a consistent way to format binary pattern matching? Sorry for asking, but I struggle to think about how to solve this and you seem to have new ideas.

I'm just happy to tell what I like and dislike :)
In arguments or elements I personally prefer not to have spaces in the binary as in the commented line, but as standalone patterns or topmost right expressions it could be clearer:
<<Code, Rest/binary>> = Data,
and
NewData = <<Code, SomethingElse, Rest/binary>> ,

Ha ha, yes even knowing what you like/dislike is also helpful :)

Okay so in this case it would also be tough to make an automated rule, or am I misinterpreting?

awalterschulze

Thank you @okeuday , @HansN and @paulo-ferraz-oliveira for taking the time to check out the formatter and provide feedback. This is much appreciated <3

Maybe it is possible to also expand with some things you like, so we can try to keep those decisions in place?

lib/crypto/src/crypto.app.src

lib/crypto/src/crypto.erl

awalterschulze · 2020-09-21T14:17:51Z

lib/crypto/src/crypto.erl

-                             Macs :: [hmac | cmac | poly1305],
-                             Curves :: [ec_named_curve() | edwards_curve_dh() | edwards_curve_ed()],
-                             RSAopts :: [rsa_sign_verify_opt() | rsa_opt()] .
+-spec supports() -> [Support] when


That is very interesting. Do you think you can find more examples, this could be worth exploring.

lib/crypto/src/crypto.erl

awalterschulze · 2020-09-21T14:23:58Z

lib/crypto/src/crypto.erl

@@ -779,21 +843,18 @@ enable_fips_mode_nif(_) -> ?nif_stub.
 equal_const_time(X1, X2) ->
    equal_const_time(X1, X2, true).

-
-equal_const_time(<<B1,R1/binary>>, <<B2,R2/binary>>, Truth) ->
+equal_const_time(<<B1, R1/binary>>, <<B2, R2/binary>>, Truth) ->


Another interesting case :) What would you propose as a consistent way to format binary pattern matching? Sorry for asking, but I struggle to think about how to solve this and you seem to have new ideas.

awalterschulze · 2020-09-21T14:41:34Z

It would be nice if the formatting aided your view of the Erlang data as your eye scans the file, starting from the upper-left and moving down, gradually to the right. To do that, it should be best to make the indentation level clear with the left-most character, so lib/crypto/src/crypto.app.src would look like (possibly with the other lists starting on the next line too):
{application, crypto,
 [{description, "CRYPTO"},
  {vsn, "%VSN%"},
  {modules,
   [crypto,
    crypto_ec_curves]},
  {registered, []},
  {applications, [kernel, stdlib]},
  {env, [{fips_mode, false}, {rand_cache_size, 896}]},
  {runtime_dependencies, ["erts-9.0", "stdlib-3.4", "kernel-5.3"]}]}.

@okeuday So it seems that this is support for tuples, but how we format lists differs from your style, is that correct or is there more that I might be misinterpreting?

Do you feel this way about lists in general or are there counter examples?

okeuday · 2020-09-21T18:21:39Z

It would be nice if the formatting aided your view of the Erlang data as your eye scans the file, starting from the upper-left and moving down, gradually to the right. To do that, it should be best to make the indentation level clear with the left-most character, so lib/crypto/src/crypto.app.src would look like (possibly with the other lists starting on the next line too):
{application, crypto,
 [{description, "CRYPTO"},
  {vsn, "%VSN%"},
  {modules,
   [crypto,
    crypto_ec_curves]},
  {registered, []},
  {applications, [kernel, stdlib]},
  {env, [{fips_mode, false}, {rand_cache_size, 896}]},
  {runtime_dependencies, ["erts-9.0", "stdlib-3.4", "kernel-5.3"]}]}.
@okeuday So it seems that this is support for tuples, but how we format lists differs from your style, is that correct or is there more that I might be misinterpreting?

The formatting above is closer to what io:format/2 provides with ~p pretty-printing. So, the format may be more familiar to Erlang users. However, the main concern is that the graphic design idea of formatting based on how a human eye scans a page is what I wanted to mention. The erlfmt formatting, with the data example is obscuring the indentation level by using a character that is right-most, due to the human eye scanning down, reading from the left. The first thing we try to look at is structure and making the structure in the page more difficult to see won't help the reader. That leads me to believe that io:format/2 pretty-printing is closer to an easy-to-read formatting method.

Do you feel this way about lists in general or are there counter examples?

The problem likely exists with things other than lists, but it seems simplest to look at the data example with lists. I found it disturbing that comments are deleted or moved, so there are likely other things I would want to avoid, but the indentation structure should be the most important concern.

IngelaAndin · 2020-09-22T05:56:44Z

lib/stdlib/src/supervisor.erl

+    count_children/1,
+    check_childspecs/1,
+    get_childspec/2
+]).


I can see why you grouped start_link variants, but I am thinking you should not make this special case as they are technical different functions and it would be easier to just have one rule. I use to flatten my exports earlier but I find that having one per row is gives a better overview and avoids merge conflicts.

Aha avoiding merge conflicts is good :)
I guess this would be true for anything we try to leave on the same line though.

Still it could be worth discussing flattening vs having each export on a separate line.
In this case it does seem as though we created at least a more flattened list.

Well it is typically annoying for export lists as you most of the time want to keep both unrelated changes but it is quite easy to resolve the conflict incorrectly when the export list is a big not so structured blob of information. And as the younger people say all modern IDEs will let you collapse that part of the code anyway ;)

The IDE might collapse it, but the merge conflict won't go away.
I hope that this way of formatting and grouping functions with the same name is a happy compromise.
But I do feel I am still missing you are trying to say?

My point is that the merge conflicts will be rare if you have an export list with one function per
line but quite common if you have a few lines with many functions per line. And I think the trade off between a long list that I usually do not care to see and having more "compressed" list but get many merge conflicts, I prefer not having the merge conflicts. I was wondering if is worth the extra effort to group functions with the same name in the same line, as they are different functions even though they normally are related. But that is your trade off!

Yes it is a trade off, I can hope that when functions have the same name, they are more likely to be edited at the same time, but that is pure speculation.

IngelaAndin · 2020-09-22T06:44:29Z

lib/stdlib/src/supervisor.erl

+    child_type :: worker(),
+    modules = [] :: modules()
+}).
+


I believe this is easier to automate but the left side is easier to read.

I have added the comment as a data point to another issue WhatsApp/erlfmt#134

What part do you think is making the left side easier to read other wise?

The alignment of "::" makes it easier to read!

Oh wow yes, really good point. This is a good example.

The reason we decided not to make the formatter do any alignment is because we do not want changing a single line to result in a big change.

For example if we take the example here:

-record(child, {% pid is undefined when child is not running pid = undefined :: child() | {restarting, pid() | undefined} | [pid()], id :: child_id(), mfargs :: mfargs(), restart_type :: restart(), shutdown :: shutdown(), child_type :: worker(), modules = [] :: modules()}).

If we provide restart_type with a default or add a new field, then we have to shift all types to keep them all aligned.

-record(child, {% pid is undefined when child is not running pid = undefined :: child() | {restarting, pid() | undefined} | [pid()], id :: child_id(), mfargs :: mfargs(), restart_type :: restart(), shutdown :: shutdown(), child_type :: worker(), new_longer_field :: longer_type(), modules = [] :: modules()}).

This would result in a modification of each line, which is something we would like to avoid.

I hope that makes sense?

lib/stdlib/src/supervisor.erl

IngelaAndin · 2020-09-22T06:52:21Z

lib/stdlib/src/supervisor.erl

+        ignore ->
+            ignore;
+        Error ->
+            {stop, {bad_return, {Mod, init, Error}}}


There are lot of such indenting changes. In general I think we should adhere to how the Emacs mode indents it in these cases as that is the most spread way at least in the OTP code base. How well supervisor adheres I do not know some of this code is really old.

In general we want to avoid alignment that is not at 4 spaces, but we also have an open issue to discuss emacs indentation WhatsApp/erlfmt#166

awalterschulze · 2020-09-22T19:39:11Z

It would be nice if the formatting aided your view of the Erlang data as your eye scans the file, starting from the upper-left and moving down, gradually to the right. To do that, it should be best to make the indentation level clear with the left-most character, so lib/crypto/src/crypto.app.src would look like (possibly with the other lists starting on the next line too):
{application, crypto,
 [{description, "CRYPTO"},
  {vsn, "%VSN%"},
  {modules,
   [crypto,
    crypto_ec_curves]},
  {registered, []},
  {applications, [kernel, stdlib]},
  {env, [{fips_mode, false}, {rand_cache_size, 896}]},
  {runtime_dependencies, ["erts-9.0", "stdlib-3.4", "kernel-5.3"]}]}.
@okeuday So it seems that this is support for tuples, but how we format lists differs from your style, is that correct or is there more that I might be misinterpreting?
The formatting above is closer to what io:format/2 provides with ~p pretty-printing. So, the format may be more familiar to Erlang users. However, the main concern is that the graphic design idea of formatting based on how a human eye scans a page is what I wanted to mention. The erlfmt formatting, with the data example is obscuring the indentation level by using a character that is right-most, due to the human eye scanning down, reading from the left. The first thing we try to look at is structure and making the structure in the page more difficult to see won't help the reader. That leads me to believe that io:format/2 pretty-printing is closer to an easy-to-read formatting method.

Do you feel this way about lists in general or are there counter examples?

The problem likely exists with things other than lists, but it seems simplest to look at the data example with lists. I found it disturbing that comments are deleted or moved, so there are likely other things I would want to avoid, but the indentation structure should be the most important concern.

@okeuday if you find a comment that is deleted, please report it, that is a HUGE bug that we would love to address asap.

Comments are moved since we do not currently support trailing comments and we move them to the line above.
Although this is an ongoing discussion WhatsApp/erlfmt#124

How the human eye scan is very interesting.
I have created an issue to discuss this further.
WhatsApp/erlfmt#182

awalterschulze · 2020-10-30T16:18:19Z

It seems that all questions have been answered and follow up issues have been created.
Are there any other concerns or can I close this pull request?

I want to thank everyone for taking the time to review this formatting of code.

zuiderkwast · 2020-12-03T13:12:08Z

lib/dialyzer/src/dialyzer.erl

 cl(Opts) ->
-  F = fun() ->
-	  {Ret, _Warnings} = dialyzer_cl:start(Opts),
-	  Ret
-      end,
-  doit(F).
+    F = fun() ->
+        {Ret, _Warnings} = dialyzer_cl:start(Opts),
+        Ret
+    end,
+    doit(F).


This is commonly (at least within Ericsson projects) indented with 'end' aligned under 'fun', like this:

cl(Opts) -> F = fun() -> {Ret, _Warnings} = dialyzer_cl:start(Opts), Ret end, doit(F).

The same with case, if, receive and try.

Thank you for commenting and bringing this up.
erlfmt does not do any alignment to avoid a change of the name F to result in a bigger diff over multiple lines.
I hope that makes sense.

uabboli · 2021-01-28T05:57:43Z

A bit off-topic as it's not about formatting, but it should be noted
that PropEr, https://github.com/proper-testing/proper, has a module
called "proper_erlang_abstract_code" which generates abstract code for
the 'master' branch of Erlang/OTP, that is, what will be Erlang/OTP
24.0.

It seems the module has not made it into the latest docs yet,
http://proper.softlab.ntua.gr/doc/, but it should be useful anyway.

We've used it at OTP to check, among other things, the formatters
'erl_pp' and 'erl_prettypr'.

erlfmt: experiment by formatting 9 "random" files in otp

f3c7a33

awalterschulze mentioned this pull request Sep 18, 2020

Feedback Tracker: Get Feedback from OTP team WhatsApp/erlfmt#156

Closed

paulo-ferraz-oliveira reviewed Sep 18, 2020

View reviewed changes

lib/crypto/src/crypto.app.src Show resolved Hide resolved

paulo-ferraz-oliveira reviewed Sep 18, 2020

View reviewed changes