From eefae0ac90ff2423f00a4d8eff321ab90f300381 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Wed, 11 Oct 2023 15:55:49 +0200 Subject: [PATCH] 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 | 103 +++++++++++------- .../scala/tests/gradle/GradleLspSuite.scala | 7 +- 4 files changed, 71 insertions(+), 45 deletions(-) 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..df9072eab95 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,28 @@ 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) { + for { + foundBuildTool <- supportedBuildTool() + } { + foundBuildTool.foreach { found => + indexer + .reloadWorkspaceAndIndex( + forceRefresh = true, + found.buildTool, + found.digest, + importBuild, + ) + .foreach { reloaded => + if (reloaded.isReconnected || reloaded.isReloaded) + focusedDocument().foreach(path => + compilations.compileFile(path) + ) + } + } } + + } } 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(