Skip to content

Commit dd37557

Browse files
bangabanga
committed
Fix incorrect spans in SpannedString.slice
Finally got a small repro and fixed it. Not sure where precisely the bug was, but rewriting the slice function to more explicitly collect all active spans until startIndex, copying over the ones in the middle and then closing all the active ones at endIndex seems to fix this. This is easier to follow and also avoids iterating past the end index, avoiding some unnecessary work. Also added id remapping to `appendSpannedString`, though that wasn't causing this bug, just to avoid other potential issues.
1 parent 70ed4dc commit dd37557

File tree

5 files changed

+69
-54
lines changed

5 files changed

+69
-54
lines changed

‎src/SpannedString.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,24 @@ describe('slice', () => {
213213

214214
expect(() => apply(a.slice(-1, 3))).toThrow();
215215
});
216+
217+
test('discard leading spans', () => {
218+
const a = S()
219+
.appendString('12')
220+
.addSpan(0, 1, 'i')
221+
.slice(1, 2)
222+
.appendString('3');
223+
expect(apply(a)).toEqual('23');
224+
});
225+
226+
test('discard trailing spans', () => {
227+
const a = S()
228+
.appendString('12')
229+
.addSpan(1, 2, 'i')
230+
.slice(0, 1)
231+
.appendString('3');
232+
expect(apply(a)).toEqual('13');
233+
});
216234
});
217235

