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

Race conditions in SourceFile.formatter_opts #402

Closed
lukaszsamson opened this issue Nov 13, 2020 · 4 comments
Closed

Race conditions in SourceFile.formatter_opts #402

lukaszsamson opened this issue Nov 13, 2020 · 4 comments
Labels
bug Something isn't working

Comments

@lukaszsamson
Copy link
Collaborator

When pwd is changing by the build in the background, SourceFile.formatter_opts crashes with various exceptions (see #318, #401). We rescue them but this only hides the real issue. The proper fix would be to use a cache with build lock. Example solution fixing only formatter issues is proposed in #394

@zachdaniel
Copy link

I copied this from discord, but I'm posting it here as I think it is more relevant:

I just got boned pretty hard by the issue where the .formatter.exs is not respected while the app is compiling in the background. Is there some way to detect the case where it might be ignoring a .formatter.exs (e.g if the app is compiling) just reject/skip the format on save? In our case, we have a bunch of locals w/o parens that got formatted back to have parens, that I now have to go back and change by hand to remove all the parens.
I know its a more complicated issue to make the behavior consistent even while the code is compiling, but given that formatting without the .formatter.exs can have "one way" changes (even more so now that you can have formatter plugins), I think it would be much friendlier to simply ignore the request to format in those cases (perhaps configurable as an option if that is problematic for others).

So my question mainly revolves around whether or not there may be a stop-gap in here, e.g if SourceFile.formatter_opts then just skipping the formatting step?

@axelson
Copy link
Member

axelson commented Feb 22, 2022

I'm not sure that silently skipping the formatting step would be a good behavior because it would be reporting that a file is formatted when it is not formatted, which in my opinion is worse than formatting with default options. Perhaps instead we could mark the request as failed, although I'm not sure exactly how that would manifest to users.

I have started working on a PR to elixir that would allow us to pass in a root path to Mix.Tasks.Format.formatter_opts_for_file/2 (which we could then backport to ElixirLS) which is in my opinion the proper fix although I haven't been able to make much progress on it so far. Anyone who is willing could take that over if they'd like.

@zachdaniel
Copy link

zachdaniel commented Feb 22, 2022

Yeah, I think some kind of failure would be the simplest answer. Even if we fix this particular issue, I think it's important to note that formatting with default options can theoretically be destructive given that we have formatted plugins now. So IMO we still want the behavior that if anything goes wrong getting the formatter opts we should fail to format.

zachdaniel referenced this issue Jun 21, 2022
* Use formatter_for_file instead of formatter_opts_for_file which is
  deprecated now

* Fallback to original Code.format_string!

Co-authored-by: Dalibor Horinek <dal@horinek.net>
@lukaszsamson
Copy link
Collaborator Author

lukaszsamson commented Jun 3, 2023

Addressed in #890
Requires elixir 1.15

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

Successfully merging a pull request may close this issue.

3 participants