Fixes microsoft#185779 (microsoft#185816)

Author: hediet

src/vs/editor/common/diff/algorithms/diffAlgorithm.ts

@@ -49,6 +49,13 @@ export class SequenceDiff {
 	public join(other: SequenceDiff): SequenceDiff {
 		return new SequenceDiff(this.seq1Range.join(other.seq1Range), this.seq2Range.join(other.seq2Range));
 	}
+
+	public delta(offset: number): SequenceDiff {
+		if (offset === 0) {
+			return this;
+		}
+		return new SequenceDiff(this.seq1Range.delta(offset), this.seq2Range.delta(offset));
+	}
 }
 
 export interface ISequence {

src/vs/editor/common/diff/algorithms/joinSequenceDiffs.ts

@@ -50,34 +50,78 @@ export function joinSequenceDiffs(sequence1: ISequence, sequence2: ISequence, se
 		result.push(sequenceDiffs[0]);
 	}
 
+	// First move them all to the left as much as possible and join them if possible
 	for (let i = 1; i < sequenceDiffs.length; i++) {
-		const lastResult = result[result.length - 1];
-		const cur = sequenceDiffs[i];
-
-		if (cur.seq1Range.isEmpty) {
-			let all = true;
-			const length = cur.seq1Range.start - lastResult.seq1Range.endExclusive;
-			for (let i = 1; i <= length; i++) {
-				if (sequence2.getElement(cur.seq2Range.start - i) !== sequence2.getElement(cur.seq2Range.endExclusive - i)) {
-					all = false;
+		const prevResult = sequenceDiffs[i - 1];
+		let cur = sequenceDiffs[i];
+
+		if (cur.seq1Range.isEmpty || cur.seq2Range.isEmpty) {
+			const length = cur.seq1Range.start - prevResult.seq1Range.endExclusive;
+			let d;
+			for (d = 1; d <= length; d++) {
+				if (
+					sequence1.getElement(cur.seq1Range.start - d) !== sequence1.getElement(cur.seq1Range.endExclusive - d) ||
+					sequence2.getElement(cur.seq2Range.start - d) !== sequence2.getElement(cur.seq2Range.endExclusive - d)) {
 					break;
 				}
 			}
+			d--;
 
-			if (all) {
+			if (d === length) {
 				// Merge previous and current diff
-				result[result.length - 1] = new SequenceDiff(lastResult.seq1Range, new OffsetRange(
-					lastResult.seq2Range.start,
-					cur.seq2Range.endExclusive - length
-				));
+				result[result.length - 1] = new SequenceDiff(
+					new OffsetRange(prevResult.seq1Range.start, cur.seq1Range.endExclusive - length),
+					new OffsetRange(prevResult.seq2Range.start, cur.seq2Range.endExclusive - length),
+				);
 				continue;
 			}
+
+			cur = cur.delta(-d);
 		}
 
 		result.push(cur);
 	}
 
-	return result;
+	const result2: SequenceDiff[] = [];
+	// Then move them all to the right and join them again if possible
+	for (let i = 0; i < result.length - 1; i++) {
+		const nextResult = result[i + 1];
+		let cur = result[i];
+
+		if (cur.seq1Range.isEmpty || cur.seq2Range.isEmpty) {
+			const length = nextResult.seq1Range.start - cur.seq1Range.endExclusive;
+			let d;
+			for (d = 0; d < length; d++) {
+				if (
+					sequence1.getElement(cur.seq1Range.start + d) !== sequence1.getElement(cur.seq1Range.endExclusive + d) ||
+					sequence2.getElement(cur.seq2Range.start + d) !== sequence2.getElement(cur.seq2Range.endExclusive + d)
+				) {
+					break;
+				}
+			}
+
+			if (d === length) {
+				// Merge previous and current diff, write to result!
+				result[i + 1] = new SequenceDiff(
+					new OffsetRange(cur.seq1Range.start + length, nextResult.seq1Range.endExclusive),
+					new OffsetRange(cur.seq2Range.start + length, nextResult.seq2Range.endExclusive),
+				);
+				continue;
+			}
+
+			if (d > 0) {
+				cur = cur.delta(d);
+			}
+		}
+
+		result2.push(cur);
+	}
+
+	if (result.length > 0) {
+		result2.push(result[result.length - 1]);
+	}
+
+	return result2;
 }
 
 // align character level diffs at whitespace characters
@@ -102,37 +146,45 @@ export function shiftSequenceDiffs(sequence1: ISequence, sequence2: ISequence, s
 	}
 
 	for (let i = 0; i < sequenceDiffs.length; i++) {
+		const prevDiff = (i > 0 ? sequenceDiffs[i - 1] : undefined);
 		const diff = sequenceDiffs[i];
+		const nextDiff = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1] : undefined);
+
+		const seq1ValidRange = new OffsetRange(prevDiff ? prevDiff.seq1Range.start + 1 : 0, nextDiff ? nextDiff.seq1Range.endExclusive - 1 : sequence1.length);
+		const seq2ValidRange = new OffsetRange(prevDiff ? prevDiff.seq2Range.start + 1 : 0, nextDiff ? nextDiff.seq2Range.endExclusive - 1 : sequence2.length);
+
 		if (diff.seq1Range.isEmpty) {
-			const seq2PrevEndExclusive = (i > 0 ? sequenceDiffs[i - 1].seq2Range.endExclusive : -1);
-			const seq2NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq2Range.start : sequence2.length);
-			sequenceDiffs[i] = shiftDiffToBetterPosition(diff, sequence1, sequence2, seq2NextStart, seq2PrevEndExclusive);
+			sequenceDiffs[i] = shiftDiffToBetterPosition(diff, sequence1, sequence2, seq1ValidRange, seq2ValidRange);
 		} else if (diff.seq2Range.isEmpty) {
-			const seq1PrevEndExclusive = (i > 0 ? sequenceDiffs[i - 1].seq1Range.endExclusive : -1);
-			const seq1NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq1Range.start : sequence1.length);
-			sequenceDiffs[i] = shiftDiffToBetterPosition(diff.reverse(), sequence2, sequence1, seq1NextStart, seq1PrevEndExclusive).reverse();
+			sequenceDiffs[i] = shiftDiffToBetterPosition(diff.reverse(), sequence2, sequence1, seq2ValidRange, seq1ValidRange).reverse();
 		}
 	}
 
 	return sequenceDiffs;
 }
 