218236
describe('appendSpannedString', () => {

‎src/SpannedString.ts

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -83,16 +83,17 @@ export class SpannedString<T> {
8383
if (otherSpans) {
8484
const index = otherIndex + overlappingSpanIndex;
8585
spanMarkers[index] = spanMarkers[index] ?? [];
86-
spanMarkers[index]?.push(...otherSpans);
86+
spanMarkers[index]?.push(
87+
...otherSpans.map((span) => ({
88+
...span,
89+
// Remap ids to avoid collisions
90+
id: span.id + this._nextId,
91+
}))
92+
);
8793
}
8894
}
8995
this._spanMarkers = spanMarkers;
90-
91-
// We don't need to remap the ids of the spans, because they only occur
92-
// together in one place, `overlappingSpanIndex` and at that index, all
93-
// the spans of the first string must be closing and all the spans of
94-
// the second string must be opening.
95-
this._nextId = Math.max(this._nextId, other._nextId);
96+
this._nextId = this._nextId + other._nextId;
9697

9798
return this;
9899
}
@@ -111,50 +112,52 @@ export class SpannedString<T> {
111112
endIndex = this._string.length;
112113
}
113114

114-
const spanMarkers = new Array(endIndex + 1 - startIndex);
115+
let index = 0;
115116
const activeSpansById = new Map<number, Span<T>>();
116-
for (let index = 0; index < this._spanMarkers.length; index++) {
117-
const sliceIndex = index - startIndex;
118-
119-
if (sliceIndex === spanMarkers.length - 1) {
120-
spanMarkers[spanMarkers.length - 1] = Array.from(
121-
activeSpansById.values()
122-
).map(({ id, attribute }) => ({
123-
id,
124-
attribute,
125-
isStart: false,
126-
}));
117+
const updatedActiveSpans = () => {
118+
for (const span of this._spanMarkers[index] ?? []) {
119+
if (span.isStart) {
120+
activeSpansById.set(span.id, span);
121+
} else {
122+
activeSpansById.delete(span.id);
123+
}
127124
}
125+
};
126+
const getActiveSpans = () => {
127+
return [...activeSpansById.values()];
128+
};
129+
130+
const newSpanMarkers: (Span<T>[] | undefined)[] = new Array(
131+
endIndex + 1 - startIndex
132+
);
128133

134+
// Collect all the active spans before the new string starts
135+
while (index < startIndex) {
136+
updatedActiveSpans();
137+
index++;
138+
}
139+
newSpanMarkers[0] = getActiveSpans().map((span) => ({ ...span }));
140+
141+
// Copy over spans active in the new string
142+
while (index < endIndex) {
129143
const spans = this._spanMarkers[index];
144+
updatedActiveSpans();
130145
if (spans) {
131-
for (const span of spans) {
132-
if (span.isStart) {
133-
activeSpansById.set(span.id, span);
134-
} else {
135-
activeSpansById.delete(span.id);
136-
}
137-
}
138-
if (sliceIndex >= 0 && sliceIndex < spanMarkers.length) {
139-
spanMarkers[sliceIndex] = spanMarkers[sliceIndex] ?? [];
140-
spanMarkers[sliceIndex].push(...spans);
141-
}
142-
}
143-
144-
if (sliceIndex === 0) {
145-
spanMarkers[0] = Array.from(activeSpansById.values()).map(
146-
({ id, attribute }) => ({
147-
id,
148-
attribute,
149-
isStart: true,
150-
})
151-
);
146+
const newIndex = index - startIndex;
147+
newSpanMarkers[newIndex] = newSpanMarkers[newIndex] ?? [];
148+
newSpanMarkers[newIndex]!.push(...spans);
152149
}
150+
index++;
153151
}
154152

153+
// Close any spans that are still active at the end of the new string
154+
newSpanMarkers[endIndex - startIndex] = getActiveSpans().map(
155+
(span) => ({ ...span, isStart: false })
156+
);
157+
155158
return new SpannedString<T>(
156159
this._string.slice(startIndex, endIndex),
157-
spanMarkers,
160+
newSpanMarkers,
158161
this._nextId
159162
);
160163
}

‎src/__snapshots__/index.test.ts.snap

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,11 +1754,11 @@ Date: Thu Sep 1 12:52:42 2016 -0700
17541754
2789 ░░░░░ 2793 ░░░░░ 2789 ░░░░░ 2793 ░░░░░
17551755
2790 2794 2790 2794
17561756
2791 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2791 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2795 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1757-
2792 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1757+
2792 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2792 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2796 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17581758
2793 + ░░░░░ 2793 + ░░░░░ 2797 + ░░░░░
17591759
2794 + 2794 + 2798 +
17601760
2795 - ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2799 - ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1761-
2796 - ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1761+
2796 - ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2800 - ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17621762
2799 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17631763
2800 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17641764
2801 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
@@ -1782,8 +1782,8 @@ Date: Thu Sep 1 12:52:42 2016 -0700
17821782
2800 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2809 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17831783
2801 + ░░░░░ 2810 + ░░░░░
17841784
2802 + 2811 +
1785-
2803 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1786-
2804 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1785+
2803 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2812 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1786+
2804 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2813 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17871787
2805 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2814 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
17881788
2806 + ░░░░░ 2815 + ░░░░░
17891789
2807 + 2816 +
@@ -1801,7 +1801,7 @@ Date: Thu Sep 1 12:52:42 2016 -0700
18011801
2807 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2825 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
18021802
2808 + ░░░░░ 2826 + ░░░░░
18031803
2809 + 2827 +
1804-
2810 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
1804+
2810 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2828 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
18051805
2811 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2829 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
18061806
2812 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2830 + ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
18071807
2808 ░░░░░ 2799 ░░░░░ 2813 ░░░░░ 2831 ░░░░░

‎src/formatAndFitHunkLine.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ export async function* formatAndFitHunkLine(
3636
return;
3737
}
3838

39-
const linePrefix = line?.slice(0, 1) ?? null;
40-
const lineText = line?.slice(1) ?? null;
39+
const linePrefix = line.slice(0, 1);
40+
const lineText = line.slice(1);
4141

4242
let lineColor: ThemeColor;
4343
let lineNoColor: ThemeColor;

‎src/index.test.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -522,12 +522,6 @@ index 2e204671f7,e56bba5ab4..ab6229d1b6
522522

523523
test('merge commit with 3 parents', async function () {
524524
// Source: the TypeScript repo
525-
526-
// TODO: The highlighting here is wrong on lines 2792, 2803 and 2804
527-
// It's not easy to repro, but looks like a bug somewhere in
528-
// maintaining spans while merging lines from multiple hunk parts
529-
// into the final formatted line. We end up formatting the line
530-
// numbers with a span from earlier in the string somewhere.
531525
expect(
532526
await transform(`
533527
commit d6d6a4aedfa78794c1b611c13d2ed1d3a66e1798

0 commit comments

Comments
 (0)