"Format on save" deletes recently written code

[kubukoz]

Describe the bug

In the latest release, I started noticing some formatting issues that I never saw before: When used with format-on-save, saving a file will remove some code I just wrote.

In case it's not possible to demonstrate with a code example, here's a recording:

Screen.Recording.2022-06-11.at.19.59.24.mov

Steps (doesn't always work):

1. Enable format on save
2. Have some working code like this

object Main extends App {

  val hello = 42

  identity()
}

3. Save the file. Now put the cursor before identity and add an indent with <tab>. Go to the paranthesis and write something, even ???. You should have something like this:

object Main extends App {

  val hello = 42

  	identity(???)
}

4. Save the file. This will (sometimes) delete the entirety of ??? or part of it.

Versioning note: the server version seems to be 0.11.6+67-926ec9a3-SNAPSHOT. It seems to be set automatically by the most recent vscode extension version (1.17.3). When I set 0.11.6 explicitly, it still doesn't work. The extension worked fine with its defaults on 1.17.1, so it seems like an extension issue. 1.17.2 also seems to be affected.

Expected behavior

Code is formatted but no non-whitespace is removed.

Installation:

• Operating system: macOS
• VSCode version: 1.67.2
• VSCode extension version: 1.17.3
• Metals version: (found in VSCode settings, under metals.serverVersion): 0.11.6 / 0.11.6+67-926ec9a3-SNAPSHOT (doesn't seem to matter)

Additional context

Search terms

scalafmt

[tanishiking]

• FormattingProvider receive the input from buffer https://github.com/scalameta/metals/blob/e354275ffc5351562593aaf8cb9c095374d6d273/metals/src/main/scala/scala/meta/internal/metals/FormattingProvider.scala#L107
• buffers will be updated on didChange and didSave
  • https://github.com/scalameta/metals/blob/e354275ffc5351562593aaf8cb9c095374d6d273/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1228
  • https://github.com/scalameta/metals/blob/e354275ffc5351562593aaf8cb9c095374d6d273/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1203

So, there could be a case where input of scalafmt (or java formatted) just before buffers are updated? 🤔 (but I couldn't reproduce the issue...)

[zmerr]

Considering all the applications of formatting in also the Code Actions, I wonder whether it would be a good idea to transfer the formatting feature entirely to the server, for getting a better control on the workflow.🧐

[tgodzik]

Considering all the applications of formatting in also the Code Actions, I wonder whether it would be a good idea to transfer the formatting feature entirely to the server, for getting a better control on the workflow.monocle_face

What do you mean? The logic is inside the server itself, but we can't control anything that's done before the LSP request is sent.

[tgodzik]

The only thing that I think that could influence this is #1015 , but no idea how is that happening. Maybe it's a race condition? Would be able to create a trace file .metals/lsp.trace.json? We could see what is the order in which requests are being sent.

[zmerr]

Considering all the applications of formatting in also the Code Actions, I wonder whether it would be a good idea to transfer the formatting feature entirely to the server, for getting a better control on the workflow.monocle_face

What do you mean? The logic is inside the server itself, but we can't control anything that's done before the LSP request is sent.

I mean the user sending a formatting request to the server and the server sending the formatting result back. Is it already getting done in this way?

So you mean the LSP request for formatting received before the LSP request for the removed edits, and that’s why this is happening?🧐

[tgodzik]

I mean the user sending a formatting request to the server and the server sending the formatting result back. Is it already getting done in this way?

Yes, that part of https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_formatting

So you mean the LSP request for formatting received before the LSP request for the removed edits, and that’s why this is happening?monocle_face

That's what I am guessing, but I think if that is the case then it's a bug on the VS Code side.

[zmerr]

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_formatting

So if this is a bug on VS Code side, then we need to see if it can be reproduced with the current version of metals and a previous version of VS Code. Any chance we can test this? @kubukoz

[tgodzik]

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_formatting

So if this is a bug on VS Code side, then we need to see if it can be reproduced with the current version of metals and a previous version of VS Code. Any chance we can test this? @kubukoz

It might also be the issue of the LSP library, or maybe there is an additional setting/plugin that is causing this?

Maybe the VS Code is not waiting for the response to didSave to arrive. But using the trace file could confirm this.

[tgodzik]

It might be related to microsoft/vscode-languageserver-node#972

[kpodsiad]

It's definitely caused by vscode-languageserver-node. I had issues with completions not triggering automatically on extension version: 1.17.3, but they're gone on 1.17.0

The easiest solution will be reverting #1015 and wait until upstream is fixed.
BTW, pre-release caught another bug 🎉

[tgodzik]

We need to update to the newest language client when it comes out, but let's wait for the next release.

I tried making it work on the next versions, but it took some tinkering so leaving the branches here:
https://github.com/scalameta/metals-vscode/compare/main...tgodzik:update-language-client?expand=1
https://github.com/tgodzik/metals-languageclient/pull/new/update-languageclient

[tgodzik]

The revert has been merged for now. @kubukoz let us know if it's working ok for now.

[zmerr]

Let’s assume it is on VS-code side, and metals server always serves the requests in sequence:

1. VS-code sends formatting request on old data
2. server formats old data
3. VS-code sends edit request
4. server processes the edit

In any permutation of the ordering of requests, still the edit should be applied in the end. I mean if the steps 1 and 3 get exchanged, then the formatting would be requested on the updated data. I cannot imagine a scenario when this can lead to the loss of the edits. Maybe loss of formatting, but not the edits, since they are done sooner than formatting.

I was imagining this parallel processing scenario which can lead to failure:

1. the client sends edit request
2. the server receives the edit request but does not start it
3. the client sends the format request
4. the server receives the format request
5. the server starts formatting on old data
6. the server starts processing the edit and finishes and sends it back
7. the server finishes formatting on old data and sends it back.

[tgodzik]

I think the exact order would be:

1. VS-code sends formatting request on old data
2. server formats old data, this happens concurrently with 3/4
3. VS-code sends edit request
4. server processes the edit
5. VS Code gets the response about the formatting, which always works on the whole file.

[kubukoz]

@tgodzik I tried 0.17.5 and now I'm getting pretty much no Metals functionality.
Logs are stuck on

Java home: /nix/store/c6dd94qvkz3r59bx9827gd8jybhxnmpf-zulu11.48.21-ca-jdk-11.0.11
Metals version: 0.11.6+67-926ec9a3-SNAPSHOT

and nothing happens, I can't even format the code (vscode says there's no formatter for scala files)

I'll stick to 0.17.1 for now ;P

[tgodzik]

Another unlucky PR got merged 😓

I reverted that one for now.

[kubukoz]

That seems to work!

[tanishiking]

Closing for #1018

[kubukoz]

I still sometimes see this, but very very rarely. I'll try to get a repro next time it happens.