rebar3_format icon indicating copy to clipboard operation
rebar3_format copied to clipboard

Formatting turns function definitions into one long line

Open fkrause98 opened this issue 2 years ago • 14 comments

Describe the bug Function definitions ends up in a unique, long line. Before formatting:

handle_handoff_command(?FOLD_REQ{foldfun=FoldFun, acc0=Acc0}, _Sender,
                       State = #{data := Data}) ->
  log("Received fold request for handoff", State),
  Result = maps:fold(FoldFun, Acc0, Data),
  {reply, Result, State};
  
handle_handoff_command({get, Key}, Sender, State) ->
  log("GET during handoff, handling locally ~p", [Key], State),
  handle_command({get, Key}, Sender, State);

handle_handoff_command(Message, Sender, State) ->
  {reply, _Result, NewState} = handle_command(Message, Sender, State),
  {forward, NewState}.

After formatting:

handle_handoff_command( ?FOLD_REQ { foldfun = FoldFun , acc0 = Acc0 } , _Sender , State = #{ data := Data } ) -> log( "Received fold request for handoff" , State ) , Result = maps : fold( FoldFun , Acc0 , Data ) , { reply , Result , State } ; handle_handoff_command( { get , Key } , Sender , State ) -> log( "GET during handoff, handling locally ~p" , [ Key ] , State ) , handle_command( { get , Key } , Sender , State ) ; handle_handoff_command( Message , Sender , State ) -> { reply , _Result , NewState } = handle_command( Message , Sender , State ) , { forward , NewState } .

To Reproduce Define the function like above, and run rebar3 format. You can clone this repo and run ./rebar3 format, this is the line that I'm quoting above

Expected behavior I expected the formatting to keep my function definition like in the example above.

Rebar3 Log

===> Load global config file /Users/fran/.config/rebar3/rebar.config
===> Compile (apps)
===> 25.0 satisfies the requirement for minimum OTP version 21
===> 25.0 satisfies the requirement for minimum OTP version 21
===> Compile (apps)
===> Expanded command sequence to be run: [app_discovery,format]
===> Running provider: app_discovery
===> Found top-level apps: [rc_example]
	using config: [{src_dirs,["src"]},{lib_dirs,["apps/*","lib/*","."]}]
===> Running provider: format
===> Formatter options: #{action => format,output_dir => current}
===> Found 9 files: ["src/rc_example.erl","src/rc_example_app.erl",
                            "src/rc_example_coverage_fsm.erl",
                            "src/rc_example_coverage_fsm_sup.erl",
                            "src/rc_example_sup.erl",
                            "src/rc_example_vnode.erl",
                            "src/rc_example.app.src",
                            "test/key_value_SUITE.erl","rebar.config"]
===> Formatting "src/rc_example.erl" with #{module => default_formatter,
                                                   opts =>
                                                       #{action => format,
                                                         output_dir =>
                                                             current},
                                                   state => nostate}
===> Formatting "src/rc_example_app.erl" with #{module =>
                                                           default_formatter,
                                                       opts =>
                                                           #{action => format,
                                                             output_dir =>
                                                                 current},
                                                       state => nostate}
===> Formatting "src/rc_example_coverage_fsm.erl" with #{module =>
                                                                    default_formatter,
                                                                opts =>
                                                                    #{action =>
                                                                          format,
                                                                      output_dir =>
                                                                          current},
                                                                state =>
                                                                    nostate}
===> Formatting "src/rc_example_coverage_fsm_sup.erl" with #{module =>
                                                                        default_formatter,
                                                                    opts =>
                                                                        #{action =>
                                                                              format,
                                                                          output_dir =>
                                                                              current},
                                                                    state =>
                                                                        nostate}
===> Formatting "src/rc_example_sup.erl" with #{module =>
                                                           default_formatter,
                                                       opts =>
                                                           #{action => format,
                                                             output_dir =>
                                                                 current},
                                                       state => nostate}
===> Formatting "src/rc_example_vnode.erl" with #{module =>
                                                             default_formatter,
                                                         opts =>
                                                             #{action =>
                                                                   format,
                                                               output_dir =>
                                                                   current},
                                                         state => nostate}
===> Formatting "src/rc_example.app.src" with #{module =>
                                                           default_formatter,
                                                       opts =>
                                                           #{action => format,
                                                             output_dir =>
                                                                 current},
                                                       state => nostate}
===> Formatting "test/key_value_SUITE.erl" with #{module =>
                                                             default_formatter,
                                                         opts =>
                                                             #{action =>
                                                                   format,
                                                               output_dir =>
                                                                   current},
                                                         state => nostate}
