mirror of
https://github.com/microsoft/vscode.git
synced 2026-05-08 09:08:48 +01:00
fix(stringEdit): prevent _tryRebase from producing non-disjoint edits
## Problem The `_tryRebase` method could produce edits that violate the sorted/disjoint invariant required by `StringEdit`, causing a `BugIndicatingError` to be thrown with the message: `Edits must be disjoint and sorted. Found [X, X) -> "..." after Y` ## Root Cause When a base edit deletes significantly more characters than it inserts (creating a negative offset), subsequent "our" edits can get transformed to positions that conflict with previously-added edits. Example scenario: 1. `ourEdit1`: [100, 110) → "A" — added to result, ends at position 110 2. `baseEdit`: [110, 125) → "" — deletes 15 chars, offset becomes -15 3. `ourEdit2`: [120, 120) → "B" — transforms to [105, 105) after offset Problem: Position 105 is BEFORE 110 (the end of `ourEdit1`), violating the invariant that edits must be sorted and disjoint. The original code only checked for direct intersections with base edits, but missed this case where the cumulative offset transformation causes an "our" edit to land before the end of a previously-added "our" edit. ## Fix Track the exclusive end position of the last added edit (`lastEndEx`) and before adding each transformed edit, verify that its start position is not before `lastEndEx`. If it is: - When `noOverlap=true` (tryRebase): return `undefined` - When `noOverlap=false` (rebaseSkipConflicting): skip the conflicting edit ## Behavior Changes - `tryRebase()`: Now returns `undefined` for cases that previously crashed - `rebaseSkipConflicting()`: Skips conflicting edits instead of crashing - `trySwap()`: Uses `tryRebase` internally, handles edge cases gracefully These are safe changes since the previous behavior was to throw an error. Callers of `tryRebase()` already handle `undefined` returns, and `rebaseSkipConflicting()` is expected to drop conflicting edits.
This commit is contained in:
committed by
Ulugbek Abdullaev
parent
e710e34297
commit
148bbfdb57
@@ -97,6 +97,7 @@ export abstract class BaseStringEdit<T extends BaseStringReplacement<T> = BaseSt
|
||||
let baseIdx = 0;
|
||||
let ourIdx = 0;
|
||||
let offset = 0;
|
||||
let lastEndEx = -1; // Track end of last added edit to ensure sorted/disjoint invariant
|
||||
|
||||
while (ourIdx < this.replacements.length || baseIdx < base.replacements.length) {
|
||||
// take the edit that starts first
|
||||
@@ -108,10 +109,17 @@ export abstract class BaseStringEdit<T extends BaseStringReplacement<T> = BaseSt
|
||||
break;
|
||||
} else if (!baseEdit) {
|
||||
// no more edits from base
|
||||
newEdits.push(new StringReplacement(
|
||||
ourEdit.replaceRange.delta(offset),
|
||||
ourEdit.newText
|
||||
));
|
||||
const transformedRange = ourEdit.replaceRange.delta(offset);
|
||||
// Check if the transformed edit would violate the sorted/disjoint invariant
|
||||
if (transformedRange.start < lastEndEx) {
|
||||
if (noOverlap) {
|
||||
return undefined;
|
||||
}
|
||||
ourIdx++; // Skip this edit as it conflicts with a previously added edit
|
||||
continue;
|
||||
}
|
||||
newEdits.push(new StringReplacement(transformedRange, ourEdit.newText));
|
||||
lastEndEx = transformedRange.endExclusive;
|
||||
ourIdx++;
|
||||
} else if (ourEdit.replaceRange.intersects(baseEdit.replaceRange) || areConcurrentInserts(ourEdit.replaceRange, baseEdit.replaceRange)) {
|
||||
ourIdx++; // Don't take our edit, as it is conflicting -> skip
|
||||
@@ -120,10 +128,17 @@ export abstract class BaseStringEdit<T extends BaseStringReplacement<T> = BaseSt
|
||||
}
|
||||
} else if (ourEdit.replaceRange.start < baseEdit.replaceRange.start) {
|
||||
// Our edit starts first
|
||||
newEdits.push(new StringReplacement(
|
||||
ourEdit.replaceRange.delta(offset),
|
||||
ourEdit.newText
|
||||
));
|
||||
const transformedRange = ourEdit.replaceRange.delta(offset);
|
||||
// Check if the transformed edit would violate the sorted/disjoint invariant
|
||||
if (transformedRange.start < lastEndEx) {
|
||||
if (noOverlap) {
|
||||
return undefined;
|
||||
}
|
||||
ourIdx++; // Skip this edit as it conflicts with a previously added edit
|
||||
continue;
|
||||
}
|
||||
newEdits.push(new StringReplacement(transformedRange, ourEdit.newText));
|
||||
lastEndEx = transformedRange.endExclusive;
|
||||
ourIdx++;
|
||||
} else {
|
||||
baseIdx++;
|
||||
|
||||
@@ -154,6 +154,62 @@ suite('Edit', () => {
|
||||
// This should return undefined because both are inserts at the same position
|
||||
assert.strictEqual(rebasedEdit, undefined);
|
||||
});
|
||||
|
||||
test('tryRebase should return undefined when rebasing would produce non-disjoint edits (negative offset case)', () => {
|
||||
// ourEdit1: [100, 110) -> "A"
|
||||
// ourEdit2: [120, 120) -> "B"
|
||||
// baseEdit: [110, 125) -> "" (delete 15 chars, offset = -15)
|
||||
// After transformation, ourEdit2 at [105, 105) < ourEdit1 end (110)
|
||||
|
||||
const ourEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(100, 110), 'A'),
|
||||
new StringReplacement(OffsetRange.emptyAt(120), 'B'),
|
||||
]);
|
||||
|
||||
const baseEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(110, 125), ''),
|
||||
]);
|
||||
|
||||
const result = ourEdit.tryRebase(baseEdit);
|
||||
assert.strictEqual(result, undefined);
|
||||
});
|
||||
|
||||
test('tryRebase should succeed when edits remain disjoint after rebasing', () => {
|
||||
// ourEdit1: [100, 110) -> "A"
|
||||
// ourEdit2: [200, 210) -> "B"
|
||||
// baseEdit: [50, 60) -> "" (delete 10 chars, offset = -10)
|
||||
// After: ourEdit1 at [90, 100), ourEdit2 at [190, 200) - still disjoint
|
||||
|
||||
const ourEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(100, 110), 'A'),
|
||||
new StringReplacement(new OffsetRange(200, 210), 'B'),
|
||||
]);
|
||||
|
||||
const baseEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(50, 60), ''),
|
||||
]);
|
||||
|
||||
const result = ourEdit.tryRebase(baseEdit);
|
||||
assert.ok(result);
|
||||
assert.strictEqual(result?.replacements[0].replaceRange.start, 90);
|
||||
assert.strictEqual(result?.replacements[1].replaceRange.start, 190);
|
||||
});
|
||||
|
||||
test('rebaseSkipConflicting should skip edits that would produce non-disjoint results', () => {
|
||||
const ourEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(100, 110), 'A'),
|
||||
new StringReplacement(OffsetRange.emptyAt(120), 'B'),
|
||||
]);
|
||||
|
||||
const baseEdit = StringEdit.create([
|
||||
new StringReplacement(new OffsetRange(110, 125), ''),
|
||||
]);
|
||||
|
||||
// Should not throw, and should skip the conflicting edit
|
||||
const result = ourEdit.rebaseSkipConflicting(baseEdit);
|
||||
assert.strictEqual(result.replacements.length, 1);
|
||||
assert.strictEqual(result.replacements[0].replaceRange.start, 100);
|
||||
});
|
||||
});
|
||||
|
||||
suite('ArrayEdit', () => {
|
||||
|
||||
Reference in New Issue
Block a user