Skip to content

Commit

Permalink
bugfix: Don't report tokenizer errors as stale
Browse files Browse the repository at this point in the history
Previously, we would log tokenizer errors as stale. Now, we only report stale errors if not match could be found, but still would be tokenized.

I added a more explicit return type when creating EditDistance, it's not neccessary to change it everywhere right now I think.
  • Loading branch information
tgodzik committed Aug 10, 2023
1 parent b0aa2da commit 4b42618
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 94 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,10 @@ final class SyntheticsDecorationProvider(
)
occ <- textDocument.occurrences
range <- occ.range.toIterable
treeRange <- semanticDbToTreeEdit.toRevisedStrict(range).toIterable
treeRange <- semanticDbToTreeEdit
.getOrElse(TokenEditDistance.NoMatch)
.toRevisedStrict(range)
.toIterable
if occ.role.isDefinition && allMissingTypeRanges(treeRange)
signature <- textDocument.symbols.find(_.symbol == occ.symbol).toIterable
decorationPosition = methodPositions.getOrElse(treeRange, treeRange)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ case class Buffers(
): TokenEditDistance = {
val bufferInput = source.toInputFromBuffers(this)
val snapshotInput = Input.VirtualFile(bufferInput.path, snapshot)
TokenEditDistance(snapshotInput, bufferInput, trees)
TokenEditDistance(snapshotInput, bufferInput, trees).getOrElse(
TokenEditDistance.NoMatch
)
}

}
94 changes: 50 additions & 44 deletions metals/src/main/scala/scala/meta/internal/metals/Diagnostics.scala
Original file line number Diff line number Diff line change
Expand Up @@ -217,19 +217,11 @@ final class Diagnostics(
queue: ju.Queue[Diagnostic],
): Unit = {
if (!path.isFile) return didDelete(path)
val current = path.toInputFromBuffers(buffers)
val snapshot = snapshots.getOrElse(path, current)
val edit = TokenEditDistance(
snapshot,
current,
trees,
doNothingWhenUnchanged = false,
)
val uri = path.toURI.toString
val all = new ju.ArrayList[Diagnostic](queue.size() + 1)
for {
diagnostic <- queue.asScala
freshDiagnostic <- toFreshDiagnostic(edit, diagnostic, snapshot)
freshDiagnostic <- toFreshDiagnostic(path, diagnostic)
} {
all.add(freshDiagnostic)
}
Expand All @@ -256,44 +248,58 @@ final class Diagnostics(
// Adjust positions for type errors for changes in the open buffer.
// Only needed when merging syntax errors with type errors.
private def toFreshDiagnostic(
edit: TokenEditDistance,
path: AbsolutePath,
d: Diagnostic,
snapshot: Input,
): Option[Diagnostic] = {
val result = edit
.toRevised(
range = d.getRange,
adjustWithinToken = d.getSource() == "scala-cli",
)
.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(
s"stale ${d.getSource} ${d.getSeverity.toString.toLowerCase()}",
d.getMessage,
)
scribe.info(message)
}
val current = path.toInputFromBuffers(buffers)
val snapshot = snapshots.getOrElse(path, current)
val edit = TokenEditDistance(
snapshot,
current,
trees,
doNothingWhenUnchanged = false,
)
edit match {
case Right(edit) =>
val result = edit
.toRevised(
range = d.getRange,
adjustWithinToken = d.getSource() == "scala-cli",
)
.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(
s"stale ${d.getSource} ${d.getSeverity.toString.toLowerCase()}",
d.getMessage,
)
scribe.info(message)
}
}
result

case Left(_) =>
// tokenization error will be shown from scalameta tokenizer
None
}
result
}

