From 8031569559cbd653fda7e6d085ed973bf3bc6e62 Mon Sep 17 00:00:00 2001 From: Laurence Warne Date: Mon, 19 Jun 2023 13:09:33 +0100 Subject: [PATCH] fix: shorten diagnostic ranges for scala-cli Previously, all diagnostic ranges where implicitly expanded to the start/end of a token which meant that scala-cli dependency update diagnostics would span a whole line which could possibly be misleading if multiple dependencies where specified on the same line. --- .../meta/internal/metals/Diagnostics.scala | 41 ++++++++------- .../internal/parsing/TokenEditDistance.scala | 51 +++++++++++++++++-- .../scalacli/BaseScalaCLIActionSuite.scala | 40 +++++++++++++++ .../tests/scalacli/ScalaCliActionsSuite.scala | 14 ++++- 4 files changed, 122 insertions(+), 24 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala b/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala index 8876c34ff24..01f26b156db 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala @@ -260,25 +260,30 @@ final class Diagnostics( d: Diagnostic, snapshot: Input, ): Option[Diagnostic] = { - val result = edit.toRevised(d.getRange).map { range => - val ld = new l.Diagnostic( - range, - d.getMessage, - d.getSeverity, - d.getSource, + val result = edit + .toRevised( + range = d.getRange, + adjustWithinToken = d.getSource() == "scala-cli", ) - // Scala 3 sets the diagnostic code to -1 for NoExplanation Messages. Ideally - // this will change and we won't need this check in the future, but for now - // let's not forward them. - if ( - d.getCode() != null && d - .getCode() - .isLeft() && d.getCode().getLeft() != "-1" - ) - ld.setCode(d.getCode()) - ld.setData(d.getData) - ld - } + .map { range => + val ld = new l.Diagnostic( + range, + d.getMessage, + d.getSeverity, + d.getSource, + ) + // Scala 3 sets the diagnostic code to -1 for NoExplanation Messages. Ideally + // this will change and we won't need this check in the future, but for now + // let's not forward them. + if ( + d.getCode() != null && d + .getCode() + .isLeft() && d.getCode().getLeft() != "-1" + ) + ld.setCode(d.getCode()) + ld.setData(d.getData) + ld + } if (result.isEmpty) { d.getRange.toMeta(snapshot).foreach { pos => val message = pos.formatMessage( diff --git a/metals/src/main/scala/scala/meta/internal/parsing/TokenEditDistance.scala b/metals/src/main/scala/scala/meta/internal/parsing/TokenEditDistance.scala index 6603ef8a56b..fe55bcb5a32 100644 --- a/metals/src/main/scala/scala/meta/internal/parsing/TokenEditDistance.scala +++ b/metals/src/main/scala/scala/meta/internal/parsing/TokenEditDistance.scala @@ -27,8 +27,15 @@ sealed trait TokenEditDistance { * - it should only return `None` in the case when the sources don't tokenize. * When the original token is removed in the revised document, we find instead the * nearest token in the original document instead. + * + * If `adjustWithinToken` is true and the original and revised range are both contained within + * a single (matching) token, then adjust the returned range to match the start/end offsets of + * the input range with respect to the token. */ - def toRevised(range: l.Range): Option[l.Range] + def toRevised( + range: l.Range, + adjustWithinToken: Boolean = false, + ): Option[l.Range] def toRevised(originalOffset: Int): Either[EmptyResult, Position] def toRevised( originalLine: Int, @@ -94,7 +101,10 @@ sealed trait TokenEditDistance { object TokenEditDistance { case object Unchanged extends TokenEditDistance { - def toRevised(range: l.Range): Option[l.Range] = Some(range) + def toRevised( + range: l.Range, + adjustWithinToken: Boolean = false, + ): Option[l.Range] = Some(range) def toRevised(originalOffset: Int): Either[EmptyResult, Position] = EmptyResult.unchanged def toRevised( @@ -116,7 +126,10 @@ object TokenEditDistance { } case object NoMatch extends TokenEditDistance { - def toRevised(range: l.Range): Option[l.Range] = None + def toRevised( + range: l.Range, + adjustWithinToken: Boolean = false, + ): Option[l.Range] = None def toRevised(originalOffset: Int): Either[EmptyResult, Position] = EmptyResult.noMatch def toRevised( @@ -145,7 +158,10 @@ object TokenEditDistance { extends TokenEditDistance { private val logger: Logger = Logger.getLogger(this.getClass.getName) - def toRevised(range: l.Range): Option[l.Range] = { + def toRevised( + range: l.Range, + adjustWithinToken: Boolean = false, + ): Option[l.Range] = { range.toMeta(originalInput).flatMap { pos => val matchingTokens = matching.lift @@ -216,7 +232,10 @@ object TokenEditDistance { val offset = end.revised.start Position.Range(revisedInput, offset - 1, offset) } else if (start.revised == end.revised) { - start.revised.pos + // new range spans one token + if (adjustWithinToken) + computeAdjustmentWithinToken(range, start) + else start.revised.pos } else { val endOffset = end.revised match { case t if t.isLF => t.start @@ -271,6 +290,28 @@ object TokenEditDistance { else if (pos.end <= offset) BinarySearch.Smaller else BinarySearch.Greater + private def computeAdjustmentWithinToken( + original: l.Range, + matching: MatchingToken[A], + ): Position.Range = { + val originalStartPositionOpt: Option[Position] = + original.getStart().toMeta(originalInput) + val originalEndPositionOpt: Option[Position] = + original.getEnd().toMeta(originalInput) + + // Original start range - start of the found token + val startCorrection = + originalStartPositionOpt.fold(0)(_.start - matching.original.start) + val endCorrection = + originalEndPositionOpt.fold(0)(matching.original.end - _.end) + + Position.Range( + revisedInput, + matching.revised.start + startCorrection, + matching.revised.end - endCorrection, + ) + } + override def toString(): String = s"Diff(${matching.length} tokens)" } diff --git a/tests/slow/src/main/scala/tests/scalacli/BaseScalaCLIActionSuite.scala b/tests/slow/src/main/scala/tests/scalacli/BaseScalaCLIActionSuite.scala index 2db12d19a3f..c21a07b2d71 100644 --- a/tests/slow/src/main/scala/tests/scalacli/BaseScalaCLIActionSuite.scala +++ b/tests/slow/src/main/scala/tests/scalacli/BaseScalaCLIActionSuite.scala @@ -8,6 +8,46 @@ import tests.codeactions.BaseCodeActionLspSuite class BaseScalaCLIActionSuite(name: String) extends BaseCodeActionLspSuite(name) { + def checkNoActionScalaCLI( + name: TestOptions, + input: String, + expectNoDiagnostics: Boolean = true, + kind: List[String] = Nil, + scalafixConf: String = "", + scalacOptions: List[String] = Nil, + scalaCliOptions: List[String] = Nil, + configuration: => Option[String] = None, + scalaVersion: String = scalaVersion, + renamePath: Option[String] = None, + extraOperations: => Unit = (), + fileName: String = "A.scala", + changeFile: String => String = identity, + expectError: Boolean = false, + filterAction: CodeAction => Boolean = _ => true, + ): Unit = { + val fileContent = input.replace("<<", "").replace(">>", "") + + checkScalaCLI( + name = name, + input = input, + expectedActions = "", + expectedCode = fileContent, + expectNoDiagnostics = expectNoDiagnostics, + kind = kind, + scalaCliOptions = scalaCliOptions, + configuration = configuration, + scalaVersion = scalaVersion, + scalafixConf = scalafixConf, + scalacOptions = scalacOptions, + renamePath = renamePath, + extraOperations = extraOperations, + fileName = fileName, + changeFile = changeFile, + expectError = expectError, + filterAction = filterAction, + ) + } + def checkScalaCLI( name: TestOptions, input: String, diff --git a/tests/slow/src/test/scala/tests/scalacli/ScalaCliActionsSuite.scala b/tests/slow/src/test/scala/tests/scalacli/ScalaCliActionsSuite.scala index 8534289ae90..23c0e2d6227 100644 --- a/tests/slow/src/test/scala/tests/scalacli/ScalaCliActionsSuite.scala +++ b/tests/slow/src/test/scala/tests/scalacli/ScalaCliActionsSuite.scala @@ -20,7 +20,7 @@ class ScalaCliActionsSuite checkScalaCLI( "actionable-diagnostic-update", - s"""|//> <<>>using lib "com.lihaoyi::os-lib:${oldOsLibVersion.repr}" + s"""|//> using lib "<<>>com.lihaoyi::os-lib:${oldOsLibVersion.repr}" | |object Hello extends App { | println("Hello") @@ -37,6 +37,18 @@ class ScalaCliActionsSuite expectNoDiagnostics = false, ) + checkNoActionScalaCLI( + "actionable-diagnostic-out-of-range", + s"""|//> <<>>using lib "com.lihaoyi::os-lib:${oldOsLibVersion.repr}" + | + |object Hello extends App { + | println("Hello") + |} + |""".stripMargin, + scalaCliOptions = List("--actions", "-S", scalaVersion), + expectNoDiagnostics = false, + ) + checkScalaCLI( "auto-import", s"""|//> using scala "${BuildInfo.scala213}"