From b189d49968201a3f41eb47da8c8def805e8b9cdd Mon Sep 17 00:00:00 2001 From: Adrien Piquerez Date: Wed, 25 Oct 2023 11:39:36 +0200 Subject: [PATCH] Refactoring --- .../internal/server/BuildServerProtocol.scala | 13 +- .../internal/server/BuildServerReporter.scala | 174 ++++++------------ .../twirlProj/src/main/twirl/main.scala.html | 14 ++ .../src/main/twirl/vHostHttpToHttps.scala.txt | 16 -- .../test/scala/testpkg/BuildServerTest.scala | 49 +++-- 5 files changed, 99 insertions(+), 167 deletions(-) create mode 100644 server-test/src/server-test/buildserver/twirlProj/src/main/twirl/main.scala.html delete mode 100644 server-test/src/server-test/buildserver/twirlProj/src/main/twirl/vHostHttpToHttps.scala.txt diff --git a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala index 36d3f437b..88e07d77d 100644 --- a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala +++ b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala @@ -1067,13 +1067,10 @@ object BuildServerProtocol { private[server] final class BspCompileState { /** - * @param problemsBySourceFiles keeps track of problems in a given file by mapping its VirtualFileRef to all the TextDocumentIdentifier affected by this problem. - * e.g., Suppose we have a source file at "target/../twirl/file.template.scala" which is generated by a Twirl file "src/main/twirl/file.scala.txt",the VirtualRef will be the ref of the Scala file, - * where the TextDocumentIdentifier will be pointing to the Twirl file .In case of a non-generated Scala source file , the textDocumentIdentifier will point to the same document . - * @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 + * keeps track of problems in a given file in a map of virtual source file to text documents. + * In most cases the only text document is the source file. In case of source generation, + * e.g. Twirl, the text documents are the input files, e.g. the Twirl files. + * We use the sourcePositionMappers to build this map. */ val problemsBySourceFiles : AtomicReference[Map[VirtualFileRef, Vector[TextDocumentIdentifier]]] = @@ -1085,6 +1082,6 @@ object BuildServerProtocol { * are compiled for the first time. * see: https://github.com/scalacenter/bloop/issues/726 */ - val compiledAtLeastOnce: AtomicBoolean = new AtomicBoolean(false) + val isFirstReport: AtomicBoolean = new AtomicBoolean(true) } } diff --git a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala index 2469cc21a..5b25d17b0 100644 --- a/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala +++ b/main/src/main/scala/sbt/internal/server/BuildServerReporter.scala @@ -8,8 +8,6 @@ package sbt.internal.server -import java.nio.file.Path - import sbt.StandardMain import sbt.internal.bsp._ import sbt.internal.util.ManagedLogger @@ -75,16 +73,9 @@ sealed trait BuildServerReporter extends Reporter { } /** - -An implementation of the BuildServerReporter for communicating with the Build Server. -Sends diagnostic messages to the client, handling success and failure cases. -@param buildTarget the identifier of the build target -@param bspCompileState state representing what has already been reported in previous compilation. -@param converter a file converter for converting between VirtualFileRef and Path -@param sourcePositionMapper a function to map an xsbti.Position from the generated file (the Scala file) to the source file of the generator (e.g. Twirl file) -@param isMetaBuild a flag indicating if this is a meta build -@param logger a ManagedLogger for logging messages -@param underlying the underlying reporter instance which reports to the sbt shell or native clients + * @param bspCompileState what has already been reported in previous compilation. + * @param sourcePositionMapper a function that maps an xsbti.Position from the generated source + * (the Scala file) to the input file of the generator (e.g. Twirl file) */ final class BuildServerReporterImpl( buildTarget: BuildTargetIdentifier, @@ -99,15 +90,13 @@ final class BuildServerReporterImpl( import sbt.internal.inc.JavaInterfaceUtil._ private lazy val exchange = StandardMain.exchange - //keeps track of problems in a given file by mapping its VirtualFileRef to a Vector of problems. - //N.B : In case of a source generator file (Twirl), the given file is the generated one (Scala). private val problemsByFile = mutable.Map[VirtualFileRef, Vector[Problem]]() // sometimes the compiler returns a fake position such as // on Windows, this causes InvalidPathException (see #5994 and #6720) - private def toSafePath(ref: VirtualFileRef): Option[Path] = + private def toDocument(ref: VirtualFileRef): Option[TextDocumentIdentifier] = if (ref.id().contains("<")) None - else Some(converter.toPath(ref)) + else Some(TextDocumentIdentifier(converter.toPath(ref).toUri)) /** * Send diagnostics from the compilation to the client. @@ -115,104 +104,46 @@ final class BuildServerReporterImpl( * * @param analysis current compile analysis */ - override def sendSuccessReport( - analysis: CompileAnalysis, - ): Unit = { - val shouldReportAllProblems = !bspCompileState.compiledAtLeastOnce.getAndSet(true) - for { - (source, infos) <- analysis.readSourceInfos.getAllSourceInfos.asScala - sourcePath <- toSafePath(source) - } { - // clear problems for current file - val oldDocuments = - bspCompileState.problemsBySourceFiles.getAndUpdate(_ - source).getOrElse(source, Seq.empty) - + override def sendSuccessReport(analysis: CompileAnalysis): Unit = { + for ((source, infos) <- analysis.readSourceInfos.getAllSourceInfos.asScala) { val problems = infos.getReportedProblems.toVector + sendReport(source, problems) + } + notifyFirstReport() + } - // 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 = oldDocuments.nonEmpty || problems.nonEmpty || shouldReportAllProblems - - if (shouldPublish) { - // Group diagnostics by document - val diagsByDocuments = problems - .flatMap(mapProblemToDiagnostic) - .groupBy { case (document, _) => document } - .mapValues(_.map { case (_, diag) => diag }) - - //Get a set of these diagnostics to remove duplicates - val newDocuments = diagsByDocuments.keySet - - bspCompileState.problemsBySourceFiles.updateAndGet(_ + (source -> newDocuments.toVector)) - - val sourceDocument = TextDocumentIdentifier(sourcePath.toUri) - val allDocuments = (newDocuments ++ oldDocuments + sourceDocument) - // Iterate through both new and old documents, sending diagnostics for each - allDocuments.foreach { document => - val diags: Vector[Diagnostic] = diagsByDocuments - .getOrElse(document, Vector.empty) - val params = PublishDiagnosticsParams( - document, - buildTarget, - originId = None, - diags, - reset = true - ) - // Notify the client with the diagnostics - exchange.notifyEvent("build/publishDiagnostics", params) - } - } + override def sendFailureReport(sources: Array[VirtualFile]): Unit = { + for (source <- sources) { + val problems = problemsByFile.getOrElse(source, Vector.empty) + sendReport(source, problems) } } - /** - *This method sends a failure report to the client when the compilation fails. It takes an array of virtual files - *as the input parameter and processes the reported problems for each source. - @param sources an array of virtual files representing the source files - */ - override def sendFailureReport(sources: Array[VirtualFile]): Unit = { - val shouldReportAllProblems = !bspCompileState.compiledAtLeastOnce.get - // Iterate through all source files - for { - source <- sources - } { - // Get the problems associated with the current source file - val problems = problemsByFile.getOrElse(source, Vector.empty) + private def sendReport(source: VirtualFileRef, problems: Vector[Problem]): Unit = { + val oldDocuments = getAndClearPreviousDocuments(source) - val oldDocuments = - bspCompileState.problemsBySourceFiles.getAndUpdate(_ - source).getOrElse(source, Seq.empty) - // Determine if diagnostics should be published - // 1. The file had problems previously - we might want to update them with new ones - // 2. The file has fresh problems - we might want to update old ones - // 3. The build project is compiled for the first time - shouldReportAllProblems is set - val shouldPublish = oldDocuments.nonEmpty || problems.nonEmpty || shouldReportAllProblems + // publish diagnostics if: + // 1. file had any problems previously: update them with new ones + // 2. file has fresh problems: report them + // 3. build project is compiled for the first time: send success report + if (oldDocuments.nonEmpty || problems.nonEmpty || isFirstReport) { + val diagsByDocuments = problems + .flatMap(mapProblemToDiagnostic) + .groupBy { case (document, _) => document } + .mapValues(_.map { case (_, diag) => diag }) + updateNewDocuments(source, diagsByDocuments.keys.toVector) - if (shouldPublish) { - // Group diagnostics by document - val diagsByDocuments = problems - .flatMap(mapProblemToDiagnostic) - .groupBy { case (document, _) => document } - .mapValues(_.map { case (_, diag) => diag }) - - val newDocuments = diagsByDocuments.keySet - - bspCompileState.problemsBySourceFiles.updateAndGet(_ + (source -> newDocuments.toVector)) - - // Iterate through both new and old documents, sending diagnostics for each - (newDocuments ++ oldDocuments).foreach { document => - val diags: Vector[Diagnostic] = diagsByDocuments - .getOrElse(document, Vector.empty) - val params = PublishDiagnosticsParams( - document, - buildTarget, - originId = None, - diags, - reset = true - ) - exchange.notifyEvent("build/publishDiagnostics", params) - } + // send a report for the new documents, the old ones and the source file + (diagsByDocuments.keySet ++ oldDocuments ++ toDocument(source)).foreach { document => + val diags = diagsByDocuments.getOrElse(document, Vector.empty) + val params = PublishDiagnosticsParams( + document, + buildTarget, + originId = None, + diags, + reset = true + ) + exchange.notifyEvent("build/publishDiagnostics", params) } } } @@ -220,7 +151,6 @@ final class BuildServerReporterImpl( protected override def publishDiagnostic(problem: Problem): Unit = { for { id <- problem.position.sourcePath.toOption - // mapProblemToDiagnostic maps the position in the Scala source file back to the source of the generator that generated this Scala file. (document, diagnostic) <- mapProblemToDiagnostic(problem) } { val fileRef = VirtualFileRef.of(id) @@ -237,20 +167,32 @@ final class BuildServerReporterImpl( } } - /** + private def getAndClearPreviousDocuments(source: VirtualFileRef): Seq[TextDocumentIdentifier] = + bspCompileState.problemsBySourceFiles.getAndUpdate(_ - source).getOrElse(source, Seq.empty) - * This function maps a given problem in a Scala source file to a diagnostic in the source of the generator that generated this Scala - * or the the file itself in case it was not generated. - * @param problem the problem to be converted into a diagnostic + private def updateNewDocuments( + source: VirtualFileRef, + documents: Vector[TextDocumentIdentifier] + ): Unit = { + val _ = bspCompileState.problemsBySourceFiles.updateAndGet(_ + (source -> documents)) + } + + private def isFirstReport: Boolean = bspCompileState.isFirstReport.get + private def notifyFirstReport(): Unit = { + val _ = bspCompileState.isFirstReport.set(false) + } + + /** + * Map a given problem, in a Scala source file, to a Diagnostic in an user-facing source file. + * E.g. if the source file is generated from Twirl, the diagnostic will be reported to the Twirl file. */ private def mapProblemToDiagnostic( problem: Problem ): Option[(TextDocumentIdentifier, Diagnostic)] = { - // Map the position of the problem from the generated file to the origin , this way we send the original position of the problem instead of the generated one val mappedPosition = sourcePositionMapper(problem.position) for { - id <- mappedPosition.sourcePath.toOption - path <- toSafePath(VirtualFileRef.of(id)) + mappedSource <- mappedPosition.sourcePath.toOption + document <- toDocument(VirtualFileRef.of(mappedSource)) } yield { val diagnostic = Diagnostic( toRange(mappedPosition), @@ -259,7 +201,7 @@ final class BuildServerReporterImpl( Option("sbt"), problem.message ) - (TextDocumentIdentifier(path.toUri), diagnostic) + (document, diagnostic) } } diff --git a/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/main.scala.html b/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/main.scala.html new file mode 100644 index 000000000..ca8f06954 --- /dev/null +++ b/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/main.scala.html @@ -0,0 +1,14 @@ +@(title: String, paragraphs: Seq[String]) + + + + + @title + + +

@tilte

+ @for(paragraph <- paragraphs) { +

@paragraph

+ } + + diff --git a/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/vHostHttpToHttps.scala.txt b/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/vHostHttpToHttps.scala.txt deleted file mode 100644 index 325c34e56..000000000 --- a/server-test/src/server-test/buildserver/twirlProj/src/main/twirl/vHostHttpToHttps.scala.txt +++ /dev/null @@ -1,16 +0,0 @@ -@(subDomain: String, mainDomain: String, redirectStatusCode: Int) -@fullDomain=@{ - if(mainDomaiiiiin) { - mainDomain - } else { - s"$subDomain.$mainDomain" - } -} - - - - ServerName @fullDomain - RewriteEngine On - RewriteRule (.*) "https://%{HTTP_HOST}%{REQUEST_URI}" [R=@redirectStatusCode,L] - - \ No newline at end of file diff --git a/server-test/src/test/scala/testpkg/BuildServerTest.scala b/server-test/src/test/scala/testpkg/BuildServerTest.scala index 58199f826..1fb2d3aeb 100644 --- a/server-test/src/test/scala/testpkg/BuildServerTest.scala +++ b/server-test/src/test/scala/testpkg/BuildServerTest.scala @@ -596,53 +596,48 @@ object BuildServerTest extends AbstractServerTest { assert(actualResult == expectedResult) } - test("buildTarget/compile twirl diagnostics (sourcePositionMapping)") { _ => + test("buildTarget/compile: twirl diagnostics (sourcePositionMappers)") { _ => val buildTarget = buildTargetUri("twirlProj", "Compile") - val testFile = - new File(svr.baseDirectory, s"twirlProj/src/main/twirl/vHostHttpToHttps.scala.txt") + val testFile = new File(svr.baseDirectory, s"twirlProj/src/main/twirl/main.scala.html") compile(buildTarget) - assert( svr.waitForString(10.seconds) { s => s.contains("build/publishDiagnostics") && - s.contains("vHostHttpToHttps.scala.txt") && + s.contains("main.scala.html") && s.contains(""""severity":1""") && - s.contains("""not found: value mainDomaiiiiin""") + s.contains("not found: value tilte") }, - "should send publishDiagnostics with serverity 1 for vHostHttpToHttps.scala.txt " + "should report diagnostic in Twirl file" ) + IO.write( testFile, - """|@(subDomain: String, mainDomain: String, redirectStatusCode: Int) - |@fullDomain=@{ - | if(mainDomain) { - | mainDomain - | } else { - | s"$subDomain.$mainDomain" - | } - |} + """|@(title: String, paragraphs: Seq[String]) | - | - | - | ServerName @fullDomain - | RewriteEngine On - | RewriteRule (.*) "https://%{HTTP_HOST}%{REQUEST_URI}" [R=@redirectStatusCode,L] - | - | + | + | + | + | @title + | + | + |

@title

+ | @for(paragraph <- paragraphs) { + |

@paragraph

+ | } + | + | |""".stripMargin ) - compile(buildTarget) - - assert( + assert.apply( svr.waitForString(30.seconds) { s => s.contains("build/publishDiagnostics") && - s.contains("vHostHttpToHttps.scala.txt") && + s.contains("main.scala.html") && s.contains(""""diagnostics":[]""") && s.contains(""""reset":true""") }, - "should send publishDiagnostics with empty diagnostics" + "should reset diagnostic in Twirl file" ) }