Skip to content

Commit f47ddbc

Browse files
committed
Fix bad line number assertion in ScriptInfo.positionToLineOffset
This is the line number side of ecddf84 (from microsoft#21924, fixing microsoft#21818). But the code is slightly improved for both cases: instead of testing that `leaf` is defined, check whether `lineCount` is zero, and if it is, return `〈1,0〉` for the one-based line and zero-based column numbers. (The requirement of `lineCount > 0` is also seen in the fact that `lineNumberToInfo` expects a "*One*BasedLine" argument.) I've stared at this code way too much, since I think that there is something more fundamentally wrong here. E.g., `EditWalker` only `push`es to `startPath` but never pops even a `children`-less node that is left after deleting the whole contents. But I can't figure out the overall structure, which is also why the test that I added is not great (see the comment there; also, microsoft#21924 is dealing with the same problem and didn't add a test). Fixes microsoft#44518.
1 parent 5cea46c commit f47ddbc

File tree

2 files changed

+20
-6
lines changed

2 files changed

+20
-6
lines changed

src/server/scriptVersionCache.ts

+8-6
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,8 @@ namespace ts.server {
6767
}
6868
const lm = LineIndex.linesFromText(insertedText);
6969
const lines = lm.lines;
70-
if (lines.length > 1) {
71-
if (lines[lines.length - 1] === "") {
72-
lines.pop();
73-
}
70+
if (lines.length > 1 && lines[lines.length - 1] === "") {
71+
lines.pop();
7472
}
7573
let branchParent: LineNode | undefined;
7674
let lastZeroCount: LineCollection | undefined;
@@ -683,8 +681,12 @@ namespace ts.server {
683681
}
684682

685683
// Skipped all children
686-
const { leaf } = this.lineNumberToInfo(this.lineCount(), 0);
687-
return { oneBasedLine: this.lineCount(), zeroBasedColumn: leaf ? leaf.charCount() : 0, lineText: undefined };
684+
const lineCount = this.lineCount();
685+
if (lineCount === 0) { // it's empty! (and lineNumberToInfo expects a one-based line)
686+
return { oneBasedLine: 1, zeroBasedColumn: 0, lineText: undefined };
687+
}
688+
const leaf = Debug.checkDefined(this.lineNumberToInfo(lineCount, 0).leaf);
689+
return { oneBasedLine: lineCount, zeroBasedColumn: leaf.charCount(), lineText: undefined };
688690
}
689691

690692
/**

src/testRunner/unittests/tsserver/versionCache.ts

+12
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,18 @@ var q:Point=<Point>p;`;
5252
assert.deepEqual(lineIndex.positionToLineOffset(0), { line: 1, offset: 1 });
5353
});
5454

55+
it("handles emptying whole file (GH#44518)", () => {
56+
// See below for the main thing that this tests; it would be better to have a test
57+
// that uses `ScriptInfo.positionToLineOffset` but I couldn't find away to do that
58+
const { lines } = server.LineIndex.linesFromText("function foo() {\n\ndsa\n\n}\n\nfo(dsa\n\n\n ");
59+
const lineIndex = new server.LineIndex();
60+
lineIndex.load(lines);
61+
const snapshot = lineIndex.edit(0, 39);
62+
assert.equal(snapshot.getText(0, snapshot.getLength()), "");
63+
// line must always be >=1, otherwise the failIfInvalidLocation(location) assertion in ScriptInfo.positionToLineOffset will fail
64+
assert.deepEqual(snapshot.positionToLineOffset(0), { line: 1, offset: 1 });
65+
});
66+
5567
it(`change 9 1 0 1 {"y"}`, () => {
5668
validateEditAtLineCharIndex(9, 1, 0, "y");
5769
});

0 commit comments

Comments
 (0)