Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(versioning): add code fixes for incompatible version errors #5459

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@typespec/versioning"
---

add code fixes for incompatible version errors
4 changes: 2 additions & 2 deletions packages/versioning/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ export const $lib = createTypeSpecLibrary({
dependentAddedAfter: paramMessage`'${"sourceName"}' was added in version '${"sourceAddedOn"}' but contains type '${"targetName"}' added in version '${"targetAddedOn"}'.`,
removedBefore: paramMessage`'${"sourceName"}' was removed in version '${"sourceRemovedOn"}' but referencing type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`,
dependentRemovedBefore: paramMessage`'${"sourceName"}' was removed in 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"}.`,
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"}'.`,
},
},
Expand Down
93 changes: 93 additions & 0 deletions packages/versioning/src/validate.codefix.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import {
getSourceLocation,
getTypeName,
type CodeFix,
type Program,
type SourceLocation,
type Type,
type TypeNameOptions,
} from "@typespec/compiler";
import type { Version } from "./types.js";
import { getAllVersions } from "./versioning.js";

export function getVersionAdditionCodefixes(
version: string | Version,
type: Type,
program: Program,
typeOptions?: TypeNameOptions,
): CodeFix[] | undefined {
if (typeof version === "string") {
return getVersionAdditionCodeFixFromString(version, type, program, typeOptions);
}

return getVersionAdditionCodeFixFromVersion(version, type, typeOptions);
}

function getVersionAdditionCodeFixFromVersion(
version: Version,
type: Type,
typeOptions?: TypeNameOptions,
): CodeFix[] | undefined {
if (type.node === undefined) return undefined;

const enumMember = version.enumMember;
const decoratorDeclaration = `@added(${enumMember.enum.name}.${enumMember.name})`;
return [
getDecorationAdditionCodeFix(
"add-version-to-type",
decoratorDeclaration,
getTypeName(type, typeOptions),
getSourceLocation(type.node),
),
];
}

function getVersionAdditionCodeFixFromString(
version: string,
type: Type,
program: Program,
typeOptions?: TypeNameOptions,
): CodeFix[] | undefined {
const targetVersion = getAllVersions(program, type)?.find((v) => v.value === version);
if (targetVersion === undefined) return undefined;

return getVersionAdditionCodeFixFromVersion(targetVersion, type, typeOptions);
}

export function getVersionRemovalCodeFixes(
version: string,
type: Type,
program: Program,
typeOptions?: TypeNameOptions,
): CodeFix[] | undefined {
if (type.node === undefined) return undefined;

const targetVersion = getAllVersions(program, type)?.find((v) => v.value === version);
if (targetVersion === undefined) return;

const enumMember = targetVersion.enumMember;
const decoratorDeclaration = `@removed(${enumMember.enum.name}.${enumMember.name})`;
return [
getDecorationAdditionCodeFix(
"remove-version-from-type",
decoratorDeclaration,
getTypeName(type, typeOptions),
getSourceLocation(type.node),
),
];
}

function getDecorationAdditionCodeFix(
id: string,
decoratorDeclaration: string,
typeName: string,
location: SourceLocation,
): CodeFix {
return {
id: id,
label: `Add '${decoratorDeclaration}' to '${typeName}'`,
fix: (context) => {
return context.prependText(location, `${decoratorDeclaration}\n`);
},
};
}
52 changes: 39 additions & 13 deletions packages/versioning/src/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ import {
getReturnTypeChangedFrom,
getTypeChangedFrom,
getUseDependencies,
getVersion,
} from "./decorators.js";
import { reportDiagnostic } from "./lib.js";
import type { Version } from "./types.js";
import { getVersionAdditionCodefixes, getVersionRemovalCodeFixes } from "./validate.codefix.js";
import {
Availability,
getAllVersions,
getAvailabilityMap,
getVersionDependencies,
getVersions,
Expand Down Expand Up @@ -208,13 +209,6 @@ export function $onValidate(program: Program) {
validateVersionedNamespaceUsage(program, namespaceDependencies);
}

function getAllVersions(p: Program, t: Type): Version[] | undefined {
Copy link
Member Author

@archerzz archerzz Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's already defined in versioning.ts:

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.
*/
Expand Down Expand Up @@ -254,6 +248,7 @@ function validateTypeAvailability(
version: prettyVersion(version),
},
target: source,
codefixes: getVersionAdditionCodefixes(version, type, program, options),
});
}

