Skip to content

Commit 095ed2d

Browse files
committed
Address review: charPosFor null-guard + boundary tests
- charPosFor now returns null for out-of-range lines (lineIdx > newlineOffsets.length) and negative columns, matching its 'number | null' return type. Previously it silently returned column or source.length in those cases. - Adds direct unit tests for Source.hbsPosFor / charPosFor covering empty source, single-char/newline, exact-newline offsets, column-past-line-end clamping, and negative/out-of-range inputs.
1 parent f9fb015 commit 095ed2d

2 files changed

Lines changed: 118 additions & 3 deletions

File tree

packages/@glimmer/syntax/lib/source/source.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,11 @@ export class Source {
5353

5454
charPosFor({ line, column }: SourcePosition): number | null {
5555
const lineIdx = line - 1;
56-
const lineStart = this.#lineStartFor(lineIdx);
57-
const nextNl = this.#newlineOffsets[lineIdx];
58-
const lineEnd = nextNl ?? this.source.length;
56+
// Valid lines are [0, newlineOffsets.length]. Anything else has no offset.
57+
if (lineIdx < 0 || lineIdx > this.#newlineOffsets.length || column < 0) return null;
58+
59+
const lineStart = lineIdx === 0 ? 0 : (this.#newlineOffsets[lineIdx - 1] as number) + 1;
60+
const lineEnd = this.#newlineOffsets[lineIdx] ?? this.source.length;
5961
const target = lineStart + column;
6062

6163
if (target <= lineEnd) {
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import { src } from '@glimmer/syntax';
2+
3+
const { test } = QUnit;
4+
5+
QUnit.module('[glimmer-syntax] Source - hbsPosFor / charPosFor boundaries');
6+
7+
test('empty source', (assert) => {
8+
const s = new src.Source('');
9+
10+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
11+
assert.strictEqual(s.hbsPosFor(1), null);
12+
13+
assert.strictEqual(s.charPosFor({ line: 1, column: 0 }), 0);
14+
assert.strictEqual(s.charPosFor({ line: 2, column: 0 }), null);
15+
});
16+
17+
test('single char, no newline', (assert) => {
18+
const s = new src.Source('a');
19+
20+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
21+
assert.deepEqual(s.hbsPosFor(1), { line: 1, column: 1 });
22+
assert.strictEqual(s.hbsPosFor(2), null);
23+
24+
assert.strictEqual(s.charPosFor({ line: 1, column: 0 }), 0);
25+
assert.strictEqual(s.charPosFor({ line: 1, column: 1 }), 1);
26+
assert.strictEqual(s.charPosFor({ line: 2, column: 0 }), null);
27+
});
28+
29+
test('single newline', (assert) => {
30+
const s = new src.Source('\n');
31+
32+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
33+
assert.deepEqual(s.hbsPosFor(1), { line: 2, column: 0 });
34+
assert.strictEqual(s.hbsPosFor(2), null);
35+
36+
assert.strictEqual(s.charPosFor({ line: 1, column: 0 }), 0);
37+
assert.strictEqual(s.charPosFor({ line: 2, column: 0 }), 1);
38+
assert.strictEqual(s.charPosFor({ line: 3, column: 0 }), null);
39+
});
40+
41+
test('char then newline', (assert) => {
42+
const s = new src.Source('a\n');
43+
44+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
45+
assert.deepEqual(s.hbsPosFor(1), { line: 1, column: 1 });
46+
assert.deepEqual(s.hbsPosFor(2), { line: 2, column: 0 });
47+
assert.strictEqual(s.hbsPosFor(3), null);
48+
49+
assert.strictEqual(s.charPosFor({ line: 1, column: 0 }), 0);
50+
assert.strictEqual(s.charPosFor({ line: 1, column: 1 }), 1);
51+
assert.strictEqual(s.charPosFor({ line: 2, column: 0 }), 2);
52+
assert.strictEqual(s.charPosFor({ line: 3, column: 0 }), null);
53+
});
54+
55+
test('two lines, no trailing newline', (assert) => {
56+
const s = new src.Source('a\nb');
57+
58+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
59+
assert.deepEqual(s.hbsPosFor(1), { line: 1, column: 1 });
60+
assert.deepEqual(s.hbsPosFor(2), { line: 2, column: 0 });
61+
assert.deepEqual(s.hbsPosFor(3), { line: 2, column: 1 });
62+
assert.strictEqual(s.hbsPosFor(4), null);
63+
64+
assert.strictEqual(s.charPosFor({ line: 1, column: 0 }), 0);
65+
assert.strictEqual(s.charPosFor({ line: 2, column: 1 }), 3);
66+
assert.strictEqual(s.charPosFor({ line: 3, column: 0 }), null);
67+
});
68+
69+
test('two lines with trailing newline', (assert) => {
70+
const s = new src.Source('a\nb\n');
71+
72+
assert.deepEqual(s.hbsPosFor(0), { line: 1, column: 0 });
73+
assert.deepEqual(s.hbsPosFor(3), { line: 2, column: 1 });
74+
assert.deepEqual(s.hbsPosFor(4), { line: 3, column: 0 });
75+
assert.strictEqual(s.hbsPosFor(5), null);
76+
77+
assert.strictEqual(s.charPosFor({ line: 3, column: 0 }), 4);
78+
assert.strictEqual(s.charPosFor({ line: 4, column: 0 }), null);
79+
});
80+
81+
test('charPosFor clamps column past line-end to line-end', (assert) => {
82+
const s = new src.Source('hello\nworld');
83+
84+
// line 1 has length 5; column 99 should clamp to the newline offset (5).
85+
assert.strictEqual(s.charPosFor({ line: 1, column: 99 }), 5);
86+
// line 2 has length 5; column 99 should clamp to source.length (11).
87+
assert.strictEqual(s.charPosFor({ line: 2, column: 99 }), 11);
88+
});
89+
90+
test('charPosFor returns null for negative or out-of-range lines', (assert) => {
91+
const s = new src.Source('a\nb\nc');
92+
93+
assert.strictEqual(s.charPosFor({ line: 0, column: 0 }), null, 'line 0');
94+
assert.strictEqual(s.charPosFor({ line: -1, column: 0 }), null, 'negative line');
95+
assert.strictEqual(s.charPosFor({ line: 4, column: 0 }), null, 'line past end');
96+
assert.strictEqual(s.charPosFor({ line: 1, column: -1 }), null, 'negative column');
97+
});
98+
99+
test('hbsPosFor returns null for negative or out-of-range offsets', (assert) => {
100+
const s = new src.Source('abc');
101+
102+
assert.strictEqual(s.hbsPosFor(4), null, 'offset past length');
103+
assert.strictEqual(s.hbsPosFor(100), null, 'offset far past length');
104+
});
105+
106+
test('hbsPosFor at exact newline offset points to that line', (assert) => {
107+
const s = new src.Source('ab\ncd');
108+
109+
// Offset 2 *is* the '\n' — it belongs to line 1 at column 2.
110+
assert.deepEqual(s.hbsPosFor(2), { line: 1, column: 2 });
111+
// Offset 3 is the first char after the newline — line 2, column 0.
112+
assert.deepEqual(s.hbsPosFor(3), { line: 2, column: 0 });
113+
});

0 commit comments

Comments
 (0)