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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 8, 2025
Merged

improvement: Don't sent text on didSave #7015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c0310e
improvement: Don't sent text on didSave
tgodzik Dec 9, 2024
39c1ac1
bugfix: Make sure everything uses didChange first
tgodzik Dec 10, 2024
848a8a3
refactor: Remove fn from didSave, force didChange before to simulate …
tgodzik Dec 23, 2024
33df8e9
chore: Fix tests after changes
tgodzik Dec 27, 2024
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
3 changes: 0 additions & 3 deletions metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,9 +872,6 @@ 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)
Future
.sequence(
List(
Expand Down
5 changes: 4 additions & 1 deletion metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala
View file
Open in desktop
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
30 changes: 17 additions & 13 deletions tests/slow/src/test/scala/tests/bazel/BazelLspSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ class BazelLspSuite
_ = assertStatus(_.isInstalled)
_ = assertNoDiff(client.workspaceDiagnostics, "")
_ <- server.didChange("WORKSPACE")(_ + "\n# comment")
_ <- server.didSave("WORKSPACE")(identity)
_ <- server.didSave("WORKSPACE")
// Comment changes do not trigger "re-import project" request
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didChange("Hello.scala") { text =>
text.replace("def hello: String", "def hello: Int")
}
_ <- server.didSave("Hello.scala")(identity)
_ <- server.didSave("Hello.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|Hello.scala:4:20: error: type mismatch;
Expand All @@ -78,7 +78,7 @@ class BazelLspSuite
}
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ = client.generateBspAndConnect = GenerateBspAndConnect.yes
_ <- server.didSave(s"BUILD")(identity)
_ <- server.didSave(s"BUILD")
} yield {
assertNoDiff(
client.workspaceMessageRequests,
Expand All @@ -105,7 +105,7 @@ class BazelLspSuite
_ <- server.didChange(s"BUILD") { text =>
text.replace("\"hello\"", "\"hello1\"")
}
_ <- server.didSave(s"BUILD")(identity)
_ <- server.didSave(s"BUILD")
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ = server.headServer.connectionProvider.buildServerPromise = Promise()
_ <- server.executeCommand(ServerCommands.GenerateBspConfig)
Expand All @@ -114,7 +114,7 @@ class BazelLspSuite
_ <- server.didChange("Hello.scala") { text =>
text.replace("def hello: String", "def hello: Int")
}
_ <- server.didSave("Hello.scala")(identity)
_ <- server.didSave("Hello.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|Hello.scala:4:20: error: type mismatch;
Expand Down Expand Up @@ -158,7 +158,7 @@ class BazelLspSuite
_ <- server.didChange(s"BUILD") { text =>
text.replace("\"hello\"", "\"hello1\"")
}
_ <- server.didSave(s"BUILD")(identity)
_ <- server.didSave(s"BUILD")
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ = server.headServer.connectionProvider.buildServerPromise = Promise()
_ <- server.executeCommand(ServerCommands.ImportBuild)
Expand Down Expand Up @@ -209,7 +209,7 @@ class BazelLspSuite
)
_ <- server.didOpen("Hello.scala")
_ <- server.didOpen("Main.scala")
_ <- server.didSave("Main.scala")(identity)
_ <- server.didSave("Main.scala")
references <- server.references("Hello.scala", "hello")
_ = assertNoDiff(
references,
Expand All @@ -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")
_ = 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")
_ = assertNoDiff(
server.client.workspaceDiagnostics,
"""|Hello.scala:11:3: warning: match may not be exhaustive.
Expand Down Expand Up @@ -313,7 +315,7 @@ class BazelLspSuite
_ <- server.didChange("Hello.scala") { text =>
text.replace("def hello: String", "def hello: Int")
}
_ <- server.didSave("Hello.scala")(identity)
_ <- server.didSave("Hello.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|Hello.scala:4:20: error: type mismatch;
Expand Down Expand Up @@ -351,7 +353,7 @@ class BazelLspSuite
_ <- server.didChange("Hello.scala") { text =>
text.replace("def hello: String", "def hello: Int")
}
_ <- server.didSave("Hello.scala")(identity)
_ <- server.didSave("Hello.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|Hello.scala:4:20: error: type mismatch;
Expand All @@ -365,12 +367,14 @@ class BazelLspSuite
)
_ = client.messageRequests.clear()
_ <- server.didOpen("Hello.scala")
_ <- server.didSave("Hello.scala") { text =>
_ <- server.didChange("Hello.scala") { text =>
text.replace("def hello: Int", "def hello: String")
}
_ <- server.didSave("Hello.scala")
_ = assertNoDiagnostics()
_ <- server.didOpen("projectview.bazelproject")
_ <- server.didSave("projectview.bazelproject")(_ => "")
_ <- server.didChange("projectview.bazelproject")(_ => "")
_ <- server.didSave("projectview.bazelproject")
_ = assertNoDiff(
client.workspaceMessageRequests,
ImportBuildChanges.params("bazel").getMessage(),
Expand Down
4 changes: 2 additions & 2 deletions tests/slow/src/test/scala/tests/feature/AmmoniteSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class Ammonite213Suite extends tests.BaseAmmoniteSuite(V.ammonite213) {
|""".stripMargin
)
_ <- server.didOpen("main.sc")
_ <- server.didSave("main.sc")(identity)
_ <- server.didSave("main.sc")
_ <- server.executeCommand(ServerCommands.StartAmmoniteBuildServer)

groupCompletionList <- server.completion(
Expand Down Expand Up @@ -52,7 +52,7 @@ class Ammonite212Suite extends tests.BaseAmmoniteSuite(V.ammonite212) {
|""".stripMargin
)
_ <- server.didOpen("main.sc")
_ <- server.didSave("main.sc")(identity)
_ <- server.didSave("main.sc")
_ <- server.executeCommand(ServerCommands.StartAmmoniteBuildServer)
} yield {
assertEmpty(client.workspaceErrorShowMessages)
Expand Down
2 changes: 1 addition & 1 deletion tests/slow/src/test/scala/tests/feature/MultipleBuildFilesLspSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class MultipleBuildFilesLspSuite
text + "\nversion := \"1.0.0\"\n"
}
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didSave("build.sbt")(identity)
_ <- server.didSave("build.sbt")
} yield {
assertNoDiff(
client.workspaceMessageRequests,
Expand Down
6 changes: 4 additions & 2 deletions tests/slow/src/test/scala/tests/feature/SyntaxErrorLspSuite.scala
View file
Open in desktop
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")
_ = 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")
_ = assertNoDiff(client.workspaceDiagnostics, "")
} yield ()
}
Expand Down
23 changes: 13 additions & 10 deletions tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
_ = assertStatus(_.isInstalled)
_ <- server.didChange("build.gradle")(_ + "\n// comment")
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didSave("build.gradle")(identity)
_ <- server.didSave("build.gradle")
// Comment changes do not trigger "re-import project" request
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didChange("build.gradle") { text =>
text + "\ndef version = \"1.0.0\"\n"
}
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ = client.importBuildChanges = ImportBuildChanges.yes
_ <- server.didSave("build.gradle")(identity)
_ <- server.didSave("build.gradle")
} yield {
assertNoDiff(
client.workspaceMessageRequests,
Expand Down Expand Up @@ -97,7 +97,7 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
workspace.resolve("inner"),
)
_ <- server.didOpen("inner/src/main/scala/A.scala")
_ <- server.didSave("inner/src/main/scala/A.scala")(identity)
_ <- server.didSave("inner/src/main/scala/A.scala")
_ = assertNoDiff(
client.pathDiagnostics("inner/src/main/scala/A.scala"),
"""|inner/src/main/scala/A.scala:2:18: error: type mismatch;
Expand Down Expand Up @@ -148,14 +148,14 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
_ = assertStatus(_.isInstalled)
_ <- server.didChange("build.gradle")(_ + "\n// comment")
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didSave("build.gradle")(identity)
_ <- server.didSave("build.gradle")
// Comment changes do not trigger "re-import project" request
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didChange("build.gradle") { text =>
text + "\ndef version = \"1.0.0\"\n"
}
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didSave("build.gradle")(identity)
_ <- server.didSave("build.gradle")
} yield {
assertNoDiff(
client.workspaceMessageRequests,
Expand Down Expand Up @@ -196,7 +196,7 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
)
_ = client.messageRequests.clear()
_ = assertStatus(_.isInstalled)
_ <- server.didSave("src/main/java/a/Main.java")(identity)
_ <- server.didSave("src/main/java/a/Main.java")
debugger <- server.startDebugging(
javaOnlyTestName,
DebugSessionParamsDataKind.SCALA_MAIN_CLASS,
Expand Down Expand Up @@ -301,19 +301,21 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
_ <- server.didOpen("src/main/scala/reload/Main.scala")
_ = assertNoDiff(client.workspaceDiagnostics, "")
_ = client.importBuildChanges = ImportBuildChanges.yes
_ <- server.didSave("build.gradle") { text =>
_ <- server.didChange("build.gradle") { text =>
s"""$text
|dependencies {
| implementation 'com.lihaoyi:sourcecode_2.12:0.1.4'
|}
|""".stripMargin
}
_ <- server.didSave("build.gradle")
_ <-
server
.didSave("src/main/scala/reload/Main.scala") { text =>
.didChange("src/main/scala/reload/Main.scala") { text =>
text.replaceAll("\"", "")
}
.recover { case e => scribe.error("compile", e) }
_ <- server.didSave("src/main/scala/reload/Main.scala")
_ = assertNoDiff(client.workspaceDiagnostics, "")
} yield ()
}
Expand All @@ -339,7 +341,7 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
)
_ = assertStatus(!_.isInstalled)
_ = client.messageRequests.clear()
_ <- server.didSave("build.gradle") { _ =>
_ <- server.didChange("build.gradle") { _ =>
s"""|plugins {
| id 'scala'
|}
Expand All @@ -351,6 +353,7 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
|}
|""".stripMargin
}
_ <- server.didSave("build.gradle")
_ = assertNoDiff(
client.workspaceMessageRequests,
importBuildMessage,
Expand Down Expand Up @@ -528,7 +531,7 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
"rootProject.name = 'new-name'\n"
)
_ = client.importBuildChanges = ImportBuildChanges.yes
_ <- server.didSave("inner/settings.gradle")(identity)
_ <- server.didSave("inner/settings.gradle")
_ = assert(
server.server.buildTargets.all
.exists(_.getDisplayName() == "new-name"),
Expand Down
15 changes: 10 additions & 5 deletions tests/slow/src/test/scala/tests/maven/MavenLspSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ class MavenLspSuite extends BaseImportSuite("maven-import") {
.replace("<!--URL-->", "<!--Your comment--><!--MARKER-->")
)
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didSave("pom.xml")(identity)
_ <- server.didSave("pom.xml")
// Comment changes do not trigger "re-import project" request
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ <- server.didChange("pom.xml") { _ =>
defaultPom.replace("<!--URL-->", "http://www.example.com")
}
_ = assertNoDiff(client.workspaceMessageRequests, "")
_ = client.importBuildChanges = ImportBuildChanges.yes
_ <- server.didSave("pom.xml")(identity)
_ <- server.didSave("pom.xml")
} yield {
assertNoDiff(
client.workspaceMessageRequests,
Expand Down Expand Up @@ -146,7 +146,7 @@ class MavenLspSuite extends BaseImportSuite("maven-import") {
_ <- server.didOpen("src/main/scala/reload/Main.scala")
_ = assertNoDiff(client.workspaceDiagnostics, "")
_ = client.importBuildChanges = ImportBuildChanges.yes
_ <- server.didSave("pom.xml") { text =>
_ <- server.didChange("pom.xml") { text =>
text.replace(
"<!--DEPENDENCY-->",
s"""
Expand All @@ -158,12 +158,16 @@ class MavenLspSuite extends BaseImportSuite("maven-import") {
|""".stripMargin,
)
}
_ <- server.didSave("pom.xml")
_ <-
server
.didSave("src/main/scala/reload/Main.scala") { text =>
.didChange("src/main/scala/reload/Main.scala") { text =>
text.replaceAll("\"", "")
}
.recover { case e => scribe.error("compile", e) }
_ <-
server
.didSave("src/main/scala/reload/Main.scala")
_ = assertNoDiff(client.workspaceDiagnostics, "")
} yield ()
}
Expand Down Expand Up @@ -197,7 +201,8 @@ class MavenLspSuite extends BaseImportSuite("maven-import") {
)
_ = assertStatus(!_.isInstalled)
_ = client.messageRequests.clear()
_ <- server.didSave("pom.xml")(_ => defaultPom)
_ <- server.didChange("pom.xml")(_ => defaultPom)
_ <- server.didSave("pom.xml")
_ = assertNoDiff(
client.workspaceMessageRequests,
importBuildMessage,
Expand Down
10 changes: 5 additions & 5 deletions tests/slow/src/test/scala/tests/mill/MillDebugDiscoverySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ class MillDebugDiscoverySuite
)
)
_ <- server.didOpen("a/src/Main.scala")
_ <- server.didSave("a/src/Main.scala")(
identity
_ <- server.didSave(
"a/src/Main.scala"
) // making sure it gets compiled to the correct destination
_ <- server.didOpen(barPath)
_ <- server.didSave(barPath)(identity)
_ <- server.didSave(barPath)
_ <- server.waitFor(TimeUnit.SECONDS.toMillis(10))
debugger <- server.startDebuggingUnresolved(
new DebugDiscoveryParams(
Expand Down Expand Up @@ -108,7 +108,7 @@ class MillDebugDiscoverySuite
)
)
_ <- server.didOpen(fooPath)
_ <- server.didSave(fooPath)(identity)
_ <- server.didSave(fooPath)
_ <- server.waitFor(TimeUnit.SECONDS.toMillis(10))
debugger <- server.startDebugging(
"a.test",
Expand Down Expand Up @@ -148,7 +148,7 @@ class MillDebugDiscoverySuite
)
)
_ <- server.didOpen(fooPath)
_ <- server.didSave(fooPath)(identity)
_ <- server.didSave(fooPath)
_ <- server.waitFor(TimeUnit.SECONDS.toMillis(10))
debugger <- server.startDebugging(
"a.test",
Expand Down
Loading
Loading