Expand Down Expand Up @@ -665,9 +660,10 @@ function validateTargetVersionCompatible(
const [targetNamespace] = getVersions(program, targetAvailability.type);
if (!targetAvailability.map || !targetNamespace) return;

let versionMap: Map<Version, Version> | Version | undefined;
if (sourceNamespace !== targetNamespace) {
const dependencies = sourceNamespace && getVersionDependencies(program, sourceNamespace);
const versionMap = dependencies?.get(targetNamespace);
versionMap = dependencies?.get(targetNamespace);
if (versionMap === undefined) return;

targetAvailability.map = translateAvailability(
Expand Down Expand Up @@ -697,6 +693,7 @@ function validateTargetVersionCompatible(
targetAvailability.map,
sourceAvailability.type,
targetAvailability.type,
versionMap instanceof Map ? versionMap : undefined,
);
}
}
Expand Down Expand Up @@ -728,6 +725,7 @@ function translateAvailability(
targetAddedOn: addedAfter,
},
target: source,
codefixes: getVersionAdditionCodefixes(version, target, program),
});
}
if (removedBefore) {
Expand All @@ -741,6 +739,7 @@ function translateAvailability(
targetAddedOn: removedBefore,
},
target: source,
codefixes: getVersionAdditionCodefixes(version, target, program),
});
}
}
Expand Down Expand Up @@ -799,8 +798,7 @@ function validateAvailabilityForRef(
targetAvail: Map<string, Availability>,
source: Type,
target: Type,
sourceOptions?: TypeNameOptions,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those two parameters are never really passed in, so I delete them for simplicity

targetOptions?: TypeNameOptions,
versionMap?: Map<Version, Version>,
) {
// if source is unversioned and target is versioned
if (sourceAvail === undefined) {
Expand Down Expand Up @@ -840,6 +838,12 @@ function validateAvailabilityForRef(
[Availability.Removed, Availability.Unavailable].includes(targetVal)
) {
const targetAddedOn = findAvailabilityAfterVersion(key, Availability.Added, targetAvail);
let targetVersion: Version | string = key;
if (versionMap) {
// the `key` here could have already been converted to source version string, thus we need to find the
// original target version so that we can provide the correct codefix
targetVersion = findMatchingTargetVersion(key, versionMap) ?? key;
}

reportDiagnostic(program, {
code: "incompatible-versioned-reference",
Expand All @@ -851,6 +855,7 @@ function validateAvailabilityForRef(
targetAddedOn: targetAddedOn!,
},
target: source,
codefixes: getVersionAdditionCodefixes(targetVersion, target, program),
});
}
if (
Expand All @@ -862,16 +867,23 @@ function validateAvailabilityForRef(
Availability.Removed,
targetAvail,
);

let targetVersion: Version | string = key;
if (versionMap) {
targetVersion = findMatchingTargetVersion(key, versionMap) ?? key;
}

reportDiagnostic(program, {
code: "incompatible-versioned-reference",
messageId: "removedBefore",
format: {
sourceName: getTypeName(source, sourceOptions),
targetName: getTypeName(target, targetOptions),
sourceName: getTypeName(source),
targetName: getTypeName(target),
sourceRemovedOn: key,
targetRemovedOn: targetRemovedOn!,
},
target: source,
codefixes: getVersionAdditionCodefixes(targetVersion, target, program),
});
}
}
Expand Down Expand Up @@ -934,6 +946,7 @@ function validateAvailabilityForContains(
targetAddedOn: key,
},
target: target,
codefixes: getVersionAdditionCodefixes(key, source, program, targetOptions),
});
}
if (
Expand All @@ -952,6 +965,7 @@ function validateAvailabilityForContains(
targetRemovedOn: targetRemovedOn!,
},
target: target,
codefixes: getVersionRemovalCodeFixes(key, target, program, targetOptions),
});
}
}
Expand All @@ -967,3 +981,15 @@ function isAvailableInAllVersion(avail: Map<string, Availability>): boolean {
function prettyVersion(version: Version | undefined): string {
return version?.value ?? "<n/a>";
}

function findMatchingTargetVersion(
sourceVersion: string,
versionMap: Map<Version, Version>,
): Version | undefined {
for (const [source, target] of versionMap.entries()) {
if (source.value === sourceVersion) {
return target;
}
}
return undefined;
}
2 changes: 1 addition & 1 deletion packages/versioning/src/versioning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ function resolveVersionsForNamespace(
}
}

