From 79e75dc1c2ee5d7a4b0ac9d26a28273b91adffc1 Mon Sep 17 00:00:00 2001 From: Maciej Gajek <61919032+MaciejG604@users.noreply.github.com> Date: Thu, 16 Nov 2023 09:42:01 +0100 Subject: [PATCH] Chore/group warnings about directives in multiple files (#2550) * Add new mechanism for displaying readable diagnostics in CLI * Move warning message out to WarningMessages.scala * Remove assertion that cannot be checked now --- .../main/scala/scala/build/CrossSources.scala | 16 +++-- .../build/internal/util/WarningMessages.scala | 16 +++++ .../scala/scala/cli/internal/CliLogger.scala | 8 +++ .../src/main/scala/scala/build/Logger.scala | 7 ++ .../cli/integration/BspTestDefinitions.scala | 71 +++++++++++++++++++ .../scala/cli/integration/FixTests.scala | 10 --- .../cli/integration/RunTestDefinitions.scala | 9 ++- 7 files changed, 120 insertions(+), 17 deletions(-) diff --git a/modules/build/src/main/scala/scala/build/CrossSources.scala b/modules/build/src/main/scala/scala/build/CrossSources.scala index bccdfe6e11..62bffee541 100644 --- a/modules/build/src/main/scala/scala/build/CrossSources.scala +++ b/modules/build/src/main/scala/scala/build/CrossSources.scala @@ -346,10 +346,18 @@ object CrossSources { .values .flatten .filter((path, _) => ScopePath.fromPath(path) != ScopePath.fromPath(projectFilePath)) - .foreach { (_, directivesPositions) => - logger.diagnostic( - s"Using directives detected in multiple files. It is recommended to keep them centralized in the $projectFilePath file.", - positions = Seq(directivesPositions) + .pipe { pathsToReport => + val diagnosticMessage = WarningMessages + .directivesInMultipleFilesWarning(projectFilePath.toString) + val cliFriendlyMessage = WarningMessages.directivesInMultipleFilesWarning( + projectFilePath.toString, + pathsToReport.map(_._2.render()) + ) + + logger.cliFriendlyDiagnostic( + message = diagnosticMessage, + cliFriendlyMessage = cliFriendlyMessage, + positions = pathsToReport.map(_._2).toSeq ) } } diff --git a/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala b/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala index 611f4b619d..92e0d33e53 100644 --- a/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala +++ b/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala @@ -104,4 +104,20 @@ object WarningMessages { val offlineModeBloopJvmNotFound = "Offline mode is ON and a JVM for Bloop could not be fetched from the local cache, using scalac as fallback" + + def directivesInMultipleFilesWarning( + projectFilePath: String, + pathsToReport: Iterable[String] = Nil + ) = { + val detectedMsg = "Using directives detected in multiple files" + val recommendedMsg = + s"It is recommended to keep them centralized in the $projectFilePath file." + if pathsToReport.isEmpty then + s"$detectedMsg. $recommendedMsg" + else + s"""$detectedMsg: + |${pathsToReport.mkString("- ", s"${System.lineSeparator}- ", "")} + |$recommendedMsg + |""".stripMargin + } } diff --git a/modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala b/modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala index 07d2d7e856..3016224b93 100644 --- a/modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala +++ b/modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala @@ -233,6 +233,14 @@ class CliLogger( } yield featureType -> (names ++ reportedNames) experimentalWarnings = Map.empty } + + override def cliFriendlyDiagnostic( + message: String, + cliFriendlyMessage: String, + severity: Severity, + positions: Seq[Position] + ): Unit = + diagnostic(cliFriendlyMessage, severity, Nil) } object CliLogger { diff --git a/modules/core/src/main/scala/scala/build/Logger.scala b/modules/core/src/main/scala/scala/build/Logger.scala index 954b00cfde..35b863aaee 100644 --- a/modules/core/src/main/scala/scala/build/Logger.scala +++ b/modules/core/src/main/scala/scala/build/Logger.scala @@ -44,6 +44,13 @@ trait Logger { */ def experimentalWarning(featureName: String, featureType: FeatureType): Unit def flushExperimentalWarnings: Unit + + def cliFriendlyDiagnostic( + message: String, + cliFriendlyMessage: String, + severity: Severity = Severity.Warning, + positions: Seq[Position] = Nil + ): Unit = diagnostic(message, severity, positions) } object Logger { diff --git a/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala index e0448c506c..5414d95ebf 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala @@ -597,6 +597,77 @@ abstract class BspTestDefinitions(val scalaVersionOpt: Option[String]) } } + test("directives in multiple files diagnostics") { + val inputs = TestInputs( + os.rel / "Foo.scala" -> + s"""//> using scala "3.3.0" + | + |object Foo extends App { + | println("Foo") + |} + |""".stripMargin, + os.rel / "Bar.scala" -> "", + os.rel / "Hello.java" -> "//> using jvm \"11\"" + ) + + withBsp(inputs, Seq(".")) { (root, localClient, remoteServer) => + async { + await(remoteServer.workspaceBuildTargets().asScala) + val buildTargetsResp = await(remoteServer.workspaceBuildTargets().asScala) + val target = { + val targets = buildTargetsResp.getTargets.asScala.map(_.getId).toSeq + expect(targets.length == 2) + extractMainTargets(targets) + } + + val targetUri = TestUtil.normalizeUri(target.getUri) + checkTargetUri(root, targetUri) + + val targets = List(target).asJava + + val compileResp = await { + remoteServer + .buildTargetCompile(new b.CompileParams(targets)) + .asScala + } + expect(compileResp.getStatusCode == b.StatusCode.OK) + + def checkDirectivesInMultipleFilesWarnings( + fileName: String, + expectedStartLine: Int, + expectedStartCharacter: Int, + expectedEndLine: Int, + expectedEndCharacter: Int + ): Unit = { + val diagnosticsParams = localClient.diagnostics().collectFirst { + case diag + if !diag.getDiagnostics.isEmpty && + TestUtil.normalizeUri(diag.getTextDocument.getUri) == + TestUtil.normalizeUri((root / fileName).toNIO.toUri.toASCIIString) => diag + } + expect(diagnosticsParams.isDefined) + val diagnostics = diagnosticsParams.get.getDiagnostics.asScala.toSeq + + val expectedMessage = + "Using directives detected in multiple files. It is recommended to keep them centralized in the" + checkDiagnostic( + diagnostic = diagnostics.head, + expectedMessage = expectedMessage, + expectedSeverity = b.DiagnosticSeverity.WARNING, + expectedStartLine = expectedStartLine, + expectedStartCharacter = expectedStartCharacter, + expectedEndLine = expectedEndLine, + expectedEndCharacter = expectedEndCharacter, + strictlyCheckMessage = false + ) + } + + checkDirectivesInMultipleFilesWarnings("Foo.scala", 0, 0, 0, 23) + checkDirectivesInMultipleFilesWarnings("Hello.java", 0, 0, 0, 18) + } + } + } + test("workspace update") { val inputs = TestInputs( os.rel / "simple.sc" -> diff --git a/modules/integration/src/test/scala/scala/cli/integration/FixTests.scala b/modules/integration/src/test/scala/scala/cli/integration/FixTests.scala index 0a342d8746..0d8f9d0ec4 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/FixTests.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/FixTests.scala @@ -383,16 +383,6 @@ class FixTests extends ScalaCliSuite { assertNoDiff(withUsedTargetContents, withUsedTargetContentsRead) assertNoDiff(withUnusedTargetContents, withUnusedTargetContentsRead) - - val runProc = os.proc(TestUtil.cli, "--power", "compile", ".") - .call(cwd = root, stderr = os.Pipe) - - val runErrOut = TestUtil.removeAnsiColors(runProc.err.trim) - expect(runErrOut.contains("Using directives detected in multiple files")) - expect(runErrOut.linesIterator.count(_.startsWith("[warn] //> using")) == 2) - expect(runErrOut.contains("[warn] //> using options -Werror")) - // TODO: Warning about using directives in multiple files for the test scope should not be displayed - expect(runErrOut.contains("[warn] //> using scala \"3.2.2\"")) } assertNoDiff( diff --git a/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala index e82e4ef67a..8beb2c8232 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala @@ -768,14 +768,17 @@ abstract class RunTestDefinitions(val scalaVersionOpt: Option[String]) os.rel / "Hello.java" -> "//> using jvm \"11\"" ) inputs.fromRoot { root => - val warningMessage = "Using directives detected in" - val output1 = os.proc(TestUtil.cli, ".").call(cwd = root, stderr = os.Pipe).err.trim() + val warningMessage = + """Using directives detected in multiple files: + |- Foo.scala:1:1-24 + |- Hello.java:1:1-19""".stripMargin + val output1 = os.proc(TestUtil.cli, ".").call(cwd = root, stderr = os.Pipe).err.trim() val output2 = os.proc(TestUtil.cli, "Foo.scala", "Bar.scala").call( cwd = root, stderr = os.Pipe ).err.trim() expect(output1.contains(warningMessage)) - expect(!output2.contains(warningMessage)) + expect(!output2.contains("Using directives detected in multiple files")) } }