Elixir: Formatter: `case`/`fn` formatting not consistent

[gasparch]

When editing code, some changes are one-way street and to get back to original formatting one should fix it by hand.

Environment

Erlang/OTP 20 [erts-9.1] [source] [64-bit] ....

Elixir 1.6.0-dev (4b714eb) (compiled with OTP 20)

Current behavior

Formatting does not change following code:

    case v do
      :val1 -> :ok
      n when is_integer(n) -> n + 1
    end

Let's suppose we changed it to

    case v do
      :val1 -> :ok
      n when is_integer(n) -> v = n + 1
        v * v
    end

Formatter will change it to this (while it is too verbose/too much space for me, its consistent and ok):

     case v do
      :val1 ->
        :ok

      n when is_integer(n) ->
        v = n + 1
        v * v
    end

But editing it back to original form do not restore original formatting and it stays like this:

    case v do
      :val1 ->
        :ok

      n when is_integer(n) ->
        n + 1
    end

fn shows even more inconsistency

fn x when x > 10 -> x * 2 end stays as it is. Adding one more clause converts it to

      x when x > 10 -> x * 2
      x when x < 10 -> x / 10
    end

removing second clause makes formatter to format it back to first case.

Adding extra operations in second clause makes it:

      x when x > 10 ->
        x * 2

      x when x < 10 ->
        x = x + 1
        x1 / 10
    end

Removing operator in second clause leaves function in following state:

      x when x > 10 ->
        x * 2

      x when x < 10 ->
        x / 10
    end

and removing second clause altogether formats as

    fn x when x > 10 ->
      x * 2
    end  

Expected behavior

Current behaviour is annoying, because small edition changes formatting completely and then there is no automatic way back. When case/fn have many clauses - reworking them back to original nice form can be time consuming too.

Proposed change - make formatting behaviour deterministic, based on code/AST and not on how currently code is formatted.

[josevalim]

If the clauses are kept on a single line with no spacing, then the formatter will respect that as long as they fit the current line.

Everything is working as intended, thanks.

[gasparch]

The question is that single change in source can suddenly change formatting and after realizing that and reworking clause (for example moving code to separate function) it still requires extra work just to get code back to original form.

So it is like punishing user for experimenting with code and making changes, because they cannot return to original form without manual work.

[OvermindDL1]

I kind of agree with @gasparch here, it is changing the context that it is working in. Specifically it formats single-lines as single lines, which on its own is fine, and it formats single lines with multiple expressions into multiple lines, which on its own is also fine, and it formats multiple lines into multiple lines, which on it's own is fine, however it is formatting all single expressions on multiple lines into multiple lines, which is inconsistent with the prior format and makes it context dependent based on the user-spacing before hand, which on its own also 'sounds' fine, except when the user is swapping between single and multi-expressions in a case for testing purposes, which then forces the user to manually reformat all the (potentially very many) cases back into single lines just because they tested a single multi-expression once in one case and changed it back to a single expression.

I'd agree that it should always be uniform, as in:

• Always ignore the user line spacing
• If every-single-case is a single-expression-only that would also fit on to the line with its own case then make all single-liners (would be awesome if it could format the expression indented equal to each other as well).
• Otherwise format as multi-line bodies.

Far less surprises that way.

[josevalim]

Folks, it is what it is.

If we remove this behaviour, then people will complain that we are putting too much on a single line and not respecting their choice. There is no way we go around this without making some people unhappy.

[gasparch]

Well, I think I'll just fix it in my vim-ide-elixir to compact selected code as dense as possible and then run formatter over it again to make it look nice.

@josevalim would you consider possibility of having option to formatter that makes it 100% deterministic? So it basically can take, say, AST and lay it down nicely in totally repeatable way? I'm willing to put some time to have it done ;)

I've read discussion that your team is trying to avoid at all cost increasing complexity in formatter, but this may be just skipping some code paths, not adding new ones.

[josevalim]

@gasparch the formatter is deterministic. What you probably want is for it ignore the user input in this particular case. And that does mean adding new code paths.

