Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function with multi-clause style collapses awkwardly with single clause #7718

Closed
chrismccord opened this issue May 30, 2018 · 9 comments
Closed

Comments

@chrismccord
Copy link
Contributor

@chrismccord chrismccord commented May 30, 2018

Environment

  • Elixir & Erlang/OTP versions (elixir --version): 1.6.0
  • Operating system: OSX 10.13.4

Current behavior

I've found functions with multi-clause style collapses awkwardly when a single clause is used. Given the following code block:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn
  %SomeStruct{key: key}, acc -> more_code(key, acc)
end)

The formatter produces:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn %SomeStruct{
                                                                              key: key
                                                                            },
                                                                            acc ->
  more_code(key, acc)
end)

Expected behavior

The formatter does not change the original code. I realize using the multi-clause style could be "cheating" in this case, but if you add a second clause, the formatter does not change the code, and the formatted code in this case is very awkwardly styled. Thanks for taking a look!

@josevalim

This comment has been minimized.

Copy link
Member

@josevalim josevalim commented May 30, 2018

This should be a straight-forward change. Either we are only checking for newlines if you have more than two clauses or the newline checking is only between clauses, which means it won't work here.

@Sihui

This comment has been minimized.

Copy link
Contributor

@Sihui Sihui commented Jun 5, 2018

@chrismccord

if you add a second clause, the formatter does not change the code

Can you provide an example?

My formatted result is a bit different than yours (but still awkward)

# Ex 1
# Original
SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn
   %SomeStruct{key: key}, acc -> more_code(key, acc)
 end)
# Formatted
SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn %SomeStruct{key: key},
                                                                            acc ->
  more_code(key, acc)
end)

# Ex 2
# Original
SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn
  %SomeStruct{key: key}, %SomeStruct{key: key}, acc -> more_code(key, acc)
end)
# Formatted
SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn %SomeStruct{key: key},
                                                                            %SomeStruct{key: key},
                                                                            acc ->
  more_code(key, acc)
end)
@josevalim

This comment has been minimized.

Copy link
Member

@josevalim josevalim commented Jun 5, 2018

@Sihui I believe he meant:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn
   %SomeStruct{key: key}, acc -> more_code(key, acc)
   %AnotherStruct{key: key}, acc -> more_code(key, acc)
 end)
@Sihui

This comment has been minimized.

Copy link
Contributor

@Sihui Sihui commented Jun 6, 2018

Thanks, @josevalim.

I took a look. Seems like to some extends, that's actually what we intended.

The place that does that is Code.formatter#anon_fun_to_algebra:

  # fn x -> y end
  # fn x ->
  #   y
  # end
  defp anon_fun_to_algebra([{:->, meta, [args, body]}] = clauses, _min_line, max_line, state) do

So if the anon_fun can be fit in one line, we will. If not, we break it into multiple lines in this fashion:

fn x ->
   y
end

fn a,
   b,
   c ->
   y
end

It becomes awkward when the string before fn is long, like this:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc,  fn a,
                                                                             b,
                                                                             c ->
   y
end

The reason multi-clasues funs being formatted differently is because we have a different method handing that case.

  # fn
  #   args1 ->
  #     block1
  #   args2 ->
  #     block2
  # end
  defp anon_fun_to_algebra(clauses, min_line, max_line, state) do
    {clauses_doc, state} = clauses_to_algebra(clauses, min_line, max_line, state)
    {"fn" |> line(clauses_doc) |> nest(2) |> line("end") |> force_unfit(), state}
  end

So in the case of multi-clasue fun, I don't think we are checking user-inputted new lines. We just always put the clauses in a new line.

It would be great if there's a way to check if fn is followed by a new line (i.e. fn\n) in here. Then we can just do the same thing we do for multi-clasues funs. Unfortunately, I don't see a way to check for fn\n in here. Thought?

@josevalim

This comment has been minimized.

Copy link
Member

@josevalim josevalim commented Jun 6, 2018

Hi @Sihui, great analysis. In order to know if fn is followed by newlines, we need to change the parser here.

In that line, you can see the parsing rule that handles fn followed by an eoe (end of expression). You should change that line to call annotate_newlines, similar to what we do here, so we have something like:

fn_eoe -> 'fn' eoe : annotate_newlines('$2', '$1').

The '$1' syntax means you will pass the first token on the right hand side of -> and '$2' the second token and so on.