function getAllVersions(p: Program, t: Type): Version[] | undefined {
export function getAllVersions(p: Program, t: Type): Version[] | undefined {
const [namespace, _] = getVersions(p, t);
if (namespace === undefined) return undefined;

Expand Down
44 changes: 41 additions & 3 deletions packages/versioning/test/incompatible-versioning.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,8 @@ describe("versioning: validate incompatible references", () => {
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.{ param: TestService.Foo }.param' is referencing versioned type 'TestService.Foo' but is not versioned itself.",
});
});
});
Expand Down Expand Up @@ -771,6 +773,8 @@ describe("versioning: validate incompatible references", () => {
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing versioned type 'TestService.Versioned' but is not versioned itself.",
});
});

Expand Down Expand Up @@ -915,7 +919,7 @@ describe("versioning: validate incompatible references", () => {
runner = await createVersioningTestRunner();
});

it("emit diagnostic when referencing incompatible version via version dependency", async () => {
it("emit diagnostic when referencing incompatible version addition via version dependency", async () => {
// Here Foo was added in v2 which makes it only available in 1 & 2.
const diagnostics = await runner.diagnose(`
@versioned(Versions)
Expand Down Expand Up @@ -949,6 +953,40 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when referencing incompatible version removal via version dependency", async () => {
// Here Foo was added in v2 which makes it only available in 1 & 2.
const diagnostics = await runner.diagnose(`
@versioned(Versions)
namespace VersionedLib {
enum Versions {l1, l2, l3}
@removed(Versions.l2)
model Foo {}
}

@versioned(Versions)
namespace TestService {
enum Versions {
@useDependency(VersionedLib.Versions.l1)
v1,
@useDependency(VersionedLib.Versions.l1)
v2,
@useDependency(VersionedLib.Versions.l2)
v3,
@useDependency(VersionedLib.Versions.l3)
v4
}

@removed(Versions.v4)
op test(): VersionedLib.Foo;
}
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' was removed in version 'v4' but referencing type 'VersionedLib.Foo' removed in version 'v3'.",
});
});

it("doesn't emit diagnostic if all version use the same one", async () => {
// Here Foo was added in v2 which makes it only available in 1 & 2.
const diagnostics = await runner.diagnose(`
Expand Down Expand Up @@ -995,7 +1033,7 @@ describe("versioning: validate incompatible references", () => {
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing type 'VersionedLib.Foo' added in version 'l2' but version used is l1.",
"'TestService.test' is referencing type 'VersionedLib.Foo' added in version 'l2' but version used is 'l1'.",
});
});

Expand All @@ -1017,7 +1055,7 @@ describe("versioning: validate incompatible references", () => {
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing type 'VersionedLib.Foo' removed in version 'l2' but version used is l2.",
"'TestService.test' is referencing type 'VersionedLib.Foo' removed in version 'l2' but version used is 'l2'.",
});
});
});
Expand Down
Loading