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

improvement: Don't sent text on didSave #7015

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -884,9 +884,9 @@ abstract class MetalsLspService(
): CompletableFuture[Unit] = {
val path = params.getTextDocument.getUri.toAbsolutePath
savedFiles.add(path)
// read file from disk, we only remove files from buffers on didClose.
val text = path.toInput.text
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem necessary and for large files might actually be problematic.

buffers.put(path, text)
if (!buffers.contains(path)) {
buffers.put(path, path.toInput.text)
}
Comment on lines +887 to +889
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!buffers.contains(path)) {
buffers.put(path, path.toInput.text)
}
buffers.putIfAbsent(path, path.toInput.text)

I think that buffers is based on a concurrent map (TrieMap), so maybe it would be safer to perform an atomic operation here, in case multiple requests are processed in parallel. 🙂

Also, a question to didSave and didChange: it looks like currently they always overwrite the in-memory buffers content for the given file, but in case of concurrent requests we could apply the older data (race condition)? Seems that the DidCloseTextDocumentParams should contain document version, could we use that to always safely keep only the newest one? Sorry if I have misunderstood how this all works. 🙂

Copy link
Contributor

@kasiaMarek kasiaMarek Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are very right. We want and we should use text document version simply nobody found the time to do it. I thought there was an issue for that but I could not find it now, here is a connected issue: #1665.

