Formatter doesn't respect formatter.exs while your code is still compiling

[mhanberg]

Environment

• Elixir & Erlang versions (elixir --version): Erlang/OTP 23 [erts-11.1.7] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1]

Elixir 1.11.3 (compiled with Erlang/OTP 21)

• VSCode ElixirLS version: 0.7.0
• Operating System Version: darwin 20.3.0

Troubleshooting

• Restart your editor (which will restart ElixirLS) sometimes fixes issues
• Stop your editor, remove the entire .elixir_ls directory, then restart your editor
  • NOTE: This will cause you to have to re-run the dialyzer build for your project

https://www.dropbox.com/s/llgnf7ham9d0hf8/Screen%20Recording%202021-04-07%20at%208.28.49%20AM.mov?dl=0

You can reproduce this by deleting the .elixir_ls directory, and then saving a file to trigger the LS to start recompiling, then run the formatter. You observe that it is not respecting the formatter configuration because it is adding parens to Ecto functions/macros that shouldn't have parens.

The screen recording I added above using VSCode, but I have also reproduced it with Neovim builtin language client.

I personally have never run into this race condition, but my coworkers constantly are pushing up code that is clearly formatted incorrectly, and I traced it down to this issue.

Thanks!

[tap349]

Up. I'm facing this issue too. When I run 2 successive commands to format file, formatter.exs is ignored.

[axelson]

This is caused because when compiling code, mix changes the current working directory, and the code that is used to fetch the .formatter.exs and compare file paths against it is dependent on the current working directory. This is related to #402

[mhanberg]

Ahh gotcha. Feel free to close this in favor of the earlier reported issue if that makes things easier for you.

[MrGrinst]

Commenting here because this seems like the central place to address it. This problem was bugging me for a while and I stumbled on #394 (the ETS cache fix), which I had checked out and running locally for a while. That PR seemed to fully fix this problem, but looks like it's been dead for a bit.

I'm happy to pick that up and try to get it across the finish line, but I also stumbled on a couple other approaches: #687 (blocking until project reload) and @axelson your work on an Elixir core PR.

Is there a preferred approach here? It seems like the Elixir core approach is most robust but this ETS cache fix might be faster to implement? Would love to help in any way I can.

[axelson]

@MrGrinst at ElixirConf this year I've made plans to start some regular pairing sessions with @zachdaniel on the Elixir core PR that I have started but that got stalled out. Hopefully we'll have something to report soon!

[DianaOlympos]

Any progress or an idea of the direction to take? I can take it over the line otherwise, I need this kind of small win in a project that is not mine right now...

[axelson]

@DianaOlympos I think this would count as a large win! The way I've been thinking about this is there's two parts to this.

First is a PR to Elixir that changes Mix.Tasks.Format.formatter_opts_for_file/2 to not rely on the current working directory (most likely by passing the root directory into the function). I discussed this with José a while ago and he stated that he is open to the PR: https://groups.google.com/g/elixir-lang-core/c/r5FA-4_Fu9o/m/rZiFgGOJBAAJ

The second is temporarily vendoring Mix.Tasks.Format.formatter_opts_for_file/2 within ElixirLS until ElixirLS can depend on a version of Mix.Tasks.Format.formatter_opts_for_file/2 that does not rely on the current working directory.

[lukaszsamson]

Maybe it would be easier to run formatter as a separate OS process instead. Another option is separating build to a different process

[stevencorona]

Hi all, has anyone found a viable short-term workaround to prevent this from happening (even if it means just ignoring formatting during compilation?) - really anything short of entirely disabling the auto-formatting. It's a huge pain point in my day-to-day.

[skbolton]

Thinking out loud here since there is a lot of people looking at this right now.

Couldn't the dot-formatter option be used to pass the path to the projects .formatter.exs instead of relying on the default .formatter.exs in the (possibly) changed cwd?

[robmerrell]

I have a branch where I did just that with the dot_formatter option: https://github.com/robmerrell/elixir-ls/tree/formatter_project_dir that I have been using for a couple of months.

It does solve this formatting issue, but I have run into a bug a few times where it can't find the import_deps in my formatter.exs that a quick restart of the lsp server fixes. It hasn't happened often enough for me to be able to reproduce it reliably or make a guess at what might be causing it.

[lukaszsamson]

