From 98db543602362a7bae059e222796c5dd27a63e7a Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Tue, 29 Aug 2023 15:02:54 -0700 Subject: [PATCH 1/3] Fixes #2341. --- packages/versioning/test/versioning.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index 89c64dcc3e9..8340877e0c0 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -381,6 +381,27 @@ describe("versioning: logic", () => { ok(v2.properties.get("b")!.optional === true); }); + it("can still change type", async () => { + const { + projections: [v1, v2, v3], + } = await versionedModel( + ["v1", "v2", "v3"], + ` + model Original {} + + @added(Versions.v2) + model Updated {} + + model Foo { + @typeChangedFrom(Versions.v2, Original) + prop: Updated; + } + ` + ); + + let test = "best"; + }); + it("can change type", async () => { const { projections: [v1, v2, v3], From 89b9be8b8abad3f7ef7ddb5cf9adbe1382cadbdd Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Fri, 8 Sep 2023 09:19:40 -0700 Subject: [PATCH 2/3] Fixes #2341. --- packages/versioning/src/lib.ts | 1 + packages/versioning/src/validate.ts | 122 +++++++++++++++++- packages/versioning/src/versioning.ts | 20 ++- .../test/incompatible-versioning.test.ts | 23 ++++ packages/versioning/test/versioning.test.ts | 16 ++- 5 files changed, 168 insertions(+), 14 deletions(-) diff --git a/packages/versioning/src/lib.ts b/packages/versioning/src/lib.ts index 4f6fd24cb15..dabd904ceca 100644 --- a/packages/versioning/src/lib.ts +++ b/packages/versioning/src/lib.ts @@ -73,6 +73,7 @@ const libDef = { dependentRemovedBefore: paramMessage`'${"sourceName"}' was removed on version '${"sourceRemovedOn"}' but contains type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`, versionedDependencyAddedAfter: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' added in version '${"targetAddedOn"}' but version used is ${"dependencyVersion"}.`, versionedDependencyRemovedBefore: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' removed in version '${"targetAddedOn"}' but version used is ${"dependencyVersion"}.`, + doesNotExist: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' which does not exist in version '${"version"}'.`, }, }, "incompatible-versioned-namespace-use-dependency": { diff --git a/packages/versioning/src/validate.ts b/packages/versioning/src/validate.ts index 01524667e1a..13e39fc792d 100644 --- a/packages/versioning/src/validate.ts +++ b/packages/versioning/src/validate.ts @@ -16,7 +16,10 @@ import { findVersionedNamespace, getAvailabilityMap, getMadeOptionalOn, + getReturnTypeChangedFrom, + getTypeChangedFrom, getUseDependencies, + getVersion, getVersionDependencies, getVersions, } from "./versioning.js"; @@ -52,7 +55,12 @@ export function $onValidate(program: Program) { validateTargetVersionCompatible(program, model, prop, { isTargetADependent: true }); // Validate model property -> type have correct versioning - validateReference(program, prop, prop.type); + const typeChangedFrom = getTypeChangedFrom(program, prop); + if (typeChangedFrom !== undefined) { + validateMultiTypeReference(program, prop); + } else { + validateReference(program, prop, prop.type); + } // Validate model property type is correct when madeOptional validateMadeOptional(program, prop); @@ -164,6 +172,95 @@ export function $onValidate(program: Program) { validateVersionedNamespaceUsage(program, namespaceDependencies); } +function getAllVersions(p: Program, t: Type): Version[] | undefined { + const [namespace, _] = getVersions(p, t); + if (namespace === undefined) return undefined; + + return getVersion(p, namespace)?.getVersions(); +} + +/** + * Ensures that properties whose type has changed with versioning are valid. + */ +function validateMultiTypeReference(program: Program, source: Type) { + const versionTypeMap = getVersionedTypeMap(program, source); + if (versionTypeMap === undefined) return; + for (const [version, type] of versionTypeMap!) { + if (type === undefined) continue; + const availMap = getAvailabilityMap(program, type); + const availability = availMap?.get(version.name) ?? Availability.Available; + if ([Availability.Added, Availability.Available].includes(availability)) { + continue; + } + reportDiagnostic(program, { + code: "incompatible-versioned-reference", + messageId: "doesNotExist", + format: { + sourceName: getTypeName(source), + targetName: getTypeName(type), + version: prettyVersion(version), + }, + target: source, + }); + } +} + +/** + * Constructs a map of version to type for the the source. + */ +function getVersionedTypeMap( + program: Program, + source: Type +): Map | undefined { + const allVersions = getAllVersions(program, source); + if (allVersions === undefined) return undefined; + + const map: Map = new Map(allVersions.map((v) => [v, undefined])); + const availMap = getAvailabilityMap(program, source); + const alwaysAvail = availMap === undefined; + + // Populate the map with any typeChangedFrom data, which may have holes. + // We will fill these holes in a later pass. + const typeChangedFrom = getTypeChangedFrom(program, source); + if (typeChangedFrom !== undefined) { + for (const [version, type] of typeChangedFrom) { + const versionIndex = allVersions.indexOf(version); + if (versionIndex !== -1) { + map.set(allVersions[versionIndex - 1], type); + } + } + } + let lastType: Type | undefined = undefined; + switch (source.kind) { + case "ModelProperty": + lastType = source.type; + break; + default: + throw new Error(`Not implemented '${source.kind}'.`); + } + for (const version of allVersions.reverse()) { + const isAvail = + alwaysAvail || + [Availability.Added, Availability.Available].includes(availMap.get(version.name)!); + + // If property is unavailable in this version, it can't have a type + if (!isAvail) { + map.set(version, undefined); + continue; + } + + // Working backwards, we fill in any holes from the last type we encountered. Since we expect + // to encounter a hole at the start, we use the raw property type + const mapType = map.get(version); + if (mapType !== undefined) { + lastType = mapType; + } else { + map.set(version, lastType); + } + } + return map; +} + /** * Ensures that the version enum for a @versioned namespace has unique values. */ @@ -292,15 +389,13 @@ function getAvailabilityMapWithParentInfo( case "ModelProperty": const parentModel = type.model; if (parentModel) { - parentMap = getAvailabilityMap(program, parentModel); + parentMap = getAvailabilityMapWithParentInfo(program, parentModel); } break; default: break; } - if (!base && !parentMap) return undefined; - else if (!base && parentMap) return parentMap; - else return base; + return base ?? parentMap; } /** @@ -466,8 +561,20 @@ function validateAvailabilityForRef( } return; } - - const keySet = new Set([...sourceAvail.keys(), ...targetAvail.keys()]); + let keyValSource: string[] = [...sourceAvail.keys(), ...targetAvail.keys()]; + const sourceTypeChanged = getTypeChangedFrom(program, source); + if (sourceTypeChanged !== undefined) { + const sourceTypeChangedKeys = [...sourceTypeChanged.keys()].map((item) => item.name); + keyValSource = [...keyValSource, ...sourceTypeChangedKeys]; + } + const sourceReturnTypeChanged = getReturnTypeChangedFrom(program, source); + if (sourceReturnTypeChanged !== undefined) { + const sourceReturnTypeChangedKeys = [...sourceReturnTypeChanged.keys()].map( + (item) => item.name + ); + keyValSource = [...keyValSource, ...sourceReturnTypeChangedKeys]; + } + const keySet = new Set(keyValSource); for (const key of keySet) { const sourceVal = sourceAvail.get(key)!; @@ -477,6 +584,7 @@ function validateAvailabilityForRef( [Availability.Removed, Availability.Unavailable].includes(targetVal) ) { const targetAddedOn = findAvailabilityAfterVersion(key, Availability.Added, targetAvail); + reportDiagnostic(program, { code: "incompatible-versioned-reference", messageId: "addedAfter", diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index a26955e2758..3d4c04af40a 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -695,10 +695,18 @@ export function getAvailabilityMap( const added = getAddedOnVersions(program, type) ?? []; const removed = getRemovedOnVersions(program, type) ?? []; + const typeChanged = getTypeChangedFrom(program, type); + const returnTypeChanged = getReturnTypeChangedFrom(program, type); // if there's absolutely no versioning information, return undefined // contextually, this might mean it inherits its versioning info from a parent // or that it is treated as unversioned - if (!added.length && !removed.length) return undefined; + if ( + !added.length && + !removed.length && + typeChanged === undefined && + returnTypeChanged === undefined + ) + return undefined; // implicitly, all versioned things are assumed to have been added at // v1 if not specified @@ -735,10 +743,18 @@ export function getAvailabilityMapInTimeline( const added = getAddedOnVersions(program, type) ?? []; const removed = getRemovedOnVersions(program, type) ?? []; + const typeChanged = getTypeChangedFrom(program, type); + const returnTypeChanged = getReturnTypeChangedFrom(program, type); // if there's absolutely no versioning information, return undefined // contextually, this might mean it inherits its versioning info from a parent // or that it is treated as unversioned - if (!added.length && !removed.length) return undefined; + if ( + !added.length && + !removed.length && + typeChanged === undefined && + returnTypeChanged === undefined + ) + return undefined; // implicitly, all versioned things are assumed to have been added at // v1 if not specified diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index 1d51d229b80..bb904af1456 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -227,6 +227,29 @@ describe("versioning: validate incompatible references", () => { }); }); + it("emit diagnostic when model property type using @typeChangedFrom does not yet exist", async () => { + const diagnostics = await runner.diagnose(` + @test + model Original {} + + @test + @added(Versions.v3) + model Updated {} + + @test + model Test { + @typeChangedFrom(Versions.v2, Original) + prop: Updated; + } + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + severity: "error", + message: + "'TestService.Test.prop' is referencing type 'TestService.Updated' which does not exist in version 'v2'.", + }); + }); + it("succeed if version are compatible in model", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index 8340877e0c0..a3a5552101b 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -381,28 +381,34 @@ describe("versioning: logic", () => { ok(v2.properties.get("b")!.optional === true); }); - it("can still change type", async () => { + it("can change type to versioned models", async () => { const { projections: [v1, v2, v3], } = await versionedModel( ["v1", "v2", "v3"], ` + @test model Original {} + @test @added(Versions.v2) model Updated {} - model Foo { - @typeChangedFrom(Versions.v2, Original) + @test + model Test { + @added(Versions.v2) + @typeChangedFrom(Versions.v3, Original) prop: Updated; } ` ); - let test = "best"; + ok(v1.properties.get("prop") === undefined); + ok((v2.properties.get("prop")!.type as Model).name === "Original"); + ok((v3.properties.get("prop")!.type as Model).name === "Updated"); }); - it("can change type", async () => { + it("can change type over multiple versions", async () => { const { projections: [v1, v2, v3], } = await versionedModel( From f89f8d66ac73e802e2e300276673d80516cd8814 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Fri, 8 Sep 2023 10:29:10 -0700 Subject: [PATCH 3/3] Code review feedback. --- .../fixChangeTypeFrom_2023-09-08-17-28.json | 10 +++++++ .../test/incompatible-versioning.test.ts | 26 ++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 common/changes/@typespec/versioning/fixChangeTypeFrom_2023-09-08-17-28.json diff --git a/common/changes/@typespec/versioning/fixChangeTypeFrom_2023-09-08-17-28.json b/common/changes/@typespec/versioning/fixChangeTypeFrom_2023-09-08-17-28.json new file mode 100644 index 00000000000..4946aa74b73 --- /dev/null +++ b/common/changes/@typespec/versioning/fixChangeTypeFrom_2023-09-08-17-28.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/versioning", + "comment": "Fixed issue with `@typeChangedFrom` complaining about incorrect versioned references.", + "type": "none" + } + ], + "packageName": "@typespec/versioning" +} \ No newline at end of file diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index bb904af1456..578a16b0c7b 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -227,7 +227,7 @@ describe("versioning: validate incompatible references", () => { }); }); - it("emit diagnostic when model property type using @typeChangedFrom does not yet exist", async () => { + it("emit diagnostic when using @typeChangedFrom with a type parameter that does not yet exist", async () => { const diagnostics = await runner.diagnose(` @test model Original {} @@ -250,6 +250,30 @@ describe("versioning: validate incompatible references", () => { }); }); + it("emit diagnostic when using @typeChangedFrom with a base parameter that does not yet exist", async () => { + const diagnostics = await runner.diagnose(` + @test + @added(Versions.v2) + model Original {} + + @test + @added(Versions.v3) + model Updated {} + + @test + model Test { + @typeChangedFrom(Versions.v3, Original) + prop: Updated; + } + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + severity: "error", + message: + "'TestService.Test.prop' is referencing type 'TestService.Original' which does not exist in version 'v1'.", + }); + }); + it("succeed if version are compatible in model", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2)