Skip to content

Commit

Permalink
fix: shorten diagnostic ranges for scala-cli
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
LaurenceWarne authored and tgodzik committed Jun 21, 2023
1 parent 3d89198 commit 8031569
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 24 deletions.
41 changes: 23 additions & 18 deletions metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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}"
Expand Down

0 comments on commit 8031569

Please sign in to comment.