Merge pull request #710 from scalacenter/topic/rework-bsp

Publish diagnostics for syntax errors and improve tests

backend/src/main/scala/sbt/internal/inc/bloop/ZincInternals.scala

@@ -62,10 +62,14 @@ object ZincInternals {
   }
 
   import sbt.internal.inc.JavaInterfaceUtil.EnrichOptional
-  object ZincExistsPos {
+  object ZincExistsStartPos {
     def unapply(position: Position): Option[(Int, Int)] = {
-      position.startLine.toOption.flatMap(startLine =>
-        position.startColumn().toOption.map(startColumn => (startLine, startColumn)))
+      position.line.toOption
+        .flatMap(line => position.pointer.toOption.map(column => (line.toInt, column.toInt)))
+        .orElse {
+          position.startLine.toOption.flatMap(startLine =>
+            position.startColumn().toOption.map(startColumn => (startLine, startColumn)))
+        }
     }
   }
 

backend/src/test/scala/bloop/logging/RecordingLogger.scala

@@ -14,8 +14,9 @@ final class RecordingLogger(
 
   def clear(): Unit = messages.clear()
 
+  def getMessagesAt(level: Option[String]): List[String] = getMessages(level).map(_._2)
   def getMessages(): List[(String, String)] = getMessages(None)
-  def getMessages(level: Option[String] = None): List[(String, String)] = {
+  private def getMessages(level: Option[String]): List[(String, String)] = {
     val initialMsgs = messages.iterator.asScala
     val msgs = level match {
       case Some(level) => initialMsgs.filter(_._1 == level)
@@ -68,10 +69,12 @@ final class RecordingLogger(
   override def asVerbose: Logger = this
   override def asDiscrete: Logger = this
 
-  def dump(): Unit = println {
-    s"""Logger contains the following messages:
-       |${getMessages().map(s => s"[${s._1}] ${s._2}").mkString("\n  ", "\n  ", "\n")}
+  def dump(out: PrintStream = System.out): Unit = {
+    out.println {
+      s"""Logger contains the following messages:
+         |${getMessages.map(s => s"[${s._1}] ${s._2}").mkString("\n  ", "\n  ", "\n")}
      """.stripMargin
+    }
   }
 
   /** Returns all the infos detected about the state of compilation */

frontend/src/main/scala/bloop/bsp/ProjectUris.scala

@@ -1,10 +1,12 @@
 package bloop.bsp
 
 import java.net.URI
+import java.nio.file.Path
 
 import bloop.data.Project
 import bloop.engine.State
 import bloop.io.AbsolutePath
+import ch.epfl.scala.bsp.Uri
 
 import scala.util.Try
 
@@ -26,9 +28,33 @@ object ProjectUris {
     }
   }
 
-  private[bsp] val Example = "file://path/to/base/directory?id=projectName"
-  def toUri(projectBaseDir: AbsolutePath, id: String): URI = {
-    val uriSyntax = projectBaseDir.underlying.toUri
-    new URI(s"${uriSyntax}?id=${id}")
+  private[bsp] val Example = "file:///path/to/base/directory?id=projectName"
+  def toURI(projectBaseDir: AbsolutePath, id: String): URI = {
+    // This is the "idiomatic" way of adding a query to a URI in Java
+    val existingUri = projectBaseDir.underlying.toUri
+    new URI(
+      existingUri.getScheme,
+      existingUri.getUserInfo,
+      existingUri.getHost,
+      existingUri.getPort,
+      existingUri.getPath,
+      s"id=${id}",
+      existingUri.getFragment
+    )
+  }
+
+  def toPath(uri: Uri): Path = {
+    val existingUri = new URI(uri.value)
+    val uriWithNoQuery = new URI(
+      existingUri.getScheme,
+      existingUri.getUserInfo,
+      existingUri.getHost,
+      existingUri.getPort,
+      existingUri.getPath,
+      null,
+      existingUri.getFragment
+    )
+
+    java.nio.file.Paths.get(uriWithNoQuery)
   }
 }

frontend/src/main/scala/bloop/data/Project.scala

@@ -34,7 +34,7 @@ final case class Project(
     origin: Origin
 ) {
   /** The bsp uri associated with this project. */
-  val bspUri: Bsp.Uri = Bsp.Uri(ProjectUris.toUri(baseDirectory, name))
+  val bspUri: Bsp.Uri = Bsp.Uri(ProjectUris.toURI(baseDirectory, name))
 
   /** This project's full classpath (classes directory and raw classpath) */
   val classpath: Array[AbsolutePath] = (classesDir :: rawClasspath).toArray

frontend/src/main/scala/bloop/logging/BspServerLogger.scala

@@ -62,14 +62,15 @@ final class BspServerLogger private (
     val sourceFile = toOption(problemPos.sourceFile())
 
     (problemPos, sourceFile) match {
-      case (ZincInternals.ZincExistsPos(startLine, startColumn), Some(file)) =>
+      case (ZincInternals.ZincExistsStartPos(startLine, startColumn), Some(file)) =>
+        // Lines in Scalac are indexed by 1, BSP expects 0-index positions
         val pos = problem.position match {
           case ZincInternals.ZincRangePos(endLine, endColumn) =>
-            val start = bsp.Position(startLine, startColumn)
-            val end = bsp.Position(endLine, endColumn)
+            val start = bsp.Position(startLine - 1, startColumn)
+            val end = bsp.Position(endLine - 1, endColumn)
             bsp.Range(start, end)
           case _ =>
-            val pos = bsp.Position(startLine, startColumn)
+            val pos = bsp.Position(startLine - 1, startColumn)
             bsp.Range(pos, pos)
         }
 

frontend/src/test/resources/cross-test-build-0.6/build.sbt

@@ -8,6 +8,7 @@ lazy val `test-project` =
       name := "test-project",
       // %%% now include Scala Native. It applies to all selected platforms
       scalaVersion := "2.11.12",
+      scalacOptions += "-Ywarn-unused",
       libraryDependencies += "com.lihaoyi" %%% "utest" % "0.6.6" % Test,
       libraryDependencies += "org.scalatest" %%% "scalatest" % "3.0.4" % "test",
       libraryDependencies += "org.scalacheck" %%% "scalacheck" % "1.13.4" % "test",

frontend/src/test/resources/cross-test-build-0.6/test-project/shared/src/main/scala/hello/App.scala

@@ -0,0 +1,9 @@
+package hello
+
+object App {
+  def main(args: Array[String]): Unit = {
+    // Do not touch the following line as its position is tested in bloop
+    val a = 1 // This is unused and will cause a warning
+    println(Hello.greet("world"))
+  }
+}

frontend/src/test/resources/cross-test-build-0.6/test-project/shared/src/main/scala/hello/Hello.scala

@@ -6,11 +6,4 @@ object Hello {
     Environment.requireEnvironmentVariable()
     s"Hello, $name!"
   }
-
-}
-
-object App {
-  def main(args: Array[String]): Unit = {
-    println(Hello.greet("world"))
-  }
 }

frontend/src/test/scala/bloop/bsp/BspClientTest.scala

@@ -1,11 +1,13 @@
 package bloop.bsp
 
 import java.nio.file.Files
+import java.util.concurrent.ExecutionException
 
 import bloop.cli.Commands
+import bloop.data.Project
 import bloop.engine.State
 import bloop.engine.caches.{ResultsCache, StateCache}
-import bloop.io.AbsolutePath
+import bloop.io.{AbsolutePath, RelativePath}
 import bloop.logging.{BspClientLogger, DebugFilter, RecordingLogger, Slf4jAdapter}
 import bloop.tasks.TestUtil
 import ch.epfl.scala.bsp
@@ -69,13 +71,15 @@ object BspClientTest {
       }
       .notification(endpoints.Build.publishDiagnostics) {
         case bsp.PublishDiagnosticsParams(uri, _, diagnostics) =>
+          // We prepend diagnostics so that tests can check they came from this notification
+          def printDiagnostic(d: bsp.Diagnostic): String = s"[diagnostic] ${d.message} ${d.range}"
           diagnostics.foreach { d =>
             d.severity match {
-              case Some(bsp.DiagnosticSeverity.Error) => logger.error(d.toString)
-              case Some(bsp.DiagnosticSeverity.Warning) => logger.warn(d.toString)
-              case Some(bsp.DiagnosticSeverity.Information) => logger.info(d.toString)
-              case Some(bsp.DiagnosticSeverity.Hint) => logger.debug(d.toString)
-              case None => logger.info(d.toString)
+              case Some(bsp.DiagnosticSeverity.Error) => logger.error(printDiagnostic(d))
+              case Some(bsp.DiagnosticSeverity.Warning) => logger.warn(printDiagnostic(d))
+              case Some(bsp.DiagnosticSeverity.Information) => logger.info(printDiagnostic(d))
+              case Some(bsp.DiagnosticSeverity.Hint) => logger.debug(printDiagnostic(d))
+              case None => logger.info(printDiagnostic(d))
             }
           }
       }
@@ -90,12 +94,10 @@ object BspClientTest {
       allowError: Boolean = false,
       reusePreviousState: Boolean = false,
   )(runEndpoints: LanguageClient => me.Task[Either[Response.Error, T]]): Unit = {
-    val workingPath = cmd.cliOptions.common.workingPath
-    val projectName = workingPath.underlying.getFileName().toString()
-
     // Set an empty results cache and update the state globally
     val state = {
-      val state0 = TestUtil.loadTestProject(projectName).copy(logger = logger)
+      val id = identity[List[Project]] _
+      val state0 = TestUtil.loadTestProject(configDirectory.underlying, id).copy(logger = logger)
       // Return if we plan to reuse it, BSP reloads the state based on the state cache
       if (reusePreviousState) state0
       else {
@@ -104,8 +106,7 @@ object BspClientTest {
       }
     }
 
-    // Clean all the project results to avoid reusing previous compiles.
-    val configPath = configDirectory.toRelative(workingPath)
+    val configPath = RelativePath("bloop-config")
     val bspServer = BspServer.run(cmd, state, configPath, scheduler).runAsync(scheduler)
 
     val bspClientExecution = establishClientConnection(cmd).flatMap { socket =>
@@ -118,7 +119,7 @@ object BspClientTest {
       val lsServer = new LanguageServer(messages, lsClient, services, scheduler, logger)
       val runningClientServer = lsServer.startTask.runAsync(scheduler)
 
-      val cwd = TestUtil.getBaseFromConfigDir(configDirectory.underlying)
+      val cwd = configDirectory.underlying.getParent
       val initializeServer = endpoints.Build.initialize.request(
         bsp.InitializeBuildParams(
           rootUri = bsp.Uri(cwd.toAbsolutePath.toUri),
@@ -146,10 +147,18 @@ object BspClientTest {
     import scala.concurrent.Await
     import scala.concurrent.duration.FiniteDuration
     val bspClient = bspClientExecution.runAsync(scheduler)
-    // The timeout for all our bsp tests, no matter what operation they run, is 60s
-    Await.result(bspClient, FiniteDuration(60, "s"))
-    Await.result(bspServer, FiniteDuration(60, "s"))
-    cleanUpLastResources(cmd)
+
+    try {
+      // The timeout for all our bsp tests, no matter what operation they run, is 60s
+      Await.result(bspClient, FiniteDuration(60, "s"))
+      Await.result(bspServer, FiniteDuration(60, "s"))
+    } catch {
+      case e: ExecutionException => throw e.getCause
+      case t: Throwable => throw t
+    } finally {
+      cleanUpLastResources(cmd)
+    }
+    ()
   }
 
   private def establishClientConnection(cmd: Commands.ValidatedBsp): me.Task[java.net.Socket] = {