===> Formatting "rebar.config" with #{module => default_formatter,
                                             opts =>
                                                 #{action => format,
                                                   output_dir => current},
                                             state => nostate}

Additional context I've tried to use both: #{parse_macro_definitions => true} and {parse_macro_definitions => false} because I suspect the macro might be the culprit here.

fkrause98 avatar Jun 21 '22 19:06 fkrause98

Yeah... Once again... It's because of macros 😢.

I'm sorry, but katana-code / erl_syntax can't parse that code.

elbrujohalcon avatar Jun 21 '22 21:06 elbrujohalcon

Oh, that's sad to read :/.

Is there a way to make the formatter ignore those specific lines, or at least the file?

fkrause98 avatar Jun 21 '22 22:06 fkrause98

Out of curiosity: What is ?FOLD_REQ and why does it need to be a macro? 🤔

Is there a way to make the formatter ignore those specific lines, or at least the file?

Sure! Just add -format ignore. to that module.

elbrujohalcon avatar Jun 22 '22 06:06 elbrujohalcon

You can also use this arguably uglier and dumber version of the code that can actually be parsed/formatted 🤷🏻 :

-module(bug).

-export([handle_handoff_command/3]).

-record(fOLD_REQ, {foldfun, acc0}).

-define(FOLD_REQ, #fOLD_REQ).
-define(FOLD_REQ(Field), #fOLD_REQ.?Field).

handle_handoff_command(Record, _Sender, State = #{data := Data})
    when is_record(Record, ?FOLD_REQ) ->
    FoldFun = element(?FOLD_REQ(foldfun), Record),
    Acc0 = element(?FOLD_REQ(acc0), Record),
    log("Received fold request for handoff", State),
    Result = maps:fold(FoldFun, Acc0, Data),
    {reply, Result, State};
handle_handoff_command({get, Key}, Sender, State) ->
    log("GET during handoff, handling locally ~p", [Key], State),
    handle_command({get, Key}, Sender, State);
handle_handoff_command(Message, Sender, State) ->
    {reply, _Result, NewState} = handle_command(Message, Sender, State),
    {forward, NewState}.

elbrujohalcon avatar Jun 22 '22 07:06 elbrujohalcon

Thanks!

fkrause98 avatar Jun 22 '22 14:06 fkrause98

Out of curiosity: What is ?FOLD_REQ and why does it need to be a macro? 🤔

Is there a way to make the formatter ignore those specific lines, or at least the file?

Sure! Just add -format ignore. to that module.

I'm not sure, I was updating an Erlang tutorial on how to set up Riak Core, and that macro is needed as part of a function needed by a behaviour.

fkrause98 avatar Jun 22 '22 15:06 fkrause98

If I were you, @fkrause98, I would dig up that macro definition and just use the value instead of the macro.

elbrujohalcon avatar Jun 23 '22 06:06 elbrujohalcon

Thanks, I'll try doing that!

fkrause98 avatar Jun 23 '22 17:06 fkrause98

Yeah... Once again... It's because of macros cry.

I'm sorry, but katana-code / erl_syntax can't parse that code.

I'd like to use this on our code base, and I'm fine with the formatter not being able to parse the code, but the show-stopper for me is that is destroys the existing formatting and dumps everything on one single line. If it is unable to parse the code, is has to leave it alone, and just print a warning pointing out what it can't parse.

jesperes avatar Apr 05 '23 12:04 jesperes

It can't "leave it alone" because that's not how this formatter works. This formatter parses the modules into ASTs and then renders them in a formatted way. At the time of rendering there's little to no information about how it was formatted before.

elbrujohalcon avatar Apr 05 '23 12:04 elbrujohalcon

Isn't it at least possible to have the parser tell you if it was able to parse the file, and if not just ignore the entire file? I was hoping to be able to apply this to our entire code base, but that won't happen if we need to review millions of line of code just to see if some function got its formatting destroyed.

jesperes avatar Apr 07 '23 07:04 jesperes

Yeah. That would be possible. We can totally add a config option to ignore unparseable files entirely. Do you mind creating a separate ticket for that?

elbrujohalcon avatar Apr 07 '23 07:04 elbrujohalcon

Sure

jesperes avatar Apr 07 '23 09:04 jesperes

FWIW with the erlfmt backend the example at hand is formatted just fine.

Furthermore, when erlfmt cannot parse a particular form (function or an attribute) it will output it back unchanged (formatting everything else in the file).

michalmuskala avatar Apr 18 '23 08:04 michalmuskala