If you do so, you can run make erlang and now in the formatter, where we match on {:fn, meta, _}, you should have a :newlines key inside the meta. But remember this field is not always present, so you need to specify a default of 0: Keyword.get(meta, :newlines, 0). Then you need to pass this newline information through the formatter and change the formatting rules accordingly.

@Sihui

This comment has been minimized.

Copy link
Contributor

@Sihui Sihui commented Jun 6, 2018

Thanks a lot, @josevalim!

I don't think fn_eoe -> 'fn' eoe : annotate_newlines('$2', '$1'). works because annotate_newlines expects the second arg to be in {Left, Meta, Right} format. Otherwise it will just return Expr directily.

annotate_newlines({_, {_, _, Count}}, {Left, Meta, Right}) when is_integer(Count), is_list(Meta) ->
  case ?formatter_metadata() of
    true -> {Left, [{newlines, Count} | Meta], Right};
    false -> {Left, Meta, Right}
  end;
annotate_newlines(_, Expr) ->
  Expr.

I thought we have to change fn from terminal to non_terminal and build it out so we can get the {Left, Meta, Right} for it, like the way we do for stab. So I tried to do the same thing with fn, and (unsurprisingly) it didn't work. 😂

fn -> fn_expr : ['$1'].
fn -> fn eoe fn_expr : [annotate_newlines('$2', '$3') | '$1'].

fn_eoe -> 'fn' : '$1'.
fn_eoe -> 'fn' eoe : '$1'.

fn_expr -> expr :
                '$1'.
fn_expr -> fn_op_eol_and_expr :
               build_op(element(1, '$1'), [], element(2, '$1')).
fn_expr -> empty_paren fn_op_eol_and_expr :
               build_op(element(1, '$2'), [], element(2, '$2')).
fn_expr -> empty_paren when_op expr fn_op_eol_and_expr :
               build_op(element(1, '$4'), [{'when', meta_from_token('$2'), ['$3']}], element(2, '$4')).
fn_expr -> call_args_no_parens_all fn_op_eol_and_expr :
               build_op(element(1, '$2'), unwrap_when(unwrap_splice('$1')), element(2, '$2')).
fn_expr -> fn_parens_many fn_op_eol_and_expr :
               build_op(element(1, '$2'), unwrap_splice('$1'), element(2, '$2')).
fn_expr -> fn_parens_many when_op expr fn_op_eol_and_expr :
               build_op(element(1, '$4'), [{'when', meta_from_token('$2'), unwrap_splice('$1') ++ ['$3']}], element(2, '$4')).

fn_op_eol_and_expr -> fn_op_eol expr : {'$1', '$2'}.
fn_op_eol_and_expr -> fn_op_eol : warn_empty_fn_clause('$1'), {'$1', handle_literal(nil, '$1')}.

fn_parens_many -> open_paren call_args_no_parens_kw close_paren : ['$2'].
fn_parens_many -> open_paren call_args_no_parens_many close_paren : '$2'.

I also saw that we have build_fn already. Maybe I should find a way to leverage that?

Sorry for all the back and forth, I'm pretty new to both Elixir and Erlang. Thank you for your patience!

@josevalim

This comment has been minimized.

Copy link
Member

@josevalim josevalim commented Jun 6, 2018

You are correct. I am not sure how to move this forward then, I would also need to sit down and try some different approaches, and I won't have the time to do it right now. Sorry.

@Sihui

This comment has been minimized.

Copy link
Contributor

@Sihui Sihui commented Jun 6, 2018

No worries. I might take another look during the weekend and see if I can come up with something. The elixir_parser.yrl is a pretty good read anyway. :)

@Sihui

This comment has been minimized.

Copy link
Contributor

@Sihui Sihui commented Jun 10, 2018

@josevalim

Not sure which behavior we want, so I have two PRs out.

V1: When fn is followed by a newline, use multi-clause formatting style.
V2: When fn is followed by a newline, preserve the newline.

For this example:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn
  %SomeStruct{key: key}, acc -> more_code(key, acc)
end)

In V1, the example will stay the same.
In V2, the example will still be formatted while preserving the newline following fn. The result will be:

SomeModule.long_function_name_that_approaches_max_columns(argument, acc, fn 
  %SomeStruct{key: key}, acc ->
  more_code(key, acc)
end)

As you can see in the test, V1 also changes how we format comments in some cases. But V2 doesn't.

@josevalim josevalim closed this Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.