-function shiftDiffToBetterPosition(diff: SequenceDiff, sequence1: ISequence, sequence2: ISequence, seq2NextStart: number, seq2PrevEndExclusive: number) {
-	const maxShiftLimit = 20; // To prevent performance issues
+function shiftDiffToBetterPosition(diff: SequenceDiff, sequence1: ISequence, sequence2: ISequence, seq1ValidRange: OffsetRange, seq2ValidRange: OffsetRange,) {
+	const maxShiftLimit = 100; // To prevent performance issues
 
 	// don't touch previous or next!
 	let deltaBefore = 1;
-	while (diff.seq2Range.start - deltaBefore > seq2PrevEndExclusive &&
+	while (
+		diff.seq1Range.start - deltaBefore >= seq1ValidRange.start &&
+		diff.seq2Range.start - deltaBefore >= seq2ValidRange.start &&
 		sequence2.getElement(diff.seq2Range.start - deltaBefore) ===
-		sequence2.getElement(diff.seq2Range.endExclusive - deltaBefore) && deltaBefore < maxShiftLimit) {
+		sequence2.getElement(diff.seq2Range.endExclusive - deltaBefore) && deltaBefore < maxShiftLimit
+	) {
 		deltaBefore++;
 	}
 	deltaBefore--;
 
 	let deltaAfter = 0;
-	while (diff.seq2Range.start + deltaAfter < seq2NextStart &&
+	while (
+		diff.seq1Range.start + deltaAfter < seq1ValidRange.endExclusive &&
+		diff.seq2Range.endExclusive + deltaAfter < seq2ValidRange.endExclusive &&
 		sequence2.getElement(diff.seq2Range.start + deltaAfter) ===
-		sequence2.getElement(diff.seq2Range.endExclusive + deltaAfter) && deltaAfter < maxShiftLimit) {
+		sequence2.getElement(diff.seq2Range.endExclusive + deltaAfter) && deltaAfter < maxShiftLimit
+	) {
 		deltaAfter++;
 	}
 
@@ -158,8 +210,5 @@ function shiftDiffToBetterPosition(diff: SequenceDiff, sequence1: ISequence, seq
 		}
 	}
 
-	if (bestDelta !== 0) {
-		return new SequenceDiff(diff.seq1Range.delta(bestDelta), diff.seq2Range.delta(bestDelta));
-	}
-	return diff;
+	return diff.delta(bestDelta);
 }

src/vs/editor/common/diff/standardLinesDiffComputer.ts

@@ -154,23 +154,23 @@ export class StandardLinesDiffComputer implements ILinesDiffComputer {
 	}
 
 	private refineDiff(originalLines: string[], modifiedLines: string[], diff: SequenceDiff, timeout: ITimeout, considerWhitespaceChanges: boolean): { mappings: RangeMapping[]; hitTimeout: boolean } {
-		const sourceSlice = new Slice(originalLines, diff.seq1Range, considerWhitespaceChanges);
-		const targetSlice = new Slice(modifiedLines, diff.seq2Range, considerWhitespaceChanges);
+		const slice1 = new Slice(originalLines, diff.seq1Range, considerWhitespaceChanges);
+		const slice2 = new Slice(modifiedLines, diff.seq2Range, considerWhitespaceChanges);
 
-		const diffResult = sourceSlice.length + targetSlice.length < 500
-			? this.dynamicProgrammingDiffing.compute(sourceSlice, targetSlice, timeout)
-			: this.myersDiffingAlgorithm.compute(sourceSlice, targetSlice, timeout);
+		const diffResult = slice1.length + slice2.length < 500
+			? this.dynamicProgrammingDiffing.compute(slice1, slice2, timeout)
+			: this.myersDiffingAlgorithm.compute(slice1, slice2, timeout);
 
 		let diffs = diffResult.diffs;
-		diffs = optimizeSequenceDiffs(sourceSlice, targetSlice, diffs);
-		diffs = coverFullWords(sourceSlice, targetSlice, diffs);
-		diffs = smoothenSequenceDiffs(sourceSlice, targetSlice, diffs);
+		diffs = optimizeSequenceDiffs(slice1, slice2, diffs);
+		diffs = coverFullWords(slice1, slice2, diffs);
+		diffs = smoothenSequenceDiffs(slice1, slice2, diffs);
 
 		const result = diffs.map(
 			(d) =>
 				new RangeMapping(
-					sourceSlice.translateRange(d.seq1Range),
-					targetSlice.translateRange(d.seq2Range)
+					slice1.translateRange(d.seq1Range),
+					slice2.translateRange(d.seq2Range)
 				)
 		);
 
@@ -297,6 +297,9 @@ export function lineRangeMappingFromRangeMappings(alignments: RangeMapping[], or
 	}
 
 	assertFn(() => {
+		if (changes.length > 0 && changes[0].originalRange.startLineNumber !== changes[0].modifiedRange.startLineNumber) {
+			return false;
+		}
 		return checkAdjacentItems(changes,
 			(m1, m2) => m2.originalRange.startLineNumber - m1.originalRange.endLineNumberExclusive === m2.modifiedRange.startLineNumber - m1.modifiedRange.endLineNumberExclusive &&
 				// There has to be an unchanged line in between (otherwise both diffs should have been joined)

src/vs/editor/test/node/diffing/diffingFixture.test.ts

@@ -4,14 +4,22 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
-import { readdirSync, readFileSync, existsSync, writeFileSync, rmSync } from 'fs';
+import { existsSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'fs';
 import { join, resolve } from 'path';
+import { setUnexpectedErrorHandler } from 'vs/base/common/errors';
 import { FileAccess } from 'vs/base/common/network';
 import { LineRangeMapping } from 'vs/editor/common/diff/linesDiffComputer';
 import { SmartLinesDiffComputer } from 'vs/editor/common/diff/smartLinesDiffComputer';
 import { StandardLinesDiffComputer } from 'vs/editor/common/diff/standardLinesDiffComputer';
 
 suite('diff fixtures', () => {
+	setup(() => {
+		setUnexpectedErrorHandler(e => {
+			throw e;
+		});
+	});
+
+
 	const fixturesOutDir = FileAccess.asFileUri('vs/editor/test/node/diffing/fixtures').fsPath;
 	// We want the dir in src, so we can directly update the source files if they disagree and create invalid files to capture the previous state.
 	// This makes it very easy to update the fixtures.
@@ -104,7 +112,7 @@ suite('diff fixtures', () => {
 	}
 
 	test(`test`, () => {
-		runTest('move-1', 'advanced');
+		runTest('issue-185779', 'advanced');
 	});
 
 	for (const folder of folders) {

src/vs/editor/test/node/diffing/fixtures/issue-185779/1.txt

@@ -0,0 +1,39 @@
+
+	private doAddView(view: IView<TLayoutContext>, size: number | Sizing, index = this.viewItems.length, skipLayout?: boolean): void {
+		if (this.state !== State.Idle) {
+			throw new Error('Cant modify splitview');
+		}
+
+		this.state = State.Busy;
+
+		// Add view
+		const container = $('.split-view-view');
+
+		if (index === this.viewItems.length) {
+			this.viewContainer.appendChild(container);
+		} else {
+			this.viewContainer.insertBefore(container, this.viewContainer.children.item(index));
+		}
+
+		const onChangeDisposable = view.onDidChange(size => this.onViewChange(item, size));
+		const containerDisposable = toDisposable(() => this.viewContainer.removeChild(container));
+		const disposable = combinedDisposable(onChangeDisposable, containerDisposable);
+
+		let viewSize: ViewItemSize;
+
+		if (typeof size === 'number') {
+			viewSize = size;
+		} else if (size.type === 'split') {
+			viewSize = this.getViewSize(size.index) / 2;
+		} else if (size.type === 'invisible') {
+			viewSize = { cachedVisibleSize: size.cachedVisibleSize };
+		} else {
+			viewSize = view.minimumSize;
+		}
+
+		const item = this.orientation === Orientation.VERTICAL
+			? new VerticalViewItem(container, view, viewSize, disposable)
+			: new HorizontalViewItem(container, view, viewSize, disposable);
+
+		this.viewItems.splice(index, 0, item);
+

src/vs/editor/test/node/diffing/fixtures/issue-185779/2.txt

@@ -0,0 +1,49 @@
+
+	private doAddView(view: IView<TLayoutContext>, size: number | Sizing, index = this.viewItems.length, skipLayout?: boolean): void {
+		if (this.state !== State.Idle) {
+			throw new Error('Cant modify splitview');
+		}
+
+		this.state = State.Busy;
+
+		// Add view
+		const container = $('.split-view-view');
+
+		if (index === this.viewItems.length) {
+			this.viewContainer.appendChild(container);
+		} else {
+			this.viewContainer.insertBefore(container, this.viewContainer.children.item(index));
+		}
+
+		const onChangeDisposable = view.onDidChange(size => this.onViewChange(item, size));
+		const containerDisposable = toDisposable(() => this.viewContainer.removeChild(container));
+		const disposable = combinedDisposable(onChangeDisposable, containerDisposable);
+
+		let viewSize: ViewItemSize;
+
+		if (typeof size === 'number') {
+			viewSize = size;
+		} else {
+			if (size.type === 'auto') {
+				if (this.areViewsDistributed()) {
+					size = { type: 'distribute' };
+				} else {
+					size = { type: 'split', index: size.index };
+				}
+			}
+
+			if (size.type === 'split') {
+				viewSize = this.getViewSize(size.index) / 2;
+			} else if (size.type === 'invisible') {
+				viewSize = { cachedVisibleSize: size.cachedVisibleSize };
+			} else {
+				viewSize = view.minimumSize;
+			}
+		}
+
+		const item = this.orientation === Orientation.VERTICAL
+			? new VerticalViewItem(container, view, viewSize, disposable)
+			: new HorizontalViewItem(container, view, viewSize, disposable);
+
+		this.viewItems.splice(index, 0, item);
+

src/vs/editor/test/node/diffing/fixtures/issue-185779/advanced.expected.diff.json

@@ -0,0 +1,50 @@
+{
+	"original": {
+		"content": "\n\tprivate doAddView(view: IView<TLayoutContext>, size: number | Sizing, index = this.viewItems.length, skipLayout?: boolean): void {\n\t\tif (this.state !== State.Idle) {\n\t\t\tthrow new Error('Cant modify splitview');\n\t\t}\n\n\t\tthis.state = State.Busy;\n\n\t\t// Add view\n\t\tconst container = $('.split-view-view');\n\n\t\tif (index === this.viewItems.length) {\n\t\t\tthis.viewContainer.appendChild(container);\n\t\t} else {\n\t\t\tthis.viewContainer.insertBefore(container, this.viewContainer.children.item(index));\n\t\t}\n\n\t\tconst onChangeDisposable = view.onDidChange(size => this.onViewChange(item, size));\n\t\tconst containerDisposable = toDisposable(() => this.viewContainer.removeChild(container));\n\t\tconst disposable = combinedDisposable(onChangeDisposable, containerDisposable);\n\n\t\tlet viewSize: ViewItemSize;\n\n\t\tif (typeof size === 'number') {\n\t\t\tviewSize = size;\n\t\t} else if (size.type === 'split') {\n\t\t\tviewSize = this.getViewSize(size.index) / 2;\n\t\t} else if (size.type === 'invisible') {\n\t\t\tviewSize = { cachedVisibleSize: size.cachedVisibleSize };\n\t\t} else {\n\t\t\tviewSize = view.minimumSize;\n\t\t}\n\n\t\tconst item = this.orientation === Orientation.VERTICAL\n\t\t\t? new VerticalViewItem(container, view, viewSize, disposable)\n\t\t\t: new HorizontalViewItem(container, view, viewSize, disposable);\n\n\t\tthis.viewItems.splice(index, 0, item);\n\n",
+		"fileName": "./1.txt"
+	},
+	"modified": {
+		"content": "\n\tprivate doAddView(view: IView<TLayoutContext>, size: number | Sizing, index = this.viewItems.length, skipLayout?: boolean): void {\n\t\tif (this.state !== State.Idle) {\n\t\t\tthrow new Error('Cant modify splitview');\n\t\t}\n\n\t\tthis.state = State.Busy;\n\n\t\t// Add view\n\t\tconst container = $('.split-view-view');\n\n\t\tif (index === this.viewItems.length) {\n\t\t\tthis.viewContainer.appendChild(container);\n\t\t} else {\n\t\t\tthis.viewContainer.insertBefore(container, this.viewContainer.children.item(index));\n\t\t}\n\n\t\tconst onChangeDisposable = view.onDidChange(size => this.onViewChange(item, size));\n\t\tconst containerDisposable = toDisposable(() => this.viewContainer.removeChild(container));\n\t\tconst disposable = combinedDisposable(onChangeDisposable, containerDisposable);\n\n\t\tlet viewSize: ViewItemSize;\n\n\t\tif (typeof size === 'number') {\n\t\t\tviewSize = size;\n\t\t} else {\n\t\t\tif (size.type === 'auto') {\n\t\t\t\tif (this.areViewsDistributed()) {\n\t\t\t\t\tsize = { type: 'distribute' };\n\t\t\t\t} else {\n\t\t\t\t\tsize = { type: 'split', index: size.index };\n\t\t\t\t}\n\t\t\t}\n\n\t\t\tif (size.type === 'split') {\n\t\t\t\tviewSize = this.getViewSize(size.index) / 2;\n\t\t\t} else if (size.type === 'invisible') {\n\t\t\t\tviewSize = { cachedVisibleSize: size.cachedVisibleSize };\n\t\t\t} else {\n\t\t\t\tviewSize = view.minimumSize;\n\t\t\t}\n\t\t}\n\n\t\tconst item = this.orientation === Orientation.VERTICAL\n\t\t\t? new VerticalViewItem(container, view, viewSize, disposable)\n\t\t\t: new HorizontalViewItem(container, view, viewSize, disposable);\n\n\t\tthis.viewItems.splice(index, 0, item);\n\n",
+		"fileName": "./2.txt"
+	},
+	"diffs": [
+		{
+			"originalRange": "[26,33)",
+			"modifiedRange": "[26,43)",
+			"innerChanges": [
+				{
+					"originalRange": "[26,10 -> 26,10]",
+					"modifiedRange": "[26,10 -> 35,4]"
+				},
+				{
+					"originalRange": "[27,1 -> 27,1]",
+					"modifiedRange": "[36,1 -> 36,2]"
+				},
+				{
+					"originalRange": "[28,1 -> 28,1]",
+					"modifiedRange": "[37,1 -> 37,2]"
+				},
+				{
+					"originalRange": "[29,1 -> 29,1]",
+					"modifiedRange": "[38,1 -> 38,2]"
+				},
+				{
+					"originalRange": "[30,1 -> 30,1]",
+					"modifiedRange": "[39,1 -> 39,2]"
+				},
+				{
+					"originalRange": "[31,1 -> 31,1]",
+					"modifiedRange": "[40,1 -> 40,2]"
+				},
+				{
+					"originalRange": "[32,1 -> 32,1]",
+					"modifiedRange": "[41,1 -> 41,2]"
+				},
+				{
+					"originalRange": "[33,1 -> 33,1]",
+					"modifiedRange": "[42,1 -> 43,1]"
+				}
+			]
+		}
+	]
+}