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.

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,

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(

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

@@ -411,7 +411,7 @@ class MetalsLspService(
         worksheetProvider.onBuildTargetDidCompile(target)
       },
       onBuildTargetDidChangeFunc = params => {
-        maybeQuickConnectToBuildServer(params)
+        onBuildTargetChanges(params)
       },
       bspErrorHandler,
     )
@@ -1984,11 +1984,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(
@@ -2009,10 +2017,12 @@ class MetalsLspService(
       }
       case buildTools =>
         for {
-          Some(buildTool) <- buildToolSelector.checkForChosenBuildTool(
+          buildTool <- buildToolSelector.checkForChosenBuildTool(
             buildTools
           )
-        } yield isCompatibleVersion(buildTool)
+        } yield {
+          buildTool.flatMap(isCompatibleVersion)
+        }
     }
   }
 
@@ -2026,38 +2036,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
 
@@ -2132,9 +2136,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)
@@ -2163,13 +2169,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] = {

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(

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(