[gasparch]

Well, so will you approve this kind of PR or no?

[josevalim]

Probably not because how much of the user input would it ignore? If you check the docs, we can consider the user input in different situations: https://hexdocs.pm/elixir/master/Code.html#format_string!/2

Are you sure you want to ignore it on all of those? I am pretty sure it will lead to worse formatter behaviour. On the other hand, if we have an option that is exclusive to this behaviour, then what happens when people ask for options for the other cases too?

[gasparch]

I totally understand that it is opening can of the worms and there will be always some people who are unhappy ;) Still, I would appreciate to have option to transform code to same look every time, disregarding what user did to it.

My use case would be to enforce same style for all team members during check-in of code/merge - because automated formatting independent of user preferences leaves no space for human arguments which way of formatting is better.

I'd rather live with sub-optimal formatting by automated tool, than develop/enforce some style guide in a team. Idea it to remove as much as possible mental efforts not related to creating product functionality.

May be if formatter will add some support for hooks (I've seen discussion about extending it for Phoenix) that will allow to write plugin to enforce strict formatting rules. Then it will be user's choice to use it in the project or not and it would not contaminate main code of formatter.

[josevalim]

I'd rather live with sub-optimal formatting by automated tool, than develop/enforce some style guide in a team.

I have put a #6992 so you can see how some code would then be formatted if we completely ignore user hints.

It is probably worth mentioning that the formatter does not fully remove the need of a style guide, things like casing conventions and similar are not handled by the formatter, although it replaces most of it.

May be if formatter will add some support for hooks

It is unlikely that the formatter will do anything that allow users to customize it in many different ways.

[gasparch]

Jose, first of all - I want to thank you a lot for listening and putting effort to discuss this topic. It really matters.

While most of the changes seem to be sensible to me - there are edge cases like

-      assert EExTest.Compiled.__info__(:attributes)[:external_resource] ==
-               [Path.join(__DIR__, "fixtures/eex_template_with_bindings.eex")]
+      assert EExTest.Compiled.__info__(:attributes)[:external_resource] == [
+               Path.join(__DIR__, "fixtures/eex_template_with_bindings.eex")
+             ]

or

-      assert get_in(mixed_map_and_list, [:foo, Access.filter(&(&1.value <= 3)), :value]) ==
-               [1, 2, 3]
+      assert get_in(mixed_map_and_list, [:foo, Access.filter(&(&1.value <= 3)), :value]) == [
+               1,
+               2,
+               3
+             ]

Which I assume will require more code to keep them back to sensible presentation :(

One more thing about formatter right now is that it consumes much more vertical space, which is scarce - we have mostly horizontal monitors. Like this case or this

Ideally formatter could take into account perception complexity of the code while splitting it to lines.
Because pipe of string |> String.downcase |> String.trim is much less complex for perception than
1..10 |> Enum.scan(&(&1 * 2 + &2)) |> Enum.map(&(&1 * 10)) . The more complex is code - the more space it needs, obviously. But cases like following, when there is a mapping between input and output look more readable for me in second form.

         case fun.(entry, acc) do
-          {:halt, acc} ->
-            {:halt, {list, acc}}
-
-          {[], acc} ->
-            {:cont, {list, acc}}
-
-          {[entry], acc} ->
-            {:cont, {[entry | list], acc}}
-
-          {entries, acc} ->
-            {:cont, {reduce(entries, list, &[&1 | &2]), acc}}
+          {:halt, acc} -> {:halt, {list, acc}}
+          {[], acc} -> {:cont, {list, acc}}
+          {[entry], acc} -> {:cont, {[entry | list], acc}}
+          {entries, acc} -> {:cont, {reduce(entries, list, &[&1 | &2]), acc}}
         end

or even better this form:

    case fun.(entry, acc) do
      {:halt, acc}   -> {:halt, {list, acc}}
      {[], acc}      -> {:cont, {list, acc}}
      {[entry], acc} -> {:cont, {[entry | list], acc}}
      {entries, acc} -> {:cont, {reduce(entries, list, &[&1 | &2]), acc}}
    end   

But, again, I can understand that users come in all forms and shapes and my opinion is one of the many possible ones and each of us have reasoning behind their opinion :)

[josevalim]

One more thing about formatter right now is that it consumes much more vertical space, which is scarce - we have mostly horizontal monitors.

I am not sure I agree, even on my macbook I get more vertical space because it doesn't matter how wide my monitor is, we are constrained to columns of 80/100 chars.

Still, in the examples you linked, you could have kept everything in a single line and the formatter would have respected that. I actually think the changes you are performing to the code today because of the formatter would also happen without the formatter. Imagine you have this code:

case foo do
  {:ok, value} -> value
  {:error, err} -> raise err
end

Now imagine that you also want to log, you would have to rewrite it to:

case foo do
  {:ok, value} ->
    value
  {:error, err} ->
    log err
    raise err
end

Then if you decide to not log or add a log_and_raise, you would have to manually rollback to the previous one. The only downside of the formatter in this case is that you have slightly more work to do because of the newlines it adds.

Ideally formatter could take into account perception complexity of the code while splitting it to lines.

We did consider some rudimentary complexity for -> clauses but it caused more confusion because it became harder to understand why the formatter was taking some decisions in some places and not in others. :(

[gasparch]

Okay, I see your point in keeping it simple. Most probably in order to deal with exploding case/functions I'll add functionality to IDE :)

Another case I noticed - just adding one field to simple decomposition leads to drastic change in code.

      %{hour: hour, minute: minute, second: second, microsecond: microsecond, calendar: calendar} =
        time

      %{
        hour: hour,
        minute: minute,
        second: second,
        microsecond: microsecond,
        calendar: calendar,
        more: more
      } = time

Is it possible to understand that it is just decomposition operation and convert it to:

      %{hour: hour, minute: minute, second: second, 
        microsecond: microsecond, calendar: calendar} = time

[OvermindDL1]

I have put a #6992 so you can see how some code would then be formatted if we completely ignore user hints.

I quite like that PR actually, including the parts that @gasparch did not really like. I find it very uniform and readable. :-)

Is it possible to understand that it is just decomposition operation and convert it to:

I prefer the way it formats it now, I find your multi-line condensed version a lot more unreadable personally.

The only downside of the formatter in this case is that you have slightly more work to do because of the newlines it adds.

It's a lot more than just a little work when there are a lot of cases as I have in some areas... ^.^;

or even better this form:

Yeah this is what I hinted of in my first message, just that if it is possible to align the expressions in each single-expression case then the formatter should do so. Single-expression cases are far far more often then not far more readable when aligned like that, and honestly if it could not do that (like due to length) then it should just fall back to multi-line cases.

[gasparch]

so basically we are stuck at the point that everyone has their preferences and its impossible to match them all ;)

About reverting explosion of case/fn into multiline - I'll write something for this. If you don't use vim - then I'm sorry :)

[OvermindDL1]

so basically we are stuck at the point that everyone has their preferences and its impossible to match them all ;)

Sounds like a normal formatting issue. ^.^

About reverting explosion of case/fn into multiline - I'll write something for this. If you don't use vim - then I'm sorry :)

As a life-long vim user I really encourage you to take a look at Spacemacs. Yes it is a vim-like plugin and control pack for Emacs, but it is truly astounding and has been the first thing to routinely get me away from vim ever. :-)

Though yes I do still type vi 90% of the time when at the console, but I do hit emacs for the larger IDE work now, which has fantastic elixir support. ^.^

But still, I've vote for not letting the user 'hint' what to do, a given ast should always go to the same text (and vice-versa, but that's a different issue), always. If something does not look right then that sounds like a fault of syntax, not of the formatter.

[josevalim]

so basically we are stuck at the point that everyone has their preferences and its impossible to match them all ;)

Alright, glad we reached a conclusion! I will go ahead and lock this as well to avoid people from adding "but i prefer X". :) Thanks @gasparch and @OvermindDL1!