fix(tree) Fix document corruption due to loss of change info for transient nodes in sequence field

experimental/dds/tree2/src/core/tree/visitDelta.ts

@@ -166,26 +166,26 @@ function visitModify(
 }
 
 function firstPass(delta: Delta.MarkList, visitor: DeltaVisitor, config: PassConfig): boolean {
-	let containsMoves = false;
+	let listHasMoveOrDelete = false;
 	let index = 0;
 	for (const mark of delta) {
 		if (typeof mark === "number") {
 			// Untouched nodes
 			index += mark;
 		} else {
-			let result = false;
+			let markHasMoveOrDelete = false;
 			// Inline into `switch(mark.type)` once we upgrade to TS 4.7
 			const type = mark.type;
 			switch (type) {
 				case Delta.MarkType.Delete:
 					// Handled in the second pass
 					visitModify(index, mark, visitor, config);
 					index += mark.count;
-					result = true;
+					markHasMoveOrDelete = true;
 					break;
 				case Delta.MarkType.MoveOut:
-					result = visitModify(index, mark, visitor, config);
-					if (result) {
+					markHasMoveOrDelete = visitModify(index, mark, visitor, config);
+					if (markHasMoveOrDelete) {
 						config.modsToMovedTrees.set(mark.moveId, mark);
 					}
 					visitor.onMoveOut(index, mark.count, mark.moveId);
@@ -197,25 +197,26 @@ function firstPass(delta: Delta.MarkList, visitor: DeltaVisitor, config: PassCon
 					);
 					break;
 				case Delta.MarkType.Modify:
-					result = visitModify(index, mark, visitor, config);
+					markHasMoveOrDelete = visitModify(index, mark, visitor, config);
 					index += 1;
 					break;
 				case Delta.MarkType.Insert:
 					visitor.onInsert(index, mark.content);
-					result = visitModify(index, mark, visitor, config);
+					markHasMoveOrDelete =
+						visitModify(index, mark, visitor, config) || (mark.isTransient ?? false);
 					index += mark.content.length;
 					break;
 				case Delta.MarkType.MoveIn:
 					// Handled in the second pass
-					result = true;
+					markHasMoveOrDelete = true;
 					break;
 				default:
 					unreachableCase(type);
 			}
-			containsMoves ||= result;
+			listHasMoveOrDelete ||= markHasMoveOrDelete;
 		}
 	}
-	return containsMoves;
+	return listHasMoveOrDelete;
 }
 
 function secondPass(delta: Delta.MarkList, visitor: DeltaVisitor, config: PassConfig): boolean {
@@ -241,7 +242,7 @@ function secondPass(delta: Delta.MarkList, visitor: DeltaVisitor, config: PassCo
 					break;
 				case Delta.MarkType.Insert:
 					visitModify(index, mark, visitor, config);
-					if (mark.isTransient === true) {
+					if (mark.isTransient ?? false) {
 						visitor.onDelete(index, mark.content.length);
 					} else {
 						index += mark.content.length;

experimental/dds/tree2/src/feature-libraries/sequence-field/compose.ts

@@ -17,11 +17,12 @@ import {
 	Changeset,
 	Mark,
 	MarkList,
-	ExistingCellMark,
 	EmptyInputCellMark,
 	DetachEvent,
 	Modify,
 	MoveId,
+	LineageEvent,
+	NoopMarkType,
 } from "./format";
 import { GapTracker, IndexTracker } from "./tracker";
 import { MarkListFactory } from "./markListFactory";
@@ -57,6 +58,9 @@ import {
 	withRevision,
 	markEmptiesCells,
 	splitMark,
+	markIsTransient,
+	GenerativeMark,
+	isGenerativeMark,
 } from "./utils";
 
 /**
@@ -118,6 +122,7 @@ function composeMarkLists<TNodeChange>(
 		revisionMetadata,
 		(a, b) => composeChildChanges(a, b, newRev, composeChild),
 	);
+	const inputIndex = new IndexTracker(revisionMetadata.getIndex);
 	while (!queue.isEmpty()) {
 		const { baseMark, newMark } = queue.pop();
 		if (newMark === undefined) {
@@ -127,6 +132,7 @@ function composeMarkLists<TNodeChange>(
 			);
 			factory.push(baseMark);
 		} else if (baseMark === undefined) {
+			inputIndex.advance(newMark);
 			factory.push(composeMark(newMark, newRev, composeChild));
 		} else {
 			// Past this point, we are guaranteed that `newMark` and `baseMark` have the same length and
@@ -136,12 +142,14 @@ function composeMarkLists<TNodeChange>(
 				baseMark,
 				newRev,
 				newMark,
+				inputIndex,
 				composeChild,
 				genId,
 				moveEffects,
 				revisionMetadata,
 			);
 			factory.push(composedMark);
+			inputIndex.advance(newMark);
 		}
 	}
 
@@ -161,6 +169,7 @@ function composeMarks<TNodeChange>(
 	baseMark: Mark<TNodeChange>,
 	newRev: RevisionTag | undefined,
 	newMark: Mark<TNodeChange>,
+	inputIndex: IndexTracker,
 	composeChild: NodeChangeComposer<TNodeChange>,
 	genId: IdAllocator,
 	moveEffects: MoveEffectTable<TNodeChange>,
@@ -173,6 +182,36 @@ function composeMarks<TNodeChange>(
 		composeChild,
 	);
 
+	if (markIsTransient(newMark)) {
+		return withNodeChange(baseMark, nodeChange);
+	}
+	if (markIsTransient(baseMark)) {
+		if (isGenerativeMark(newMark)) {
+			// TODO: Make `withNodeChange` preserve type information so we don't need to cast here
+			const nonTransient = withNodeChange(
+				baseMark,
+				nodeChange,
+			) as GenerativeMark<TNodeChange>;
+			delete nonTransient.detachedBy;
+			return nonTransient;
+		}
+		// Modify and Placeholder marks must be muted because the node they target has been deleted.
+		// Detach marks must be muted because the cell is empty.
+		if (newMark.type === "Modify" || newMark.type === "Placeholder" || isDetachMark(newMark)) {
+			assert(
+				newMark.detachEvent !== undefined,
+				"Invalid node-targeting mark after transient",
+			);
+			return baseMark;
+		}
+		// - MoveIn marks are invalid in an existing cell.
+		assert(newMark.type !== "MoveIn", "Invalid MoveIn after transient");
+		// - ReturnTo marks are invalid in for a cell whose node has not been moved out.
+		assert(newMark.type !== "ReturnTo", "Invalid ReturnTo after transient");
+		assert(newMark.type === NoopMarkType, "Unexpected mark type after transient");
+		return baseMark;
+	}
+
 	if (!markHasCellEffect(baseMark) && !markHasCellEffect(newMark)) {
 		if (isNoopMark(baseMark)) {
 			return withNodeChange(newMark, nodeChange);
@@ -199,15 +238,7 @@ function composeMarks<TNodeChange>(
 		}
 		return withNodeChange(baseMark, nodeChange);
 	} else if (areInputCellsEmpty(baseMark)) {
-		const moveInId = getMarkMoveId(baseMark);
-		const moveOutId = getMarkMoveId(newMark);
-
-		if (moveInId !== undefined && moveOutId !== undefined) {
-			assert(
-				isMoveMark(baseMark) && isMoveMark(newMark),
-				0x68f /* Only move marks have move IDs */,
-			);
-
+		if (isMoveMark(baseMark) && isMoveMark(newMark)) {
 			// `baseMark` must be a move destination since it is filling cells, and `newMark` must be a move source.
 			const baseIntention = getIntention(baseMark.revision, revisionMetadata);
 			const newIntention = getIntention(newMark.revision ?? newRev, revisionMetadata);
@@ -245,8 +276,7 @@ function composeMarks<TNodeChange>(
 			return { count: 0 };
 		}
 
-		if (moveInId !== undefined) {
-			assert(isMoveMark(baseMark), 0x690 /* Only move marks have move IDs */);
+		if (isMoveMark(baseMark)) {
 			setReplacementMark(
 				moveEffects,
 				CrossFieldTarget.Source,
@@ -258,9 +288,7 @@ function composeMarks<TNodeChange>(
 			return { count: 0 };
 		}
 
-		if (moveOutId !== undefined) {
-			assert(isMoveMark(newMark), 0x691 /* Only move marks have move IDs */);
-
+		if (isMoveMark(newMark)) {
 			// The nodes attached by `baseMark` have been moved by `newMark`.
 			// We can represent net effect of the two marks by moving `baseMark` to the destination of `newMark`.
 			setReplacementMark(
@@ -273,8 +301,21 @@ function composeMarks<TNodeChange>(
 			);
 			return { count: 0 };
 		}
-		// TODO: Create modify mark for transient node.
-		return { count: 0 };
+
+		assert(isDeleteMark(newMark), "Unexpected mark type");
+		assert(isGenerativeMark(baseMark), "Expected generative mark");
+		const newMarkRevision = newMark.revision ?? newRev;
+		assert(newMarkRevision !== undefined, "Unable to compose anonymous marks");
+		return withNodeChange(
+			{
+				...baseMark,
+				detachedBy: {
+					revision: newMarkRevision,
+					index: inputIndex.getIndex(newMarkRevision),
+				},
+			},
+			nodeChange,
+		);
 	} else {
 		if (isMoveMark(baseMark) && isMoveMark(newMark)) {
 			// The marks must be inverses, since `newMark` is filling the cells which `baseMark` emptied.
@@ -515,48 +556,62 @@ export class ComposeQueue<T> {
 			const length = getOutputLength(baseMark);
 			return this.dequeueBase(length);
 		} else if (areOutputCellsEmpty(baseMark) && areInputCellsEmpty(newMark)) {
-			// TODO: `baseMark` might be a MoveIn, which is not an ExistingCellMark.
-			// See test "[Move ABC, Return ABC] ↷ Delete B" in sequenceChangeRebaser.spec.ts
-			assert(
-				isExistingCellMark(baseMark),
-				0x693 /* Only existing cell mark can have empty output */,
-			);
 			let baseCellId: DetachEvent;
-			if (markEmptiesCells(baseMark)) {
-				assert(isDetachMark(baseMark), 0x694 /* Only detach marks can empty cells */);
-				const baseRevision = baseMark.revision ?? this.baseMarks.revision;
-				const baseIntention = getIntention(baseRevision, this.revisionMetadata);
-				if (baseRevision === undefined || baseIntention === undefined) {
-					// The base revision always be defined except when squashing changes into a transaction.
-					// In the future, we want to support reattaches in the new change here.
-					// We will need to be able to order the base mark relative to the new mark by looking at the lineage of the new mark
-					// (which will be obtained by rebasing the reattach over interim changes
-					// (which requires the local changes to have a revision tag))
+			let cmp: number;
+			if (markIsTransient(baseMark)) {
+				cmp = compareCellPositions(
+					baseMark.detachedBy,
+					baseMark.lineage,
+					isNewAttach(baseMark),
+					newMark,
+					this.newRevision,
+					this.cancelledInserts,
+					this.baseGap,
+				);
+			} else {
+				// TODO: `baseMark` might be a MoveIn, which is not an ExistingCellMark.
+				// See test "[Move ABC, Return ABC] ↷ Delete B" in sequenceChangeRebaser.spec.ts
+				assert(
+					isExistingCellMark(baseMark),
+					0x693 /* Only existing cell mark can have empty output */,
+				);
+				if (markEmptiesCells(baseMark)) {
+					assert(isDetachMark(baseMark), 0x694 /* Only detach marks can empty cells */);
+					const baseRevision = baseMark.revision ?? this.baseMarks.revision;
+					const baseIntention = getIntention(baseRevision, this.revisionMetadata);
+					if (baseRevision === undefined || baseIntention === undefined) {
+						// The base revision always be defined except when squashing changes into a transaction.
+						// In the future, we want to support reattaches in the new change here.
+						// We will need to be able to order the base mark relative to the new mark by looking at the lineage of the new mark
+						// (which will be obtained by rebasing the reattach over interim changes
+						// (which requires the local changes to have a revision tag))
+						assert(
+							isNewAttach(newMark),
+							0x695 /* TODO: Assign revision tags to each change in a transaction */,
+						);
+						return this.dequeueNew();
+					}
+					baseCellId = {
+						revision: baseIntention,
+						index: this.baseIndex.getIndex(baseRevision),
+					};
+				} else {
 					assert(
-						isNewAttach(newMark),
-						0x695 /* TODO: Assign revision tags to each change in a transaction */,
+						areInputCellsEmpty(baseMark),
+						0x696 /* Mark with empty output must either be a detach or also have input empty */,
 					);
-					return this.dequeueNew();
+					baseCellId = baseMark.detachEvent;
 				}
-				baseCellId = {
-					revision: baseIntention,
-					index: this.baseIndex.getIndex(baseRevision),
-				};
-			} else {
-				assert(
-					areInputCellsEmpty(baseMark),
-					0x696 /* Mark with empty output must either be a detach or also have input empty */,
+				cmp = compareCellPositions(
+					baseCellId,
+					baseMark.lineage,
+					isNewAttach(baseMark),
+					newMark,
+					this.newRevision,
+					this.cancelledInserts,
+					this.baseGap,
 				);
-				baseCellId = baseMark.detachEvent;
 			}
-			const cmp = compareCellPositions(
-				baseCellId,
-				baseMark,
-				newMark,
-				this.newRevision,
-				this.cancelledInserts,
-				this.baseGap,
-			);
 			if (cmp < 0) {
 				return { baseMark: this.baseMarks.dequeueUpTo(-cmp) };
 			} else if (cmp > 0) {
@@ -790,7 +845,8 @@ function areInverseMovesAtIntermediateLocation(
  */
 function compareCellPositions(
 	baseCellId: DetachEvent,
-	baseMark: ExistingCellMark<unknown>,
+	baseLineage: readonly LineageEvent[] | undefined,
+	baseIsNewAttach: boolean,
 	newMark: EmptyInputCellMark<unknown>,
 	newIntention: RevisionTag | undefined,
 	cancelledInserts: Set<RevisionTag>,
@@ -814,7 +870,7 @@ function compareCellPositions(
 	}
 
 	if (newCellId !== undefined) {
-		const baseOffset = getOffsetAtRevision(baseMark.lineage, newCellId.revision);
+		const baseOffset = getOffsetAtRevision(baseLineage, newCellId.revision);
 		if (baseOffset !== undefined) {
 			// BUG: `newCellId.revision` may not be the revision of a change in the composition.
 			const newOffset = gapTracker.getOffset(newCellId.revision);
@@ -837,7 +893,7 @@ function compareCellPositions(
 		}
 	}
 
-	const cmp = compareLineages(baseMark.lineage, newMark.lineage);
+	const cmp = compareLineages(baseLineage, newMark.lineage);
 	if (cmp !== 0) {
 		return Math.sign(cmp) * Infinity;
 	}
@@ -862,7 +918,7 @@ function compareCellPositions(
 	// because otherwise `baseMark` would have lineage refering to the emptying of the cell.
 	// We use `baseMark`'s tiebreak policy as if `newMark`'s cells were created concurrently and before `baseMark`.
 	// TODO: Use specified tiebreak instead of always tiebreaking left.
-	if (isNewAttach(baseMark)) {
+	if (baseIsNewAttach) {
 		return -Infinity;
 	}