From 57f98dcc884605ca83ec59b84d440090cf446040 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Wed, 11 Oct 2023 15:55:49 +0200 Subject: [PATCH 1/2] improvement: Only reload workspace on onBuildTargetChanged Previously, we would always reconnect to build server, which is not needed and takes longer. Instead now we only reload and reindex the workspace. --- .../meta/internal/builds/BuildTool.scala | 2 + .../meta/internal/metals/Compilations.scala | 4 +- .../internal/metals/MetalsLspService.scala | 95 +++++++++++-------- .../scala/tests/gradle/GradleLspSuite.scala | 7 +- .../scala/tests/scalacli/ScalaCliSuite.scala | 36 +++++++ .../scala/tests/BspBuildChangedSuite.scala | 61 ------------ 6 files changed, 99 insertions(+), 106 deletions(-) delete mode 100644 tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala diff --git a/metals/src/main/scala/scala/meta/internal/builds/BuildTool.scala b/metals/src/main/scala/scala/meta/internal/builds/BuildTool.scala index 7420900501f..5724e745a19 100644 --- a/metals/src/main/scala/scala/meta/internal/builds/BuildTool.scala +++ b/metals/src/main/scala/scala/meta/internal/builds/BuildTool.scala @@ -47,6 +47,8 @@ trait BuildTool { } object BuildTool { + + case class Found(buildTool: BuildTool, digest: String) def copyFromResource( tempDir: Path, filePath: String, diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index 07f7ba8cdf4..5889eaad22b 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -21,7 +21,7 @@ final class Compilations( workspace: () => AbsolutePath, languageClient: MetalsLanguageClient, refreshTestSuites: () => Unit, - afterCompilation: () => Unit, + afterSuccesfulCompilation: () => Unit, isCurrentlyFocused: b.BuildTargetIdentifier => Boolean, compileWorksheets: Seq[AbsolutePath] => Future[Unit], onStartCompilation: () => Unit, @@ -226,7 +226,7 @@ final class Compilations( val result = compilation.asScala .andThen { case result => updateCompiledTargetState(result) - afterCompilation() + afterSuccesfulCompilation() // See https://github.com/scalacenter/bloop/issues/1067 classes.rebuildIndex( diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index ef2c828080e..b94ea533f04 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -401,7 +401,7 @@ class MetalsLspService( worksheetProvider.onBuildTargetDidCompile(target) }, onBuildTargetDidChangeFunc = params => { - maybeQuickConnectToBuildServer(params) + onBuildTargetChanges(params) }, bspErrorHandler, ) @@ -1966,11 +1966,19 @@ class MetalsLspService( buildTool.version, ) - def supportedBuildTool(): Future[Option[BuildTool]] = { + def supportedBuildTool(): Future[Option[BuildTool.Found]] = { def isCompatibleVersion(buildTool: BuildTool) = { val isCompatibleVersion = this.isCompatibleVersion(buildTool) if (isCompatibleVersion) { - Some(buildTool) + buildTool.digest(folder) match { + case Some(digest) => + Some(BuildTool.Found(buildTool, digest)) + case None => + scribe.warn( + s"Could not calculate checksum for ${buildTool.executableName} in $folder" + ) + None + } } else { scribe.warn(s"Unsupported $buildTool version ${buildTool.version}") languageClient.showMessage( @@ -1991,10 +1999,12 @@ class MetalsLspService( } case buildTools => for { - Some(buildTool) <- buildToolSelector.checkForChosenBuildTool( + buildTool <- buildToolSelector.checkForChosenBuildTool( buildTools ) - } yield isCompatibleVersion(buildTool) + } yield { + buildTool.flatMap(isCompatibleVersion) + } } } @@ -2008,38 +2018,32 @@ class MetalsLspService( _ == BloopServers.name ) buildChange <- possibleBuildTool match { - case Some(buildTool) => - (buildTool.digest(folder), buildTool) match { - case (None, _) => - scribe.warn(s"Skipping build import, no checksum.") - Future.successful(BuildChange.None) - case (Some(_), buildTool: ScalaCliBuildTool) - if chosenBuildServer.isEmpty => - tables.buildServers.chooseServer(ScalaCliBuildTool.name) - val scalaCliBspConfigExists = - ScalaCliBuildTool.pathsToScalaCliBsp(folder).exists(_.isFile) - if (scalaCliBspConfigExists) Future.successful(BuildChange.None) - else - buildTool - .generateBspConfig( - folder, - args => - bspConfigGenerator.runUnconditionally(buildTool, args), - statusBar, - ) - .flatMap(_ => quickConnectToBuildServer()) - case (Some(digest), _) if isBloopOrEmpty => - slowConnectToBloopServer(forceImport, buildTool, digest) - case (Some(digest), _) => - indexer.reloadWorkspaceAndIndex( - forceImport, - buildTool, - digest, - importBuild, + case Some(BuildTool.Found(buildTool: ScalaCliBuildTool, _)) + if chosenBuildServer.isEmpty => + tables.buildServers.chooseServer(ScalaCliBuildTool.name) + val scalaCliBspConfigExists = + ScalaCliBuildTool.pathsToScalaCliBsp(folder).exists(_.isFile) + if (scalaCliBspConfigExists) Future.successful(BuildChange.None) + else + buildTool + .generateBspConfig( + folder, + args => bspConfigGenerator.runUnconditionally(buildTool, args), + statusBar, ) - } + .flatMap(_ => quickConnectToBuildServer()) + case Some(found) if isBloopOrEmpty => + slowConnectToBloopServer(forceImport, found.buildTool, found.digest) + case Some(found) => + indexer.reloadWorkspaceAndIndex( + forceImport, + found.buildTool, + found.digest, + importBuild, + ) case None => Future.successful(BuildChange.None) + } } yield buildChange @@ -2114,9 +2118,11 @@ class MetalsLspService( change } - private def maybeQuickConnectToBuildServer( + private def onBuildTargetChanges( params: b.DidChangeBuildTarget ): Unit = { + // Make sure that no compilation is running, if it is it might not get completed properly + compilations.cancel() val (ammoniteChanges, otherChanges) = params.getChanges.asScala.partition { change => val connOpt = buildTargets.buildServerOf(change.getTarget) @@ -2145,13 +2151,20 @@ class MetalsLspService( .error("Error re-importing Scala CLI build", exception) } - if (otherChanges0.nonEmpty) - quickConnectToBuildServer().onComplete { - case Failure(e) => - scribe.warn("Error refreshing build", e) - case Success(_) => - scribe.info("Refreshed build after change") + if (otherChanges0.nonEmpty) { + bspSession match { + case None => scribe.warn("No build server connected") + case Some(session) => + for { + _ <- importBuild(session) + _ <- indexer.profiledIndexWorkspace(runDoctorCheck) + } { + focusedDocument().foreach(path => compilations.compileFile(path)) + compilers.cancel() + } } + + } } def autoConnectToBuildServer(): Future[BuildChange] = { diff --git a/tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala b/tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala index 22cdf49e0f9..0b0bddf9160 100644 --- a/tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala +++ b/tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala @@ -96,8 +96,11 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") { _ <- server.server.indexingPromise.future _ = assert(server.server.bspSession.get.main.isBloop) buildTool <- server.server.supportedBuildTool() - _ = assertEquals(buildTool.get.executableName, "gradle") - _ = assertEquals(buildTool.get.projectRoot, workspace.resolve("inner")) + _ = assertEquals(buildTool.get.buildTool.executableName, "gradle") + _ = assertEquals( + buildTool.get.buildTool.projectRoot, + workspace.resolve("inner"), + ) _ <- server.didOpen("inner/src/main/scala/A.scala") _ <- server.didSave("inner/src/main/scala/A.scala")(identity) _ = assertNoDiff( diff --git a/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala b/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala index 6b8adcc4104..93e7f645873 100644 --- a/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala +++ b/tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala @@ -371,6 +371,42 @@ class ScalaCliSuite extends BaseScalaCliSuite(V.scala3) { } yield () } + test("properly-reindex") { + cleanWorkspace() + server.client.importBuild = Messages.ImportBuild.yes + for { + _ <- scalaCliInitialize(useBsp = true)( + s"""|/Main.scala + |//> using scala 2.13.11 + |// > using toolkit latest + | + |object Main { + | println(os.pwd) + |} + | + |""".stripMargin + ) + _ <- server.server.buildServerPromise.future + _ <- server.didOpen("Main.scala") + _ = assertEquals( + server.client.workspaceDiagnostics, + """|Main.scala:5:13: error: not found: value os + | println(os.pwd) + | ^^ + |""".stripMargin, + ) + _ <- server.didSave("Main.scala") { text => + text.replace("// >", "//>") + } + // cause another compilation to wait on workspace reload, the previous gets cancelled + _ <- server.didSave("Main.scala")(identity) + _ = assertEquals( + server.client.workspaceDiagnostics, + "", + ) + } yield () + } + test("single-file-config") { cleanWorkspace() val msg = FileOutOfScalaCliBspScope.askToRegenerateConfigAndRestartBspMsg( diff --git a/tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala b/tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala deleted file mode 100644 index fca8f121bb4..00000000000 --- a/tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala +++ /dev/null @@ -1,61 +0,0 @@ -package tests - -import scala.concurrent.Promise - -import scala.meta.internal.metals.MetalsEnrichments._ - -import bill.Bill - -class BspBuildChangedSuite extends BaseLspSuite("bsp-build-changed") { - - test("build changed") { - cleanWorkspace() - Bill.installWorkspace(workspace.toNIO) - for { - _ <- initialize( - """ - |/src/com/App.scala - |object App { - | val x: Int = "" - |} - |/shutdown-trace - |true - """.stripMargin - ) - _ <- server.didOpen("src/com/App.scala") - _ = assertNoDiff( - client.workspaceDiagnostics, - """ - |src/com/App.scala:2:16: error: type mismatch; - | found : String("") - | required: Int - | val x: Int = "" - | ^^ - """.stripMargin, - ) - _ = { - server.server.buildServerPromise = Promise() - } - // workspaceReload on Bill triggers a buildTarget/didChange, - // which should trigger a build server disconnection / reconnection - _ <- server.server.bspSession.get.workspaceReload() - _ <- server.server.buildServerPromise.future - _ = { - val logs = workspace - .resolve(Bill.logName) - .readText - .linesIterator - .filter(_.startsWith("trace:")) - .mkString("\n") - assertNoDiff( - logs, - """|trace: initialize - |trace: shutdown - |trace: initialize - |""".stripMargin, - ) - } - } yield () - } - -} From 74082aae41fed57ad9e2ddf4b5f3fdee2c82f482 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Thu, 12 Oct 2023 17:14:06 +0200 Subject: [PATCH 2/2] bugfix: Move Compilers.cancel() to the end of importBuild function --- .../scala/scala/meta/internal/metals/MetalsLspService.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index b94ea533f04..8e920d729a1 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -2160,7 +2160,6 @@ class MetalsLspService( _ <- indexer.profiledIndexWorkspace(runDoctorCheck) } { focusedDocument().foreach(path => compilations.compileFile(path)) - compilers.cancel() } } @@ -2251,7 +2250,6 @@ class MetalsLspService( } private def importBuild(session: BspSession) = { - compilers.cancel() val importedBuilds0 = timerProvider.timed("Imported build") { session.importBuilds() } @@ -2265,7 +2263,7 @@ class MetalsLspService( } mainBuildTargetsData.resetConnections(idToConnection) } - } yield () + } yield compilers.cancel() } private def connectToNewBuildServer(