elixir-lsp
Race conditions in SourceFile.formatter_opts
When pwd is changing by the build in the background, SourceFile.formatter_opts crashes with various exceptions (see https://github.com/elixir-lsp/elixir-ls/issues/318, https://github.com/elixir-lsp/elixir-ls/issues/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 https://github.com/elixir-lsp/elixir-ls/pull/394
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?
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.
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.
Addressed in https://github.com/elixir-lsp/elixir-ls/pull/890 Requires elixir 1.15