Skip to content

Commit

Permalink
Chore/group warnings about directives in multiple files (#2550)
Browse files Browse the repository at this point in the history
* Add new mechanism for displaying readable diagnostics in CLI

* Move warning message out to WarningMessages.scala

* Remove assertion that cannot be checked now
  • Loading branch information
MaciejG604 authored Nov 16, 2023
1 parent 9b7a75d commit 79e75dc
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 17 deletions.
16 changes: 12 additions & 4 deletions modules/build/src/main/scala/scala/build/CrossSources.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
8 changes: 8 additions & 0 deletions modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions modules/core/src/main/scala/scala/build/Logger.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
}

Expand Down

0 comments on commit 79e75dc

Please sign in to comment.