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..bfc5fb324be --- /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" +--- + +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/mutator.ts b/packages/versioning/src/mutator.ts index 44f7edccf0a..0cc2df07ff3 100644 --- a/packages/versioning/src/mutator.ts +++ b/packages/versioning/src/mutator.ts @@ -9,7 +9,12 @@ import { } from "./decorators.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. @@ -226,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 51413b7ba7f..e9bd9474e01 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -689,6 +689,53 @@ 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; }", + "@withOptionalProperties model Derived { @madeRequired(Versions.v2) name: string; }", + ` + model Spread { ...Source; } + model Copy is Spread; + model Optional is OptionalProperties; + model AfterSpread { ...Optional; } + model Derived is AfterSpread; + `, + ])("allows optionality changed from the declaration: %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; } + `, + ])( + "cannot distinguish new augments when optionality differs from the declaration: %s", + async (derived) => { + const diagnostics = await runner.diagnose(` + model Source { + @madeRequired(Versions.v2) + name: string; + } + ${derived} + @@madeRequired(Derived.name, Versions.v2); + `); + expectDiagnosticEmpty(diagnostics); + }, + ); }); 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..c0e62aa1602 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 { 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"; 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,217 @@ 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; + } + }, + $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"); + + 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)?.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); + 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); + const getter = optional ? getMadeRequiredOn : getMadeOptionalOn; + expect(getter(program, property)?.name).toBe("v2"); + } + }); + + it.each([true, false])( + "cannot distinguish 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); + `); + expectDiagnosticEmpty(diagnostics); + }, + ); + + it("retains history when a later transform restores the declared 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, + ); + 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 () => { + 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("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; + } + @withOptionalProperties + model Changed { ...Source; } + @@madeOptional(Changed.a, Versions.v3); + model Test { ...Changed; } + `); + 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", + }); + }); + }); }); 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..d660efa0662 100644 --- a/website/src/content/docs/docs/libraries/versioning/guide.md +++ b/website/src/content/docs/docs/libraries/versioning/guide.md @@ -167,3 +167,31 @@ 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 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: + +```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 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 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.