fix: correctly report error position on unknown directive without values (#3518)

* fix: correctly report error position on unknown directive without values

* revert unneeded change

Author: kasiaMarek

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

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)

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(

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)
     )

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[_]],

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
     )
   }
 }

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("  ")}"

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
           )
         }
     }

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
+      )
+    }
+  }
 }