private def clearDiagnosticsBuffer(): Iterable[AbsolutePath] = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,7 @@ class MetalsLspService(
else {
val edit = TokenEditDistance(old, newBuffer, trees)
edit
.getOrElse(TokenEditDistance.NoMatch)
.toRevised(
params.getPosition.getLine,
params.getPosition.getCharacter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ final class FoldingRangeProvider(
val revised = Input.VirtualFile(filePath.toURI.toString(), code)
val fromTree =
Input.VirtualFile(filePath.toURI.toString(), tree.pos.input.text)
val distance = TokenEditDistance(fromTree, revised, trees)
val distance = TokenEditDistance(fromTree, revised, trees).getOrElse(
TokenEditDistance.NoMatch
)
val extractor = new FoldingRangeExtractor(distance, foldOnlyLines)
extractor.extract(tree)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
package scala.meta.internal.parsing

import scala.collection.mutable.ArrayBuffer
import scala.util.Failure
import scala.util.Success
import scala.util.Try

import scala.meta.inputs.Input
import scala.meta.inputs.Input.File
import scala.meta.inputs.Input.VirtualFile
import scala.meta.inputs.Position

import org.eclipse.jdt.core.ToolFactory
Expand All @@ -32,7 +28,7 @@ object JavaTokens {
* @param source to tokenize
* @return tokenized source
*/
def tokenize(source: Input): Option[Array[JavaToken]] = Try {
def tokenize(source: Input): Try[Array[JavaToken]] = Try {

val scanner = ToolFactory.createScanner(
/*tokenizeComments = */ true, /*tokenizeWhiteSpace = */ true,
Expand Down Expand Up @@ -74,16 +70,5 @@ object JavaTokens {

loop(scanner.getNextToken())
buffer.toArray
} match {
case Failure(exception) =>
source match {
case VirtualFile(path, _) =>
scribe.warn(s"Cannot tokenize Java file $path", exception)
case File(path, _) =>
scribe.warn(s"Cannot tokenize Java file $path", exception)
case _ =>
}
None
case Success(value) => Some(value)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import java.util.logging.Logger
import scala.annotation.tailrec
import scala.collection.compat.immutable.ArraySeq
import scala.reflect.ClassTag
import scala.util.Failure
import scala.util.Success

import scala.meta.Input
import scala.meta.Position
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.parsing.TokenOps.syntax._
import scala.meta.internal.{semanticdb => s}
import scala.meta.tokenizers.Tokenized

import difflib._
import org.eclipse.{lsp4j => l}
Expand Down Expand Up @@ -391,7 +394,7 @@ object TokenEditDistance {
revisedInput: Input.VirtualFile,
trees: Trees,
doNothingWhenUnchanged: Boolean = true,
): TokenEditDistance = {
): Either[String, TokenEditDistance] = {
val isScala =
originalInput.path.isScalaFilename &&
revisedInput.path.isScalaFilename
Expand All @@ -401,37 +404,57 @@ object TokenEditDistance {

if (!isScala && !isJava) {
// Ignore non-scala/java Files.
Unchanged
Right(Unchanged)
} else if (originalInput.value.isEmpty() || revisedInput.value.isEmpty()) {
NoMatch
Right(NoMatch)
} else if (doNothingWhenUnchanged && originalInput == revisedInput) {
Unchanged
Right(Unchanged)
} else if (isJava) {
val result = for {
revised <- JavaTokens.tokenize(revisedInput)
original <- JavaTokens.tokenize(originalInput)
} yield {
TokenEditDistance.fromTokens(
originalInput,
original,
revisedInput,
revised,
)
val tokenizedRevised = JavaTokens.tokenize(revisedInput)
val tokenizedOriginal = JavaTokens.tokenize(originalInput)
(tokenizedRevised, tokenizedOriginal) match {
case (Success(revised), Success(original)) =>
Right(
TokenEditDistance.fromTokens(
originalInput,
original,
revisedInput,
revised,
)
)
case (err: Failure[_], _) => Left(err.exception.getMessage())
case (_, err: Failure[_]) => Left(err.exception.getMessage())
}
result.getOrElse(NoMatch)
} else {
val result = for {
revised <- trees.tokenized(revisedInput).toOption
original <- trees.tokenized(originalInput).toOption
} yield {
TokenEditDistance.fromTokens(
originalInput,
original.tokens,
revisedInput,
revised.tokens,
)
val tokenizedRevised = trees.tokenized(revisedInput)
val tokenizedOriginal = trees.tokenized(originalInput)
val result = (tokenizedRevised, tokenizedOriginal) match {
case (Tokenized.Success(revised), Tokenized.Success(original)) =>
Right(
TokenEditDistance.fromTokens(
originalInput,
original.tokens,
revisedInput,
revised.tokens,
)
)
case (err: Tokenized.Error, _) =>
Left(err.message)
case (_, err: Tokenized.Error) =>
Left(err.message)
case _ => Right(NoMatch)
}
result.getOrElse(NoMatch)

result match {
case Left(message) =>
scribe.debug(
s"Could not tokenize file ${revisedInput.path} because of:",
message,
)
case _ =>
}

result
}
}

Expand Down
38 changes: 38 additions & 0 deletions tests/unit/src/test/scala/tests/DiagnosticsLspSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -329,4 +329,42 @@ class DiagnosticsLspSuite extends BaseLspSuite("diagnostics") {
)
} yield ()
}

test("tokenization-error") {
cleanWorkspace()
for {
_ <- initialize(
"""
|/metals.json
|{
| "a": {}
|}
|/a/src/main/scala/a/A.scala
|object A {
| val n: Int = ""
|}
""".stripMargin
)
_ <- server.didOpen("a/src/main/scala/a/A.scala")
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/A.scala:2:16: error: type mismatch;
| found : String("")
| required: Int
| val n: Int = ""
| ^^
|""".stripMargin,
)
_ <- server.didSave("a/src/main/scala/a/A.scala")(
_.replace("val n: Int = \"\"", "val n: Int = \" ")
)
_ = assertNoDiff(
client.workspaceDiagnostics,
"""|a/src/main/scala/a/A.scala:2:16: error: unclosed string literal
| val n: Int = "
| ^
|""".stripMargin,
)
} yield ()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,14 @@ class JavaEditDistanceSuite extends BaseSuite {

val distance = TokenEditDistance(original, revised, trees = null)

if (isWindows)
assertNoDiff(distance.toString(), "Diff(22 tokens)")
else
assertNoDiff(distance.toString(), "Diff(19 tokens)")
val diffString =
if (isWindows) "Diff(22 tokens)"
else "Diff(19 tokens)"

assertNoDiff(
distance.map(_.toString()).getOrElse("TokenizationError"),
diffString,
)
}

test("no-change") {
Expand All @@ -59,7 +62,10 @@ class JavaEditDistanceSuite extends BaseSuite {

val distance = TokenEditDistance(original, revised, trees = null)

assertNoDiff(distance.toString(), "unchanged")
assertNoDiff(
distance.map(_.toString()).getOrElse("TokenizationError"),
"unchanged",
)

}
}

0 comments on commit 4b42618

Please sign in to comment.