From 0dca581bdce77a7226618b445d89fe629d1b9c82 Mon Sep 17 00:00:00 2001 From: Ilya Chernikov Date: Tue, 7 Mar 2023 12:43:00 +0100 Subject: [PATCH] FIR LT: fix column calculation with crlf line endings #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 --- .../FirDiagnosticsCompilerResultsReporter.kt | 56 +++++++++----- .../kotlin/lightTree/LightTreeParsingTest.kt | 73 ++++++++++++++----- 2 files changed, 92 insertions(+), 37 deletions(-) diff --git a/compiler/cli/src/org/jetbrains/kotlin/cli/common/fir/FirDiagnosticsCompilerResultsReporter.kt b/compiler/cli/src/org/jetbrains/kotlin/cli/common/fir/FirDiagnosticsCompilerResultsReporter.kt index 6b0c0431a219f..aa1785909fce3 100644 --- a/compiler/cli/src/org/jetbrains/kotlin/cli/common/fir/FirDiagnosticsCompilerResultsReporter.kt +++ b/compiler/cli/src/org/jetbrains/kotlin/cli/common/fir/FirDiagnosticsCompilerResultsReporter.kt @@ -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)) { @@ -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) @@ -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) @@ -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 } @@ -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 } @@ -204,6 +225,7 @@ private class SequentialFilePositionFinder(file: File) : Closeable { charsRead++ when { c == '\n' && skipNextLf -> { + charsRead-- skipNextLf = false } c == '\n' || c == '\r' -> { @@ -219,8 +241,4 @@ private class SequentialFilePositionFinder(file: File) : Closeable { } } } - - override fun close() { - reader.close() - } } diff --git a/compiler/tests/org/jetbrains/kotlin/lightTree/LightTreeParsingTest.kt b/compiler/tests/org/jetbrains/kotlin/lightTree/LightTreeParsingTest.kt index b040fffe41d52..5a2bf4898f05c 100644 --- a/compiler/tests/org/jetbrains/kotlin/lightTree/LightTreeParsingTest.kt +++ b/compiler/tests/org/jetbrains/kotlin/lightTree/LightTreeParsingTest.kt @@ -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 @@ -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) } } @@ -59,4 +95,5 @@ val a = 1 val b = 2 val c = 3 val d = 4 + val e = 5 """ \ No newline at end of file