Make BSP requests robust to some target failures

The request of the form buildTarget/* often take a sequence of build
targets as parameter. So far if there is an error on a single build
target, the entire request fails.
This is not the best because the client wants the result of the other
build targets anyway:
For example:
- workspace/buildTargets: if one build target has an invalid Scala
version we still want to import the other ones
- buildTarget/scalacOptions: if a dependency cannot be resolved we still
want to import the build targets that do not depend on it
- buildTarget/scalaMainClasses: if buildTarget does not compile we still
want the main classes of the other targets
...

The change is to respond to BSP requests with the successful
build targets and  to ignore the failed ones.
This is implemented the same in Bloop since before BSP in sbt.

In  https://github.com/build-server-protocol/build-server-protocol/issues/204,
I made a proposal to also add the failed build targets in the response.

main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala

@@ -90,16 +90,16 @@ object BuildServerProtocol {
       val workspace = Keys.bspFullWorkspace.value
       val state = Keys.state.value
       val allTargets = ScopeFilter.in(workspace.scopes.values.toSeq)
-      val sbtTargets: List[Def.Initialize[Task[BuildTarget]]] = workspace.builds.map {
+      val sbtTargets = workspace.builds.map {
         case (buildTargetIdentifier, loadedBuildUnit) =>
           val buildFor = workspace.buildToScope.getOrElse(buildTargetIdentifier, Nil)
-          sbtBuildTarget(loadedBuildUnit, buildTargetIdentifier, buildFor)
+          sbtBuildTarget(loadedBuildUnit, buildTargetIdentifier, buildFor).result
       }.toList
       Def.task {
-        val buildTargets = Keys.bspBuildTarget.all(allTargets).value.toVector
+        val buildTargets = Keys.bspBuildTarget.result.all(allTargets).value
-        val allBuildTargets = buildTargets ++ sbtTargets.join.value
+        val successfulBuildTargets = anyOrThrow(buildTargets ++ sbtTargets.join.value)
-        state.respondEvent(WorkspaceBuildTargetsResult(allBuildTargets))
+        state.respondEvent(WorkspaceBuildTargetsResult(successfulBuildTargets.toVector))
-        allBuildTargets
+        successfulBuildTargets
       }
     }.value,
     // https://github.com/build-server-protocol/build-server-protocol/blob/master/docs/specification.md#build-target-sources-request
@@ -110,8 +110,8 @@ object BuildServerProtocol {
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       // run the worker task concurrently
       Def.task {
-        val items = bspBuildTargetSourcesItem.all(filter).value
+        val items = bspBuildTargetSourcesItem.result.all(filter).value
-        val buildItems = workspace.builds.toVector.map {
+        val buildItems = workspace.builds.map {
           case (id, loadedBuildUnit) =>
             val base = loadedBuildUnit.localBase
             val sbtFiles = configurationSources(base)
@@ -130,9 +130,10 @@ object BuildServerProtocol {
             add(pluginData.managedSourceDirectories, SourceItemKind.Directory, generated = true)
             add(pluginData.managedSources, SourceItemKind.File, generated = true)
             add(sbtFiles, SourceItemKind.File, generated = false)
-            SourcesItem(id, all.result())
+            Value(SourcesItem(id, all.result()))
         }
-        val result = SourcesResult((items ++ buildItems).toVector)
+        val successfulItems = anyOrThrow(items ++ buildItems)
+        val result = SourcesResult(successfulItems.toVector)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -145,8 +146,9 @@ object BuildServerProtocol {
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       // run the worker task concurrently
       Def.task {
-        val items = bspBuildTargetResourcesItem.all(filter).value
+        val items = bspBuildTargetResourcesItem.result.all(filter).value
-        val result = ResourcesResult(items.toVector)
+        val successfulItems = anyOrThrow(items)
+        val result = ResourcesResult(successfulItems.toVector)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -159,8 +161,9 @@ object BuildServerProtocol {
       // run the worker task concurrently
       Def.task {
         import sbt.internal.bsp.codec.JsonProtocol._
-        val items = bspBuildTargetDependencySourcesItem.all(filter).value
+        val items = bspBuildTargetDependencySourcesItem.result.all(filter).value
-        val result = DependencySourcesResult(items.toVector)
+        val successfulItems = anyOrThrow(items)
+        val result = DependencySourcesResult(successfulItems.toVector)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -172,8 +175,12 @@ object BuildServerProtocol {
       workspace.warnIfBuildsNonEmpty(Method.Compile, s.log)
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       Def.task {
-        val statusCode = Keys.bspBuildTargetCompileItem.all(filter).value.max
+        val statusCodes = Keys.bspBuildTargetCompileItem.result.all(filter).value
-        s.respondEvent(BspCompileResult(None, statusCode))
+        val aggregatedStatusCode = allOrThrow(statusCodes) match {
+          case Seq() => StatusCode.Success
+          case codes => codes.max
+        }
+        s.respondEvent(BspCompileResult(None, aggregatedStatusCode))
       }
     }.evaluated,
     bspBuildTargetCompile / aggregate := false,
@@ -188,7 +195,7 @@ object BuildServerProtocol {
 
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       Def.task {
-        val items = bspBuildTargetScalacOptionsItem.all(filter).value
+        val items = bspBuildTargetScalacOptionsItem.result.all(filter).value
         val appProvider = appConfiguration.value.provider()
         val sbtJars = appProvider.mainClasspath()
         val buildItems = builds.map {
@@ -197,14 +204,16 @@ object BuildServerProtocol {
             val scalacOptions = plugins.pluginData.scalacOptions
             val pluginClassPath = plugins.classpath
             val classpath = (pluginClassPath ++ sbtJars).map(_.toURI).toVector
-            ScalacOptionsItem(
+            val item = ScalacOptionsItem(
               build._1,
               scalacOptions.toVector,
               classpath,
               new File(build._2.localBase, "project/target").toURI
             )
+            Value(item)
         }
-        val result = ScalacOptionsResult((items ++ buildItems).toVector)
+        val successfulItems = anyOrThrow(items ++ buildItems)
+        val result = ScalacOptionsResult(successfulItems.toVector)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -216,8 +225,9 @@ object BuildServerProtocol {
       workspace.warnIfBuildsNonEmpty(Method.ScalaTestClasses, s.log)
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       Def.task {
-        val items = bspScalaTestClassesItem.all(filter).value
+        val items = bspScalaTestClassesItem.result.all(filter).value
-        val result = ScalaTestClassesResult(items.toVector, None)
+        val successfulItems = anyOrThrow(items)
+        val result = ScalaTestClassesResult(successfulItems.toVector, None)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -228,8 +238,9 @@ object BuildServerProtocol {
       workspace.warnIfBuildsNonEmpty(Method.ScalaMainClasses, s.log)
       val filter = ScopeFilter.in(workspace.scopes.values.toList)
       Def.task {
-        val items = bspScalaMainClassesItem.all(filter).value
+        val items = bspScalaMainClassesItem.result.all(filter).value
-        val result = ScalaMainClassesResult(items.toVector, None)
+        val successfulItems = anyOrThrow(items)
+        val result = ScalaMainClassesResult(successfulItems.toVector, None)
         s.respondEvent(result)
       }
     }.evaluated,
@@ -846,6 +857,20 @@ object BuildServerProtocol {
       case _ => sys.error(s"unexpected $ref")
     }
 
+  private def anyOrThrow[T](results: Seq[Result[T]]): Seq[T] = {
+    val successes = results.collect { case Value(v) => v }
+    val errors = results.collect { case Inc(cause)  => cause }
+    if (successes.nonEmpty || errors.isEmpty) successes
+    else throw Incomplete(None, causes = errors)
+  }
+
+  private def allOrThrow[T](results: Seq[Result[T]]): Seq[T] = {
+    val successes = results.collect { case Value(v) => v }
+    val errors = results.collect { case Inc(cause)  => cause }
+    if (errors.isEmpty) successes
+    else throw Incomplete(None, causes = errors)
+  }
+
   private case class SemanticVersion(major: Int, minor: Int) extends Ordered[SemanticVersion] {
     override def compare(that: SemanticVersion): Int = {
       if (that.major != major) major.compare(that.major)

server-test/src/server-test/buildserver/build.sbt

@@ -25,3 +25,18 @@ lazy val respondError = project.in(file("respond-error"))
   )
 
 lazy val util = project
+
+def somethingBad = throw new MessageOnlyException("I am a bad build target")
+// other build targets should not be affected by this bad build target
+lazy val badBuildTarget = project.in(file("bad-build-target"))
+  .settings(
+    Compile / bspBuildTarget := somethingBad,
+    Compile / bspBuildTargetSourcesItem := somethingBad,
+    Compile / bspBuildTargetResourcesItem := somethingBad,
+    Compile / bspBuildTargetDependencySourcesItem := somethingBad,
+    Compile / bspBuildTargetScalacOptionsItem := somethingBad,
+    Compile / bspBuildTargetCompileItem := somethingBad,
+    Compile / bspScalaMainClasses := somethingBad,
+    Test / bspBuildTarget := somethingBad,
+    Test / bspScalaTestClasses := somethingBad,
+  )

server-test/src/test/scala/testpkg/BuildServerTest.scala

@@ -7,8 +7,7 @@
 
 package testpkg
 
-import sbt.internal.bsp.SourcesResult
+import sbt.internal.bsp.{ BspCompileResult, SourcesResult, StatusCode, WorkspaceBuildTargetsResult }
-import sbt.internal.bsp.WorkspaceBuildTargetsResult
 import sbt.internal.langserver.ErrorCodes
 import sbt.IO
 
@@ -41,13 +40,15 @@ object BuildServerTest extends AbstractServerTest {
     val buildServerBuildTarget =
       result.targets.find(_.displayName.contains("buildserver-build")).get
     assert(buildServerBuildTarget.id.uri.toString.endsWith("#buildserver-build"))
+    assert(!result.targets.exists(_.displayName.contains("badBuildTarget")))
   }
 
   test("buildTarget/sources") { _ =>
     val buildTarget = buildTargetUri("util", "Compile")
+    val badBuildTarget = buildTargetUri("badBuildTarget", "Compile")
     svr.sendJsonRpc(
       s"""{ "jsonrpc": "2.0", "id": "24", "method": "buildTarget/sources", "params": {
-        |  "targets": [{ "uri": "$buildTarget" }]
+        |  "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }]
         |} }""".stripMargin
     )
     assert(processing("buildTarget/sources"))
@@ -88,17 +89,16 @@ object BuildServerTest extends AbstractServerTest {
          |} }""".stripMargin
     )
     assert(processing("buildTarget/compile"))
-    assert(svr.waitForString(10.seconds) { s =>
+    val res = svr.waitFor[BspCompileResult](10.seconds)
-      (s contains """"id":"32"""") &&
+    assert(res.statusCode == StatusCode.Success)
-      (s contains """"statusCode":1""")
-    })
   }
 
   test("buildTarget/scalacOptions") { _ =>
     val buildTarget = buildTargetUri("util", "Compile")
+    val badBuildTarget = buildTargetUri("badBuildTarget", "Compile")
     svr.sendJsonRpc(
       s"""{ "jsonrpc": "2.0", "id": "40", "method": "buildTarget/scalacOptions", "params": {
-        |  "targets": [{ "uri": "$buildTarget" }]
+        |  "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }]
         |} }""".stripMargin
     )
     assert(processing("buildTarget/scalacOptions"))
@@ -176,9 +176,10 @@ object BuildServerTest extends AbstractServerTest {
 
   test("buildTarget/scalaMainClasses") { _ =>
     val buildTarget = buildTargetUri("runAndTest", "Compile")
+    val badBuildTarget = buildTargetUri("badBuildTarget", "Compile")
     svr.sendJsonRpc(
       s"""{ "jsonrpc": "2.0", "id": "56", "method": "buildTarget/scalaMainClasses", "params": {
-         |  "targets": [{ "uri": "$buildTarget" }]
+         |  "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }]
          |} }""".stripMargin
     )
     assert(processing("buildTarget/scalaMainClasses"))
@@ -210,9 +211,10 @@ object BuildServerTest extends AbstractServerTest {
 
   test("buildTarget/scalaTestClasses") { _ =>
     val buildTarget = buildTargetUri("runAndTest", "Test")
+    val badBuildTarget = buildTargetUri("badBuildTarget", "Test")
     svr.sendJsonRpc(
       s"""{ "jsonrpc": "2.0", "id": "72", "method": "buildTarget/scalaTestClasses", "params": {
-         |  "targets": [{ "uri": "$buildTarget" }]
+         |  "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }]
          |} }""".stripMargin
     )
     assert(processing("buildTarget/scalaTestClasses"))
@@ -305,9 +307,10 @@ object BuildServerTest extends AbstractServerTest {
 
   test("buildTarget/resources") { _ =>
     val buildTarget = buildTargetUri("util", "Compile")
+    val badBuildTarget = buildTargetUri("badBuildTarget", "Compile")
     svr.sendJsonRpc(
       s"""{ "jsonrpc": "2.0", "id": "96", "method": "buildTarget/resources", "params": {
-         |  "targets": [{ "uri": "$buildTarget" }]
+         |  "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }]
          |} }""".stripMargin
     )
     assert(processing("buildTarget/resources"))