lexical-lsp / lexical

Lexical is a next-generation elixir language server
779 stars 77 forks source link

Don't pass project without project module to formatter lookup #727

Closed soundmonster closed 1 month ago

soundmonster commented 1 month ago

This fixes an edge case described in #227 for the formatter code action:

Then the formatter will pick up the right .formatter.exs but the wrong dependency tree / Mix.Project context, and formatting will fail. There's a fallback but it relies on Code.format_string! with the raw contents of .formatter.exs passed as formatter options. This does not always result in correct formatting because import_deps is ignored and not an option one can use with Code.format_string!.

This PR does not have a solution for the last problem of the fallback not yielding correct results, it rather makes sure that the "normal" formatting is more likely to work.

The solution is to not pass the project struct to the formatter handler, but instead to have the in_project helper fetch the cached project struct from ETS. The cached one is set during bootstrap and has the project_module field set, while the project passed from the LSP does not. This leads Mix.Project.in_project to be invoked with foo instead of the module name of the umbrella mixfile, and ultimately confusing the umbrella with the dependency of the same name.

soundmonster commented 1 month ago

I added #728 to flag that the default formatter fallback doesn't always work.

soundmonster commented 1 month ago

It's not the same, the project that's being passed into Format.edits/2 doesn't have a project_module set (i.e. it's nil), while the cached project from RemoteControl.get_project/0 does have it. The project module is being detected and set in RemoteControl.Bootstrap, then cached. I suspect that all this only happens in :remote_control, not in :server. But then the project from :server is passed around, and its module is nil.

scohen commented 1 month ago

That's the bug then.

scohen commented 1 month ago

@soundmonster pushed a pontential fix in #730 ...and in #731

soundmonster commented 1 month ago

Closing in favor of #731