From b1b5caa1ca566cc5863d0e2b688965a5a1f6257b Mon Sep 17 00:00:00 2001
From: SlowBrainDude <126806769+SlowBrainDude@users.noreply.github.com>
Date: Wed, 24 Jul 2024 02:45:24 +0200
Subject: [PATCH] Fix stale BSP diagnostics

The BSP server didn't reset old diagnostic messages sent to BSP clients under
certain circumstances. This commit mitigates this edge case and ensures that
diagnostics for files that previously had compilation problems are properly
reset when fresh diagnostics messages are sent.

The culprit was a mismatch of map keys: Files with problems were sometimes recorded
under an absolute path, but later attempted to be retrieved by virtual path.
---
 .../internal/server/BuildServerReporter.scala | 12 +++-
 .../test/scala/testpkg/BuildServerTest.scala  | 72 +++++++++++++++++++
 2 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala
index 3b796a408..f64e9d68d 100644
--- a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala
+++ b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala
@@ -25,6 +25,7 @@ import xsbti.{
 
 import scala.jdk.CollectionConverters.*
 import scala.collection.mutable
+import java.nio.file.Path
 
 /**
  * Provides methods for sending success and failure reports and publishing diagnostics.
@@ -88,7 +89,7 @@ final class BuildServerReporterImpl(
   import sbt.internal.inc.JavaInterfaceUtil._
 
   private lazy val exchange = StandardMain.exchange
-  private val problemsByFile = mutable.Map[VirtualFileRef, Vector[Problem]]()
+  private val problemsByFile = mutable.Map[Path, Vector[Problem]]()
 
   // sometimes the compiler returns a fake position such as <macro>
   // on Windows, this causes InvalidPathException (see #5994 and #6720)
@@ -112,9 +113,10 @@ final class BuildServerReporterImpl(
 
   override def sendFailureReport(sources: Array[VirtualFile]): Unit = {
     for (source <- sources) {
-      val problems = problemsByFile.getOrElse(source, Vector.empty)
+      val problems = problemsByFile.getOrElse(converter.toPath(source), Vector.empty)
       sendReport(source, problems)
     }
+    notifyFirstReport()
   }
 
   private def sendReport(source: VirtualFileRef, problems: Vector[Problem]): Unit = {
@@ -150,7 +152,11 @@ final class BuildServerReporterImpl(
       id <- problem.position.sourcePath.toOption
       (document, diagnostic) <- mapProblemToDiagnostic(problem)
     } {
-      val fileRef = VirtualFileRef.of(id)
+      // Note: We're putting the real path in `fileRef` because the `id` String can take
+      // two forms, either a ${something}/relativePath, or the absolute path of the source.
+      // But where we query this, we always have _only_ a ${something}/relativePath available.
+      // So here we "normalize" to the real path.
+      val fileRef = converter.toPath(VirtualFileRef.of(id))
       problemsByFile(fileRef) = problemsByFile.getOrElse(fileRef, Vector.empty) :+ problem
 
       val params = PublishDiagnosticsParams(
diff --git a/server-test/src/test/scala/testpkg/BuildServerTest.scala b/server-test/src/test/scala/testpkg/BuildServerTest.scala
index a7f5857c9..c32b78939 100644
--- a/server-test/src/test/scala/testpkg/BuildServerTest.scala
+++ b/server-test/src/test/scala/testpkg/BuildServerTest.scala
@@ -252,6 +252,78 @@ class BuildServerTest extends AbstractServerTest {
     )
   }
 
+  test("buildTarget/compile [Java diagnostics] clear stale warnings") {
+    val buildTarget = buildTargetUri("javaProj", "Compile")
+    val testFile = new File(svr.baseDirectory, s"java-proj/src/main/java/example/Hello.java")
+
+    val otherBuildFile = new File(svr.baseDirectory, "force-java-out-of-process-compiler.sbt")
+    // Setting `javaHome` will force SBT to shell out to an external Java compiler instead
+    // of using the local compilation service offered by the JVM running this SBT instance.
+    IO.write(
+      otherBuildFile,
+      """
+        |lazy val javaProj = project
+        |  .in(file("java-proj"))
+        |  .settings(
+        |    javacOptions += "-Xlint:all",
+        |    javaHome := Some(file(System.getProperty("java.home")))
+        |  )
+        |""".stripMargin
+    )
+    reloadWorkspace()
+
+    compile(buildTarget)
+
+    assertMessage(
+      "build/publishDiagnostics",
+      "Hello.java",
+      """"severity":2""",
+      """found raw type: List"""
+    )(message = "should send publishDiagnostics with severity 2 for Hello.java")
+
+    assertMessage(
+      "build/publishDiagnostics",
+      "Hello.java",
+      """"severity":1""",
+      """incompatible types: int cannot be converted to String"""
+    )(
+      message = "should send publishDiagnostics with severity 1 for Hello.java"
+    )
+    // Note the messages changed slightly in both cases. That's interesting…
+
+    IO.write(
+      testFile,
+      """|package example;
+         |
+         |import java.util.List;
+         |import java.util.ArrayList;
+         |
+         |class Hello {
+         |    public static void main(String[] args) {
+         |        List<String> list = new ArrayList<>();
+         |        String msg = "42";
+         |        System.out.println(msg);
+         |    }
+         |}
+         |""".stripMargin
+    )
+
+    compile(buildTarget)
+
+    assertMessage(
+      "build/publishDiagnostics",
+      "Hello.java",
+      "\"diagnostics\":[]",
+      "\"reset\":true"
+    )(
+      message = "should send publishDiagnostics with empty diagnostics"
+    )
+
+    IO.delete(otherBuildFile)
+    reloadWorkspace()
+    ()
+  }
+
   test("buildTarget/scalacOptions, buildTarget/javacOptions") {
     val buildTargets = Seq(
       buildTargetUri("util", "Compile"),