Skip to content

Commit

Permalink
FIR LT: fix column calculation with crlf line endings
Browse files Browse the repository at this point in the history 
#KT-57117 fixed
also:
- refactor the position finder (mostly for testability)
- add testing of position finder to the testLightTreeReadLineEndings
- refactor the test for readability
- add mixed line ending scenario
  • Loading branch information
@ligee @qodana-bot
ligee authored and qodana-bot committed Mar 16, 2023
1 parent 82904fe commit 0dca581
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 37 deletions.
56 changes: 37 additions & 19 deletions ...iler/cli/src/org/jetbrains/kotlin/cli/common/fir/FirDiagnosticsCompilerResultsReporter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,19 @@ object FirDiagnosticsCompilerResultsReporter {
): Boolean {
var hasErrors = false
for (filePath in diagnosticsCollector.diagnosticsByFilePath.keys) {
val positionFinder: SequentialFilePositionFinder? by lazy(LazyThreadSafetyMode.NONE) {
filePath?.let(::File)?.takeIf { it.isFile }?.let(::SequentialFilePositionFinder)
}
// emulating lazy because of the usage pattern in finally block below (should not initialize in finally)
var positionFinderInitialized = false
var positionFinder: SequentialFilePositionFinder? = null

fun getPositionFinder() =
if (positionFinderInitialized) positionFinder
else {
positionFinderInitialized = true
filePath?.let(::File)?.takeIf { it.isFile }?.let(::SequentialFilePositionFinder)?.also {
positionFinder = it
}
}

@Suppress("ConvertTryFinallyToUseCall")
try {
for (diagnostic in diagnosticsCollector.diagnosticsByFilePath[filePath].orEmpty().sortedWith(InFileDiagnosticsComparator)) {
Expand All @@ -60,9 +70,10 @@ object FirDiagnosticsCompilerResultsReporter {
// NOTE: SequentialPositionFinder relies on the ascending order of the input offsets, so the code relies
// on the the appropriate sorting above
// Also the end offset is ignored, as it is irrelevant for the CLI reporting
positionFinder?.findNextPosition(DiagnosticUtils.firstRange(diagnostic.textRanges).startOffset)?.let { pos ->
MessageUtil.createMessageLocation(filePath, pos.lineContent, pos.line, pos.column, -1, -1)
}
getPositionFinder()?.findNextPosition(DiagnosticUtils.firstRange(diagnostic.textRanges).startOffset)
?.let { pos ->
MessageUtil.createMessageLocation(filePath, pos.lineContent, pos.line, pos.column, -1, -1)
}
}
}?.let { location ->
report(diagnostic, location)
Expand Down Expand Up @@ -136,19 +147,29 @@ fun BaseDiagnosticsCollector.reportToMessageCollector(messageCollector: MessageC
FirDiagnosticsCompilerResultsReporter.reportToMessageCollector(this, messageCollector, renderDiagnosticName)
}

private class KtSourceFilePos(val line: Int, val column: Int, val lineContent: String?) {
// public only because of tests
class KtSourceFileDiagnosticPos(val line: Int, val column: Int, val lineContent: String?) {

// NOTE: This method is used for presenting positions to the user
override fun toString(): String = if (line < 0) "(offset: $column line unknown)" else "($line,$column)"

companion object {
val NONE = KtSourceFilePos(-1, -1, null)
val NONE = KtSourceFileDiagnosticPos(-1, -1, null)
}
}

private class SequentialFilePositionFinder(file: File) : Closeable {
private class SequentialFilePositionFinder private constructor(private val reader: InputStreamReader)
: Closeable, SequentialPositionFinder(reader)
{
constructor(file: File) : this(file.reader(/* TODO: select proper charset */))

private var reader: InputStreamReader = file.reader(/* TODO: select proper charset */)
override fun close() {
reader.close()
}
}

// public only for tests
open class SequentialPositionFinder(private val reader: InputStreamReader) {

private var currentLineContent: String? = null
private val buffer = CharArray(255)
Expand All @@ -161,13 +182,13 @@ private class SequentialFilePositionFinder(file: File) : Closeable {
private var currentLine = 0

// assuming that if called multiple times, calls should be sorted by ascending offset
fun findNextPosition(offset: Int, withLineContents: Boolean = true): KtSourceFilePos {
fun findNextPosition(offset: Int, withLineContents: Boolean = true): KtSourceFileDiagnosticPos {

fun posInCurrentLine(): KtSourceFilePos? {
fun posInCurrentLine(): KtSourceFileDiagnosticPos? {
val col = offset - (charsRead - currentLineContent!!.length - 1)/* beginning of line offset */ + 1 /* col is 1-based */
assert(col > 0)
return if (col <= currentLineContent!!.length + 1 /* accounting for a report on EOL (e.g. syntax errors) */ )
KtSourceFilePos(currentLine, col, if (withLineContents) currentLineContent else null)
return if (col <= currentLineContent!!.length + 1 /* accounting for a report on EOL (e.g. syntax errors) */)
KtSourceFileDiagnosticPos(currentLine, col, if (withLineContents) currentLineContent else null)
else null
}

Expand All @@ -182,7 +203,7 @@ private class SequentialFilePositionFinder(file: File) : Closeable {

posInCurrentLine()?.let { return@findNextPosition it }

if (endOfStream) return KtSourceFilePos(-1, offset, if (withLineContents) currentLineContent else null)
if (endOfStream) return KtSourceFileDiagnosticPos(-1, offset, if (withLineContents) currentLineContent else null)

currentLineContent = null
}
Expand All @@ -204,6 +225,7 @@ private class SequentialFilePositionFinder(file: File) : Closeable {
charsRead++
when {
c == '\n' && skipNextLf -> {
charsRead--
skipNextLf = false
}
c == '\n' || c == '\r' -> {
Expand All @@ -219,8 +241,4 @@ private class SequentialFilePositionFinder(file: File) : Closeable {
}
}
}

override fun close() {
reader.close()
}
}
73 changes: 55 additions & 18 deletions compiler/tests/org/jetbrains/kotlin/lightTree/LightTreeParsingTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package org.jetbrains.kotlin.lightTree
import com.intellij.lang.LighterASTNode
import com.intellij.openapi.util.Ref
import com.intellij.util.diff.FlyweightCapableTreeStructure
import org.jetbrains.kotlin.cli.common.fir.SequentialPositionFinder
import org.jetbrains.kotlin.fir.lightTree.LightTree2Fir
import org.jetbrains.kotlin.readSourceFileWithMapping
import org.junit.Assert
Expand All @@ -18,33 +19,68 @@ class LightTreeParsingTest {

@Test
fun testLightTreeReadLineEndings() {
fun String.makeCodeMappingAndLines() = run {

data class LinePos(
val mappingLine: Int,
val line: Int,
val col: Int,
val content: String?
) {
override fun toString(): String = "$mappingLine: \"$content\" ($line:$col)"
}

fun String.makeCodeMappingAndPositions() = run {
val (code, mapping) = ByteArrayInputStream(toByteArray()).reader().readSourceFileWithMapping()
val lines =
LightTree2Fir.buildLightTree(code).getChildrenAsArray().mapNotNull { it?.startOffset }.map { mapping.getLineByOffset(it) }
Triple(code, mapping, lines)
val positionFinder = SequentialPositionFinder(ByteArrayInputStream(toByteArray()).reader())
val linePositions =
LightTree2Fir.buildLightTree(code).getChildrenAsArray()
.mapNotNull { it?.startOffset }
.map {
val nextPos = positionFinder.findNextPosition(it)
LinePos( mapping.getLineByOffset(it), nextPos.line, nextPos.column, nextPos.lineContent)
}
Triple(code.toString(), mapping, linePositions)
}

val (codeFromTextWithLf, mappingFromTextWithLf, linesFromTextWithLf) = MULTILINE_SOURCE.makeCodeMappingAndLines()
val (codeLf, mappingLf, positionsLf) = MULTILINE_SOURCE.makeCodeMappingAndPositions()

val (codeCrLf, mappingCrLf, positionsCrLf) =
MULTILINE_SOURCE.replace("\n", "\r\n").makeCodeMappingAndPositions()

val (codeFromTextWithCrLf, mappingFromTextWithCrLf, linesFromTextWithCrLf) =
MULTILINE_SOURCE.replace("\n", "\r\n").makeCodeMappingAndLines()
val (codeCrLfMixed, mappingCrLfMixed, positionsCrLfMixed) =
MULTILINE_SOURCE.let {
var toReplace = false
buildString {
it.forEach { c ->
if (c == '\n') {
if (toReplace) append("\r\n") else append(c)
toReplace = !toReplace
} else append(c)
}
}
}.also { s ->
Assert.assertEquals(s.count { it == '\r' }, s.count { it == '\n' } / 2)
}.makeCodeMappingAndPositions()

// classic Mac OS line endings are probably not to be found in the wild, but checking the support nevertheless
val (codeFromTextWithCr, mappingFromTextWithCr, linesFromTextWithCr) =
MULTILINE_SOURCE.replace("\n", "\r").makeCodeMappingAndLines()
// classic MacOS line endings are probably not to be found in the wild, but checking the support nevertheless
val (codeCr, mappingCr, positionsCr) =
MULTILINE_SOURCE.replace("\n", "\r").makeCodeMappingAndPositions()

Assert.assertEquals(codeFromTextWithLf.toString(), codeFromTextWithCrLf.toString())
Assert.assertEquals(codeFromTextWithLf.toString(), codeFromTextWithCr.toString())
Assert.assertEquals(codeLf, codeCrLf)
Assert.assertEquals(codeLf, codeCrLfMixed)
Assert.assertEquals(codeLf, codeCr)

Assert.assertEquals(mappingFromTextWithLf.linesCount, mappingFromTextWithCrLf.linesCount)
Assert.assertEquals(mappingFromTextWithLf.linesCount, mappingFromTextWithCr.linesCount)
Assert.assertEquals(mappingLf.linesCount, mappingCrLf.linesCount)
Assert.assertEquals(mappingLf.linesCount, mappingCrLfMixed.linesCount)
Assert.assertEquals(mappingLf.linesCount, mappingCr.linesCount)

Assert.assertEquals(linesFromTextWithLf, linesFromTextWithCrLf)
Assert.assertEquals(linesFromTextWithLf, linesFromTextWithCr)
Assert.assertEquals(positionsLf, positionsCrLf)
Assert.assertEquals(positionsLf, positionsCrLfMixed)
Assert.assertEquals(positionsLf, positionsCr)

Assert.assertEquals(mappingFromTextWithLf.lastOffset, mappingFromTextWithCrLf.lastOffset)
Assert.assertEquals(mappingFromTextWithLf.lastOffset, mappingFromTextWithCr.lastOffset)
Assert.assertEquals(mappingLf.lastOffset, mappingCrLf.lastOffset)
Assert.assertEquals(mappingLf.lastOffset, mappingCrLfMixed.lastOffset)
Assert.assertEquals(mappingLf.lastOffset, mappingCr.lastOffset)
}
}

Expand All @@ -59,4 +95,5 @@ val a = 1
val b = 2
val c = 3
val d = 4
val e = 5
"""

0 comments on commit 0dca581

Please sign in to comment.