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

Metals Only Processes the Head Option of the contentChanges Array #6220

Closed
tsxjsx opened this issue Mar 12, 2024 · 2 comments · Fixed by #6237
Closed

Metals Only Processes the Head Option of the contentChanges Array #6220

tsxjsx opened this issue Mar 12, 2024 · 2 comments · Fixed by #6237
Labels
bug Something that is making a piece of functionality unusable
Milestone

Comments

@tsxjsx
Copy link

tsxjsx commented Mar 12, 2024

Describe the bug

When the client sends a textDocument/didChange request with parameters, Metals only considers the first index element (text='') option from the contentChanges array, disregarding the remaining index elements.

textDocument/didChange(LSP) specification
While the Language Server Protocol (LSP) specification dictates that the second index element from the contentChanges array in a textDocument/didChange request should take effect, Metals, when processing such requests, only considers the first index element (text=''), thus presenting a deviation from the standard

Client side:-

{
  "textDocument": {
    "version": 88,
    "uri": "file:///home/local/user/scala-cosmos/src/main/scala/org/cosmos/scala/Main.scala"
  },
  "contentChanges": [
    {
      "text": ""
    },
    {
      "range": {
        "start": {
          "line": 0,
          "character": 0
        },
        "end": {
          "line": 0,
          "character": 0
        }
      },
      "rangeLength": 0,
      "text": "package org.cosmos.scala\n\n// A new way to define the program entry point. Note the automatic argument extraction.\n@main def startApp() \u003d \n  Server.run(8888)                                                        \n}                         \n  \n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n              \n\n\n\n\n\n            "
    }
  ]

Server Side:-

  override def didChange(
      params: DidChangeTextDocumentParams
  ): CompletableFuture[Unit] =
    params.getContentChanges.asScala.headOption match {
      case None => CompletableFuture.completedFuture(())
      case Some(change) =>
        val path = params.getTextDocument.getUri.toAbsolutePath
        buffers.put(path, change.getText)
        diagnostics.didChange(path)

        parseTrees(path)
          .flatMap { _ =>
            treeView.onWorkspaceFileDidChange(path)
            publishSynthetics(path)
          }
          .ignoreValue
          .asJava
    }
    
     /** Optionally selects the first element.
    *  $orderDependent
    *  @return  the first element of this $coll if it is nonempty,
    *           `None` if it is empty.
    */
  def headOption: Option[A] = {
    val it = iterator
    if (it.hasNext) Some(it.next()) else None
  }

Result:
Due to the exclusive consideration of the first index element, the server interprets the document as empty, resulting in the absence of functioning language features for the textDocument.

Expected behavior

According to the Language Server Protocol (LSP) specification, when a client sends a textDocument/didChange request with parameters containing multiple elements within the contentChanges array, the server should consider all elements for processing. Each element represents a change in the document, facilitating incremental updates.

In the ideal scenario, the server should iterate through each element within the contentChanges array, applying modifications to the document accordingly. This behavior ensures that language features are fully functional and responsive to incremental changes made by the user.

The server's adherence to the LSP specification enables seamless integration with various development environments and enhances the overall user experience by providing accurate and timely feedback on code modifications.

Operating system

Linux

Editor/Extension

None

Version of Metals

1.2.2+39-68cbb4ce-SNAPSHOT

Extra context or search terms

No response

@tgodzik
Copy link
Contributor

tgodzik commented Mar 12, 2024

Thanks for reporting! Metals only supports lsp4j.TextDocumentSyncKind.Full so I assumed there wouldn't be multiple versions changed. Did you find that while using VS Code?

I think in this case we would instead just take the last item and ignore everything else, since we are not interested on previous state of the document, but the latest one.

@kasiaMarek could this be related to the recent changes with saveAs?

@tgodzik tgodzik added the bug Something that is making a piece of functionality unusable label Mar 12, 2024
@tgodzik tgodzik added this to Triage in Metals Issue Board via automation Mar 12, 2024
@kasiaMarek
Copy link
Contributor

@kasiaMarek could this be related to the recent changes with saveAs?

No, it doesn't seem related.

@tgodzik tgodzik moved this from Triage to To do in Metals Issue Board Mar 13, 2024
@kasiaMarek kasiaMarek moved this from To do to In progress in Metals Issue Board Mar 26, 2024
Metals Issue Board automation moved this from In progress to Done Mar 26, 2024
@tgodzik tgodzik added this to the Metals v1.3.0 milestone Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is making a piece of functionality unusable
Projects
Development

Successfully merging a pull request may close this issue.

3 participants