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

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) {
+            // emulating lazy because of the usage pattern in finally block below (should not initialize in finally)
-                filePath?.let(::File)?.takeIf { it.isFile }?.let(::SequentialFilePositionFinder)
+            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 ->
+                            getPositionFinder()?.findNextPosition(DiagnosticUtils.firstRange(diagnostic.textRanges).startOffset)
-                                MessageUtil.createMessageLocation(filePath, pos.lineContent, pos.line, pos.column, -1, -1)
+                                ?.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) */ )
+            return if (col <= currentLineContent!!.length + 1 /* accounting for a report on EOL (e.g. syntax errors) */)
-                KtSourceFilePos(currentLine, col, if (withLineContents) currentLineContent else null)
+                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()
-    }
 }

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 {
+
-            val (code, mapping) = ByteArrayInputStream(toByteArray()).reader().readSourceFileWithMapping()
+        data class LinePos(
-            val lines =
+            val mappingLine: Int,
-                LightTree2Fir.buildLightTree(code).getChildrenAsArray().mapNotNull { it?.startOffset }.map { mapping.getLineByOffset(it) }
+            val line: Int,
-            Triple(code, mapping, lines)
+            val col: Int,
+            val content: String?
+        ) {
+            override fun toString(): String = "$mappingLine: \"$content\" ($line:$col)"
         }
 
-        val (codeFromTextWithLf, mappingFromTextWithLf, linesFromTextWithLf) = MULTILINE_SOURCE.makeCodeMappingAndLines()
+        fun String.makeCodeMappingAndPositions() = run {
+            val (code, mapping) = ByteArrayInputStream(toByteArray()).reader().readSourceFileWithMapping()
+            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 (codeFromTextWithCrLf, mappingFromTextWithCrLf, linesFromTextWithCrLf) =
+        val (codeLf, mappingLf, positionsLf) = MULTILINE_SOURCE.makeCodeMappingAndPositions()
-            MULTILINE_SOURCE.replace("\n", "\r\n").makeCodeMappingAndLines()
 
-        // classic Mac OS line endings are probably not to be found in the wild, but checking the support nevertheless
+        val (codeCrLf, mappingCrLf, positionsCrLf) =
-        val (codeFromTextWithCr, mappingFromTextWithCr, linesFromTextWithCr) =
+            MULTILINE_SOURCE.replace("\n", "\r\n").makeCodeMappingAndPositions()
-            MULTILINE_SOURCE.replace("\n", "\r").makeCodeMappingAndLines()
 
-        Assert.assertEquals(codeFromTextWithLf.toString(), codeFromTextWithCrLf.toString())
+        val (codeCrLfMixed, mappingCrLfMixed, positionsCrLfMixed) =
-        Assert.assertEquals(codeFromTextWithLf.toString(), codeFromTextWithCr.toString())
+            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()
 
-        Assert.assertEquals(mappingFromTextWithLf.linesCount, mappingFromTextWithCrLf.linesCount)
+        // classic MacOS line endings are probably not to be found in the wild, but checking the support nevertheless
-        Assert.assertEquals(mappingFromTextWithLf.linesCount, mappingFromTextWithCr.linesCount)
+        val (codeCr, mappingCr, positionsCr) =
+            MULTILINE_SOURCE.replace("\n", "\r").makeCodeMappingAndPositions()
 
-        Assert.assertEquals(linesFromTextWithLf, linesFromTextWithCrLf)
+        Assert.assertEquals(codeLf, codeCrLf)
-        Assert.assertEquals(linesFromTextWithLf, linesFromTextWithCr)
+        Assert.assertEquals(codeLf, codeCrLfMixed)
+        Assert.assertEquals(codeLf, codeCr)
 
-        Assert.assertEquals(mappingFromTextWithLf.lastOffset, mappingFromTextWithCrLf.lastOffset)
+        Assert.assertEquals(mappingLf.linesCount, mappingCrLf.linesCount)
-        Assert.assertEquals(mappingFromTextWithLf.lastOffset, mappingFromTextWithCr.lastOffset)
+        Assert.assertEquals(mappingLf.linesCount, mappingCrLfMixed.linesCount)
+        Assert.assertEquals(mappingLf.linesCount, mappingCr.linesCount)
+
+        Assert.assertEquals(positionsLf, positionsCrLf)
+        Assert.assertEquals(positionsLf, positionsCrLfMixed)
+        Assert.assertEquals(positionsLf, positionsCr)
+
+        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
 """