Skip to content

Commit

Permalink
Fix #2690.
Browse files Browse the repository at this point in the history
  • Loading branch information
tjprescott committed Nov 21, 2023
1 parent 32c98ed commit 137d94e
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 25 deletions.
7 changes: 5 additions & 2 deletions packages/compiler/src/core/helpers/type-name-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
export interface TypeNameOptions {
namespaceFilter?: (ns: Namespace) => boolean;
printable?: boolean;
operationName?: string;
}

export function getTypeName(type: Type | ValueType, options?: TypeNameOptions): string {
Expand Down Expand Up @@ -118,8 +119,9 @@ function getModelName(model: Model, options: TypeNameOptions | undefined) {
}
}

const defaultName = options?.operationName ?? "(anonymous model)";
if (model.name === "") {
return nsPrefix + "(anonymous model)";
return nsPrefix + defaultName;
}
const modelName = nsPrefix + getIdentifierName(model.name, options);
if (isTemplateInstance(model)) {
Expand Down Expand Up @@ -164,8 +166,9 @@ function isInTypeSpecNamespace(type: Type & { namespace?: Namespace }): boolean

function getModelPropertyName(prop: ModelProperty, options: TypeNameOptions | undefined) {
const modelName = prop.model ? getModelName(prop.model, options) : undefined;
const defaultName = options?.operationName ?? "(anonymous mode)";

return `${modelName ?? "(anonymous model)"}.${prop.name}`;
return `${modelName ?? defaultName}.${prop.name}`;
}

function getInterfaceName(iface: Interface, options: TypeNameOptions | undefined) {
Expand Down
107 changes: 95 additions & 12 deletions packages/versioning/src/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -99,16 +102,12 @@ export function $onValidate(program: Program) {
for (const prop of op.parameters.properties.values()) {
addNamespaceDependency(namespace, prop.type);

// Validate model -> property have correct versioning
validateTargetVersionCompatible(program, op, prop, { isTargetADependent: true });

// Validate model property -> type have correct versioning
const typeChangedFrom = getTypeChangedFrom(program, prop);
if (typeChangedFrom !== undefined) {
// FIXME: This is probably busted too
validateMultiTypeReference(program, prop);
} else {
// FIXME: This is broken.
validateReference(program, prop, prop.type);
validateOperationParameter(program, op, prop);
}

// Validate model property type is correct when madeOptional
Expand Down Expand Up @@ -478,6 +477,86 @@ 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<string, Availability>();
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, { operationName: operation.name }),
targetName: getTypeName(parameter, { operationName: operation.name }),
},
target: operation,
});
return;
} else if (paramTypeAvailability !== undefined) {
reportDiagnostic(program, {
code: "incompatible-versioned-reference",
messageId: "default",
format: {
sourceName: getTypeName(operation, { operationName: operation.name }),
targetName: getTypeName(parameter.type, { operationName: operation.name }),
},
target: operation,
});
return;
}
}

if (paramAvailability !== undefined) {
validateAvailabilityForContains(
program,
operationAvailability,
paramAvailability,
operation,
parameter,
{ operationName: operation.name },
{ operationName: operation.name }
);
} else if (paramTypeAvailability !== undefined) {
validateAvailabilityForRef(
program,
operationAvailability,
paramTypeAvailability,
operation,
parameter.type,
{ operationName: operation.name },
{ operationName: operation.name }
);
}
}

/**
* Validate the target reference versioning is compatible with the source versioning.
* This will also validate any template arguments used in the reference.
Expand Down Expand Up @@ -681,7 +760,9 @@ function validateAvailabilityForRef(
sourceAvail: Map<string, Availability> | undefined,
targetAvail: Map<string, Availability>,
source: Type,
target: Type
target: Type,
sourceOptions?: TypeNameOptions,
targetOptions?: TypeNameOptions
) {
// if source is unversioned and target is versioned
if (sourceAvail === undefined) {
Expand Down Expand Up @@ -765,8 +846,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!,
},
Expand All @@ -781,7 +862,9 @@ function validateAvailabilityForContains(
sourceAvail: Map<string, Availability> | undefined,
targetAvail: Map<string, Availability>,
source: Type,
target: Type
target: Type,
sourceOptions?: TypeNameOptions,
targetOptions?: TypeNameOptions
) {
if (!sourceAvail) return;

Expand All @@ -799,8 +882,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,
},
Expand Down
22 changes: 11 additions & 11 deletions packages/versioning/test/incompatible-versioning.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ describe("versioning: validate incompatible references", () => {
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing versioned type 'TestService.Foo' but is not versioned itself.",
"'TestService.test' is referencing versioned type 'TestService.test.newParam' but is not versioned itself.",
});
});

Expand Down Expand Up @@ -161,34 +161,34 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when when op 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 {}
op Template<T>(param: T): void;
@added(Versions.v1)
op test(@added(Versions.v2) param: Foo): void;
op test is Template<Foo>;
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' was added in version 'v1' but referencing type 'TestService.(anonymous model).param' added in version 'v2'.",
"'TestService.test' was added in version 'v1' but referencing type 'TestService.Foo' added in version 'v2'.",
});
});

it("emit diagnostic when op based on a template was added before parameter", async () => {
it("emit diagnostic when when parameter was added before op", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v2)
model Foo {}
op Template<T>(param: T): void;
@added(Versions.v1)
op test is Template<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 'v1' but referencing type 'TestService.Foo' added in version 'v2'.",
"'TestService.test' was added in version 'v2' but contains type 'TestService.test.param' added in version 'v1'.",
});
});
});
Expand Down

0 comments on commit 137d94e

Please sign in to comment.