Future
.sequence(
List(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1215,7 +1215,10 @@ class WorkspaceLspService(

val textDocumentSyncOptions = new lsp4j.TextDocumentSyncOptions
textDocumentSyncOptions.setChange(lsp4j.TextDocumentSyncKind.Full)
textDocumentSyncOptions.setSave(new lsp4j.SaveOptions(true))
// We don't use the text at all.
textDocumentSyncOptions.setSave(
new lsp4j.SaveOptions( /* includeText = */ false)
)
textDocumentSyncOptions.setOpenClose(true)

val scalaFilesPattern = new lsp4j.FileOperationPattern("**/*.scala")
Expand Down
6 changes: 4 additions & 2 deletions tests/slow/src/test/scala/tests/bazel/BazelLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ class BazelLspSuite
BazelBuildLayout(workspaceLayout, V.bazelScalaVersion, bazelVersion)
)
_ <- server.didOpen("Hello.scala")
_ <- server.didSave("Hello.scala") { _ =>
_ <- server.didChange("Hello.scala") { _ =>
"""|package examples.scala3
|
|sealed trait A
Expand All @@ -251,6 +251,7 @@ class BazelLspSuite
|}
|""".stripMargin
}
_ <- server.didSave("Hello.scala")(identity)
_ = assertNoDiff(
server.client.workspaceDiagnostics,
"""|Hello.scala:11:3: warning: match may not be exhaustive.
Expand All @@ -262,12 +263,13 @@ class BazelLspSuite
|""".stripMargin,
)
// warnings should not disappear after updating
_ <- server.didSave("Hello.scala") { text =>
_ <- server.didChange("Hello.scala") { text =>
s"""|$text
|
|class Additional
|""".stripMargin
}
_ <- server.didSave("Hello.scala")(identity)
_ = assertNoDiff(
server.client.workspaceDiagnostics,
"""|Hello.scala:11:3: warning: match may not be exhaustive.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,17 @@ class SyntaxErrorLspSuite extends BaseLspSuite("syntax-error") {
_ = assertNoDiff(client.workspaceDiagnostics, "")
_ <- server.didOpen("Main.scala")
_ <- server.didOpen("project/plugins.sbt")
_ <- server.didSave("Main.scala")(_ => "object A\n")
_ <- server.didChange("Main.scala")(_ => "object A\n")
_ <- server.didSave("Main.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|project/plugins.sbt:1:8: error: `identifier` expected but `object` found
|object object val x = 1
| ^^^^^^
|""".stripMargin,
)
_ <- server.didSave("project/plugins.sbt")(_ => "lazy val x = 1\n")
_ <- server.didChange("project/plugins.sbt")(_ => "lazy val x = 1\n")
_ <- server.didSave("project/plugins.sbt")(identity)
_ = assertNoDiff(client.workspaceDiagnostics, "")
} yield ()
}
Expand Down
12 changes: 8 additions & 4 deletions tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,12 @@ class SbtServerSuite
)
// reload build after build.sbt changes
_ <- server.executeCommand(ServerCommands.ResetNotifications)
_ <- server.didSave("build.sbt") { text =>
_ <- server.didChange("build.sbt") { text =>
s"""$text
|ibraryDependencies += "com.lihaoyi" %% "sourcecode" % "0.3.0"
|""".stripMargin
}
_ <- server.didSave("build.sbt")(identity)
_ = {
assertNoDiff(
client.workspaceErrorShowMessages,
Expand All @@ -209,9 +210,10 @@ class SbtServerSuite
// This is a little hacky but up above this promise is succeeded already, so down
// below it won't wait until it reconnects to Sbt and indexed like we want
_ = server.server.indexingPromise = Promise()
_ <- server.didSave("build.sbt") { text =>
_ <- server.didChange("build.sbt") { text =>
text.replace("ibraryDependencies", "libraryDependencies")
}
_ <- server.didSave("build.sbt")(identity)
_ = {
assert(client.workspaceErrorShowMessages.isEmpty)
}
Expand All @@ -231,7 +233,7 @@ class SbtServerSuite
| ^^^
|""".stripMargin,
)
_ <- server.didSave("build.sbt") { text =>
_ <- server.didChange("build.sbt") { text =>
text.replace(
"val a = project.in(file(\"a\"))",
"""|val a = project.in(file("a")).settings(
Expand All @@ -240,15 +242,17 @@ class SbtServerSuite
|""".stripMargin,
)
}
_ <- server.didSave("build.sbt")(identity)
_ = {
assert(client.workspaceErrorShowMessages.isEmpty)
}
_ <- server.didSave("a/src/main/scala/A.scala") { _ =>
_ <- server.didChange("a/src/main/scala/A.scala") { _ =>
"""|object A{
| val a: scala.meta.Defn.Class = ???
|}
|""".stripMargin
}
_ <- server.didSave("a/src/main/scala/A.scala")(identity)
_ <- server.assertHoverAtLine(
"a/src/main/scala/A.scala",
" val a: scala.meta.Defn.C@@lass = ???",
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/src/main/scala/tests/BaseAmmoniteSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,10 @@ abstract class BaseAmmoniteSuite(scalaVersion: String)
server.client.workspaceShowMessages,
s"Error importing Scala script ${workspace.resolve("main.sc")}. See the logs for more details.",
)
_ <- server.didSave("main.sc") { text =>
_ <- server.didChange("main.sc") { text =>
text.replace(fakeScalaVersion, scalaVersion)
}
_ <- server.didSave("main.sc")(identity)
_ <- server.server.indexingPromise.future
targets <- server.executeCommand(ServerCommands.ListBuildTargets)
_ = assertEquals(
Expand Down
12 changes: 8 additions & 4 deletions tests/unit/src/main/scala/tests/BaseRenameLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ abstract class BaseRenameLspSuite(name: String) extends BaseLspSuite(name) {
// possible breaking changes for testing
_ <- Future.sequence {
openedFiles.map { file =>
server.didSave(file) { code => breakingChange(code) }
server.didChange(file) { code => breakingChange(code) }
server.didSave(file)(identity)
}
}
_ = if (!expectedError) assertNoDiagnostics()
Expand All @@ -124,9 +125,12 @@ abstract class BaseRenameLspSuite(name: String) extends BaseLspSuite(name) {
Future
.sequence {
files.map { case (file, code) =>
server.didSave(file)(_ =>
code.replaceAll(allMarkersRegex, "")
)
for {
_ <- server.didChange(file)(_ =>
code.replaceAll(allMarkersRegex, "")
)
_ <- server.didSave(file)(identity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do either of two:

  • remove change function from server.didChange (it will always de identity)
  • have server.didSave with the function send first didChange and only later didSave.

} yield ()
}.toList

}
Expand Down
16 changes: 11 additions & 5 deletions tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -285,12 +285,14 @@ abstract class BaseWorksheetLspSuite(
_ <- server.didOpen("a/src/main/scala/Main.worksheet.sc")
_ <- cancelled.future
_ = client.onWorkDoneProgressStart = (_, _) => {}
_ <- server.didSave("a/src/main/scala/Main.worksheet.sc")(
_ <- server.didChange("a/src/main/scala/Main.worksheet.sc")(
_.replace("Stream", "// Stream")
)
_ <- server.didSave("a/src/main/scala/Main.worksheet.sc")(
_ <- server.didSave("a/src/main/scala/Main.worksheet.sc")(identity)
_ <- server.didChange("a/src/main/scala/Main.worksheet.sc")(
_.replace("42", "43")
)
_ <- server.didSave("a/src/main/scala/Main.worksheet.sc")(identity)
_ = assertNoDiff(
client.syntheticDecorations,
"""|
Expand Down Expand Up @@ -469,9 +471,11 @@ abstract class BaseWorksheetLspSuite(
|a.Util.increase(1) // : Int = 2
|""".stripMargin,
)
_ <- server.didSave("a/src/main/scala/a/Util.scala")(
_ <- server.didChange("a/src/main/scala/a/Util.scala")(
_.replace("n + 1", "n + 2")
)
_ <- server.didSave("a/src/main/scala/a/Util.scala")(identity)
_ <- server.didFocus("a/src/main/scala/a/Main.worksheet.sc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should ever need didFocus, since it may not be supported, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just focus switched to Util.scala and the worksheet doesn't evaluate. This will work in VS Code, thoguh wonder what happens if did focus is not supported

_ <- server.didSave("a/src/main/scala/a/Main.worksheet.sc")(identity)
_ = assertNoDiff(
client.workspaceDecorations("a/src/main/scala/a/Main.worksheet.sc"),
Expand Down Expand Up @@ -782,12 +786,13 @@ abstract class BaseWorksheetLspSuite(
)
),
)
_ <- server.didSave("a/src/main/scala/foo/Main.worksheet.sc")(
_ <- server.didChange("a/src/main/scala/foo/Main.worksheet.sc")(
_.replace(
"Hi(1, 2, 3)",
"Hi(7, 8, 9)",
)
)
_ <- server.didSave("a/src/main/scala/foo/Main.worksheet.sc")(identity)
export = server.exportEvaluation(
"a/src/main/scala/foo/Main.worksheet.sc"
)
Expand Down Expand Up @@ -951,9 +956,10 @@ abstract class BaseWorksheetLspSuite(
Nil,
)
_ <- client.applyCodeAction(0, codeActions, server)
_ <- server.didSave(path) { _ =>
_ <- server.didChange(path) { _ =>
server.bufferContents(path)
}
_ <- server.didSave(path)(identity)
// Assert if indentation is correct. See `AutoImports.renderImport`
_ = assertNoDiff(
server.bufferContents(path),
Expand Down
1 change: 1 addition & 0 deletions tests/unit/src/main/scala/tests/TestingServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,7 @@ final case class TestingServer(
val abspath = toPath(filename)
val oldText = abspath.toInputFromBuffers(buffers).text
val newText = fn(oldText)
buffers.put(abspath, newText)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

didChange(filename, newText)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,10 @@ abstract class BaseNonCompilingLspSuite(name: String)
|}
|""".stripMargin
input = newText.replace("<<", "").replace(">>", "")
_ <- server.didSave("a/src/main/scala/a/A.scala") { _ =>
_ <- server.didChange("a/src/main/scala/a/A.scala") { _ =>
newText.replace("<<", "").replace(">>", "")
}
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ <-
server
.assertCodeAction(
Expand All @@ -119,9 +120,10 @@ abstract class BaseNonCompilingLspSuite(name: String)
kind = Nil,
)
// make sure that the now change UniqueObject is not suggested
_ <- server.didSave("a/src/main/scala/a/A.scala") { _ =>
_ <- server.didChange("a/src/main/scala/a/A.scala") { _ =>
input.replace("UniqueObjectOther", "UniqueObject")
}
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ <-
server
.assertCodeAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ class BuildServerConnectionLspSuite
_ = server.server.bspSession.get.cancel()
_ = assertNoDiagnostics()
_ <- server.executeCommand(ServerCommands.ConnectBuildServer)
_ <- server.didSave("a/src/main/scala/a/A.scala")(
_ <- server.didChange("a/src/main/scala/a/A.scala")(
_.replace("val n = 42", "val n: String = 42")
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/A.scala:3:19: error: type mismatch;
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/src/test/scala/tests/DefinitionLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,10 @@ class DefinitionLspSuite
|}
""".stripMargin,
)
_ <- server.didSave("a/src/main/scala/a/Main.scala")(
_ <- server.didChange("a/src/main/scala/a/Main.scala")(
_.replace("max(1, 2)", "max")
)
_ <- server.didSave("a/src/main/scala/a/Main.scala")(identity)
_ = assertNoDiff(
server.workspaceDefinitions,
"""|/a/src/main/scala/a/Main.scala
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/src/test/scala/tests/DiagnosticsLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,10 @@ class DiagnosticsLspSuite extends BaseLspSuite("diagnostics") {
| ^^
|""".stripMargin,
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(
_ <- server.didChange("a/src/main/scala/a/A.scala")(
_.replace("val n: Int = \"\"", "val n: Int = \" ")
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/A.scala:2:16: error: unclosed string literal
Expand Down
9 changes: 6 additions & 3 deletions tests/unit/src/test/scala/tests/DidFocusLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -185,23 +185,26 @@ class DidFocusWhileCompilingLspSuite
|""".stripMargin
}
_ = fakeTime.elapseSeconds(10)
_ <- server.didSave("a/src/main/scala/a/A.scala")(
_ <- server.didChange("a/src/main/scala/a/A.scala")(
_.replace("1", "\"\"")
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
xMismatch,
)
_ <- server.didSave("b/src/main/scala/b/B.scala")(
_ <- server.didChange("b/src/main/scala/b/B.scala")(
_.replace("2", "\"\"")
)
_ <- server.didSave("b/src/main/scala/b/B.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
xMismatch,
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(
_ <- server.didChange("a/src/main/scala/a/A.scala")(
_.replace("Int", "String")
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|b/src/main/scala/b/B.scala:3:16: error: type mismatch;
Expand Down
6 changes: 4 additions & 2 deletions tests/unit/src/test/scala/tests/FingerprintsLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ class FingerprintsLspSuite extends BaseLspSuite("fingerprints") {
_ <- server.executeCommand(ServerCommands.CascadeCompile)
_ = assertNoDiff(client.workspaceDiagnostics, "")
_ = server.assertReferenceDefinitionBijection()
_ <- server.didSave("a/src/main/scala/a/Names.scala")(text =>
_ <- server.didChange("a/src/main/scala/a/Names.scala")(text =>
text.replace("+ surname", "+ surname2")
)
_ <- server.didSave("a/src/main/scala/a/Adresses.scala")(text =>
_ <- server.didSave("a/src/main/scala/a/Names.scala")(identity)
_ <- server.didChange("a/src/main/scala/a/Adresses.scala")(text =>
text.replace("+ number", "+ number2")
)
_ <- server.didSave("a/src/main/scala/a/Adresses.scala")(identity)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/Adresses.scala:5:44: error: not found: value number2
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/src/test/scala/tests/TreeViewLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class TreeViewLspSuite extends BaseLspSuite("tree-view") {
|c symbol-variable
|""".stripMargin,
)
_ <- server.didSave("a/src/main/scala/a/Zero.scala") { text =>
_ <- server.didChange("a/src/main/scala/a/Zero.scala") { text =>
text.replace("val a = 1", "val a = 1\nval b = 1.0")
}
_ = assertEquals(
Expand All @@ -191,7 +191,7 @@ class TreeViewLspSuite extends BaseLspSuite("tree-view") {
s"projects-$folder:${server.buildTarget("a")}!/_root_/",
isCollapsed = true,
)
_ <- server.didSave("a/src/main/scala/a/Zero.scala") { text =>
_ <- server.didChange("a/src/main/scala/a/Zero.scala") { text =>
text.replace("val a = 1", "val a = 1\nval c = 1.0")
}
_ = assertEmpty(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ class BestEffortCompilationSuite
|""".stripMargin
}
_ <- server.didOpen("a/src/main/scala/a/A.scala")
_ <- server.didOpen("b/src/main/scala/b/B.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/A.scala:5:3: error: value completionUnknown is not a member of object B
Expand Down
Loading