diff --git a/common/changes/@typespec/compiler/versioning-OpParamsFix_2023-11-21-16-31.json b/common/changes/@typespec/compiler/versioning-OpParamsFix_2023-11-21-16-31.json new file mode 100644 index 0000000000..5945164bd2 --- /dev/null +++ b/common/changes/@typespec/compiler/versioning-OpParamsFix_2023-11-21-16-31.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/compiler", + "comment": "", + "type": "none" + } + ], + "packageName": "@typespec/compiler" +} \ No newline at end of file diff --git a/common/changes/@typespec/versioning/versioning-OpParamsFix_2023-11-21-16-31.json b/common/changes/@typespec/versioning/versioning-OpParamsFix_2023-11-21-16-31.json new file mode 100644 index 0000000000..3c8fd8571b --- /dev/null +++ b/common/changes/@typespec/versioning/versioning-OpParamsFix_2023-11-21-16-31.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/versioning", + "comment": "", + "type": "none" + } + ], + "packageName": "@typespec/versioning" +} \ No newline at end of file diff --git a/packages/versioning/src/validate.ts b/packages/versioning/src/validate.ts index 5748c727c1..03205a6391 100644 --- a/packages/versioning/src/validate.ts +++ b/packages/versioning/src/validate.ts @@ -3,11 +3,14 @@ import { getService, getTypeName, isTemplateInstance, + ModelProperty, Namespace, navigateProgram, NoTarget, + Operation, Program, Type, + TypeNameOptions, } from "@typespec/compiler"; import { reportDiagnostic } from "./lib.js"; import { Version } from "./types.js"; @@ -28,7 +31,7 @@ import { export function $onValidate(program: Program) { const namespaceDependencies = new Map>(); - function addDependency(source: Namespace | undefined, target: Type | undefined) { + function addNamespaceDependency(source: Namespace | undefined, target: Type | undefined) { if (!target || !("namespace" in target) || !target.namespace) { return; } @@ -47,13 +50,15 @@ export function $onValidate(program: Program) { if (isTemplateInstance(model)) { return; } - addDependency(model.namespace, model.sourceModel); - addDependency(model.namespace, model.baseModel); + addNamespaceDependency(model.namespace, model.sourceModel); + addNamespaceDependency(model.namespace, model.baseModel); for (const prop of model.properties.values()) { - addDependency(model.namespace, prop.type); + addNamespaceDependency(model.namespace, prop.type); // Validate model -> property have correct versioning - validateTargetVersionCompatible(program, model, prop, { isTargetADependent: true }); + validateTargetVersionCompatible(program, model, prop, { + isTargetADependent: true, + }); // Validate model property -> type have correct versioning const typeChangedFrom = getTypeChangedFrom(program, prop); @@ -77,7 +82,7 @@ export function $onValidate(program: Program) { return; } for (const variant of union.variants.values()) { - addDependency(union.namespace, variant.type); + addNamespaceDependency(union.namespace, variant.type); } validateVersionedPropertyNames(program, union); }, @@ -88,16 +93,26 @@ export function $onValidate(program: Program) { } const namespace = op.namespace ?? op.interface?.namespace; - addDependency(namespace, op.sourceOperation); - addDependency(namespace, op.parameters); - addDependency(namespace, op.returnType); - + addNamespaceDependency(namespace, op.sourceOperation); + addNamespaceDependency(namespace, op.returnType); if (op.interface) { validateTargetVersionCompatible(program, op.interface, op, { isTargetADependent: true }); } + for (const prop of op.parameters.properties.values()) { - validateReference(program, op, prop.type); + addNamespaceDependency(namespace, prop.type); + + const typeChangedFrom = getTypeChangedFrom(program, prop); + if (typeChangedFrom !== undefined) { + validateMultiTypeReference(program, prop); + } else { + validateOperationParameter(program, op, prop); + } + + // Validate model property type is correct when madeOptional + validateMadeOptional(program, prop); } + validateVersionedPropertyNames(program, op.parameters); validateReference(program, op, op.returnType); }, interface: (iface) => { @@ -188,7 +203,7 @@ function getAllVersions(p: Program, t: Type): Version[] | undefined { /** * Ensures that properties whose type has changed with versioning are valid. */ -function validateMultiTypeReference(program: Program, source: Type) { +function validateMultiTypeReference(program: Program, source: Type, options?: TypeNameOptions) { const versionTypeMap = getVersionedTypeMap(program, source); if (versionTypeMap === undefined) return; for (const [version, type] of versionTypeMap!) { @@ -202,8 +217,8 @@ function validateMultiTypeReference(program: Program, source: Type) { code: "incompatible-versioned-reference", messageId: "doesNotExist", format: { - sourceName: getTypeName(source), - targetName: getTypeName(type), + sourceName: getTypeName(source, options), + targetName: getTypeName(type, options), version: prettyVersion(version), }, target: source, @@ -461,6 +476,82 @@ interface IncompatibleVersionValidateOptions { isTargetADependent?: boolean; } +/** + * Validate the target reference versioning is compatible with the source versioning. + * @param operation The operation being containing the parameter + * @param parameter The parameter used in the operation + */ +function validateOperationParameter( + program: Program, + operation: Operation, + parameter: ModelProperty +) { + const allVersions = getAllVersions(program, operation); + if (allVersions === undefined) return; + const alwaysAvailMap = new Map(); + allVersions.forEach((ver) => alwaysAvailMap.set(ver.name, Availability.Available)); + + const operationAvailability = getAvailabilityMapWithParentInfo(program, operation); + const paramAvailability = getAvailabilityMapWithParentInfo(program, parameter); + const paramTypeAvailability = getAvailabilityMapWithParentInfo(program, parameter.type); + const [paramTypeNamespace] = getVersions(program, parameter.type); + // everything is available in all versions + if ( + operationAvailability === undefined && + paramAvailability === undefined && + paramTypeAvailability === undefined + ) { + return; + } + // intrinstic types are always available + if (paramTypeNamespace === undefined) return; + + // check if a parameter or parameter type is versioned but the operation is not + if (operationAvailability === undefined) { + if (paramAvailability !== undefined) { + reportDiagnostic(program, { + code: "incompatible-versioned-reference", + messageId: "default", + format: { + sourceName: getTypeName(operation), + targetName: getTypeName(parameter), + }, + target: operation, + }); + return; + } else if (paramTypeAvailability !== undefined) { + reportDiagnostic(program, { + code: "incompatible-versioned-reference", + messageId: "default", + format: { + sourceName: getTypeName(operation), + targetName: getTypeName(parameter.type), + }, + target: operation, + }); + return; + } + } + + if (paramAvailability !== undefined) { + validateAvailabilityForContains( + program, + operationAvailability, + paramAvailability, + operation, + parameter + ); + } else if (paramTypeAvailability !== undefined) { + validateAvailabilityForRef( + program, + operationAvailability, + paramTypeAvailability, + operation, + parameter.type + ); + } +} + /** * Validate the target reference versioning is compatible with the source versioning. * This will also validate any template arguments used in the reference. @@ -664,7 +755,9 @@ function validateAvailabilityForRef( sourceAvail: Map | undefined, targetAvail: Map, source: Type, - target: Type + target: Type, + sourceOptions?: TypeNameOptions, + targetOptions?: TypeNameOptions ) { // if source is unversioned and target is versioned if (sourceAvail === undefined) { @@ -717,6 +810,24 @@ function validateAvailabilityForRef( target: source, }); } + if ( + [Availability.Unavailable].includes(sourceVal) && + [Availability.Added, Availability.Available].includes(targetVal) + ) { + const targetAddedOn = findAvailabilityAfterVersion(key, Availability.Added, sourceAvail); + + reportDiagnostic(program, { + code: "incompatible-versioned-reference", + messageId: "addedAfter", + format: { + sourceName: getTypeName(target), + targetName: getTypeName(source), + sourceAddedOn: key, + targetAddedOn: targetAddedOn!, + }, + target: target, + }); + } if ( [Availability.Removed].includes(sourceVal) && [Availability.Unavailable].includes(targetVal) @@ -730,8 +841,8 @@ function validateAvailabilityForRef( code: "incompatible-versioned-reference", messageId: "removedBefore", format: { - sourceName: getTypeName(source), - targetName: getTypeName(target), + sourceName: getTypeName(source, sourceOptions), + targetName: getTypeName(target, targetOptions), sourceRemovedOn: key, targetRemovedOn: targetRemovedOn!, }, @@ -746,7 +857,9 @@ function validateAvailabilityForContains( sourceAvail: Map | undefined, targetAvail: Map, source: Type, - target: Type + target: Type, + sourceOptions?: TypeNameOptions, + targetOptions?: TypeNameOptions ) { if (!sourceAvail) return; @@ -764,8 +877,8 @@ function validateAvailabilityForContains( code: "incompatible-versioned-reference", messageId: "dependentAddedAfter", format: { - sourceName: getTypeName(source), - targetName: getTypeName(target), + sourceName: getTypeName(source, sourceOptions), + targetName: getTypeName(target, targetOptions), sourceAddedOn: sourceAddedOn!, targetAddedOn: key, }, diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index 2b1b9aea39..e3365de289 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -117,6 +117,19 @@ describe("versioning: validate incompatible references", () => { }); }); + it("emit diagnostic when unversioned op has a versioned parameter", async () => { + const diagnostics = await runner.diagnose(` + model Foo {} + + op test(param: string, @added(Versions.v2) newParam: Foo): void; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' is referencing versioned type 'TestService.(anonymous model).newParam' but is not versioned itself.", + }); + }); + it("emit diagnostic when unversioned op based on a template has a versioned model as a parameter", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) @@ -133,7 +146,7 @@ describe("versioning: validate incompatible references", () => { }); }); - it("emit diagnostic when when op was added before parameter", async () => { + it("emit diagnostic when when op was added before parameter type", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) model Foo {} @@ -148,7 +161,7 @@ describe("versioning: validate incompatible references", () => { }); }); - it("emit diagnostic when op based on a template was added before parameter", async () => { + it("emit diagnostic when op based on a template was added before parameter type", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) model Foo {} @@ -164,6 +177,47 @@ describe("versioning: validate incompatible references", () => { "'TestService.test' was added in version 'v1' but referencing type 'TestService.Foo' added in version 'v2'.", }); }); + + it("emit diagnostic when when parameter was added before op", async () => { + const diagnostics = await runner.diagnose(` + model Foo {} + + @added(Versions.v2) + op test(@added(Versions.v1) param: Foo): void; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' was added in version 'v2' but contains type 'TestService.(anonymous model).param' added in version 'v1'.", + }); + }); + + it("emit diagnostic when type changed to types that don't exist", async () => { + const diagnostics = await runner.diagnose(` + @added(Versions.v3) + model Foo {} + + @removed(Versions.v1) + model Doo {} + + @added(Versions.v3) + op test(@typeChangedFrom(Versions.v2, Doo) param: Foo): void; + `); + expectDiagnostics(diagnostics, [ + { + code: "@typespec/versioning/incompatible-versioned-reference", + severity: "error", + message: + "'TestService.(anonymous model).param' is referencing type 'TestService.Doo' which does not exist in version 'v1'.", + }, + { + code: "@typespec/versioning/incompatible-versioned-reference", + severity: "error", + message: + "'TestService.(anonymous model).param' is referencing type 'TestService.Foo' which does not exist in version 'v2'.", + }, + ]); + }); }); describe("operation return type", () => { diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index d2c1a06a19..a9af49eedc 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -5,17 +5,23 @@ import { Model, Namespace, Operation, + Program, ProjectionApplication, projectProgram, Scalar, Type, Union, } from "@typespec/compiler"; -import { BasicTestRunner, createTestWrapper, expectDiagnostics } from "@typespec/compiler/testing"; -import { fail, ok, strictEqual } from "assert"; +import { + BasicTestRunner, + createTestWrapper, + expectDiagnosticEmpty, + expectDiagnostics, +} from "@typespec/compiler/testing"; +import { deepStrictEqual, fail, ok, strictEqual } from "assert"; import { Version } from "../src/types.js"; import { VersioningTimeline } from "../src/versioning-timeline.js"; -import { getVersions, indexTimeline } from "../src/versioning.js"; +import { buildVersionProjections, getVersions, indexTimeline } from "../src/versioning.js"; import { createVersioningTestHost } from "./test-host.js"; import { assertHasMembers, @@ -873,6 +879,39 @@ describe("versioning: logic", () => { assertHasProperties(v2.parameters, ["a"]); }); + it("can be added on the same version the underlying model is added", async () => { + const { MyService } = (await runner.compile( + ` + @test("MyService") + @versioned(Versions) + namespace MyService; + + enum Versions { v1, v2 }; + + @added(Versions.v1) + op create(body: Widget, @added(Versions.v2) newThing: NewThing): Widget; + + model Widget { + @key id: string; + @added(Versions.v2) name: string; + } + + @added(Versions.v2) + model NewThing { + name: string; + } + ` + )) as { MyService: Namespace }; + + const [v1, v2] = runProjections(runner.program, MyService); + const w1 = v1.projectedTypes.get(MyService) as Namespace; + const w2 = v2.projectedTypes.get(MyService) as Namespace; + w1.models.get("Widget")?.properties.size === 1; + w1.operations.get("create")?.parameters.properties.size === 1; + w2.models.get("Widget")?.properties.size === 2; + w2.operations.get("create")?.parameters.properties.size === 2; + }); + it("can be removed", async () => { const { projections: [v1, v2], @@ -934,6 +973,45 @@ describe("versioning: logic", () => { assertHasProperties(v6.parameters, []); }); + it("can change type over multiple versions", async () => { + const { + projections: [v1, v2, v3, v4, v5], + } = await versionedOperation( + ["v1", "v2", "v3", "v4", "v5"], + `op Test( + @typeChangedFrom(Versions.v2, string) + @typeChangedFrom(Versions.v4, utcDateTime) + date: int64 + ): void;` + ); + + strictEqual((v1.parameters.properties.get("date")?.type as Scalar).name, "string"); + strictEqual((v2.parameters.properties.get("date")?.type as Scalar).name, "utcDateTime"); + strictEqual((v3.parameters.properties.get("date")?.type as Scalar).name, "utcDateTime"); + strictEqual((v4.parameters.properties.get("date")?.type as Scalar).name, "int64"); + strictEqual((v5.parameters.properties.get("date")?.type as Scalar).name, "int64"); + }); + + it("can be made optional", async () => { + const { + projections: [v1, v2], + } = await versionedOperation( + ["v1", "v2"], + `op Test(a: string, @madeOptional(Versions.v2) b?: string): void;` + ); + + const prop1 = [...v1.parameters.properties.values()]; + const prop2 = [...v2.parameters.properties.values()]; + deepStrictEqual( + prop1.map((x) => x.optional), + [false, false] + ); + deepStrictEqual( + prop2.map((x) => x.optional), + [false, true] + ); + }); + async function versionedOperation(versions: string[], operation: string) { const { Test } = (await runner.compile(` @versioned(Versions) @@ -1780,3 +1858,9 @@ describe("versioning: logic", () => { return projector.projectedTypes.get(target) as T; } }); +function runProjections(program: Program, rootNs: Namespace) { + const versions = buildVersionProjections(program, rootNs); + const projectedPrograms = versions.map((x) => projectProgram(program, x.projections)); + projectedPrograms.forEach((p) => expectDiagnosticEmpty(p.diagnostics)); + return projectedPrograms.map((p) => p.projector); +}