diff --git a/packages/diffs/src/editor/textDocument.ts b/packages/diffs/src/editor/textDocument.ts index 313f5baf9..f2725ca52 100644 --- a/packages/diffs/src/editor/textDocument.ts +++ b/packages/diffs/src/editor/textDocument.ts @@ -316,16 +316,28 @@ export class TextDocument { } normalizePosition(position: Position): Position { - const line = Math.max(0, Math.min(position.line, this.lineCount - 1)); + const line = TextDocument.#clampIndex(position.line, this.lineCount - 1); return { line, - character: Math.max( - 0, - Math.min(position.character, this.getLineLength(line)) + character: TextDocument.#clampIndex( + position.character, + this.getLineLength(line) ), }; } + // Math.min/max pass NaN through, and a fractional index breaks the + // integer-keyed line-offset lookup — either way the resolved offset becomes + // NaN and a degenerate edit range can swallow the whole document. Malformed + // components clamp like any other out-of-range value: NaN and -Infinity act + // as 0, fractions floor, +Infinity clamps to the max. + static #clampIndex(value: number, max: number): number { + if (Number.isNaN(value)) { + return 0; + } + return Math.max(0, Math.min(Math.floor(value), max)); + } + #resolveEdit(edit: TextEdit): ResolvedTextEdit { let start = this.offsetAt(edit.range.start); let end = this.offsetAt(edit.range.end); diff --git a/packages/diffs/test/README.md b/packages/diffs/test/README.md index 1d715b1bb..cc2b9e12f 100644 --- a/packages/diffs/test/README.md +++ b/packages/diffs/test/README.md @@ -103,7 +103,7 @@ order. If you touch one of these areas, consider adding the missing coverage: - **IME composition deferrals** — IME/composition interaction with editing and undo-coalescing is deferred and not covered. -## Known bugs pinned as `test.failing` (32) +## Known bugs pinned as `test.failing` (30) - **EditStack coalescing** (3) — coalescing decisions compare a new edit against whatever sits on top of the undo stack purely by geometry, with no state reset @@ -168,9 +168,3 @@ order. If you touch one of these areas, consider adding the missing coverage: (or up across a logical-line boundary) can park the caret between the halves of an astral character; a subsequent insert there splits the pair into lone surrogates. -- **Malformed position components destroy the document** (2) — position - normalization has no finiteness/integer guard, so a `NaN` (or fractional) - line/character flows through min/max clamping into the offset computation, the - resolved offset becomes `NaN`, and the degenerate range resolves to a - whole-document replace — an insert with one malformed position component - silently erases everything else. diff --git a/packages/diffs/test/editorTextDocument.test.ts b/packages/diffs/test/editorTextDocument.test.ts index 098053c70..1c964dc3b 100644 --- a/packages/diffs/test/editorTextDocument.test.ts +++ b/packages/diffs/test/editorTextDocument.test.ts @@ -1573,54 +1573,42 @@ function insertAtMalformed( } describe('malformed numeric position components', () => { - // A stricter contract would reject non-numeric components outright; - // pierre-fe's normalizePosition has no finiteness guard, so NaN flows - // through Math.min/max into offsetAt, the resolved offset becomes NaN, - // and the edit range degenerates into a whole-document replace. - - test.failing( - 'an insert with a NaN component never destroys unrelated content', - () => { - // KNOWN BUG: NaN in either component resolves to a NaN offset and the - // edit replaces the ENTIRE document with the inserted text. - for (const [line, character] of [ - [Number.NaN, 1], - [0, Number.NaN], - [Number.NaN, Number.NaN], - ]) { - const { threw, text } = insertAtMalformed( - 'harbor\nlantern', - line, - character - ); - if (!threw) { - expect(text).toContain('harbor'); - expect(text).toContain('lantern'); - } - } + // normalizePosition sanitizes malformed components the same way it clamps + // out-of-range ones: NaN and -Infinity act as 0, fractions floor, and + // +Infinity clamps to the document/line end. An edit carrying a malformed + // position therefore lands at a valid clamped spot instead of resolving to + // a NaN offset (which used to degenerate into a whole-document replace). + + test('an insert with a NaN component never destroys unrelated content', () => { + // A NaN component sanitizes to 0, so the insert lands at the start of + // the resolved axis: [NaN, 1] keeps the intact character 1, [0, NaN] and + // [NaN, NaN] land at the document start. + for (const [line, character, expected] of [ + [Number.NaN, 1, 'h#arbor\nlantern'], + [0, Number.NaN, '#harbor\nlantern'], + [Number.NaN, Number.NaN, '#harbor\nlantern'], + ] as const) { + const { threw, text } = insertAtMalformed( + 'harbor\nlantern', + line, + character + ); + expect(threw).toBe(false); + expect(text).toBe(expected); } - ); + }); - test.failing( - 'an insert with fractional components never destroys unrelated content', - () => { - // KNOWN BUG: a fractional line (0.5) makes offsetAt return NaN via the - // line-offset lookup, taking the same whole-document-replace path as - // NaN inputs. - const { threw, text } = insertAtMalformed('harbor\nlantern', 0.5, 2.5); - if (!threw) { - expect(text).toContain('harbor'); - expect(text).toContain('lantern'); - } - } - ); + test('an insert with fractional components floors to a valid position', () => { + const { threw, text } = insertAtMalformed('harbor\nlantern', 0.5, 2.5); + expect(threw).toBe(false); + // line 0.5 floors to 0, character 2.5 floors to 2. + expect(text).toBe('ha#rbor\nlantern'); + }); test('Infinity components clamp to a valid position without data loss', () => { // DIVERGENCE: a stricter contract would reject non-finite components by - // throwing; pierre-fe's Math.min/max clamping happens to bound Infinity - // to a real offset, so the insert lands at a valid spot and nothing is - // lost. Pinned so a future finiteness guard (which may prefer to throw) - // shows up here. + // throwing; the sanitizing clamp bounds +Infinity to the last line + // instead, keeping every position-taking API total. const { threw, text } = insertAtMalformed('harbor\nlantern', Infinity, 0); expect(threw).toBe(false); expect(text).toContain('harbor');