@skbolton @robmerrell while dot_formatter seemingly resolves the issue with changing cwd during build it still has race conditions. See e.g. https://github.com/elixir-lang/elixir/blob/da8d89438271ee042a3295d3723efb04e330288b/lib/mix/lib/mix/tasks/format.ex#L371 the formatter task is making some changes to Mix.Project stack or calls mix compile task https://github.com/elixir-lang/elixir/blob/da8d89438271ee042a3295d3723efb04e330288b/lib/mix/lib/mix/tasks/format.ex#L308. Normal elixirLS build is also triggering mix compile and pushing/popping on Mix.Project stack.

[DianaOlympos]

Ok so proper fixing options seem to be

1. Spawn a new OS process. No idea what would be the impact and/or problems there or how we would handle it
2. Make Mix.Tasks.Format.formatter_opts_for_file/2 in Elixir take a root option and inject the directory into it. Then wait for elixir to catch up
3. Vendor Mix.Tasks.Format.formatter_opts_for_file/2 and make it take the root option.
4. Do both 3 and 2.

Intuitively I want to go for 3 first, to fix the damn problem, then upstream with 2. 1 would be a nice fix, but I really do not want to deal with wiring up everything.

What do you think @lukaszsamson ? I may have time to do 2 today.

[lukaszsamson]

Go ahead. It would be best if we could solve the problem upstream

[DianaOlympos]

So after a dive, I will just upstream it. This function got far more complex recently, probably due to plugins, which would make vendoring a pain and a lot of code.

Once upstreamed, I will come back to implement it here, in particular because there are at least 5 different ways we call it and validate the correct version to call in the LSP codebase, so I will try to consolidate this a bit

[DianaOlympos]

Opened the PR above with elixir, we will see how it goes.

[akash-akya]

FWIW, I had this issue while I was working with a very large codebase. I wanted the formatter must be fast and reliabl -- it should work even if code can not compile but has valid syntax (semantic errors) as I added to "on-save" callback.

As I am using Emacs, I just created a library to run an inferior shell iex -S mix run --no-start --no-compile at the project root. Then I would send Mix.Task.rerun("format", ["%s"]) command on file save. This was way faster since we are starting on OS process only once (especially on macos). Emacs has a lot of tooling with similar approach for other languages (CIDER, SLiME etc.) so it is not that hard to build these tooling there.

Interestingly, this approach helped me to speed up running test suite as well. Start iex shell, load test helper code, start all OTP applications, and run recompile() && Tester.run(["%s"]) to test. Since applications are started only once when shell startes, it will be much faster than starting applications every time we run tests. There are some corner cases but it works for common cases. I also created utility tools like json-to-map & map-to-json which converts the selection and copy the output to clipboard.

[DianaOlympos]

Ok so this is now merged.
elixir-lang/elixir#12541

I will look at bringing it to lsp.

I quickly grepped the codebase, it seems we have multiple places where we call the formatter, with different styles and using different ways to check which call we should do/elixir version.

Is there an interest in unifying this while i am at it?

If yes do you prefer a version check or a "if function exported" ?

I suppose we would path the project path as root?

[lukaszsamson]

Is there an interest in unifying this while i am at it?

Sure

If yes do you prefer a version check or a "if function exported" ?

I prefer version check, it's easier to find and remove them when we bump min version required. Here we'd need to check version anyway as the fun is exported but the new opts will be >= 1.15 only

I suppose we would path the project path as root?

Exactly. By default elixir-ls looks for mix.exs under workspace root but that can be overwritten by settingelixirLS.projectDir. The function ElixirLS.LanguageServer.Server.set_project_dir combines workspace root with elixirLS.projectDir, calls File.cd! and sets the resulting dir as project_dir property in server state.

[lukaszsamson]

Is there an interest in unifying this while i am at it?

Sure

If yes do you prefer a version check or a "if function exported" ?

I prefer version check, it's easier to find and remove them when we bump min version required. Here we'd need to check version anyway as the fun is exported but the new opts will be >= 1.15 only

I suppose we would path the project path as root?

Exactly. By default elixir-ls looks for mix.exs under workspace root but that can be overwritten by settingelixirLS.projectDir. The function ElixirLS.LanguageServer.Server.set_project_dir combines workspace root with elixirLS.projectDir, calls File.cd! and sets the resulting dir as project_dir property in server state.

[DianaOlympos]

Here we'd need to check version anyway as the fun is exported but the new opts will be >= 1.15 only

Note that this would not be a problem @lukaszsamson because the option is optional. We can set it up pre 1.15 and it will just be ignored and works as it does today.