FieldKind cleanup (#25869)

## Description

Refactors to cleanup FlexFieldKind and its implementations.

Author: CraigMacomber

packages/dds/tree/src/core/schema-stored/schema.ts

@@ -91,13 +91,24 @@ export enum ValueSchema {
 export type TreeTypeSet = ReadonlySet<TreeNodeSchemaIdentifier>;
 
 /**
- * Declarative portion of a Field Kind.
+ * Declarative portion of a {@link FlexFieldKind}.
  *
  * @remarks
  * Enough info about a field kind to know if a given tree is is schema.
+ *
+ * Note that compatibility between trees and schema is not sufficient to evaluate if a schema upgrade should be allowed.
+ * Currently schema upgrades are restricted to field kind changes which can not be cyclic (like version upgrades but not down grades).
+ * See {@link FlexFieldKind.allowsFieldSuperset} for more details.
  */
 export interface FieldKindData {
+	/**
+	 * Globally scoped identifier.
+	 */
 	readonly identifier: FieldKindIdentifier;
+	/**
+	 * Bound on the number of children that fields of this kind may have.
+	 * TODO: consider replacing this with numeric upper and lower bounds.
+	 */
 	readonly multiplicity: Multiplicity;
 }
 

packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts

@@ -59,7 +59,15 @@ export const noChangeHandler: FieldChangeHandler<0> = {
 	getCrossFieldKeys: () => [],
 };
 
-export interface ValueFieldEditor extends FieldEditor<OptionalChangeset> {
+/**
+ * {@link FieldEditor} for required fields (always contain exactly 1 child).
+ * @remarks
+ * This shares code with optional fields, since they are the same edit wise except setting to empty is not allowed,
+ * and the content is always assumed to not be empty.
+ * This means the actual edits implemented for optional fields are sufficient to support required fields
+ * which is why this is defined and implemented in terms of optional fields.
+ */
+export interface RequiredFieldEditor extends FieldEditor<OptionalChangeset> {
 	/**
 	 * Creates a change which replaces the current value of the field with `newValue`.
 	 * @param ids - The ids for the fill and detach fields.
@@ -68,42 +76,41 @@ export interface ValueFieldEditor extends FieldEditor<OptionalChangeset> {
 }
 
 const optionalIdentifier = brandConst("Optional")<FieldKindIdentifier>();
+
 /**
  * 0 or 1 items.
  */
-export const optional = new FlexFieldKind(
-	optionalIdentifier,
-	Multiplicity.Optional,
-	optionalChangeHandler,
-	(types, other) =>
+export const optional = new FlexFieldKind(optionalIdentifier, Multiplicity.Optional, {
+	changeHandler: optionalChangeHandler,
+	allowsTreeSupersetOf: (types, other) =>
 		(other.kind === sequence.identifier || other.kind === optionalIdentifier) &&
 		allowsTreeSchemaIdentifierSuperset(types, other.types),
-	new Set([]),
-);
+});
 
-export const valueFieldEditor: ValueFieldEditor = {
+export const requiredFieldEditor: RequiredFieldEditor = {
 	...optionalFieldEditor,
 	set: (ids: {
 		fill: ChangeAtomId;
 		detach: ChangeAtomId;
 	}): OptionalChangeset => optionalFieldEditor.set(false, ids),
 };
 
-export const valueChangeHandler: FieldChangeHandler<OptionalChangeset, ValueFieldEditor> = {
-	...optional.changeHandler,
-	editor: valueFieldEditor,
+export const requiredFieldChangeHandler: FieldChangeHandler<
+	OptionalChangeset,
+	RequiredFieldEditor
+> = {
+	...optionalChangeHandler,
+	editor: requiredFieldEditor,
 };
 
 const requiredIdentifier = brandConst("Value")<FieldKindIdentifier>();
 
 /**
  * Exactly one item.
  */
-export const required = new FlexFieldKind(
-	requiredIdentifier,
-	Multiplicity.Single,
-	valueChangeHandler,
-	(types, other) =>
+export const required = new FlexFieldKind(requiredIdentifier, Multiplicity.Single, {
+	changeHandler: requiredFieldChangeHandler,
+	allowsTreeSupersetOf: (types, other) =>
 		// By omitting Identifier here,
 		// this is making a policy choice that a schema upgrade cannot be done from required to identifier.
 		// Since an identifier can be upgraded into a required field,
@@ -113,43 +120,35 @@ export const required = new FlexFieldKind(
 			other.kind === requiredIdentifier ||
 			other.kind === optional.identifier) &&
 		allowsTreeSchemaIdentifierSuperset(types, other.types),
-	new Set(),
-);
+});
 
 const sequenceIdentifier = brandConst("Sequence")<FieldKindIdentifier>();
 
 /**
  * 0 or more items.
  */
-export const sequence = new FlexFieldKind(
-	sequenceIdentifier,
-	Multiplicity.Sequence,
-	sequenceFieldChangeHandler,
-	(types, other) =>
+export const sequence = new FlexFieldKind(sequenceIdentifier, Multiplicity.Sequence, {
+	changeHandler: sequenceFieldChangeHandler,
+	allowsTreeSupersetOf: (types, other) =>
 		other.kind === sequenceIdentifier &&
 		allowsTreeSchemaIdentifierSuperset(types, other.types),
-	// TODO: add normalizer/importers for handling ops from other kinds.
-	new Set([]),
-);
+});
 
 const identifierFieldIdentifier = brandConst("Identifier")<FieldKindIdentifier>();
 
 /**
  * Exactly one identifier.
  */
-export const identifier = new FlexFieldKind(
-	identifierFieldIdentifier,
-	Multiplicity.Single,
-	noChangeHandler,
-	(types, other) =>
+export const identifier = new FlexFieldKind(identifierFieldIdentifier, Multiplicity.Single, {
+	changeHandler: noChangeHandler,
+	allowsTreeSupersetOf: (types, other) =>
 		// Allows upgrading from identifier to required: which way this upgrade is allowed to go is a subjective policy choice.
 		(other.kind === sequence.identifier ||
 			other.kind === requiredIdentifier ||
 			other.kind === optional.identifier ||
 			other.kind === identifierFieldIdentifier) &&
 		allowsTreeSchemaIdentifierSuperset(types, other.types),
-	new Set(),
-);
+});
 
 /**
  * Exactly 0 items.
@@ -182,10 +181,12 @@ export const identifier = new FlexFieldKind(
 export const forbidden = new FlexFieldKind(
 	forbiddenFieldKindIdentifier,
 	Multiplicity.Forbidden,
-	noChangeHandler,
-	// All multiplicities other than Value support empty.
-	(types, other) => fieldKinds.get(other.kind)?.multiplicity !== Multiplicity.Single,
-	new Set(),
+	{
+		changeHandler: noChangeHandler,
+		// All multiplicities other than Value support empty.
+		allowsTreeSupersetOf: (types, other) =>
+			fieldKinds.get(other.kind)?.multiplicity !== Multiplicity.Single,
+	},
 );
 
 export const fieldKindConfigurations: ReadonlyMap<
@@ -270,15 +271,31 @@ export const fieldKinds: ReadonlyMap<FieldKindIdentifier, FlexFieldKind> = new M
 // TODO: ensure thy work in generated docs.
 // TODO: add these comments to the rest of the cases below.
 export interface Required
-	extends FlexFieldKind<ValueFieldEditor, "Value", Multiplicity.Single> {}
+	extends FlexFieldKind<RequiredFieldEditor, typeof requiredIdentifier, Multiplicity.Single> {}
 export interface Optional
-	extends FlexFieldKind<OptionalFieldEditor, "Optional", Multiplicity.Optional> {}
+	extends FlexFieldKind<
+		OptionalFieldEditor,
+		typeof optionalIdentifier,
+		Multiplicity.Optional
+	> {}
 export interface Sequence
-	extends FlexFieldKind<SequenceFieldEditor, "Sequence", Multiplicity.Sequence> {}
+	extends FlexFieldKind<
+		SequenceFieldEditor,
+		typeof sequenceIdentifier,
+		Multiplicity.Sequence
+	> {}
 export interface Identifier
-	extends FlexFieldKind<FieldEditor<0>, "Identifier", Multiplicity.Single> {}
+	extends FlexFieldKind<
+		FieldEditor<0>,
+		typeof identifierFieldIdentifier,
+		Multiplicity.Single
+	> {}
 export interface Forbidden
-	extends FlexFieldKind<FieldEditor<0>, "Forbidden", Multiplicity.Forbidden> {}
+	extends FlexFieldKind<
+		FieldEditor<0>,
+		typeof forbiddenFieldKindIdentifier,
+		Multiplicity.Forbidden
+	> {}
 
 /**
  * Default FieldKinds with their editor types erased.

packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts

@@ -36,43 +36,34 @@ export class FlexFieldKind<
 	// TODO: stronger typing
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any
 	TEditor extends FieldEditor<any> = FieldEditor<any>,
-	TName extends string = string,
+	TName extends FieldKindIdentifier = FieldKindIdentifier,
 	TMultiplicity extends Multiplicity = Multiplicity,
 > implements FieldKindData
 {
 	protected _typeCheck!: MakeNominal;
 
 	/**
-	 * @param identifier - Globally scoped identifier.
-	 * @param multiplicity - bound on the number of children that fields of this kind may have.
-	 * TODO: replace with numeric upper and lower bounds.
-	 * @param changeHandler - Change handling policy.
-	 * @param allowsTreeSupersetOf - returns true iff `superset` supports all that this does
-	 * and `superset` is an allowed upgrade. Does not have to handle the `never` cases.
-	 * See {@link isNeverField}.
-	 * TODO: when used as a method (instead of a free function like the other superset related functions),
-	 * this name is/signature is confusing and seems backwards.
-	 * @param handlesEditsFrom - Kinds (in addition to this) whose edits can be processed by changeHandler.
-	 * If the kind of a field changes, and edits are rebased across that kind change,
-	 * listing the other old kind here can prevent those edits from being conflicted and
-	 * provide a chance to handle them.
+	 * Change handling policy.
 	 */
+	// TODO: stronger typing
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any
+	public readonly changeHandler: FieldChangeHandler<any, TEditor>;
+
 	public constructor(
-		public readonly identifier: TName & FieldKindIdentifier,
+		public readonly identifier: TName,
 		public readonly multiplicity: TMultiplicity,
 		// TODO: stronger typing
 		// eslint-disable-next-line @typescript-eslint/no-explicit-any
-		public readonly changeHandler: FieldChangeHandler<any, TEditor>,
-		private readonly allowsTreeSupersetOf: (
-			originalTypes: TreeTypeSet,
-			superset: TreeFieldStoredSchema,
-		) => boolean,
-		public readonly handlesEditsFrom: ReadonlySet<FieldKindIdentifier>,
-	) {}
+		private readonly options: FieldKindOptions<FieldChangeHandler<any, TEditor>>,
+	) {
+		this.changeHandler = options.changeHandler;
+	}
 
 	/**
 	 * Returns true if and only if `superset` permits a (non-strict) superset of the subtrees
 	 * allowed by field made from `this` and `originalTypes`.
+	 *
+	 * TODO: clarify the relationship between this and FieldKindData, and issues with cyclic schema upgrades.
 	 */
 	public allowsFieldSuperset(
 		policy: SchemaPolicy,
@@ -93,10 +84,43 @@ export class FlexFieldKind<
 		if (isNeverField(policy, originalData, superset)) {
 			return false;
 		}
-		return this.allowsTreeSupersetOf(originalTypes, superset);
+		return this.options.allowsTreeSupersetOf(originalTypes, superset);
 	}
 }
 
+/**
+ * Additional options for {@link FlexFieldKind}.
+ *
+ * @remarks
+ * Puts the more confusing parameters into this object so they get explicit names to help with clarity.
+ */
+export interface FieldKindOptions<TFieldChangeHandler> {
+	/**
+	 * Change handling policy.
+	 */
+	readonly changeHandler: TFieldChangeHandler;
+
+	/**
+	 * Returns true if and only if `superset` permits a (non-strict) superset of the subtrees
+	 * allowed by field made from `this` and `originalTypes`.
+	 * @remarks
+	 * Used by {@link FlexFieldKind.allowsFieldSuperset}, which handles the `never` cases before calling this.
+	 */
+	readonly allowsTreeSupersetOf: (
+		originalTypes: TreeTypeSet,
+		superset: TreeFieldStoredSchema,
+	) => boolean;
+
+	/**
+	 * Kinds (in addition to this) whose edits can be processed by changeHandler.
+	 * If the kind of a field changes, and edits are rebased across that kind change,
+	 * listing the other old kind here can prevent those edits from being conflicted and
+	 * provide a chance to handle them.
+	 */
+	// TODO: provide this and use it for improved support for rebasing changes across schema upgrades.
+	// readonly handlesEditsFrom: ReadonlySet<FieldKindIdentifier>;
+}
+
 /**
  * Policy from the app for interpreting the stored schema.
  * The app must ensure consistency for all users of the document.

packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts

@@ -169,9 +169,7 @@ function replaceRevisions(
 export const genericFieldKind: FlexFieldKind = new FlexFieldKind(
 	brandConst("ModularEditBuilder.Generic")<FieldKindIdentifier>(),
 	Multiplicity.Sequence,
-	genericChangeHandler,
-	(types, other) => false,
-	new Set(),
+	{ changeHandler: genericChangeHandler, allowsTreeSupersetOf: (types, other) => false },
 );
 
 /**

packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts

@@ -7,9 +7,9 @@ import { strict as assert, fail } from "node:assert";
 
 import { makeAnonChange } from "../../../core/index.js";
 import {
-	type ValueFieldEditor,
-	valueChangeHandler,
-	valueFieldEditor,
+	type RequiredFieldEditor,
+	requiredFieldChangeHandler,
+	requiredFieldEditor,
 	// Allow import from file being tested.
 	// eslint-disable-next-line import-x/no-internal-modules
 } from "../../../feature-libraries/default-schema/defaultFieldKinds.js";
@@ -62,15 +62,15 @@ const childComposer1_2 = (
 };
 
 describe("defaultFieldKinds", () => {
-	describe("valueFieldEditor.set", () => {
-		it("valueFieldEditor.set", () => {
+	describe("requiredFieldEditor.set", () => {
+		it("requiredFieldEditor.set", () => {
 			const expected = Change.atOnce(
 				Change.clear("self", brand(1)),
 				Change.move(brand(41), "self"),
 			);
 			const revision = mintRevisionTag();
 			assertEqual(
-				valueFieldEditor.set({
+				requiredFieldEditor.set({
 					detach: { localId: brand(1), revision },
 					fill: { localId: brand(41), revision },
 				}),
@@ -80,11 +80,11 @@ describe("defaultFieldKinds", () => {
 	});
 
 	// TODO:
-	// These tests are covering value field usage patterns of optional field's rebaser (which value field uses).
+	// These tests are covering required field usage patterns of optional field's rebaser (which required field uses).
 	// These patterns should be covered in the optional field tests and not be needed here (except perhaps for a minimal integration test).
-	describe("value field rebaser", () => {
-		const fieldHandler: FieldChangeHandler<OptionalChangeset, ValueFieldEditor> =
-			valueChangeHandler;
+	describe("required field rebaser", () => {
+		const fieldHandler: FieldChangeHandler<OptionalChangeset, RequiredFieldEditor> =
+			requiredFieldChangeHandler;
 
 		const childChange1 = Change.child(nodeChange1);
 		const childChange2 = Change.child(nodeChange2);

packages/dds/tree/src/test/feature-libraries/modular-schema/basicRebasers.ts

@@ -113,7 +113,5 @@ export const valueHandler = {
 export const valueField = new FlexFieldKind(
 	brandConst("Value")<FieldKindIdentifier>(),
 	Multiplicity.Single,
-	valueHandler,
-	(a, b) => false,
-	new Set(),
+	{ changeHandler: valueHandler, allowsTreeSupersetOf: (a, b) => false },
 );