Fixes invariant violation in tryRebase (microsoft#291840)

Author: hediet

src/vs/editor/common/core/edits/stringEdit.ts

@@ -97,48 +97,36 @@ 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
-			const baseEdit = base.replacements[baseIdx];
-			const ourEdit = this.replacements[ourIdx];
+			const baseEdit = base.replacements.at(baseIdx);
+			const ourEdit = this.replacements.at(ourIdx);
 
 			if (!ourEdit) {
 				// We processed all our edits
 				break;
 			} else if (!baseEdit) {
 				// no more edits from base
 				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)) {
+			} else if (
+				ourEdit.replaceRange.intersects(baseEdit.replaceRange) ||
+				areConcurrentInserts(ourEdit.replaceRange, baseEdit.replaceRange) ||
+				isInsertStrictlyInsideRange(ourEdit.replaceRange, baseEdit.replaceRange) ||
+				isInsertStrictlyInsideRange(baseEdit.replaceRange, ourEdit.replaceRange)
+			) {
 				ourIdx++; // Don't take our edit, as it is conflicting -> skip
 				if (noOverlap) {
 					return undefined;
 				}
-			} else if (ourEdit.replaceRange.start < baseEdit.replaceRange.start) {
-				// Our edit starts first
+			} else if (ourEdit.replaceRange.start < baseEdit.replaceRange.start ||
+				(ourEdit.replaceRange.isEmpty && ourEdit.replaceRange.start === baseEdit.replaceRange.start)) {
+				// Our edit starts first, or is an insert at the start of base's range
 				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++;
@@ -299,6 +287,25 @@ export abstract class BaseStringReplacement<T extends BaseStringReplacement<T> =
  * All these replacements are applied at once.
 */
 export class StringEdit extends BaseStringEdit<StringReplacement, StringEdit> {
+	/**
+	 * Parses an edit from its string representation.
+	 * E.g. [[2, 12) -> "fgh", [14, 20) -> "qrst", [22, 22) -> "de\n"]
+	*/
+	public static parse(toStringValue: string): StringEdit {
+		const replacements: StringReplacement[] = [];
+		const regex = /\[(\d+),\s*(\d+)\)\s*->\s*"([^"]*)"/g;
+		let match;
+
+		while ((match = regex.exec(toStringValue)) !== null) {
+			const start = parseInt(match[1], 10);
+			const endEx = parseInt(match[2], 10);
+			const text = match[3].replace(/\\n/g, '\n').replace(/\\r/g, '\r').replace(/\\\\/g, '\\');
+			replacements.push(new StringReplacement(new OffsetRange(start, endEx), text));
+		}
+
+		return new StringEdit(replacements);
+	}
+
 	public static readonly empty = new StringEdit([]);
 
 	public static create(replacements: readonly StringReplacement[]): StringEdit {
@@ -596,3 +603,11 @@ export class AnnotatedStringReplacement<T extends IEditData<T>> extends BaseStri
 function areConcurrentInserts(r1: OffsetRange, r2: OffsetRange): boolean {
 	return r1.isEmpty && r2.isEmpty && r1.start === r2.start;
 }
+
+/**
+ * Returns true if `insert` is an empty range (insert) strictly inside `range`.
+ * For example, insert at position 5 is inside [3, 7) but not inside [5, 7) or [3, 5).
+ */
+function isInsertStrictlyInsideRange(insert: OffsetRange, range: OffsetRange): boolean {
+	return insert.isEmpty && range.start < insert.start && insert.start < range.endExclusive;
+}

src/vs/editor/common/core/ranges/offsetRange.ts

@@ -130,6 +130,10 @@ export class OffsetRange implements IOffsetRange {
 		return Math.max(0, end - start);
 	}
 
+	/**
+	 * `a.intersects(b)` iff there exists a number n so that `a.contains(n)` and `b.contains(n)`.
+	 * Warning: If one range is empty, this method returns always false.
+	*/
 	public intersects(other: OffsetRange): boolean {
 		const start = Math.max(this.start, other.start);
 		const end = Math.min(this.endExclusive, other.endExclusive);

src/vs/editor/test/common/core/stringEdit.test.ts

@@ -276,4 +276,69 @@ suite('Edit', () => {
 			str.substring(textStart, textStart + textLen)
 		);
 	}
+
+	suite('tryRebase invariants', () => {
+		for (let i = 0; i < 1000; i++) {
+			test('case' + i, () => {
+				runTest(i);
+			});
+		}
+
+		function runTest(seed: number) {
+			const rng = Random.create(seed);
+			const s0 = 'abcde\nfghij\nklmno\npqrst\n';
+
+			const e1 = getRandomEdit(s0, rng.nextIntRange(1, 4), rng);
+			const e2 = getRandomEdit(s0, rng.nextIntRange(1, 4), rng);
+
+			const e1RebasedOnE2 = e1.tryRebase(e2);
+			const e2RebasedOnE1 = e2.tryRebase(e1);
+
+			// Invariant 1: e1.rebase(e2) != undefined <=> e2.rebase(e1) != undefined
+			assert.strictEqual(
+				e1RebasedOnE2 !== undefined,
+				e2RebasedOnE1 !== undefined,
+				`Symmetry violated: e1.rebase(e2)=${e1RebasedOnE2 !== undefined}, e2.rebase(e1)=${e2RebasedOnE1 !== undefined}`
+			);
+
+			// Invariant 2: e1.rebase(e2) != undefined => e1.compose(e2.rebase(e1)) = e2.compose(e1.rebase(e2))
+			if (e1RebasedOnE2 !== undefined && e2RebasedOnE1 !== undefined) {
+				const path1 = e1.compose(e2RebasedOnE1);
+				const path2 = e2.compose(e1RebasedOnE2);
+
+				// Both paths should produce the same result when applied to s0
+				const result1 = path1.apply(s0);
+				const result2 = path2.apply(s0);
+				assert.strictEqual(result1, result2, `Diamond property violated`);
+			}
+
+			// Invariant 3: empty.rebase(e) = empty
+			const emptyRebasedOnE1 = StringEdit.empty.tryRebase(e1);
+			assert.ok(emptyRebasedOnE1 !== undefined);
+			assert.ok(emptyRebasedOnE1.isEmpty);
+
+			// Invariant 4: e.rebase(empty) = e
+			const e1RebasedOnEmpty = e1.tryRebase(StringEdit.empty);
+			assert.ok(e1RebasedOnEmpty !== undefined);
+			assert.ok(e1.equals(e1RebasedOnEmpty), `e.rebase(empty) should equal e`);
+
+			// Invariant 5 (TP2): T(T(e3, e1), T(e2, e1)) = T(T(e3, e2), T(e1, e2))
+			// For 3+ concurrent operations, transformation order shouldn't matter
+			const e3 = getRandomEdit(s0, rng.nextIntRange(1, 4), rng);
+
+			const e2OnE1 = e2.tryRebase(e1);
+			const e1OnE2 = e1.tryRebase(e2);
+			const e3OnE1 = e3.tryRebase(e1);
+			const e3OnE2 = e3.tryRebase(e2);
+
+			if (e2OnE1 && e1OnE2 && e3OnE1 && e3OnE2) {
+				const path1 = e3OnE1.tryRebase(e2OnE1); // T(T(e3, e1), T(e2, e1))
+				const path2 = e3OnE2.tryRebase(e1OnE2); // T(T(e3, e2), T(e1, e2))
+
+				if (path1 && path2) {
+					assert.ok(path1.equals(path2), `TP2 violated: transformation order matters`);
+				}
+			}
+		}
+	});
 });