From 226d3f5db64f7f9791facabbfcd0f9adeac0940b Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 15 Sep 2026 10:39:23 -0400 Subject: [PATCH 1/2] fix(versioning): honor derived property optionality changes --- ...tionality-provenance-2026-8-15-10-37-14.md | 7 + packages/versioning/src/decorators.ts | 56 ++++- packages/versioning/src/lib.ts | 3 + packages/versioning/src/mutator.ts | 2 + .../test/incompatible-versioning.test.ts | 49 +++++ .../apply-snapshot-versioning.test.ts | 199 +++++++++++++++++- .../docs/docs/libraries/versioning/guide.md | 21 ++ 7 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 .chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md diff --git a/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md b/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md new file mode 100644 index 00000000000..9128092167d --- /dev/null +++ b/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/versioning" +--- + +Ignore inherited optionality history when a derived property structurally changes optionality, avoiding incorrect diagnostics and preserving the transformed optionality in version snapshots. \ No newline at end of file diff --git a/packages/versioning/src/decorators.ts b/packages/versioning/src/decorators.ts index e3c655f2c88..806138c1098 100644 --- a/packages/versioning/src/decorators.ts +++ b/packages/versioning/src/decorators.ts @@ -222,7 +222,10 @@ export const $madeOptional: MadeOptionalDecorator = ( if (!version) { return; } - program.stateMap(VersioningStateKeys.madeOptional).set(t, version); + program.stateMap(VersioningStateKeys.madeOptional).set(t, { + version, + application: context.decoratorTarget, + } satisfies OptionalityHistory); }; export const $madeRequired: MadeRequiredDecorator = ( @@ -235,14 +238,54 @@ export const $madeRequired: MadeRequiredDecorator = ( if (!version) { return; } - program.stateMap(VersioningStateKeys.madeRequired).set(t, version); + program.stateMap(VersioningStateKeys.madeRequired).set(t, { + version, + application: context.decoratorTarget, + } satisfies OptionalityHistory); }; +interface OptionalityHistory { + version: Version; + application: DiagnosticTarget; +} + +function getOptionalitySource(program: Program, type: Type): Type { + const sources = program.stateMap(VersioningStateKeys.optionalitySource); + while (sources.has(type)) { + type = sources.get(type); + } + return type; +} + +function getEffectiveOptionalityHistory( + program: Program, + type: Type, + key: symbol, +): Version | undefined { + // Snapshot optionality can differ from the current API without being a structural + // transform. Inspect the original graph, including its intermediate spread/is copies. + type = getOptionalitySource(program, type); + const state: Map = program.stateMap(key); + const history = state.get(type); + if (!history) return undefined; + + while (type.kind === "ModelProperty" && type.sourceProperty) { + const source = getOptionalitySource(program, type.sourceProperty); + const sourceHistory = state.get(source); + // A new decorator application on a copy owns its history and must still be validated. + if (!history.application || sourceHistory?.application !== history.application) break; + if (source.kind !== "ModelProperty" || type.optional !== source.optional) return undefined; + type = source; + } + return history.version; +} + /** - * @returns version when the given type was made required if applicable. + * @returns version when the given type was made required, unless a derived property + * structurally changed optionality and superseded the inherited history. */ export function getMadeRequiredOn(p: Program, t: Type): Version | undefined { - return p.stateMap(VersioningStateKeys.madeRequired).get(t); + return getEffectiveOptionalityHistory(p, t, VersioningStateKeys.madeRequired); } /** @@ -268,10 +311,11 @@ export function getRemovedOnVersions(p: Program, t: Type): Version[] | undefined } /** - * @returns version when the given type was made optional if applicable. + * @returns version when the given type was made optional, unless a derived property + * structurally changed optionality and superseded the inherited history. */ export function getMadeOptionalOn(p: Program, t: Type): Version | undefined { - return p.stateMap(VersioningStateKeys.madeOptional).get(t); + return getEffectiveOptionalityHistory(p, t, VersioningStateKeys.madeOptional); } export class VersionMap { diff --git a/packages/versioning/src/lib.ts b/packages/versioning/src/lib.ts index 8c6474b206e..7a5f870f7df 100644 --- a/packages/versioning/src/lib.ts +++ b/packages/versioning/src/lib.ts @@ -95,6 +95,9 @@ export const $lib = createTypeSpecLibrary({ renamedFrom: { description: "State for @renamedFrom decorator" }, madeOptional: { description: "State for @madeOptional decorator" }, madeRequired: { description: "State for @madeRequired decorator" }, + optionalitySource: { + description: "Original property for version snapshot optionality history", + }, typeChangedFrom: { description: "State for @typeChangedFrom decorator" }, returnTypeChangedFrom: { description: "State for @returnTypeChangedFrom decorator" }, }, diff --git a/packages/versioning/src/mutator.ts b/packages/versioning/src/mutator.ts index 44f7edccf0a..be9865273e8 100644 --- a/packages/versioning/src/mutator.ts +++ b/packages/versioning/src/mutator.ts @@ -7,6 +7,7 @@ import { getReturnTypeChangedFrom, getTypeChangedFrom, } from "./decorators.js"; +import { VersioningStateKeys } from "./lib.js"; import type { Version } from "./types.js"; import { VersioningTimeline, type TimelineMoment } from "./versioning-timeline.js"; import { Availability, getAvailabilityMapInTimeline, resolveVersions } from "./versioning.js"; @@ -163,6 +164,7 @@ export function createVersionMutator( }, Tuple: (original, clone, p, realm) => {}, ModelProperty: (original, clone, p, realm) => { + p.stateMap(VersioningStateKeys.optionalitySource).set(clone, original); rename(original, clone); clone.optional = versioning.getOptionalAtVersion(original, moment); const typeAtVersion = versioning.getTypeAtVersion(original, moment); diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index 51413b7ba7f..f7128b3424d 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -689,6 +689,55 @@ describe("versioning: validate incompatible references", () => { message: "Property 'name?' marked with @madeRequired but is optional. Should be 'name'", }); }); + + it.each([ + "model Derived is OptionalProperties;", + "model Derived { ...OptionalProperties; }", + ` + model Spread { ...Source; } + model Copy is Spread; + model Optional is OptionalProperties; + model AfterSpread { ...Optional; } + model Derived is AfterSpread; + `, + ])("allows superseded optionality history: %s", async (derived) => { + const diagnostics = await runner.diagnose(` + model Source { + @madeRequired(Versions.v2) + name: string; + } + ${derived} + `); + expectDiagnosticEmpty(diagnostics); + }); + + it.each([ + ` + @withOptionalProperties + model Derived { ...Source; } + `, + ` + @withOptionalProperties + model Changed { ...Source; } + model Derived { ...Changed; } + `, + ])( + "validates a newly authored optionality decorator on a transformed copy: %s", + async (derived) => { + const diagnostics = await runner.diagnose(` + model Source { + @madeRequired(Versions.v2) + name: string; + } + ${derived} + @@madeRequired(Derived.name, Versions.v2); + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/made-required-optional", + message: "Property 'name?' marked with @madeRequired but is optional. Should be 'name'", + }); + }, + ); }); describe("operations", () => { diff --git a/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts b/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts index 7e0828f423a..3edf9a55e2f 100644 --- a/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts +++ b/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts @@ -1,14 +1,17 @@ import { getMediaTypeHint, + type DecoratorContext, + type Model, type Namespace, type Program, type Scalar, type Type, } from "@typespec/compiler"; import { unsafe_mutateSubgraphWithNamespace } from "@typespec/compiler/experimental"; -import { t } from "@typespec/compiler/testing"; +import { expectDiagnostics, mockFile, t } from "@typespec/compiler/testing"; import { strictEqual } from "assert"; import { describe, expect, it } from "vitest"; +import { getMadeOptionalOn, getMadeRequiredOn } from "../../src/decorators.js"; import { getVersioningMutators } from "../../src/mutator.js"; import { Tester } from "../test-host.js"; @@ -21,8 +24,9 @@ const baseCode = ` `; async function testMutationLogic( code: string, + tester = Tester, ): Promise<{ program: Program; v1: Namespace; v2: Namespace; v3: Namespace }> { - const runner = await Tester.createInstance(); + const runner = await tester.createInstance(); const fullCode = baseCode + "\n" + code; const { Service } = await runner.compile(fullCode); const mutators = getVersioningMutators(runner.program, Service as Namespace); @@ -166,6 +170,197 @@ describe("model properties", () => { expect(accessor(v2).get("a")!.optional).toBe(false); expect(accessor(v3).get("a")!.optional).toBe(false); }); + + describe("derived optionality", () => { + const optionalityTester = Tester.files({ + "optionality.js": mockFile.js({ + $setOptionality(_context: DecoratorContext, model: Model, optional: boolean) { + for (const property of model.properties.values()) { + property.optional = optional; + } + }, + }), + "optionality.tsp": ` + import "./optionality.js"; + extern dec setOptionality(target: TypeSpec.Reflection.Model, optional: valueof boolean); + `, + }).import("./optionality.tsp"); + + it.each([ + "model Test is OptionalProperties;", + "model Test { ...OptionalProperties; }", + ` + model BeforeSpread { ...Source; } + model BeforeCopy is BeforeSpread; + model Optional is OptionalProperties; + model AfterSpread { ...Optional; } + model Test is AfterSpread; + `, + ])("keeps transformed properties optional in every snapshot: %s", async (derived) => { + const { program, v1, v2, v3 } = await testMutationLogic(` + model Source { + @madeRequired(Versions.v2) + a: string; + } + ${derived} + `); + for (const ns of [v1, v2, v3]) { + const property = accessor(ns).get("a")!; + expect(property.optional).toBe(true); + expect(getMadeRequiredOn(program, property)).toBeUndefined(); + } + expect(v1.models.get("Source")!.properties.get("a")!.optional).toBe(true); + expect(v2.models.get("Source")!.properties.get("a")!.optional).toBe(false); + expect(v3.models.get("Source")!.properties.get("a")!.optional).toBe(false); + }); + + it.each([true, false])("recognizes custom optionality transforms to %s", async (optional) => { + const { program, v1, v2, v3 } = await testMutationLogic( + ` + model Source { + @${optional ? "madeRequired" : "madeOptional"}(Versions.v2) + a${optional ? "" : "?"}: string; + } + model Before { ...Source; } + @setOptionality(${optional}) + model Changed { ...Before; } + model After is Changed; + model Test { ...After; } + `, + optionalityTester, + ); + for (const ns of [v1, v2, v3]) { + const property = accessor(ns).get("a")!; + expect(property.optional).toBe(optional); + expect(getMadeOptionalOn(program, property)).toBeUndefined(); + expect(getMadeRequiredOn(program, property)).toBeUndefined(); + } + }); + + it.each([true, false])( + "validates new history on a copy transformed to %s", + async (optional) => { + const diagnostics = await optionalityTester.diagnose(` + ${baseCode} + model Source { + @${optional ? "madeRequired" : "madeOptional"}(Versions.v2) + a${optional ? "" : "?"}: string; + } + @setOptionality(${optional}) + model Changed { ...Source; } + model Test { ...Changed; } + @@${optional ? "madeRequired" : "madeOptional"}(Test.a, Versions.v2); + `); + expectDiagnostics(diagnostics, { + code: `@typespec/versioning/${optional ? "made-required-optional" : "made-optional-not-optional"}`, + }); + }, + ); + + it("does not revive history when a later transform restores the original optionality", async () => { + const { program, v1, v2, v3 } = await testMutationLogic( + ` + model Source { + @madeRequired(Versions.v2) + a: string; + } + @setOptionality(true) + model Optional { ...Source; } + model Copy { ...Optional; } + @setOptionality(false) + model Required { ...Copy; } + model Test { ...Required; } + `, + optionalityTester, + ); + for (const ns of [v1, v2, v3]) { + expect(accessor(ns).get("a")!.optional).toBe(false); + expect(getMadeRequiredOn(program, accessor(ns).get("a")!)).toBeUndefined(); + } + }); + + it("retains history when a transform makes no discernible optionality change", async () => { + const { program, v1, v2, v3 } = await testMutationLogic(` + model Source { + @madeOptional(Versions.v2) + a?: string; + } + model Test { ...OptionalProperties; } + `); + expect(accessor(v1).get("a")!.optional).toBe(false); + expect(accessor(v2).get("a")!.optional).toBe(true); + expect(accessor(v3).get("a")!.optional).toBe(true); + for (const ns of [v1, v2, v3]) { + expect(getMadeOptionalOn(program, accessor(ns).get("a")!)?.name).toBe("v2"); + } + }); + + it.each([true, false])("preserves unchanged optionality history (%s)", async (optional) => { + const { program, v1, v2, v3 } = await testMutationLogic(` + model Source { + @${optional ? "madeOptional" : "madeRequired"}(Versions.v2) + a${optional ? "?" : ""}: string; + } + model Spread { ...Source; } + model Copy is Spread; + model Test { ...Copy; } + `); + expect(accessor(v1).get("a")!.optional).toBe(!optional); + expect(accessor(v2).get("a")!.optional).toBe(optional); + expect(accessor(v3).get("a")!.optional).toBe(optional); + for (const ns of [v1, v2, v3]) { + const getter = optional ? getMadeOptionalOn : getMadeRequiredOn; + expect(getter(program, accessor(ns).get("a")!)?.name).toBe("v2"); + } + }); + + it("preserves unrelated history on transformed properties", async () => { + const { v1, v2, v3 } = await testMutationLogic(` + model Source { + @madeRequired(Versions.v2) + @renamedFrom(Versions.v2, "old") + @typeChangedFrom(Versions.v2, string) + a: int32; + @added(Versions.v2) + @madeRequired(Versions.v3) + added: string; + @removed(Versions.v3) + @madeRequired(Versions.v2) + removed: string; + } + model Test { ...OptionalProperties; } + `); + expect(accessor(v1).get("old")!.optional).toBe(true); + expect((accessor(v1).get("old")!.type as Scalar).name).toBe("string"); + expect(accessor(v1).has("a")).toBe(false); + for (const ns of [v2, v3]) { + expect(accessor(ns).get("a")!.optional).toBe(true); + expect((accessor(ns).get("a")!.type as Scalar).name).toBe("int32"); + expect(accessor(ns).has("old")).toBe(false); + expect(accessor(ns).get("added")!.optional).toBe(true); + } + expect(accessor(v1).has("added")).toBe(false); + expect(accessor(v1).get("removed")!.optional).toBe(true); + expect(accessor(v2).get("removed")!.optional).toBe(true); + expect(accessor(v3).has("removed")).toBe(false); + }); + + it("uses newly authored optionality history after a transform", async () => { + const { v1, v2, v3 } = await testMutationLogic(` + model Source { + @madeRequired(Versions.v2) + a: string; + } + @withOptionalProperties + model Changed { ...Source; } + @@madeOptional(Changed.a, Versions.v3); + model Test { ...Changed; } + `); + expect(accessor(v1).get("a")!.optional).toBe(false); + expect(accessor(v2).get("a")!.optional).toBe(false); + expect(accessor(v3).get("a")!.optional).toBe(true); + }); + }); }); describe("enums", () => { diff --git a/website/src/content/docs/docs/libraries/versioning/guide.md b/website/src/content/docs/docs/libraries/versioning/guide.md index b7645cd483e..5c34e8f2d4e 100644 --- a/website/src/content/docs/docs/libraries/versioning/guide.md +++ b/website/src/content/docs/docs/libraries/versioning/guide.md @@ -167,3 +167,24 @@ Widget: ``` This is a common pattern with the versioning decorators. The TypeSpec should represent the _current state_ of the API. The decorators indicate the version at which this definition became accurate and, depending on the decorator, the other parameters reflect the previous values to retain that information. + +## Optionality of derived properties + +Properties copied through model `is` or spread retain their versioning history. If a transformation changes a copied property's optionality relative to its source, the inherited `@madeOptional` or `@madeRequired` history is superseded: it no longer affects validation or version snapshots. This also applies through multiple copies before or after the transformation, and does not depend on which decorator performs the transformation. + +For example: + +```typespec +model Source { + @madeRequired(Versions.v2) + foo: string; +} + +model Patch { + ...OptionalProperties; +} +``` + +`Source.foo` is optional before `v2` and required from `v2` onward. `Patch.foo` is optional in every version. Using `model Patch is OptionalProperties` also works. Other inherited history, such as `@added`, `@removed`, `@renamedFrom`, and `@typeChangedFrom`, continues to apply. Newly applied optionality decorators on copied properties are still validated against those properties. + +This rule only detects an actual optionality difference between a copy and its source. Applying `OptionalProperties` to an already-optional property does not provide such a difference. In particular, spreading `OptionalProperties` when `Source.foo` is declared as `@madeOptional(Versions.v2) foo?: string` retains that history, so the spread property remains required before `v2`. To avoid inherited optionality history in that case, declare the derived property explicitly. From 823e7f0b786607c1cbec6bfd39ab2b4aeb0dfc35 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 15 Sep 2026 11:09:06 -0400 Subject: [PATCH 2/2] fix(versioning): simplify optionality detection to declaration comparison --- ...tionality-provenance-2026-8-15-10-37-14.md | 2 +- packages/versioning/src/decorators.ts | 56 ++----------------- packages/versioning/src/lib.ts | 3 - packages/versioning/src/mutator.ts | 13 ++++- packages/versioning/src/validate.ts | 5 +- packages/versioning/src/versioning.ts | 8 +++ .../test/incompatible-versioning.test.ts | 10 ++-- .../apply-snapshot-versioning.test.ts | 56 +++++++++++++------ .../docs/docs/libraries/versioning/guide.md | 13 ++++- 9 files changed, 80 insertions(+), 86 deletions(-) diff --git a/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md b/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md index 9128092167d..bfc5fb324be 100644 --- a/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md +++ b/.chronus/changes/fix-versioning-optionality-provenance-2026-8-15-10-37-14.md @@ -4,4 +4,4 @@ packages: - "@typespec/versioning" --- -Ignore inherited optionality history when a derived property structurally changes optionality, avoiding incorrect diagnostics and preserving the transformed optionality in version snapshots. \ No newline at end of file +Preserve transformed property optionality in version snapshots and skip incompatible optionality diagnostics when the property differs from its original declaration, without special-casing transformation helpers. \ No newline at end of file diff --git a/packages/versioning/src/decorators.ts b/packages/versioning/src/decorators.ts index 806138c1098..e3c655f2c88 100644 --- a/packages/versioning/src/decorators.ts +++ b/packages/versioning/src/decorators.ts @@ -222,10 +222,7 @@ export const $madeOptional: MadeOptionalDecorator = ( if (!version) { return; } - program.stateMap(VersioningStateKeys.madeOptional).set(t, { - version, - application: context.decoratorTarget, - } satisfies OptionalityHistory); + program.stateMap(VersioningStateKeys.madeOptional).set(t, version); }; export const $madeRequired: MadeRequiredDecorator = ( @@ -238,54 +235,14 @@ export const $madeRequired: MadeRequiredDecorator = ( if (!version) { return; } - program.stateMap(VersioningStateKeys.madeRequired).set(t, { - version, - application: context.decoratorTarget, - } satisfies OptionalityHistory); + program.stateMap(VersioningStateKeys.madeRequired).set(t, version); }; -interface OptionalityHistory { - version: Version; - application: DiagnosticTarget; -} - -function getOptionalitySource(program: Program, type: Type): Type { - const sources = program.stateMap(VersioningStateKeys.optionalitySource); - while (sources.has(type)) { - type = sources.get(type); - } - return type; -} - -function getEffectiveOptionalityHistory( - program: Program, - type: Type, - key: symbol, -): Version | undefined { - // Snapshot optionality can differ from the current API without being a structural - // transform. Inspect the original graph, including its intermediate spread/is copies. - type = getOptionalitySource(program, type); - const state: Map = program.stateMap(key); - const history = state.get(type); - if (!history) return undefined; - - while (type.kind === "ModelProperty" && type.sourceProperty) { - const source = getOptionalitySource(program, type.sourceProperty); - const sourceHistory = state.get(source); - // A new decorator application on a copy owns its history and must still be validated. - if (!history.application || sourceHistory?.application !== history.application) break; - if (source.kind !== "ModelProperty" || type.optional !== source.optional) return undefined; - type = source; - } - return history.version; -} - /** - * @returns version when the given type was made required, unless a derived property - * structurally changed optionality and superseded the inherited history. + * @returns version when the given type was made required if applicable. */ export function getMadeRequiredOn(p: Program, t: Type): Version | undefined { - return getEffectiveOptionalityHistory(p, t, VersioningStateKeys.madeRequired); + return p.stateMap(VersioningStateKeys.madeRequired).get(t); } /** @@ -311,11 +268,10 @@ export function getRemovedOnVersions(p: Program, t: Type): Version[] | undefined } /** - * @returns version when the given type was made optional, unless a derived property - * structurally changed optionality and superseded the inherited history. + * @returns version when the given type was made optional if applicable. */ export function getMadeOptionalOn(p: Program, t: Type): Version | undefined { - return getEffectiveOptionalityHistory(p, t, VersioningStateKeys.madeOptional); + return p.stateMap(VersioningStateKeys.madeOptional).get(t); } export class VersionMap { diff --git a/packages/versioning/src/lib.ts b/packages/versioning/src/lib.ts index 7a5f870f7df..8c6474b206e 100644 --- a/packages/versioning/src/lib.ts +++ b/packages/versioning/src/lib.ts @@ -95,9 +95,6 @@ export const $lib = createTypeSpecLibrary({ renamedFrom: { description: "State for @renamedFrom decorator" }, madeOptional: { description: "State for @madeOptional decorator" }, madeRequired: { description: "State for @madeRequired decorator" }, - optionalitySource: { - description: "Original property for version snapshot optionality history", - }, typeChangedFrom: { description: "State for @typeChangedFrom decorator" }, returnTypeChangedFrom: { description: "State for @returnTypeChangedFrom decorator" }, }, diff --git a/packages/versioning/src/mutator.ts b/packages/versioning/src/mutator.ts index be9865273e8..0cc2df07ff3 100644 --- a/packages/versioning/src/mutator.ts +++ b/packages/versioning/src/mutator.ts @@ -7,10 +7,14 @@ import { getReturnTypeChangedFrom, getTypeChangedFrom, } from "./decorators.js"; -import { VersioningStateKeys } from "./lib.js"; import type { Version } from "./types.js"; import { VersioningTimeline, type TimelineMoment } from "./versioning-timeline.js"; -import { Availability, getAvailabilityMapInTimeline, resolveVersions } from "./versioning.js"; +import { + Availability, + getAvailabilityMapInTimeline, + hasChangedOptionality, + resolveVersions, +} from "./versioning.js"; /** * When the service is versioned. @@ -164,7 +168,6 @@ export function createVersionMutator( }, Tuple: (original, clone, p, realm) => {}, ModelProperty: (original, clone, p, realm) => { - p.stateMap(VersioningStateKeys.optionalitySource).set(clone, original); rename(original, clone); clone.optional = versioning.getOptionalAtVersion(original, moment); const typeAtVersion = versioning.getTypeAtVersion(original, moment); @@ -228,6 +231,10 @@ class VersioningHelper { return type.returnType; } getOptionalAtVersion(type: ModelProperty, moment: TimelineMoment): boolean { + // Compare before creating the snapshot, whose optionality may legitimately + // differ from its declaration because of versioning itself. + if (hasChangedOptionality(type)) return type.optional; + const optionalAt = getMadeOptionalOn(this.#program, type); const requiredAt = getMadeRequiredOn(this.#program, type); if (!optionalAt && !requiredAt) return type.optional; diff --git a/packages/versioning/src/validate.ts b/packages/versioning/src/validate.ts index ae2bab1d07a..5eee6bc8bdd 100644 --- a/packages/versioning/src/validate.ts +++ b/packages/versioning/src/validate.ts @@ -32,6 +32,7 @@ import { getAvailabilityMap, getVersionDependencies, getVersions, + hasChangedOptionality, } from "./versioning.js"; const relationCacheKey = Symbol.for("TypeSpec.Versioning.NamespaceRelationCache"); @@ -480,7 +481,7 @@ function validateVersionedPropertyNames(program: Program, source: Type) { } function validateMadeOptional(program: Program, target: Type) { - if (target.kind === "ModelProperty") { + if (target.kind === "ModelProperty" && !hasChangedOptionality(target)) { const madeOptionalOn = getMadeOptionalOn(program, target); if (!madeOptionalOn) { return; @@ -500,7 +501,7 @@ function validateMadeOptional(program: Program, target: Type) { } function validateMadeRequired(program: Program, target: Type) { - if (target.kind === "ModelProperty") { + if (target.kind === "ModelProperty" && !hasChangedOptionality(target)) { const madeRequiredOn = getMadeRequiredOn(program, target); if (!madeRequiredOn) { return; diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index 0999228273f..f0aee8d93cd 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -2,10 +2,12 @@ import { getNamespaceFullName, type Enum, type EnumMember, + type ModelProperty, type Namespace, type Program, type Type, } from "@typespec/compiler"; +import { SyntaxKind } from "@typespec/compiler/ast"; import { getAddedOnVersions, getRemovedOnVersions, @@ -19,6 +21,12 @@ import type { Version, VersionResolution } from "./types.js"; import { getCachedNamespaceDependencies } from "./validate.js"; import { TimelineMoment, VersioningTimeline } from "./versioning-timeline.js"; +export function hasChangedOptionality(property: ModelProperty): boolean { + return ( + property.node?.kind === SyntaxKind.ModelProperty && property.optional !== property.node.optional + ); +} + export function getVersionDependencies( program: Program, namespace: Namespace, diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index f7128b3424d..e9bd9474e01 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -693,6 +693,7 @@ describe("versioning: validate incompatible references", () => { it.each([ "model Derived is OptionalProperties;", "model Derived { ...OptionalProperties; }", + "@withOptionalProperties model Derived { @madeRequired(Versions.v2) name: string; }", ` model Spread { ...Source; } model Copy is Spread; @@ -700,7 +701,7 @@ describe("versioning: validate incompatible references", () => { model AfterSpread { ...Optional; } model Derived is AfterSpread; `, - ])("allows superseded optionality history: %s", async (derived) => { + ])("allows optionality changed from the declaration: %s", async (derived) => { const diagnostics = await runner.diagnose(` model Source { @madeRequired(Versions.v2) @@ -722,7 +723,7 @@ describe("versioning: validate incompatible references", () => { model Derived { ...Changed; } `, ])( - "validates a newly authored optionality decorator on a transformed copy: %s", + "cannot distinguish new augments when optionality differs from the declaration: %s", async (derived) => { const diagnostics = await runner.diagnose(` model Source { @@ -732,10 +733,7 @@ describe("versioning: validate incompatible references", () => { ${derived} @@madeRequired(Derived.name, Versions.v2); `); - expectDiagnostics(diagnostics, { - code: "@typespec/versioning/made-required-optional", - message: "Property 'name?' marked with @madeRequired but is optional. Should be 'name'", - }); + expectDiagnosticEmpty(diagnostics); }, ); }); diff --git a/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts b/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts index 3edf9a55e2f..c0e62aa1602 100644 --- a/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts +++ b/packages/versioning/test/mutations/apply-snapshot-versioning.test.ts @@ -8,7 +8,7 @@ import { type Type, } from "@typespec/compiler"; import { unsafe_mutateSubgraphWithNamespace } from "@typespec/compiler/experimental"; -import { expectDiagnostics, mockFile, t } from "@typespec/compiler/testing"; +import { expectDiagnosticEmpty, expectDiagnostics, mockFile, t } from "@typespec/compiler/testing"; import { strictEqual } from "assert"; import { describe, expect, it } from "vitest"; import { getMadeOptionalOn, getMadeRequiredOn } from "../../src/decorators.js"; @@ -179,10 +179,16 @@ describe("model properties", () => { property.optional = optional; } }, + $withoutPropertyNodes(_context: DecoratorContext, model: Model) { + for (const property of model.properties.values()) { + delete property.node; + } + }, }), "optionality.tsp": ` import "./optionality.js"; extern dec setOptionality(target: TypeSpec.Reflection.Model, optional: valueof boolean); + extern dec withoutPropertyNodes(target: TypeSpec.Reflection.Model); `, }).import("./optionality.tsp"); @@ -207,7 +213,7 @@ describe("model properties", () => { for (const ns of [v1, v2, v3]) { const property = accessor(ns).get("a")!; expect(property.optional).toBe(true); - expect(getMadeRequiredOn(program, property)).toBeUndefined(); + expect(getMadeRequiredOn(program, property)?.name).toBe("v2"); } expect(v1.models.get("Source")!.properties.get("a")!.optional).toBe(true); expect(v2.models.get("Source")!.properties.get("a")!.optional).toBe(false); @@ -232,13 +238,13 @@ describe("model properties", () => { for (const ns of [v1, v2, v3]) { const property = accessor(ns).get("a")!; expect(property.optional).toBe(optional); - expect(getMadeOptionalOn(program, property)).toBeUndefined(); - expect(getMadeRequiredOn(program, property)).toBeUndefined(); + const getter = optional ? getMadeRequiredOn : getMadeOptionalOn; + expect(getter(program, property)?.name).toBe("v2"); } }); it.each([true, false])( - "validates new history on a copy transformed to %s", + "cannot distinguish new history on a copy transformed to %s", async (optional) => { const diagnostics = await optionalityTester.diagnose(` ${baseCode} @@ -251,13 +257,11 @@ describe("model properties", () => { model Test { ...Changed; } @@${optional ? "madeRequired" : "madeOptional"}(Test.a, Versions.v2); `); - expectDiagnostics(diagnostics, { - code: `@typespec/versioning/${optional ? "made-required-optional" : "made-optional-not-optional"}`, - }); + expectDiagnosticEmpty(diagnostics); }, ); - it("does not revive history when a later transform restores the original optionality", async () => { + it("retains history when a later transform restores the declared optionality", async () => { const { program, v1, v2, v3 } = await testMutationLogic( ` model Source { @@ -273,10 +277,10 @@ describe("model properties", () => { `, optionalityTester, ); - for (const ns of [v1, v2, v3]) { - expect(accessor(ns).get("a")!.optional).toBe(false); - expect(getMadeRequiredOn(program, accessor(ns).get("a")!)).toBeUndefined(); - } + expect(accessor(v1).get("a")!.optional).toBe(true); + expect(accessor(v2).get("a")!.optional).toBe(false); + expect(accessor(v3).get("a")!.optional).toBe(false); + expect(getMadeRequiredOn(program, accessor(v1).get("a")!)?.name).toBe("v2"); }); it("retains history when a transform makes no discernible optionality change", async () => { @@ -345,8 +349,8 @@ describe("model properties", () => { expect(accessor(v3).has("removed")).toBe(false); }); - it("uses newly authored optionality history after a transform", async () => { - const { v1, v2, v3 } = await testMutationLogic(` + it("also ignores new history when optionality differs from the declaration", async () => { + const { program, v1, v2, v3 } = await testMutationLogic(` model Source { @madeRequired(Versions.v2) a: string; @@ -356,9 +360,25 @@ describe("model properties", () => { @@madeOptional(Changed.a, Versions.v3); model Test { ...Changed; } `); - expect(accessor(v1).get("a")!.optional).toBe(false); - expect(accessor(v2).get("a")!.optional).toBe(false); - expect(accessor(v3).get("a")!.optional).toBe(true); + for (const ns of [v1, v2, v3]) { + expect(accessor(ns).get("a")!.optional).toBe(true); + expect(getMadeOptionalOn(program, accessor(ns).get("a")!)?.name).toBe("v3"); + } + }); + + it("does not infer an optionality change without a declaration node", async () => { + const diagnostics = await optionalityTester.diagnose(` + ${baseCode} + @withoutPropertyNodes + model Source { + @madeRequired(Versions.v2) + a: string; + } + model Test { ...OptionalProperties; } + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/made-required-optional", + }); }); }); }); diff --git a/website/src/content/docs/docs/libraries/versioning/guide.md b/website/src/content/docs/docs/libraries/versioning/guide.md index 5c34e8f2d4e..d660efa0662 100644 --- a/website/src/content/docs/docs/libraries/versioning/guide.md +++ b/website/src/content/docs/docs/libraries/versioning/guide.md @@ -170,7 +170,7 @@ This is a common pattern with the versioning decorators. The TypeSpec should rep ## Optionality of derived properties -Properties copied through model `is` or spread retain their versioning history. If a transformation changes a copied property's optionality relative to its source, the inherited `@madeOptional` or `@madeRequired` history is superseded: it no longer affects validation or version snapshots. This also applies through multiple copies before or after the transformation, and does not depend on which decorator performs the transformation. +Properties copied through model `is` or spread retain their versioning history. If a property's current optionality differs from its original declaration, versioning treats that difference as a transformation: it skips `@madeOptional` and `@madeRequired` validation for the property and preserves its current optionality in version snapshots. This compares the property with its syntax node, without recognizing any particular helper or tracking how it was copied. For example: @@ -185,6 +185,13 @@ model Patch { } ``` -`Source.foo` is optional before `v2` and required from `v2` onward. `Patch.foo` is optional in every version. Using `model Patch is OptionalProperties` also works. Other inherited history, such as `@added`, `@removed`, `@renamedFrom`, and `@typeChangedFrom`, continues to apply. Newly applied optionality decorators on copied properties are still validated against those properties. +`Source.foo` is optional before `v2` and required from `v2` onward. `Patch.foo` is optional in every version. Using `model Patch is OptionalProperties` also works. Other history, such as `@added`, `@removed`, `@renamedFrom`, and `@typeChangedFrom`, continues to apply. The metadata getters still return the recorded decorator values; only validation and snapshot optionality use the declaration comparison. -This rule only detects an actual optionality difference between a copy and its source. Applying `OptionalProperties` to an already-optional property does not provide such a difference. In particular, spreading `OptionalProperties` when `Source.foo` is declared as `@madeOptional(Versions.v2) foo?: string` retains that history, so the spread property remains required before `v2`. To avoid inherited optionality history in that case, declare the derived property explicitly. +This is a declaration-based heuristic, with limitations: + +- Applying `OptionalProperties` to an already-optional property produces no difference. Spreading a property declared as `@madeOptional(Versions.v2) foo?: string` therefore retains its historical requiredness before `v2`. +- Restoring a property's declared optionality after an intermediate transformation makes its history apply again. +- When optionality differs from the declaration, newly authored optionality augments are ignored for validation and snapshots too: this rule cannot distinguish them from inherited annotations. +- Properties without a model-property declaration node cannot be identified as transformed. + +Declare the derived property explicitly when independent optionality history is needed. Invalid optionality annotations on unchanged original declarations are still diagnosed.