search

AdRoll / rebar3_format

Erlang Formatter for Rebar3
https://tech.nextroll.com/blog/dev/2020/02/25/erlang-rebar3-format.html
MIT License
109 stars 21 forks source link

Do not attempt to format source files which cannot be parsed #344

Open jesperes opened 1 year ago

jesperes commented 1 year ago

Describe the bug If the parser fails to correctly parse a form (e.g. due to macros), it will (silently) destroy the formatting of the function. This is very bad, as it renders the formatter very unreliable to use on large/legacy (unformatted) code bases. If applied on a large code base, every single module would need to be reviewed to ensure that no functions have gotten their formatting destroyed.

Expected behavior Of course, it would be preferrable that the formatter could parse all syntactically correct Erlang source files, but if the parser is unable to correctly parse the source file, it would be better to (1) print an error message stating that it failed to parse the file (and preferrably enough information to understand where the error occurred) and (2) leave the file unformatted.

SuddenGunter commented 1 month ago

This issue blocks rebar3_format to be used on file save in editor (even though erlang plugin I use for vscode tries to do it automatically). For comparison, Go's gofmt does not change a file at all, if it fails to parse syntax - imo this is a good safe default behavior

elbrujohalcon commented 1 month ago

I see your point folks. I'm not going to say it's impossible to fix that way, but… there are two issues at play here:

  1. The parser (katana-code) is used for more than just rebar3_format. It's also used in tools like rebar3_hank, which can totally work, even with the way in which it returns the "unparseable" pieces of code. So, making it fail when it meets them should, at the very least, be an optional thing and maybe not even its default behaviour.
  2. The way in which rebar3_format works makes it impossible to just leave that unparseable part alone and format the rest of the file. I agree that it might be better if it would just fail to format the whole file, but considering item 1 (i.e., the parser doesn't tell the formatter that it failed to parse anything) and the fact that rebar3_format already checks that the resulting AST is unaffected… we initially decided to leave it as it is while we waited for a good way to specify sections of the code that the formatter should ignore. We might review that decision, particularly now that the OTP team decided against the possibility of interleaving custom attributes with functions in a module.

All in all, this is not something that will change soon, but it may change… eventually.

elbrujohalcon commented 1 month ago

On the other hand, if you feel like working on a PR for katana-code and a related PR for rebar3_format… that could speed up the process considerably 🙄