improvement: clear diagnostics in downstream targets (#6604)

* improvement: clear diagnostics in downstream targets, when compilation failed

* add build target mapping

* don't remove downstream diagnostics for best effort compilation

* clean up

* rename things

* use `enableBestEffort` setting

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

@@ -26,11 +26,12 @@ final class Compilations(
     workspace: () => AbsolutePath,
     languageClient: MetalsLanguageClient,
     refreshTestSuites: () => Unit,
-    afterSuccesfulCompilation: () => Unit,
+    afterSuccessfulCompilation: () => Unit,
     isCurrentlyFocused: b.BuildTargetIdentifier => Boolean,
     compileWorksheets: Seq[AbsolutePath] => Future[Unit],
     onStartCompilation: () => Unit,
     userConfiguration: () => UserConfiguration,
+    downstreamTargets: PreviouslyCompiledDownsteamTargets,
 )(implicit ec: ExecutionContext) {
   private val compileTimeout: Timeout =
     Timeout("compile", Duration(10, TimeUnit.MINUTES))
@@ -42,7 +43,10 @@ final class Compilations(
       b.BuildTargetIdentifier,
       Map[BuildTargetIdentifier, b.CompileResult],
     ](
-      compile(timeout = Some(compileTimeout)),
+      buildTargets =>
+        compile(timeout = Some(compileTimeout))(
+          downstreamTargets.transitiveTargetsOf(buildTargets)
+        ),
       "compileBatch",
       shouldLogQueue = true,
       Some(Map.empty),
@@ -273,7 +277,7 @@ final class Compilations(
     val result = compilation.asScala
       .andThen { case result =>
         updateCompiledTargetState(result)
-        afterSuccesfulCompilation()
+        afterSuccessfulCompilation()
 
         // See https://github.com/scalacenter/bloop/issues/1067
         classes.rebuildIndex(

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

@@ -47,6 +47,9 @@ final class Diagnostics(
     statistics: StatisticsConfig,
     workspace: Option[AbsolutePath],
     trees: Trees,
+    buildTargets: BuildTargets,
+    downstreamTargets: PreviouslyCompiledDownsteamTargets,
+    config: MetalsServerConfig,
 ) {
   private val diagnostics =
     TrieMap.empty[AbsolutePath, ju.Queue[Diagnostic]]
@@ -91,11 +94,23 @@ final class Diagnostics(
       report: bsp4j.CompileReport,
       statusCode: bsp4j.StatusCode,
   ): Unit = {
+    val target = report.getTarget()
+
+    // if we use best effort compilation downstream targets
+    // should get recompiled even if compilation fails
+    def shouldUnpublishForDownstreamTargets =
+      !(buildTargets
+        .scalaTarget(target)
+        .exists(_.isBestEffort) && config.enableBestEffort)
+    if (statusCode.isError && shouldUnpublishForDownstreamTargets) {
+      removeInverseDependenciesDiagnostics(target)
+    } else {
+      downstreamTargets.remove(target)
+    }
+
     publishDiagnosticsBuffer()
 
-    val target = report.getTarget()
     compileTimer.remove(target)
-
     val status = CompilationStatus(statusCode, report.getErrors())
     compilationStatus.update(target, status)
   }
@@ -197,6 +212,28 @@ final class Diagnostics(
     }
   }
 
+  private def removeInverseDependenciesDiagnostics(
+      buildTarget: BuildTargetIdentifier
+  ) = {
+    val inverseDeps =
+      buildTargets.allInverseDependencies(buildTarget) - buildTarget
+
+    val targets = for {
+      path <- diagnostics.keySet
+      targets <- buildTargets.sourceBuildTargets(path)
+      if targets.exists(inverseDeps.apply)
+    } yield {
+      diagnostics.remove(path)
+      publishDiagnostics(path)
+      targets
+    }
+
+    val targetsSet = targets.flatten.toSet
+    if (targetsSet.nonEmpty) {
+      downstreamTargets.addMapping(buildTarget, targetsSet)
+    }
+  }
+
   private def publishDiagnostics(path: AbsolutePath): Unit = {
     publishDiagnostics(
       path,

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

@@ -198,6 +198,8 @@ abstract class MetalsLspService(
     buffers,
   )
 
+  protected val downstreamTargets = new PreviouslyCompiledDownsteamTargets
+
   protected val scalaVersionSelector = new ScalaVersionSelector(
     () => userConfig,
     buildTargets,
@@ -218,6 +220,7 @@ abstract class MetalsLspService(
     worksheets => onWorksheetChanged(worksheets),
     onStartCompilation,
     () => userConfig,
+    downstreamTargets,
   )
   var indexingPromise: Promise[Unit] = Promise[Unit]()
   var buildServerPromise: Promise[Unit] = Promise[Unit]()
@@ -258,6 +261,9 @@ abstract class MetalsLspService(
     clientConfig.initialConfig.statistics,
     Option(folder),
     trees,
+    buildTargets,
+    downstreamTargets,
+    initialServerConfig,
   )
 
   protected def semanticdbs(): Semanticdbs

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

@@ -0,0 +1,49 @@
+package scala.meta.internal.metals
+
+import scala.collection.concurrent.TrieMap
+import scala.collection.mutable
+
+import ch.epfl.scala.bsp4j.BuildTargetIdentifier
+
+/**
+ * When there are some upstream compile errors we remove diagnostics from downstream targets,
+ * since those can be stale. Not to give a false impression, that the project compiles
+ * when upstream errors get fixed, we map compilation of upstream targets to
+ * appropriate downstream targets. This class holds this mapping.
+ */
+class PreviouslyCompiledDownsteamTargets {
+  private val map =
+    TrieMap.empty[BuildTargetIdentifier, Set[BuildTargetIdentifier]]
+
+  def transitiveTargetsOf(
+      targets: Seq[BuildTargetIdentifier]
+  ): Seq[BuildTargetIdentifier] = {
+    if (map.isEmpty) targets
+    else {
+      val finalSet = mutable.Set[BuildTargetIdentifier]()
+      for (key <- targets)
+        map.get(key) match {
+          case Some(set) if set.nonEmpty => finalSet ++= set
+          case _ => finalSet += key
+        }
+      finalSet.toSeq
+    }
+  }
+
+  def addMapping(
+      id: BuildTargetIdentifier,
+      to: Set[BuildTargetIdentifier],
+  ): Option[Set[BuildTargetIdentifier]] = synchronized {
+    val newValue = map.get(id).map(to ++ _).getOrElse(to)
+    map.put(id, newValue)
+  }
+
+  def remove(id: BuildTargetIdentifier): Unit = synchronized {
+    for (key <- map.keySet) {
+      for {
+        oldSet <- map.get(key)
+        if (oldSet.contains(id))
+      } map.put(id, oldSet - id)
+    }
+  }
+}

tests/unit/src/test/scala/tests/DiagnosticsLspSuite.scala

@@ -367,4 +367,99 @@ class DiagnosticsLspSuite extends BaseLspSuite("diagnostics") {
       )
     } yield ()
   }
+
+  test("errors-in-dependency") {
+    cleanWorkspace()
+    for {
+      _ <- initialize(
+        s"""
+           |/metals.json
+           |{
+           |  "a": {},
+           |  "b": { "dependsOn": [ "a" ] }
+           |}
+           |/${A.path}
+           |${A.content("Int", "1")}
+           |/${B.path}
+           |${B.content("Int", "1")}
+        """.stripMargin
+      )
+      _ <- server.didOpen(B.path)
+      _ = assertNoDiagnostics()
+      _ <- server.didChange(B.path) { _ => B.content("String", "1") }
+      _ <- server.didSave(B.path)(identity)
+      _ = assertNoDiff(
+        client.pathDiagnostics(B.path),
+        """|b/src/main/scala/b/B.scala:4:19: error: type mismatch;
+           | found   : Int(1)
+           | required: String
+           |  val b: String = 1
+           |                  ^
+           |b/src/main/scala/b/B.scala:5:20: error: type mismatch;
+           | found   : Int
+           | required: String
+           |  val a1: String = a.A.a
+           |                   ^^^^^
+           |""".stripMargin,
+      )
+      _ <- server.didOpen(A.path)
+      _ <- server.didChange(A.path) { _ => A.content("String", "1") }
+      _ <- server.didSave(A.path)(identity)
+      _ = assertNoDiff(
+        client.pathDiagnostics(A.path),
+        """|a/src/main/scala/a/A.scala:4:19: error: type mismatch;
+           | found   : Int(1)
+           | required: String
+           |  val a: String = 1
+           |                  ^
+           |""".stripMargin,
+      )
+      // we want the diagnostics for B to disappear
+      _ = assertNoDiff(client.pathDiagnostics(B.path), "")
+
+      _ <- server.didChange(A.path) { _ => A.content("String", "\"aa\"") }
+      _ <- server.didSave(A.path)(identity)
+      _ = assertNoDiff(client.pathDiagnostics(A.path), "")
+
+      // we want the diagnostics for B to appear
+      _ = assertNoDiff(
+        client.pathDiagnostics(B.path),
+        """|b/src/main/scala/b/B.scala:4:19: error: type mismatch;
+           | found   : Int(1)
+           | required: String
+           |  val b: String = 1
+           |                  ^
+           |""".stripMargin,
+      )
+
+      _ <- server.didOpen(B.path)
+      _ <- server.didChange(B.path) { _ => B.content("String", "\"aa\"") }
+      _ <- server.didSave(B.path)(identity)
+      _ = assertNoDiagnostics()
+
+    } yield ()
+  }
+
+  object A {
+    val path = "a/src/main/scala/a/A.scala"
+    def content(tpe: String, value: String): String =
+      s"""|package a
+          |
+          |object A {
+          |  val a: $tpe = $value
+          |}
+          |""".stripMargin
+  }
+
+  object B {
+    val path = "b/src/main/scala/b/B.scala"
+    def content(tpe: String, value: String): String =
+      s"""|package b
+          |
+          |object B {
+          |  val b: $tpe = $value
+          |  val a1: $tpe = a.A.a
+          |}
+          |""".stripMargin
+  }
 }

tests/unit/src/test/scala/tests/best-effort/BestEffortCompilationSuite.scala

@@ -200,7 +200,7 @@ class BestEffortCompilationSuite
     } yield ()
   }
 
-  // we check if previous sucessful best effort compilation artefacts remain
+  // we check if previous successful best effort compilation artifacts remain
   // after an unsuccessful best effort attempt.
   // Unsuccessful best effort attempts tend surface detailed exceptions
   // from bloop, so those will show up while this test is running.