From a2fb50e700d881c38c190a764ffb27a328156157 Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher <239676+necolas@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:14:02 -0700 Subject: [PATCH] fix(diffs): Correct line count when edits split or form a CR/LF Reproduce: 1. Open a document whose content is "a\r\nb" (two lines). 2. Insert "X" between the \r and the \n (offset 2) to make "a\rX\nb". 3. Ask the document for its line count. It reports 2 lines instead of 3. In that state positionAt(2) returns {line:0,character:2} instead of {line:1,character:0}, getLineText(2) throws "Line index out of range", and a search for "X" returns the wrong range. The mirror case is also wrong: inserting a "\r" just before an existing "\n" reports an extra line instead of merging the two into one \r\n break. Each piece derived its line-break count from the shared buffer's offsets, which record a \r\n as one break after the \n. An edit that split a \r\n across a piece boundary, or formed one across two pieces, was then miscounted, corrupting the line count and every position, line-text, and search result that relies on it. The add buffer had the same flaw: appending text that begins with "\n" after a buffer ending in "\r" recorded two breaks for the resulting \r\n. Count each piece's breaks as if its text stood alone, then reconcile boundary pairs in the tree by folding a \r ending one piece with a \n starting the next into a single break. Drop the stale lone-\r offset in the add buffer when an append merges it into a \r\n. Trim the trailing line break in getTextSlice once at the slice end instead of per piece, so an interior \r split across pieces is preserved. --- packages/diffs/src/editor/pieceTable.ts | 206 +++++++++++++----- packages/diffs/test/editorPieceTable.test.ts | 54 ++--- .../diffs/test/editorPieceTableCrlf.test.ts | 191 ++++++++++++++++ 3 files changed, 366 insertions(+), 85 deletions(-) create mode 100644 packages/diffs/test/editorPieceTableCrlf.test.ts diff --git a/packages/diffs/src/editor/pieceTable.ts b/packages/diffs/src/editor/pieceTable.ts index e0879e291..0c4687967 100644 --- a/packages/diffs/src/editor/pieceTable.ts +++ b/packages/diffs/src/editor/pieceTable.ts @@ -16,12 +16,19 @@ class Piece { public readonly offset: number, public readonly length: number, public readonly lineOffsetStart: number, - public readonly lineOffsetEnd: number + public readonly lineOffsetEnd: number, + // Line breaks counted as if this piece's text stood on its own: `\r`, `\n`, + // and `\r\n` each count once, and a `\r` at the very end counts as a lone + // break even when the shared buffer paired it with a following `\n` that + // now lives in a different piece. Pairs that straddle a piece boundary are + // reconciled by the tree (see PieceNode.updateSubtreeLength). + public readonly standaloneBreakCount: number, + // Whether the piece's first char is `\n` and its last char is `\r`. Used to + // fold a `\r` ending one piece with a `\n` starting the next into a single + // `\r\n` line break. + public readonly startsWithLF: boolean, + public readonly endsWithCR: boolean ) {} - - get lineBreakCount(): number { - return this.lineOffsetEnd - this.lineOffsetStart; - } } // A text buffer is a string with its line offsets. @@ -36,6 +43,21 @@ class TextBuffer { // elements to the lineOffsets array in the end append(text: string): number { const offset = this.text.length; + // If the buffer ends with a lone `\r` and the appended text begins with + // `\n`, the two now form a single `\r\n`. The previous append recorded that + // trailing `\r` as its own break at `offset`; drop that entry so the pair + // counts once, at the position after the `\n` (which computeLineOffsets + // below re-adds as `offset + 1`). Without this, a `\r\n` split across two + // appends would be counted as two line breaks. Only a piece ending exactly + // on that `\r` referenced the dropped entry, and callers derive that + // piece's break count from the live buffer, so the array stays consistent. + if ( + offset > 0 && + this.text.charCodeAt(offset - 1) === /* \r */ 13 && + text.charCodeAt(0) === /* \n */ 10 + ) { + this.lineOffsets.pop(); + } const appendedLineOffsets = computeLineOffsets(text); for (let i = 1; i < appendedLineOffsets.length; i++) { this.lineOffsets.push(offset + appendedLineOffsets[i]); @@ -57,18 +79,39 @@ class PieceNode { constructor( public piece: Piece, public subtreeLength: number = piece.length, - public subtreeLineBreakCount: number = piece.lineBreakCount + public subtreeLineBreakCount: number = piece.standaloneBreakCount, + public subtreeStartsWithLF: boolean = piece.startsWithLF, + public subtreeEndsWithCR: boolean = piece.endsWithCR ) {} updateSubtreeLength(): void { + const left = this.left; + const right = this.right; this.subtreeLength = - (this.left?.subtreeLength ?? 0) + + (left?.subtreeLength ?? 0) + this.piece.length + - (this.right?.subtreeLength ?? 0); - this.subtreeLineBreakCount = - (this.left?.subtreeLineBreakCount ?? 0) + - this.piece.lineBreakCount + - (this.right?.subtreeLineBreakCount ?? 0); + (right?.subtreeLength ?? 0); + + // Sum each piece's standalone breaks, then fold every `\r`/`\n` pair that + // straddles a piece boundary: the `\r` ending the left side and the `\n` + // starting the right side were each counted once, but at runtime they read + // as a single `\r\n` break, so drop the duplicate. + let breakCount = + (left?.subtreeLineBreakCount ?? 0) + + this.piece.standaloneBreakCount + + (right?.subtreeLineBreakCount ?? 0); + if ((left?.subtreeEndsWithCR ?? false) && this.piece.startsWithLF) { + breakCount--; + } + if (this.piece.endsWithCR && (right?.subtreeStartsWithLF ?? false)) { + breakCount--; + } + this.subtreeLineBreakCount = breakCount; + + this.subtreeStartsWithLF = + left !== null ? left.subtreeStartsWithLF : this.piece.startsWithLF; + this.subtreeEndsWithCR = + right !== null ? right.subtreeEndsWithCR : this.piece.endsWithCR; } } @@ -194,19 +237,24 @@ export class PieceTable { const takeLength = Math.min(node.piece.length - offsetInPiece, remaining); const buffer = this.#bufferFor(node.piece.source); const start = node.piece.offset + offsetInPiece; - let end = start + takeLength; - if (trimEOF) { - while (end > start && isEOL(buffer.text.charCodeAt(end - 1))) { - end--; - } - } - chunks.push(buffer.text.slice(start, end)); + chunks.push(buffer.text.slice(start, start + takeLength)); remaining -= takeLength; offsetInPiece = 0; node = this.#nextNode(node); } - return chunks.join(''); + let result = chunks.join(''); + if (trimEOF) { + // Trim trailing CR/LF from the end of the whole slice only. Trimming each + // piece chunk instead would drop an interior line break that lands on a + // piece boundary (e.g. a lone `\r` mid-line split across two pieces). + let end = result.length; + while (end > 0 && isEOL(result.charCodeAt(end - 1))) { + end--; + } + result = result.slice(0, end); + } + return result; } charAt(offset: number): string { @@ -523,9 +571,20 @@ export class PieceTable { continue; } + // Account the whole left subtree, then fold a `\r`/`\n` pair straddling + // the left-subtree/this-piece boundary; the subtree count only reconciles + // boundaries internal to itself. line += node.left?.subtreeLineBreakCount ?? 0; + if ((node.left?.subtreeEndsWithCR ?? false) && node.piece.startsWithLF) { + line--; + } remaining -= leftLength; - if (remaining <= node.piece.length) { + + if (remaining < node.piece.length) { + // Breaks the buffer records within the first `remaining` chars of the + // piece. A trailing lone `\r` only matters at the piece's very end, + // which lands in the whole-piece branch below, so the owned-break scan + // is exact for a strict prefix. const buffer = this.#bufferFor(node.piece.source); line += upperBound(buffer.lineOffsets, node.piece.offset + remaining) - @@ -533,12 +592,15 @@ export class PieceTable { return line; } - line += node.piece.lineBreakCount; + line += node.piece.standaloneBreakCount; + if (node.piece.endsWithCR && (node.right?.subtreeStartsWithLF ?? false)) { + line--; + } remaining -= node.piece.length; node = node.right; } - return this.#lineCount - 1; + return line; } #lineBreakOffset(lineBreakIndex: number): number { @@ -547,25 +609,54 @@ export class PieceTable { let documentOffset = 0; while (node !== null) { - const leftLineBreakCount = node.left?.subtreeLineBreakCount ?? 0; - if (remaining < leftLineBreakCount) { + // A `\r` ending the left subtree that folds into this piece's leading + // `\n` is not separately addressable: the merged `\r\n` break is reached + // through this piece's `\n` below, so drop it from the left count. + const leftBreakCount = node.left?.subtreeLineBreakCount ?? 0; + const leftFold = + (node.left?.subtreeEndsWithCR ?? false) && node.piece.startsWithLF + ? 1 + : 0; + const addressableLeftCount = leftBreakCount - leftFold; + if (remaining < addressableLeftCount) { node = node.left; continue; } - const leftLength = node.left?.subtreeLength ?? 0; - documentOffset += leftLength; - remaining -= leftLineBreakCount; - - if (remaining < node.piece.lineBreakCount) { - const bufferLineOffset = this.#bufferFor(node.piece.source).lineOffsets[ - node.piece.lineOffsetStart + remaining - ]; - return documentOffset + (bufferLineOffset - node.piece.offset); + documentOffset += node.left?.subtreeLength ?? 0; + remaining -= addressableLeftCount; + + // Likewise a `\r` ending this piece folds into the right subtree's + // leading `\n`, so it is not addressable here. + const pieceFold = + node.piece.endsWithCR && (node.right?.subtreeStartsWithLF ?? false) + ? 1 + : 0; + const addressablePieceCount = node.piece.standaloneBreakCount - pieceFold; + if (remaining < addressablePieceCount) { + const buffer = this.#bufferFor(node.piece.source); + // Count owned breaks from the live buffer offsets rather than the + // piece's cached end index: a later append that merged a seam `\r\n` + // can shift that index for a piece ending on the `\r`, but the live + // count stays correct (lineOffsetStart never shifts, since appends only + // add entries past the piece). + const ownedBreaks = + upperBound( + buffer.lineOffsets, + node.piece.offset + node.piece.length + ) - node.piece.lineOffsetStart; + if (remaining < ownedBreaks) { + const bufferLineOffset = + buffer.lineOffsets[node.piece.lineOffsetStart + remaining]; + return documentOffset + (bufferLineOffset - node.piece.offset); + } + // The break past every owned one is a trailing lone `\r` the buffer + // paired away; the next line starts one char later, at the piece end. + return documentOffset + node.piece.length; } documentOffset += node.piece.length; - remaining -= node.piece.lineBreakCount; + remaining -= addressablePieceCount; node = node.right; } @@ -609,12 +700,30 @@ export class PieceTable { #createPiece(source: number, offset: number, length: number): Piece { const buffer = this.#bufferFor(source); + const lineOffsetStart = upperBound(buffer.lineOffsets, offset); + const lineOffsetEnd = upperBound(buffer.lineOffsets, offset + length); + const startsWithLF = + length > 0 && buffer.text.charCodeAt(offset) === /* \n */ 10; + const endsWithCR = + length > 0 && buffer.text.charCodeAt(offset + length - 1) === /* \r */ 13; + // The buffer counts a `\r\n` as one break positioned after the `\n`. When a + // piece ends on the `\r` of such a pair, the buffer attributed that break to + // the piece holding the `\n`, so this piece's owned count misses it. On its + // own the trailing `\r` is a line break, so add it back for the standalone + // count. (charCodeAt past the buffer end returns NaN, which is not `\n`.) + const trailingCRPaired = + endsWithCR && buffer.text.charCodeAt(offset + length) === /* \n */ 10; + const standaloneBreakCount = + lineOffsetEnd - lineOffsetStart + (trailingCRPaired ? 1 : 0); return new Piece( source, offset, length, - upperBound(buffer.lineOffsets, offset), - upperBound(buffer.lineOffsets, offset + length) + lineOffsetStart, + lineOffsetEnd, + standaloneBreakCount, + startsWithLF, + endsWithCR ); } @@ -785,7 +894,15 @@ export class PieceTable { last = last.right; } if (canCoalescePieces(last.piece, piece)) { - last.piece = coalesceTwoPieces(last.piece, piece); + // Rebuild the merged piece from its combined byte range so its line-break + // count and CR/LF edge flags are recomputed rather than naively summed; + // the two pieces are adjacent in the same buffer, so the range is + // contiguous and a fresh `#createPiece` is exact. + last.piece = this.#createPiece( + last.piece.source, + last.piece.offset, + last.piece.length + piece.length + ); // The last piece grew, so refresh aggregates from it up to the root. for (let node: PieceNode | null = last; node !== null; ) { node.updateSubtreeLength(); @@ -913,19 +1030,6 @@ function canCoalescePieces(prev: Piece, next: Piece): boolean { ); } -// Joins two adjacent pieces (see canCoalescePieces) into one. Keeping the table -// compact after every edit is what stops the piece count from growing without -// bound during normal typing. -function coalesceTwoPieces(prev: Piece, next: Piece): Piece { - return new Piece( - prev.source, - prev.offset, - prev.length + next.length, - prev.lineOffsetStart, - next.lineOffsetEnd - ); -} - // Returns the index of the first element in the array that is greater than the target. function upperBound(values: number[], target: number): number { let lo = 0; diff --git a/packages/diffs/test/editorPieceTable.test.ts b/packages/diffs/test/editorPieceTable.test.ts index 0176e17ee..c1fd0e7fe 100644 --- a/packages/diffs/test/editorPieceTable.test.ts +++ b/packages/diffs/test/editorPieceTable.test.ts @@ -2,24 +2,17 @@ import { describe, expect, test } from 'bun:test'; import { PieceTable } from '../src/editor/pieceTable'; import type { Position } from '../src/editor/textDocument'; +import { computeLineOffsets } from '../src/utils/computeFileOffsets'; +// Splits into lines the way the piece table does: `\r`, `\n`, and `\r\n` (as +// one) all terminate a line, and each returned line keeps its trailing break. +// Reuses computeLineOffsets so the oracle stays in lockstep with construction, +// including lone `\r` (which a `\n`-only split would miscount). function lineTexts(text: string): string[] { - if (text === '') { - return ['']; - } - - const lines: string[] = []; - let start = 0; - for (let i = 0; i < text.length; i++) { - if (text.charCodeAt(i) === 10) { - lines.push(text.slice(start, i + 1)); - start = i + 1; - } - } - if (start <= text.length) { - lines.push(text.slice(start)); - } - return lines; + const starts = computeLineOffsets(text); + return starts.map((start, i) => + text.slice(start, starts[i + 1] ?? text.length) + ); } /** Trailing CR/LF removed, matching `PieceTable.getLineText` / `getTextSlice(..., true)`. */ @@ -37,26 +30,15 @@ function isLineEnding(c: number): boolean { function positionAt(text: string, offset: number): Position { const clampedOffset = Math.min(Math.max(offset, 0), text.length); + const starts = computeLineOffsets(text); + // The line is the last one whose start offset is at or before `offset`; a + // `\r\n` pair contributes a single start (after the `\n`), so an offset + // between the `\r` and `\n` stays on the earlier line. let line = 0; - let lineStart = 0; - - for (let i = 0; i < text.length; i++) { - if (text.charCodeAt(i) !== 10) { - continue; - } - - const lineEnd = i + 1; - if (clampedOffset < lineEnd) { - return { line, character: clampedOffset - lineStart }; - } - line++; - lineStart = lineEnd; + for (let i = 0; i < starts.length && starts[i] <= clampedOffset; i++) { + line = i; } - - return { - line, - character: clampedOffset - lineStart, - }; + return { line, character: clampedOffset - starts[line] }; } function offsetAt(text: string, position: Position): number { @@ -283,6 +265,10 @@ describe('PieceTable', () => { table.insert('\r', 1); + // The inserted `\r` and the original `\n` now read as a single `\r\n` + // break, so the document still has two lines (not three). + expect(table.getText()).toBe('a\r\nb'); + expect(table.lineCount).toBe(2); expect(table.includes('\r\n')).toBe(true); expect(table.includes('missing')).toBe(false); expect(table.includes('')).toBe(true); diff --git a/packages/diffs/test/editorPieceTableCrlf.test.ts b/packages/diffs/test/editorPieceTableCrlf.test.ts new file mode 100644 index 000000000..681040e32 --- /dev/null +++ b/packages/diffs/test/editorPieceTableCrlf.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, test } from 'bun:test'; + +import { PieceTable } from '../src/editor/pieceTable'; + +/** + * Asserts that an incrementally-edited table is indistinguishable from a table + * freshly constructed from the same text. Construction runs the trusted + * `computeLineOffsets` path, so this pins every line/offset query the + * incremental split/merge path must match — including the CR/LF-at-a-piece- + * boundary cases that motivated these tests. + */ +function expectMatchesFreshConstruction(table: PieceTable, text: string): void { + const fresh = new PieceTable(text); + + expect(table.getText()).toBe(text); + expect(table.getText()).toBe(fresh.getText()); + expect(table.lineCount).toBe(fresh.lineCount); + + for (let line = 0; line < fresh.lineCount; line++) { + expect(table.getLineText(line)).toBe(fresh.getLineText(line)); + expect(table.getLineText(line, true)).toBe(fresh.getLineText(line, true)); + expect(table.getLineLength(line)).toBe(fresh.getLineLength(line)); + expect(table.getLineLength(line, true)).toBe( + fresh.getLineLength(line, true) + ); + } + + for (let offset = 0; offset <= text.length; offset++) { + expect(table.positionAt(offset)).toEqual(fresh.positionAt(offset)); + } + + for (let line = 0; line < fresh.lineCount; line++) { + const lineLength = fresh.getLineLength(line, true); + for (let character = 0; character <= lineLength; character++) { + expect(table.offsetAt({ line, character })).toBe( + fresh.offsetAt({ line, character }) + ); + } + } + + for (let start = 0; start <= text.length; start++) { + for (let end = start; end <= text.length; end++) { + expect(table.getTextSlice(start, end)).toBe( + fresh.getTextSlice(start, end) + ); + } + } +} + +function createRandom(seed: number): () => number { + let state = seed; + return () => { + state = (state * 1664525 + 1013904223) >>> 0; + return state / 0x100000000; + }; +} + +describe('PieceTable CR/LF at piece boundaries', () => { + test('undercount: an insert that splits a CRLF pair adds a line', () => { + // "a\r\nb" (2 lines) -> insert between \r and \n -> "a\rX\nb" (3 lines): + // the \r and \n are now lone breaks on either side of "X". + const table = new PieceTable('a\r\nb'); + + table.insert('X', 2); + + expect(table.getText()).toBe('a\rX\nb'); + expect(table.lineCount).toBe(3); + expect(table.positionAt(2)).toEqual({ line: 1, character: 0 }); + expect(table.getLineText(0)).toBe('a'); + expect(table.getLineText(1)).toBe('X'); + expect(table.getLineText(2)).toBe('b'); + expect(table.search(makeQuery('X'))).toEqual([[2, 3]]); + expectMatchesFreshConstruction(table, 'a\rX\nb'); + }); + + test('overcount: an inserted CR that forms a CRLF pair removes a line', () => { + // "a\nb" (2 lines) -> insert \r before the \n -> "a\r\nb" (still 2 lines): + // the added \r and original \n now read as a single \r\n break. + const table = new PieceTable('a\nb'); + + table.insert('\r', 1); + + expect(table.getText()).toBe('a\r\nb'); + expect(table.lineCount).toBe(2); + expect(table.positionAt(2)).toEqual({ line: 0, character: 2 }); + expect(table.positionAt(3)).toEqual({ line: 1, character: 0 }); + expect(table.getLineText(0)).toBe('a'); + expect(table.getLineText(1)).toBe('b'); + expectMatchesFreshConstruction(table, 'a\r\nb'); + }); + + test('getLineText keeps an interior lone CR that is not at the slice end', () => { + // The trailing-EOL trim in getTextSlice must only strip the end of the + // whole slice, not the end of every per-piece chunk. "a\rX\nb" line 0 is + // "a\r"; trimming to the slice end yields "a", never "aX". + const table = new PieceTable('a\r\nb'); + + table.insert('X', 2); + + expect(table.getLineText(0)).toBe('a'); + expect(table.getLineText(0, true)).toBe('a\r'); + expect(table.getText()).toBe('a\rX\nb'); + }); + + test('getLineLength does not depend on the getLineText cache', () => { + // getLineLength has a fresh path and a cache-fed path (populated by + // getLineText). Both must agree once a lone CR sits mid-line. + const table = new PieceTable('a\r\nb'); + table.insert('X', 2); + + const fresh = table.getLineLength(0, false); + table.getLineText(0); + const cached = table.getLineLength(0, false); + + expect(fresh).toBe(1); + expect(cached).toBe(1); + }); + + test('deleting the inserted char between a split CRLF rejoins the pair', () => { + const table = new PieceTable('a\r\nb'); + + table.insert('X', 2); // "a\rX\nb" (3 lines) + table.delete(2, 1); // back to "a\r\nb" (2 lines) + + expect(table.getText()).toBe('a\r\nb'); + expect(table.lineCount).toBe(2); + expectMatchesFreshConstruction(table, 'a\r\nb'); + }); + + test('deleting an interior char that forms a CRLF removes a line', () => { + // "a\rZ\nb" (3 lines): delete Z so the \r and \n become adjacent -> a + // single \r\n break -> "a\r\nb" (2 lines). + const table = new PieceTable('a\rZ\nb'); + + table.delete(2, 1); + + expect(table.getText()).toBe('a\r\nb'); + expect(table.lineCount).toBe(2); + expectMatchesFreshConstruction(table, 'a\r\nb'); + }); + + test('applyResolvedEdits with a raw CRLF newText counts lines correctly', () => { + // Mirrors an LSP server sending \r\n in newText with independently computed + // offsets; the raw offset path must not miscount. + const table = new PieceTable('line0\nline1'); + + table.applyEdits([{ start: 5, end: 5, text: '\r\ninserted' }]); + + expectMatchesFreshConstruction(table, 'line0\r\ninserted\nline1'); + expect(table.lineCount).toBe(3); + }); + + test('matches fresh construction across random CR/LF-splitting edits', () => { + for (let seed = 1; seed <= 8; seed++) { + const random = createRandom(seed * 2654435761 + 11); + let text = 'a\r\nb\nc\r\rd\n'; + const table = new PieceTable(text); + // Deliberately includes lone \r and \r\n so edits both split existing + // pairs and form new ones across piece boundaries. + const inserts = ['\r', '\n', '\r\n', 'x', 'YZ', '\r\nq', 'p\r', '']; + + for (let i = 0; i < 200; i++) { + if (random() < 0.6) { + const insert = inserts[Math.floor(random() * inserts.length)]; + const offset = Math.floor(random() * (text.length + 1)); + table.insert(insert, offset); + text = text.slice(0, offset) + insert + text.slice(offset); + } else { + const offset = Math.floor(random() * (text.length + 1)); + const length = Math.floor(random() * 4); + table.delete(offset, length); + text = text.slice(0, offset) + text.slice(offset + length); + } + expect(table.getText()).toBe(text); + expect(table.lineCount).toBe(new PieceTable(text).lineCount); + } + + expectMatchesFreshConstruction(table, text); + } + }); +}); + +function makeQuery(text: string) { + return { + text, + replaceText: '', + caseSensitive: false, + wholeWord: false, + regex: false, + }; +}