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

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

Open
jesperes opened this issue Apr 7, 2023 · 3 comments
Open

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

jesperes opened this issue Apr 7, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@jesperes
Copy link

jesperes commented Apr 7, 2023

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.

@jesperes jesperes added the bug Something isn't working label Apr 7, 2023
@SuddenGunter
Copy link

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

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

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 🙄

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

3 participants