diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index f3fe55029..f90f91de3 100644 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -2307,11 +2307,10 @@ object Defaults extends BuildCommon { val ci = (compile / compileInputs).value val ping = earlyOutputPing.value val reporter = (compile / bspReporter).value - val prevAnalysis = previousCompile.value.analysis.toOption.getOrElse(Analysis.empty) BspCompileTask.compute(bspTargetIdentifier.value, thisProjectRef.value, configuration.value) { task => // TODO - Should readAnalysis + saveAnalysis be scoped by the compile task too? - compileIncrementalTaskImpl(task, s, ci, ping, reporter, prevAnalysis) + compileIncrementalTaskImpl(task, s, ci, ping, reporter) } } private val incCompiler = ZincUtil.defaultIncrementalCompiler @@ -2326,7 +2325,7 @@ object Defaults extends BuildCommon { val result0 = incCompiler .asInstanceOf[sbt.internal.inc.IncrementalCompilerImpl] .compileAllJava(in, s.log) - reporter.sendSuccessReport(result0.analysis(), Analysis.empty, false) + reporter.sendSuccessReport(result0.analysis()) result0.withHasModified(result0.hasModified || r.hasModified) } else r } catch { @@ -2341,7 +2340,6 @@ object Defaults extends BuildCommon { ci: Inputs, promise: PromiseWrap[Boolean], reporter: BuildServerReporter, - prev: CompileAnalysis ): CompileResult = { lazy val x = s.text(ExportStream) def onArgs(cs: Compilers) = { @@ -2363,11 +2361,7 @@ object Defaults extends BuildCommon { .withSetup(onProgress(setup)) try { val result = incCompiler.compile(i, s.log) - reporter.sendSuccessReport( - result.getAnalysis, - prev, - BuildServerProtocol.shouldReportAllPreviousProblems(task.targetId) - ) + reporter.sendSuccessReport(result.getAnalysis) result } catch { case e: Throwable => diff --git a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala index f288d05a3..4e2abdbe8 100644 --- a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala +++ b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala @@ -32,33 +32,20 @@ import sjsonnew.shaded.scalajson.ast.unsafe.{ JNull, JValue } import sjsonnew.support.scalajson.unsafe.{ CompactPrinter, Converter, Parser => JsonParser } import xsbti.CompileFailed +import java.nio.file.Path import java.io.File +import java.util.concurrent.atomic.AtomicBoolean import scala.collection.mutable // import scala.annotation.nowarn import scala.util.control.NonFatal import scala.util.{ Failure, Success, Try } import scala.annotation.nowarn -import scala.collection.concurrent.TrieMap import sbt.testing.Framework object BuildServerProtocol { import sbt.internal.bsp.codec.JsonProtocol._ - /** - * Keep track of those projects that were compiled at least once so that we can - * decide to enable fresh reporting for projects that are compiled for the first time. - * - * see: https://github.com/scalacenter/bloop/issues/726 - */ - private val compiledTargetsAtLeastOnce = new TrieMap[bsp.BuildTargetIdentifier, Boolean]() - def shouldReportAllPreviousProblems(id: bsp.BuildTargetIdentifier): Boolean = { - compiledTargetsAtLeastOnce.putIfAbsent(id, true) match { - case Some(_) => false - case None => true - } - } - private val capabilities = BuildServerCapabilities( CompileProvider(BuildServerConnection.languages), TestProvider(BuildServerConnection.languages), @@ -305,12 +292,20 @@ object BuildServerProtocol { bspScalaMainClassesItem := scalaMainClassesTask.value, Keys.compile / bspReporter := { val targetId = bspTargetIdentifier.value + val bspCompileStateInstance = bspCompileState.value val converter = fileConverter.value val underlying = (Keys.compile / compilerReporter).value val logger = streams.value.log val meta = isMetaBuild.value if (bspEnabled.value) { - new BuildServerReporterImpl(targetId, converter, meta, logger, underlying) + new BuildServerReporterImpl( + targetId, + bspCompileStateInstance, + converter, + meta, + logger, + underlying + ) } else { new BuildServerForwarder(meta, logger, underlying) } @@ -356,7 +351,6 @@ object BuildServerProtocol { case r if r.method == Method.Initialize => val params = Converter.fromJson[InitializeBuildParams](json(r)).get checkMetalsCompatibility(semanticdbEnabled, semanticdbVersion, params, callback.log) - compiledTargetsAtLeastOnce.clear() val response = InitializeBuildResult( "sbt", @@ -714,6 +708,10 @@ object BuildServerProtocol { DependencySourcesItem(targetId, sources.toVector.distinct) } + private def bspCompileState: Initialize[BuildServerProtocol.BspCompileState] = Def.setting { + new BuildServerProtocol.BspCompileState() + } + private def bspCompileTask: Def.Initialize[Task[Int]] = Def.task { Keys.compile.result.value match { case Value(_) => StatusCode.Success @@ -1000,4 +998,20 @@ object BuildServerProtocol { ) } } + + /** + * Additional information about compilation status for given build target. + * + * @param hasAnyProblems keeps track of problems in given file so BSP reporter + * can omit unnecessary diagnostics updates. + * @param compiledAtLeastOnce keeps track of those projects that were compiled at + * least once so that we can decide to enable fresh reporting for projects that + * are compiled for the first time. + * see: https://github.com/scalacenter/bloop/issues/726 + */ + private[server] final class BspCompileState { + val hasAnyProblems: java.util.Set[Path] = + java.util.concurrent.ConcurrentHashMap.newKeySet[Path] + val compiledAtLeastOnce: AtomicBoolean = new AtomicBoolean(false) + } } diff --git a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala index 98d59d645..6e04053da 100644 --- a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala +++ b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala @@ -12,6 +12,7 @@ import java.nio.file.Path import sbt.StandardMain import sbt.internal.bsp._ import sbt.internal.util.ManagedLogger +import sbt.internal.server.BuildServerProtocol.BspCompileState import xsbti.compile.CompileAnalysis import xsbti.{ FileConverter, @@ -40,8 +41,6 @@ sealed trait BuildServerReporter extends Reporter { def sendSuccessReport( analysis: CompileAnalysis, - prev: CompileAnalysis, - reportAllPreviousProblems: Boolean ): Unit def sendFailureReport(sources: Array[VirtualFile]): Unit @@ -73,6 +72,7 @@ sealed trait BuildServerReporter extends Reporter { final class BuildServerReporterImpl( buildTarget: BuildTargetIdentifier, + bspCompileState: BspCompileState, converter: FileConverter, protected override val isMetaBuild: Boolean, protected override val logger: ManagedLogger, @@ -90,22 +90,38 @@ final class BuildServerReporterImpl( if (ref.id().contains("<")) None else Some(converter.toPath(ref)) + /** + * Send diagnostics from the compilation to the client. + * Do not send empty diagnostics if previous ones were also empty ones. + * + * @param analysis current compile analysis + */ override def sendSuccessReport( analysis: CompileAnalysis, - prev: CompileAnalysis, - reportAllPreviousProblems: Boolean ): Unit = { - val prevInfos = prev.readSourceInfos().getAllSourceInfos().asScala + val shouldReportAllProblems = !bspCompileState.compiledAtLeastOnce.getAndSet(true) for { (source, infos) <- analysis.readSourceInfos.getAllSourceInfos.asScala filePath <- toSafePath(source) } { - val prevProblems = prevInfos.get(source).map(_.getReportedProblems()).getOrElse(Array.empty) - val dontPublish = prevProblems.length == 0 && infos.getReportedProblems().length == 0 - val shouldPublish = reportAllPreviousProblems || !dontPublish + // clear problems for current file + val hadProblems = bspCompileState.hasAnyProblems.remove(filePath) + + val reportedProblems = infos.getReportedProblems.toVector + val diagnostics = reportedProblems.flatMap(toDiagnostic) + + // publish diagnostics if: + // 1. file had any problems previously - we might want to update them with new ones + // 2. file has fresh problems - we might want to update old ones + // 3. build project is compiled first time - shouldReportAllProblems is set + val shouldPublish = hadProblems || diagnostics.nonEmpty || shouldReportAllProblems + + // file can have some warnings + if (diagnostics.nonEmpty) { + bspCompileState.hasAnyProblems.add(filePath) + } if (shouldPublish) { - val diagnostics = infos.getReportedProblems.toSeq.flatMap(toDiagnostic) val params = PublishDiagnosticsParams( textDocument = TextDocumentIdentifier(filePath.toUri), buildTarget, @@ -117,21 +133,32 @@ final class BuildServerReporterImpl( } } } - override def sendFailureReport(sources: Array[VirtualFile]): Unit = { + val shouldReportAllProblems = !bspCompileState.compiledAtLeastOnce.get for { source <- sources filePath <- toSafePath(source) } { - val diagnostics = problemsByFile.getOrElse(filePath, Vector()) - val params = PublishDiagnosticsParams( - textDocument = TextDocumentIdentifier(filePath.toUri), - buildTarget, - originId = None, - diagnostics, - reset = true - ) - exchange.notifyEvent("build/publishDiagnostics", params) + val diagnostics = problemsByFile.getOrElse(filePath, Vector.empty) + + val hadProblems = bspCompileState.hasAnyProblems.remove(filePath) + val shouldPublish = hadProblems || diagnostics.nonEmpty || shouldReportAllProblems + + // mark file as file with problems + if (diagnostics.nonEmpty) { + bspCompileState.hasAnyProblems.add(filePath) + } + + if (shouldPublish) { + val params = PublishDiagnosticsParams( + textDocument = TextDocumentIdentifier(filePath.toUri), + buildTarget, + originId = None, + diagnostics, + reset = true + ) + exchange.notifyEvent("build/publishDiagnostics", params) + } } } @@ -141,7 +168,7 @@ final class BuildServerReporterImpl( diagnostic <- toDiagnostic(problem) filePath <- toSafePath(VirtualFileRef.of(id)) } { - problemsByFile(filePath) = problemsByFile.getOrElse(filePath, Vector()) :+ diagnostic + problemsByFile(filePath) = problemsByFile.getOrElse(filePath, Vector.empty) :+ diagnostic val params = PublishDiagnosticsParams( TextDocumentIdentifier(filePath.toUri), buildTarget, @@ -196,8 +223,6 @@ final class BuildServerForwarder( override def sendSuccessReport( analysis: CompileAnalysis, - prev: CompileAnalysis, - reportAllPreviousProblems: Boolean ): Unit = () override def sendFailureReport(sources: Array[VirtualFile]): Unit = () diff --git a/server-test/src/server-test/buildserver/build.sbt b/server-test/src/server-test/buildserver/build.sbt index 9d2de35fc..fe2cf4db5 100644 --- a/server-test/src/server-test/buildserver/build.sbt +++ b/server-test/src/server-test/buildserver/build.sbt @@ -32,6 +32,8 @@ lazy val respondError = project.in(file("respond-error")) lazy val util = project +lazy val diagnostics = 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")) diff --git a/server-test/src/server-test/buildserver/diagnostics/src/main/scala/Diagnostics.scala b/server-test/src/server-test/buildserver/diagnostics/src/main/scala/Diagnostics.scala new file mode 100644 index 000000000..a74b3da77 --- /dev/null +++ b/server-test/src/server-test/buildserver/diagnostics/src/main/scala/Diagnostics.scala @@ -0,0 +1,3 @@ +object Diagnostics { + private val a: Int = 5 +} \ No newline at end of file diff --git a/server-test/src/server-test/buildserver/diagnostics/src/main/scala/PatternMatch.scala b/server-test/src/server-test/buildserver/diagnostics/src/main/scala/PatternMatch.scala new file mode 100644 index 000000000..626a59f8f --- /dev/null +++ b/server-test/src/server-test/buildserver/diagnostics/src/main/scala/PatternMatch.scala @@ -0,0 +1,7 @@ + +class PatternMatch { + val opt: Option[Int] = None + opt match { + case Some(value) => () + } +} diff --git a/server-test/src/test/scala/testpkg/BuildServerTest.scala b/server-test/src/test/scala/testpkg/BuildServerTest.scala index 817b0e6ad..1b8fd06fb 100644 --- a/server-test/src/test/scala/testpkg/BuildServerTest.scala +++ b/server-test/src/test/scala/testpkg/BuildServerTest.scala @@ -71,6 +71,7 @@ object BuildServerTest extends AbstractServerTest { ) assert(sources.contains(expectedSource)) } + test("buildTarget/sources: sbt") { _ => val x = new URI(s"${svr.baseDirectory.getAbsoluteFile.toURI}#buildserver-build") svr.sendJsonRpc(buildTargetSources(26, Seq(x))) @@ -89,13 +90,12 @@ object BuildServerTest extends AbstractServerTest { ).map(rel => new File(svr.baseDirectory.getAbsoluteFile, rel).toURI).sorted assert(sources == expectedSources) } + test("buildTarget/compile") { _ => val buildTarget = buildTargetUri("util", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "32", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + + compile(buildTarget, id = 32) + assert(processing("buildTarget/compile")) val res = svr.waitFor[BspCompileResult](10.seconds) assert(res.statusCode == StatusCode.Success) @@ -103,11 +103,8 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/compile - reports compilation progress") { _ => val buildTarget = buildTargetUri("runAndTest", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "33", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + + compile(buildTarget, id = 33) // This doesn't always come back in 10s on CI. assert(svr.waitForString(60.seconds) { s => @@ -134,16 +131,112 @@ object BuildServerTest extends AbstractServerTest { s.contains("build/taskFinish") && s.contains(""""message":"Compiled runAndTest"""") }) + } - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "34", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin + test( + "buildTarget/compile [diagnostics] don't publish unnecessary for successful compilation case" + ) { _ => + val buildTarget = buildTargetUri("diagnostics", "Compile") + val mainFile = new File(svr.baseDirectory, "diagnostics/src/main/scala/Diagnostics.scala") + + compile(buildTarget, id = 33) + + assert(svr.waitForString(30.seconds) { s => + s.contains("build/taskFinish") && + s.contains(""""message":"Compiled diagnostics"""") + }) + + // introduce compile error + IO.write( + mainFile, + """|object Diagnostics { + | private val a: Int = "" + |}""".stripMargin + ) + + reloadWorkspace(id = 55) + compile(buildTarget, id = 66) + + assert( + svr.waitForString(30.seconds) { s => + s.contains("build/publishDiagnostics") && + s.contains("Diagnostics.scala") && + s.contains("\"message\":\"type mismatch") + }, + "should send publishDiagnostics with type error for Main.scala" + ) + + // fix compilation error + IO.write( + mainFile, + """|object Diagnostics { + | private val a: Int = 5 + |}""".stripMargin + ) + + reloadWorkspace(id = 77) + compile(buildTarget, id = 88) + + assert( + svr.waitForString(30.seconds) { s => + s.contains("build/publishDiagnostics") && + s.contains("Diagnostics.scala") && + s.contains("\"diagnostics\":[]") + }, + "should send publishDiagnostics with empty diagnostics" + ) + + // trigger no-op compilation + compile(buildTarget, id = 99) + + assert( + !svr.waitForString(20.seconds) { s => + s.contains("build/publishDiagnostics") && + s.contains("Diagnostics.scala") + }, + "shouldn't send publishDiagnostics if there's no change in diagnostics (were empty, are empty)" + ) + } + + test("buildTarget/compile [diagnostics] clear stale warnings") { _ => + val buildTarget = buildTargetUri("diagnostics", "Compile") + val testFile = new File(svr.baseDirectory, s"diagnostics/src/main/scala/PatternMatch.scala") + + compile(buildTarget, id = 33) + + assert( + svr.waitForString(30.seconds) { s => + s.contains("build/publishDiagnostics") && + s.contains("PatternMatch.scala") && + s.contains(""""message":"match may not be exhaustive""") + }, + "should send publishDiagnostics with type error for PatternMatch.scala" + ) + + IO.write( + testFile, + """|class PatternMatch { + | val opt: Option[Int] = None + | opt match { + | case Some(value) => () + | case None => () + | } + |} + |""".stripMargin + ) + + reloadWorkspace(id = 55) + compile(buildTarget, id = 66) + + assert( + svr.waitForString(30.seconds) { s => + s.contains("build/publishDiagnostics") && + s.contains("PatternMatch.scala") && + s.contains("\"diagnostics\":[]") + }, + "should send publishDiagnostics with empty diagnostics" ) - assert(!svr.waitForString(30.seconds) { s => - s.contains("build/publishDiagnostics") - }, "shouldn't send publishDiagnostics if there's no change in diagnostics") } test("buildTarget/scalacOptions") { _ => @@ -171,11 +264,7 @@ object BuildServerTest extends AbstractServerTest { .toFile val buildTarget = buildTargetUri("runAndTest", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "43", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + compile(buildTarget, id = 43) svr.waitFor[BspCompileResult](10.seconds) assert(targetDir.list().contains("Main.class")) @@ -233,9 +322,7 @@ object BuildServerTest extends AbstractServerTest { |""".stripMargin ) // reload - svr.sendJsonRpc( - """{ "jsonrpc": "2.0", "id": "52", "method": "workspace/reload"}""" - ) + reloadWorkspace(id = 52) assert( svr.waitForString(10.seconds) { s => s.contains(s""""buildTarget":{"uri":"$metaBuildTarget"}""") && @@ -261,9 +348,7 @@ object BuildServerTest extends AbstractServerTest { |) |""".stripMargin ) - svr.sendJsonRpc( - """{ "jsonrpc": "2.0", "id": "52", "method": "workspace/reload"}""" - ) + reloadWorkspace(id = 52) // assert received an empty diagnostic assert( svr.waitForString(10.seconds) { s => @@ -335,10 +420,10 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/jvmTestEnvironment") { _ => val buildTarget = buildTargetUri("runAndTest", "Test") svr.sendJsonRpc( - s"""|{ "jsonrpc": "2.0", - | "id": "98", - | "method": "buildTarget/jvmTestEnvironment", - | "params": { "targets": [{ "uri": "$buildTarget" }] } + s"""|{ "jsonrpc": "2.0", + | "id": "98", + | "method": "buildTarget/jvmTestEnvironment", + | "params": { "targets": [{ "uri": "$buildTarget" }] } |}""".stripMargin ) assert(processing("buildTarget/jvmTestEnvironment")) @@ -410,11 +495,7 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/compile: report error") { _ => val buildTarget = buildTargetUri("reportError", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "88", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + compile(buildTarget, id = 88) assert(svr.waitForString(10.seconds) { s => (s contains s""""buildTarget":{"uri":"$buildTarget"}""") && (s contains """"severity":1""") && @@ -424,11 +505,7 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/compile: report warning") { _ => val buildTarget = buildTargetUri("reportWarning", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "90", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + compile(buildTarget, id = 90) assert(svr.waitForString(10.seconds) { s => (s contains s""""buildTarget":{"uri":"$buildTarget"}""") && (s contains """"severity":2""") && @@ -438,11 +515,7 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/compile: respond error") { _ => val buildTarget = buildTargetUri("respondError", "Compile") - svr.sendJsonRpc( - s"""{ "jsonrpc": "2.0", "id": "92", "method": "buildTarget/compile", "params": { - | "targets": [{ "uri": "$buildTarget" }] - |} }""".stripMargin - ) + compile(buildTarget, id = 92) assert(svr.waitForString(10.seconds) { s => s.contains(""""id":"92"""") && s.contains(""""error"""") && @@ -487,6 +560,21 @@ object BuildServerTest extends AbstractServerTest { } } + private def reloadWorkspace(id: Int): Unit = + svr.sendJsonRpc( + s"""{ "jsonrpc": "2.0", "id": "$id", "method": "workspace/reload"}""" + ) + + private def compile(buildTarget: URI, id: Int): Unit = + compile(buildTarget.toString, id) + + private def compile(buildTarget: String, id: Int): Unit = + svr.sendJsonRpc( + s"""{ "jsonrpc": "2.0", "id": "$id", "method": "buildTarget/compile", "params": { + | "targets": [{ "uri": "$buildTarget" }] + |} }""".stripMargin + ) + private def buildTargetSources(id: Int, buildTargets: Seq[URI]): String = { val targets = buildTargets.map(BuildTargetIdentifier.apply).toVector request(id, "buildTarget/sources", SourcesParams(targets))