BSP diagnostics fixes / tweaking (#5349)

This adds tests related to diagnostics in BSP integration tests, and
fixes or adjusts several things related to diagnostics in BSP.

In particular, this ensures that a `CompileProblemReporter` is always
passed around when running tasks for BSP. This ensures that users get
all diagnostics related to compilation happening in BSP requests, not
only diagnostics happening in the `buildTarget/compile` request. This
includes compilation of the build itself, that happens upfront right
after having started the BSP server. This allows users to get
diagnostics for errors that make the build not compile for example
(which can help address those errors).

This fixes things related to generated files. Some diagnostics
notifications were not sent for those, which was making diagnostics
stick around in VSCode, even when the errors were addressed. Line
adjustment was incorrect for `build.mill` files having no `package`
directive. This fixes that too.

There was also a hardcoded limit of maximum 10 errors being reported
(both in BSP and in the console). This removes this limit for BSP, where
a large amount of errors shouldn't be a problem, given they're reported
inline in sources.

Lastly, the "Don't send diagnostics twice" commit ensures we don't send
duplicate diagnostics via BSP. Ensuring a CompileProblemReporter is
passed around everywhere (see above) can sometimes trigger that (when
semanticdb generation is enabled, compilation happens twice: once for
classes, a second time for semanticdbs, which would report each
diagnostic twice).

Author: alexarchambault

core/api/src/mill/api/internal/CompileProblemReporter.scala

@@ -15,6 +15,12 @@ trait CompileProblemReporter {
   def printSummary(): Unit
   def finish(): Unit
   def notifyProgress(percentage: Long, total: Long): Unit
+
+  def maxErrors: Int = CompileProblemReporter.defaultMaxErrors
+}
+
+object CompileProblemReporter {
+  def defaultMaxErrors: Int = 10
 }
 
 /**

core/api/src/mill/api/internal/EvaluatorApi.scala

@@ -7,6 +7,7 @@ trait EvaluatorApi extends AutoCloseable {
   def evaluate(
       scriptArgs: Seq[String],
       selectMode: SelectMode,
+      reporter: Int => Option[CompileProblemReporter] = _ => None,
       selectiveExecution: Boolean = false
   ): Result[EvaluatorApi.Result[Any]]
 

core/define/src/mill/define/Evaluator.scala

@@ -81,6 +81,7 @@ trait Evaluator extends AutoCloseable with EvaluatorApi {
   def evaluate(
       scriptArgs: Seq[String],
       selectMode: SelectMode = SelectMode.Separated,
+      reporter: Int => Option[CompileProblemReporter] = _ => None,
       selectiveExecution: Boolean = false
   ): mill.api.Result[Evaluator.Result[Any]]
 

core/define/src/mill/define/EvaluatorProxy.scala

@@ -98,9 +98,10 @@ final class EvaluatorProxy(var delegate0: () => Evaluator) extends Evaluator {
   def evaluate(
       scriptArgs: Seq[String],
       selectMode: SelectMode,
+      reporter: Int => Option[CompileProblemReporter] = _ => None,
       selectiveExecution: Boolean = false
   ): mill.api.Result[Evaluator.Result[Any]] = {
-    delegate.evaluate(scriptArgs, selectMode, selectiveExecution)
+    delegate.evaluate(scriptArgs, selectMode, reporter, selectiveExecution)
   }
   def close = delegate0 = null
 

core/eval/src/mill/eval/EvaluatorImpl.scala

@@ -247,6 +247,7 @@ final class EvaluatorImpl private[mill] (
   def evaluate(
       scriptArgs: Seq[String],
       selectMode: SelectMode,
+      reporter: Int => Option[CompileProblemReporter] = _ => None,
       selectiveExecution: Boolean = false
   ): mill.api.Result[Evaluator.Result[Any]] = {
     val resolved = os.checker.withValue(ResolveChecker(workspace)) {
@@ -261,7 +262,7 @@ final class EvaluatorImpl private[mill] (
     }
 
     for (targets <- resolved)
-      yield execute(Seq.from(targets), selectiveExecution = selectiveExecution)
+      yield execute(Seq.from(targets), reporter = reporter, selectiveExecution = selectiveExecution)
   }
 
   def close(): Unit = execution.close()

integration/bsp-util/src/BspServerTestUtil.scala

@@ -64,49 +64,39 @@ object BspServerTestUtil {
     // This can be false only when generating test data for the first time.
     // In that case, updateSnapshots needs to be true, so that we write test data on disk.
     val snapshotExists = os.exists(snapshotPath)
-    val expectedValueOpt = Option.when(snapshotExists) {
-      gson.fromJson(
-        normalizeLocalValues(os.read(snapshotPath), inverse = true),
-        implicitly[ClassTag[T]].runtimeClass
-      )
-    }
 
-    lazy val jsonStr = normalizeLocalValues(
+    val jsonStr = normalizeLocalValues(
       gson.toJson(
         value,
         implicitly[ClassTag[T]].runtimeClass
       )
     )
 
-    lazy val expectedJsonStr = expectedValueOpt match {
-      case Some(v) => normalizeLocalValues(gson.toJson(v, implicitly[ClassTag[T]].runtimeClass))
-      case None => ""
-    }
     if (updateSnapshots) {
       System.err.println(if (snapshotExists) s"Updating $snapshotPath"
       else s"Writing $snapshotPath")
       os.write.over(snapshotPath, jsonStr, createFolders = true)
-    } else {
-      Predef.assert(
-        jsonStr == expectedJsonStr,
-        if (snapshotExists) {
-          val diff = os.call((
-            // "git",
-            "diff",
-            "-u",
-            os.temp(expectedJsonStr, suffix = s"${snapshotPath.last}-expectedJsonStr"),
-            os.temp(jsonStr, suffix = s"${snapshotPath.last}-jsonStr")
-          ))
+    } else if (snapshotExists) {
+      val expectedJsonStr = os.read(snapshotPath)
+      if (jsonStr != expectedJsonStr) {
+        val diff = os.call((
+          "diff",
+          "-u",
+          os.temp(expectedJsonStr, suffix = s"${snapshotPath.last}-expectedJsonStr"),
+          os.temp(jsonStr, suffix = s"${snapshotPath.last}-jsonStr")
+        ))
+        sys.error(
           s"""Error: value differs from snapshot in $snapshotPath
              |
              |You might want to set BspServerTestUtil.updateSnapshots to true,
              |run this test again, and commit the updated test data files.
              |
              |$diff
              |""".stripMargin
-        } else s"Error: no snapshot found at $snapshotPath"
-      )
-    }
+        )
+      }
+    } else
+      sys.error(s"Error: no snapshot found at $snapshotPath")
   }
 
   def compareLogWithSnapshot(

integration/ide/bsp-server/resources/project/build.mill

@@ -91,3 +91,18 @@ object delayed extends scalalib.ScalaModule {
     res
   }
 }
+
+object diag extends scalalib.ScalaModule {
+  def scalaVersion = Option(System.getenv("TEST_SCALA_2_13_VERSION")).getOrElse(???)
+  def scalacOptions = Seq("-deprecation")
+
+  @deprecated("deprecated", "0.0.1")
+  def thing = 2
+
+  def theThing = thing
+
+  object many extends scalalib.ScalaModule {
+    def scalaVersion = Option(System.getenv("TEST_SCALA_2_13_VERSION")).getOrElse(???)
+    def scalacOptions = Seq("-deprecation")
+  }
+}

integration/ide/bsp-server/resources/project/diag/many/src/WarningAndErrors.scala

@@ -0,0 +1,71 @@
+package diag
+
+object WarningAndErrors {
+
+  @deprecated("thing", "0.0.1")
+  def printThing() = println(2)
+
+  // 12 warnings for "printThing()" calls
+  // 12 errors for not found value "foo"
+
+  def thing0 = {
+    printThing()
+    foo
+  }
+
+  def thing1 = {
+    printThing()
+    foo
+  }
+
+  def thing2 = {
+    printThing()
+    foo
+  }
+
+  def thing3 = {
+    printThing()
+    foo
+  }
+
+  def thing4 = {
+    printThing()
+    foo
+  }
+
+  def thing5 = {
+    printThing()
+    foo
+  }
+
+  def thing6 = {
+    printThing()
+    foo
+  }
+
+  def thing7 = {
+    printThing()
+    foo
+  }
+
+  def thing8 = {
+    printThing()
+    foo
+  }
+
+  def thing9 = {
+    printThing()
+    foo
+  }
+
+  def thing10 = {
+    printThing()
+    foo
+  }
+
+  def thing11 = {
+    printThing()
+    foo
+  }
+
+}

integration/ide/bsp-server/resources/project/diag/src/DiagCheck.scala

@@ -0,0 +1,10 @@
+package check
+
+object DiagCheck {
+
+  @deprecated("deprecated thing", "0.0.1")
+  def thing = 2
+
+  println(thing)
+
+}

integration/ide/bsp-server/resources/snapshots/build-targets-compile-classpaths.json

@@ -35,6 +35,24 @@
         "file:///workspace/delayed/compile-resources"
       ]
     },
+    {
+      "target": {
+        "uri": "file:///workspace/diag"
+      },
+      "classpath": [
+        "file:///coursier-cache/https/repo1.maven.org/maven2/org/scala-lang/scala-library/<scala-version>/scala-library-<scala-version>.jar",
+        "file:///workspace/diag/compile-resources"
+      ]
+    },
+    {
+      "target": {
+        "uri": "file:///workspace/diag/many"
+      },
+      "classpath": [
+        "file:///coursier-cache/https/repo1.maven.org/maven2/org/scala-lang/scala-library/<scala-version>/scala-library-<scala-version>.jar",
+        "file:///workspace/diag/many/compile-resources"
+      ]
+    },
     {
       "target": {
         "uri": "file:///workspace/errored/compilation-error"