From 5b44e427735fd650ca4886ee62aa98e627b04710 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Tue, 25 Feb 2025 10:19:01 +0100 Subject: [PATCH 1/2] fix: correctly report error position on unknown directive without values --- .../DirectivesPreprocessor.scala | 2 +- .../preprocessing/ExtractedDirectives.scala | 12 +++-- .../scala/cli/commands/fix/BuiltInRules.scala | 2 +- .../build/errors/UnusedDirectiveError.scala | 8 ++-- .../scala/scala/build/internals/OsLibc.scala | 4 +- .../directives/DirectiveUtil.scala | 16 ++----- .../directives/ScopedDirective.scala | 13 +++-- .../directives/StrictDirective.scala | 3 +- .../cli/integration/BspTestDefinitions.scala | 8 ++-- .../integration/CompileTestDefinitions.scala | 48 +++++++++++++++++++ 10 files changed, 83 insertions(+), 33 deletions(-) diff --git a/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala b/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala index 6119ae5b1b..15bde02c3b 100644 --- a/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala +++ b/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala @@ -147,7 +147,7 @@ case class DirectivesPreprocessor( val res = directives .iterator .flatMap { - case d @ StrictDirective(k, _, _) => + case d @ StrictDirective(k, _, _, _) => handlersMap.get(k).iterator.map(_(ScopedDirective(d, path, cwd), logger)) } .toVector diff --git a/modules/build/src/main/scala/scala/build/preprocessing/ExtractedDirectives.scala b/modules/build/src/main/scala/scala/build/preprocessing/ExtractedDirectives.scala index 61f3ce1954..f347b91c17 100644 --- a/modules/build/src/main/scala/scala/build/preprocessing/ExtractedDirectives.scala +++ b/modules/build/src/main/scala/scala/build/preprocessing/ExtractedDirectives.scala @@ -77,11 +77,13 @@ object ExtractedDirectives { case sl: StringLiteral => Seq(StringValue(sl.getValue(), sl)) case bl: BooleanLiteral => Seq(BooleanValue(bl.getValue(), bl)) } - def toStrictDirective(ud: UsingDef) = StrictDirective( - ud.getKey(), - toStrictValue(ud.getValue()), - ud.getPosition().getColumn() - ) + def toStrictDirective(ud: UsingDef) = + StrictDirective( + ud.getKey(), + toStrictValue(ud.getValue()), + ud.getPosition().getColumn(), + ud.getPosition().getLine() + ) directives.getAst match case uds: UsingDefs => uds.getUsingDefs.asScala.toSeq.map(toStrictDirective) diff --git a/modules/cli/src/main/scala/scala/cli/commands/fix/BuiltInRules.scala b/modules/cli/src/main/scala/scala/cli/commands/fix/BuiltInRules.scala index 974ce708a8..ae88854602 100644 --- a/modules/cli/src/main/scala/scala/cli/commands/fix/BuiltInRules.scala +++ b/modules/cli/src/main/scala/scala/cli/commands/fix/BuiltInRules.scala @@ -286,7 +286,7 @@ object BuiltInRules extends CommandHelpers { val (withTestEquivalent, noTestEquivalent) = noInitialTestPrefix.partition(_.existsTestEquivalent) val transformedToTestEquivalents = withTestEquivalent.map { - case StrictDirective(key, values, _) => StrictDirective("test." + key, values) + case StrictDirective(key, values, _, _) => StrictDirective("test." + key, values) } TransformedTestDirectives( diff --git a/modules/core/src/main/scala/scala/build/errors/UnusedDirectiveError.scala b/modules/core/src/main/scala/scala/build/errors/UnusedDirectiveError.scala index 44d6d4474d..58806a40ab 100644 --- a/modules/core/src/main/scala/scala/build/errors/UnusedDirectiveError.scala +++ b/modules/core/src/main/scala/scala/build/errors/UnusedDirectiveError.scala @@ -2,8 +2,10 @@ package scala.build.errors import scala.build.Position -final class UnusedDirectiveError(key: String, values: Seq[String], positions: Seq[Position]) +final class UnusedDirectiveError(key: String, values: Seq[String], position: Position) extends BuildException( - s"Unrecognized directive: $key with values: ${values.mkString(", ")}", - positions = positions + s"Unrecognized directive: $key${ + if values.isEmpty then "" else s" with values: ${values.mkString(", ")}" + }", + positions = List(position) ) diff --git a/modules/core/src/main/scala/scala/build/internals/OsLibc.scala b/modules/core/src/main/scala/scala/build/internals/OsLibc.scala index 9e9613c4ad..9c6561d1e8 100644 --- a/modules/core/src/main/scala/scala/build/internals/OsLibc.scala +++ b/modules/core/src/main/scala/scala/build/internals/OsLibc.scala @@ -1,7 +1,7 @@ package scala.build.internal import bloop.rifle.VersionUtil.parseJavaVersion -import coursier.jvm.{JavaHome, JvmChannel} +import coursier.jvm.{JavaHome, JvmChannel, JvmIndex} import java.io.IOException import java.nio.charset.Charset @@ -56,7 +56,7 @@ object OsLibc { // FIXME These values should be the default ones in coursier-jvm lazy val jvmIndexOs: String = { - val default = JvmChannel.defaultOs + val default = JvmIndex.defaultOs if (default == "linux" && isMusl.getOrElse(false)) "linux-musl" else default } diff --git a/modules/directives/src/main/scala/scala/build/preprocessing/directives/DirectiveUtil.scala b/modules/directives/src/main/scala/scala/build/preprocessing/directives/DirectiveUtil.scala index 7df6a9b6b3..f32b334e58 100644 --- a/modules/directives/src/main/scala/scala/build/preprocessing/directives/DirectiveUtil.scala +++ b/modules/directives/src/main/scala/scala/build/preprocessing/directives/DirectiveUtil.scala @@ -38,18 +38,10 @@ object DirectiveUtil { def concatAllValues( scopedDirective: ScopedDirective - ): Seq[Positioned[String]] = - scopedDirective.directive.values.map { - case v: StringValue => - val pos = position(v, scopedDirective.maybePath) - Positioned(pos, v.get) - case v: BooleanValue => - val pos = position(v, scopedDirective.maybePath) - Positioned(pos, v.get.toString) - case v: EmptyValue => - val pos = position(v, scopedDirective.maybePath) - Positioned(pos, v.get) - } + ): Seq[String] = + scopedDirective.directive.values.collect: + case v: StringValue => v.get + case v: BooleanValue => v.get.toString def positions( values: Seq[Value[_]], diff --git a/modules/directives/src/main/scala/scala/build/preprocessing/directives/ScopedDirective.scala b/modules/directives/src/main/scala/scala/build/preprocessing/directives/ScopedDirective.scala index 48d9e30bab..2240faef9c 100644 --- a/modules/directives/src/main/scala/scala/build/preprocessing/directives/ScopedDirective.scala +++ b/modules/directives/src/main/scala/scala/build/preprocessing/directives/ScopedDirective.scala @@ -1,5 +1,6 @@ package scala.build.preprocessing.directives +import scala.build.Position import scala.build.errors.UnusedDirectiveError import scala.build.preprocessing.ScopePath @@ -9,12 +10,16 @@ case class ScopedDirective( cwd: ScopePath ) { def unusedDirectiveError: UnusedDirectiveError = { - val values = - DirectiveUtil.concatAllValues(this) + val values = DirectiveUtil.concatAllValues(this) + val keyPos = Position.File( + maybePath, + (directive.startLine, directive.startColumn), + (directive.startLine, directive.startColumn + directive.key.length()) + ) new UnusedDirectiveError( directive.key, - values.map(_.value), - values.flatMap(_.positions) + values, + keyPos ) } } diff --git a/modules/directives/src/main/scala/scala/build/preprocessing/directives/StrictDirective.scala b/modules/directives/src/main/scala/scala/build/preprocessing/directives/StrictDirective.scala index 37f154cf1e..68b0ce640e 100644 --- a/modules/directives/src/main/scala/scala/build/preprocessing/directives/StrictDirective.scala +++ b/modules/directives/src/main/scala/scala/build/preprocessing/directives/StrictDirective.scala @@ -17,7 +17,8 @@ import scala.build.Position case class StrictDirective( key: String, values: Seq[Value[?]], - startColumn: Int = 0 + startColumn: Int = 0, + startLine: Int = 0 ) { override def toString: String = { val suffix = if validValues.isEmpty then "" else s" ${validValues.mkString(" ")}" 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 e9dd6975bb..ebb54abf7a 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala @@ -402,9 +402,9 @@ abstract class BspTestDefinitions extends ScalaCliSuite with TestScalaVersionArg expectedMessage = "Unrecognized directive: resource with values: ./resources", expectedSeverity = b.DiagnosticSeverity.ERROR, expectedStartLine = 0, - expectedStartCharacter = 19, + expectedStartCharacter = 10, expectedEndLine = 0, - expectedEndCharacter = 30, + expectedEndCharacter = 18, strictlyCheckMessage = false ) } @@ -1374,9 +1374,9 @@ abstract class BspTestDefinitions extends ScalaCliSuite with TestScalaVersionArg s"Unrecognized directive: $directiveKey with values: $directiveValue", expectedSeverity = b.DiagnosticSeverity.ERROR, expectedStartLine = 0, - expectedStartCharacter = 33, + expectedStartCharacter = 10, expectedEndLine = 0, - expectedEndCharacter = 38 + expectedEndCharacter = 32 ) } } diff --git a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala index 2af6f3e902..23a3ccc5e5 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/CompileTestDefinitions.scala @@ -821,4 +821,52 @@ abstract class CompileTestDefinitions } } + + test("i3389") { + val filename = "Main.scala" + val inputs = TestInputs( + os.rel / filename -> + """//> using optionsdeprecation + |""".stripMargin + ) + + inputs.fromRoot { root => + val result = os.proc(TestUtil.cli, "compile", ".", extraOptions).call( + cwd = root, + check = false, + mergeErrIntoOut = true + ) + assertEquals( + TestUtil.fullStableOutput(result).trim(), + s"""|[error] .${File.separatorChar}Main.scala:1:11 + |[error] Unrecognized directive: optionsdeprecation + |[error] //> using optionsdeprecation + |[error] ^^^^^^^^^^^^^^^^^^""".stripMargin + ) + } + } + + test("i3389-2") { + val filename = "Main.scala" + val inputs = TestInputs( + os.rel / filename -> + """//> using unrecognised.directive value1 value2 + |""".stripMargin + ) + + inputs.fromRoot { root => + val result = os.proc(TestUtil.cli, "compile", ".", extraOptions).call( + cwd = root, + check = false, + mergeErrIntoOut = true + ) + assertEquals( + TestUtil.fullStableOutput(result).trim(), + s"""|[error] .${File.separatorChar}Main.scala:1:11 + |[error] Unrecognized directive: unrecognised.directive with values: value1, value2 + |[error] //> using unrecognised.directive value1 value2 + |[error] ^^^^^^^^^^^^^^^^^^^^^^""".stripMargin + ) + } + } } From b14f3cd98dc54a02db6209b90c3e6c9bcffecc20 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 26 Feb 2025 10:20:34 +0100 Subject: [PATCH 2/2] revert unneeded change --- .../core/src/main/scala/scala/build/internals/OsLibc.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/src/main/scala/scala/build/internals/OsLibc.scala b/modules/core/src/main/scala/scala/build/internals/OsLibc.scala index 9c6561d1e8..9e9613c4ad 100644 --- a/modules/core/src/main/scala/scala/build/internals/OsLibc.scala +++ b/modules/core/src/main/scala/scala/build/internals/OsLibc.scala @@ -1,7 +1,7 @@ package scala.build.internal import bloop.rifle.VersionUtil.parseJavaVersion -import coursier.jvm.{JavaHome, JvmChannel, JvmIndex} +import coursier.jvm.{JavaHome, JvmChannel} import java.io.IOException import java.nio.charset.Charset @@ -56,7 +56,7 @@ object OsLibc { // FIXME These values should be the default ones in coursier-jvm lazy val jvmIndexOs: String = { - val default = JvmIndex.defaultOs + val default = JvmChannel.defaultOs if (default == "linux" && isMusl.getOrElse(false)) "linux-musl" else default }