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

Formatting turns function definitions into one long line #313

Open
fkrause98 opened this issue Jun 21, 2022 · 14 comments
Open

Formatting turns function definitions into one long line #313

fkrause98 opened this issue Jun 21, 2022 · 14 comments
Labels
bug Something isn't working

Comments

@fkrause98
Copy link

fkrause98 commented Jun 21, 2022

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.

@elbrujohalcon
Copy link
Collaborator

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

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

@fkrause98
Copy link
Author

fkrause98 commented Jun 21, 2022

Oh, that's sad to read :/.

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

@elbrujohalcon
Copy link
Collaborator

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
Copy link
Collaborator

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}.

@fkrause98
Copy link
Author

Thanks!

@fkrause98
Copy link
Author

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.

@elbrujohalcon
Copy link
Collaborator

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

@fkrause98
Copy link
Author

Thanks, I'll try doing that!

@jesperes
Copy link

jesperes commented Apr 5, 2023

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.

@elbrujohalcon
Copy link
Collaborator

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.

@jesperes
Copy link

jesperes commented Apr 7, 2023

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.

@elbrujohalcon
Copy link
Collaborator

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?

@jesperes
Copy link

jesperes commented Apr 7, 2023

Sure

@michalmuskala
Copy link

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants