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

[EFv2] cleanup error and warnings #6704

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
11 changes: 11 additions & 0 deletions .chronus/changes/internal-js-cleanup-errors-2025-2-26-19-57-59.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
changeKind: internal
packages:
- "@typespec/compiler"
- "@typespec/emitter-framework"
- "@typespec/http-client-js"
- "@typespec/http-client"
- "@typespec/http"
---

Cleanup errors and diagnostics in Typekit, efv2 and http-client-js
8 changes: 7 additions & 1 deletion packages/compiler/src/experimental/typekit/kits/array.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isArrayModelType } from "../../../core/type-utils.js";
import { Model, Type } from "../../../core/types.js";
import { defineKit } from "../define-kit.js";
import { reportTypekitDiagnostic } from "../lib.js";

/**
* @experimental
Expand Down Expand Up @@ -38,7 +39,12 @@ defineKit<TypekitExtension>({
},
getElementType(type) {
if (!this.array.is(type)) {
throw new Error("Type is not an array.");
reportTypekitDiagnostic(this.program, {
code: "model-not-array",
target: type,
format: { arrayTypekit: "$.array.getElementType(type)" },
});
return type;
}
return type.indexer!.value;
},
Expand Down
14 changes: 12 additions & 2 deletions packages/compiler/src/experimental/typekit/kits/enum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { type UnionKit } from "./union.js";

import { createRekeyableMap } from "../../../utils/misc.js";
import { defineKit } from "../define-kit.js";
import { reportTypekitDiagnostic } from "../lib.js";
import { decoratorApplication, DecoratorArgs } from "../utils.js";

/**
Expand Down Expand Up @@ -95,21 +96,30 @@ defineKit<TypekitExtension>({

createFromUnion(type) {
if (!type.name) {
throw new Error("Cannot create an enum from an anonymous union.");
reportTypekitDiagnostic(this.program, {
code: "enum-from-anonymous-union",
target: type,
});
}

const name = type.name ?? "AnonymousEnum";

const enumMembers: EnumMember[] = [];
for (const variant of type.variants.values()) {
if (
(variant.name && typeof variant.name === "symbol") ||
(!this.literal.isString(variant.type) && !this.literal.isNumeric(variant.type))
) {
reportTypekitDiagnostic(this.program, {
code: "union-variant-not-convertible-to-enum",
target: type,
});
continue;
}
enumMembers.push(this.enumMember.create({ name: variant.name, value: variant.type.value }));
}

return this.enum.create({ name: type.name, members: enumMembers });
return this.enum.create({ name, members: enumMembers });
},
},
});
8 changes: 7 additions & 1 deletion packages/compiler/src/experimental/typekit/kits/record.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isRecordModelType } from "../../../core/type-utils.js";
import { Model, Type } from "../../../core/types.js";
import { defineKit } from "../define-kit.js";
import { reportTypekitDiagnostic } from "../lib.js";

/**
* RecordKit provides utilities for working with Record Model types.
Expand Down Expand Up @@ -43,7 +44,12 @@ defineKit<TypekitExtension>({
},
getElementType(type) {
if (!this.record.is(type)) {
throw new Error("Type is not a record.");
reportTypekitDiagnostic(this.program, {
Copy link
Member

Choose a reason for hiding this comment

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

reporting diagnostic from a getter is a bad pattern, it carries the error to the user potentially multiple times without expoissting that to the library author.

Any getter that report diagnmostic should return the error tuple

Copy link
Member

Choose a reason for hiding this comment

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

in general the same applies to probably all typekits

Copy link
Member

@timotheeguerin timotheeguerin Apr 2, 2025

Choose a reason for hiding this comment

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

i think it might make more sense to also throw if invalid inputs are used, this means that the library author made an error calling this intead of pushing that onto the user

code: "model-not-record",
target: type,
format: { recordTypekit: "$.record.getElementType(type)" },
});
return type;
}
return type.indexer!.value;
},
Expand Down
36 changes: 36 additions & 0 deletions packages/compiler/src/experimental/typekit/lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { createTypeSpecLibrary, paramMessage } from "../../core/library.js";

export const $lib = createTypeSpecLibrary({
name: "@typespec/compiler/experimental/typekit",
diagnostics: {
"model-not-array": {
severity: "warning",
messages: {
default: paramMessage`Expected a model to be an array. Make sure to check $.array.is(type) before calling ${"arrayTypekit"}. Returning the model type as is.`,
},
},
"model-not-record": {
severity: "warning",
messages: {
default: paramMessage`Expected a model to be an array. Make sure to check $.record.is(type) before calling ${"recordTypekit"}. Returning the model type as is.`,
},
},
"enum-from-anonymous-union": {
severity: "warning",
messages: {
default: `Enum created from an anonymous union. Consider using a named union instead. This will generate an enum with a generated name.`,
},
},
"union-variant-not-convertible-to-enum": {
severity: "warning",
messages: {
default: `Union variant is not convertible to an enum. Union variants have to be named and have a string or numeric literal value. This variant will be ignored.`,
},
},
},
});

export const {
reportDiagnostic: reportTypekitDiagnostic,
createDiagnostic: createTypekitDiagnostic,
} = $lib;
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,32 @@ import * as ts from "@alloy-js/typescript";
import { Enum, EnumMember as TspEnumMember, Union } from "@typespec/compiler";
import { $ } from "@typespec/compiler/experimental/typekit";
import { reportDiagnostic } from "../../lib.js";
import { reportTypescriptDiagnostic } from "../lib.js";

export interface EnumDeclarationProps extends Omit<ts.TypeDeclarationProps, "name"> {
name?: string;
type: Union | Enum;
}

export function EnumDeclaration(props: EnumDeclarationProps) {
if (!props.type.name || props.type.name === "") {
reportDiagnostic($.program, { code: "type-declaration-missing-name", target: props.type });
}

let type: Enum;

if ($.union.is(props.type)) {
if (!$.union.isValidEnum(props.type)) {
throw new Error("The provided union type cannot be represented as an enum");
reportTypescriptDiagnostic($.program, {
code: "invalid-enum-type",
target: props.type,
});
}
type = $.enum.createFromUnion(props.type);
} else {
type = props.type;
}

if (!props.type.name || props.type.name === "") {
reportDiagnostic($.program, { code: "type-declaration-missing-name", target: props.type });
}

const name = props.name ?? ts.useTSNamePolicy().getName(props.type.name!, "enum");
const members = Array.from(type.members.entries());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,7 @@ export function FunctionDeclaration(props: FunctionDeclarationProps) {

const refkey = props.refkey ?? getRefkey(props.type);

let name = props.name ? props.name : ts.useTSNamePolicy().getName(props.type.name, "function");

// TODO: This should probably be a broader check in alloy to guard\
// any identifier.
if (reservedFunctionKeywords.has(name)) {
name = `${name}_`;
}
const name = props.name ? props.name : ts.useTSNamePolicy().getName(props.type.name, "function");

const returnType = props.returnType ?? <TypeExpression type={getReturnType(props.type)} />;
const allParameters = buildParameterDescriptors(props.type.parameters, {
Expand Down Expand Up @@ -84,49 +78,3 @@ function isTypedFunctionParametersProps(
): props is TypedFunctionParametersProps {
return "type" in props;
}

const reservedFunctionKeywords = new Set([
"break",
"case",
"catch",
"class",
"const",
"continue",
"debugger",
"default",
"delete",
"do",
"else",
"enum",
"export",
"extends",
"finally",
"for",
"function",
"if",
"import",
"in",
"instanceof",
"new",
"return",
"super",
"switch",
"this",
"throw",
"try",
"typeof",
"var",
"void",
"while",
"with",
"yield",
"let",
"static",
"implements",
"interface",
"package",
"private",
"protected",
"public",
"await",
]);
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ export function TypeExpression(props: TypeExpressionProps) {
// todo: probably need abstraction around deciding what's a declaration in the output
// (it may not correspond to things which are declarations in TypeSpec?)
return <Reference refkey={refkey(type)} />;
//throw new Error("Reference not implemented");
}

// TODO: Make sure this is an exhaustive switch, including EnumMember and such
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ function UnionTransformExpression(props: UnionTransformProps) {
const discriminator = $.type.getDiscriminator(props.type);

if (!discriminator) {
// TODO: Handle non-discriminated unions
reportTypescriptDiagnostic($.program, {
code: "typescript-unsupported-nondiscriminated-union",
target: props.type,
Expand Down Expand Up @@ -179,19 +178,6 @@ export interface ModelTransformExpressionProps {
* Component that represents an object expression that transforms a model to a transport or application model.
*/
export function ModelTransformExpression(props: ModelTransformExpressionProps) {
if (props.type.baseModel) {
reportTypescriptDiagnostic($.program, {
code: "typescript-extended-model-transform-nyi",
target: props.type,
});
}

if ($.model.getSpreadType(props.type)) {
reportTypescriptDiagnostic($.program, {
code: "typescript-spread-model-transformation-nyi",
target: props.type,
});
}
const namePolicy = ts.useTSNamePolicy();
const modelProperties: RekeyableMap<string, ModelProperty> = createRekeyableMap();

Expand Down
29 changes: 7 additions & 22 deletions packages/emitter-framework/src/typescript/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,47 +10,32 @@ export const $typescriptLib = createTypeSpecLibrary({
},
},
"typescript-unsupported-type": {
severity: "error", // TODO: Warning for release and error for debug
severity: "warning",
messages: {
default: "Unsupported type, falling back to any",
},
description: "This type is not supported by the emitter",
},
"typescript-unsupported-model-discriminator": {
severity: "error", // TODO: Warning for release and error for debug
messages: {
default:
"Unsupported model discriminator, falling back to not discriminating on serialization/deserialization",
},
description: "Discriminators at the model are not supported",
},
"typescript-unsupported-type-transform": {
severity: "error", // TODO: Warning for release and error for debug
severity: "warning",
messages: {
default: "Unsupported type for transformation, falling back to not transforming this type",
},
description: "Discriminators at the model are not supported",
},
"typescript-unsupported-nondiscriminated-union": {
severity: "error", // TODO: Warning for release and error for debug
severity: "error",
messages: {
default: "Unsupported non-discriminated union, falling back to not transforming this type",
},
description: "Non-discriminated unions are not supported",
},
"typescript-extended-model-transform-nyi": {
severity: "warning", // TODO: Warning for release and error for debug
"invalid-enum-type": {
severity: "error",
messages: {
default: "Extended model transformation is not yet implemented",
},
description: "Extended model transformation is not yet implemented",
},
"typescript-spread-model-transformation-nyi": {
severity: "warning", // TODO: Warning for release and error for debug
messages: {
default: "Spread model transformation is not yet implemented",
default:
"For unions to be represented as enums, all variants must be string or numeric literals.",
},
description: "Spread model transformation is not yet implemented",
},
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,22 @@ const scalarTests: { key: keyof typeof scalarTransformerMap; test: ScalarTest }[
*
* @param type - The scalar type to check.
* @returns The corresponding key from the scalarTransformerMap.
* @throws Error if the scalar type is unknown.
*/
function getScalarTransformKey(type: Scalar): keyof typeof scalarTransformerMap {
for (const { key, test } of scalarTests) {
if (test(type)) {
return key;
}
}
throw new Error(`Unknown scalar type: ${type}`);

reportDiagnostic($.program, {
code: "unknown-scalar",
target: type,
message: `Unknown scalar type: ${type.name}`,
});

return "string";
// This return is unreachable.
}

function passthroughTransformer(itemRef: Refkey | Children) {
Expand Down
12 changes: 6 additions & 6 deletions packages/http-client-js/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ export const $lib = createTypeSpecLibrary({
description:
"Key credential in query or cookie is not implemented. Falling back to not sending auth details with the requests",
},
"unsupported-nondiscriminated-union": {
severity: "warning",
messages: {
default: "Unsupported non-discriminated union, skipping deserializer",
},
},
"no-name-type": {
severity: "warning",
messages: {
Expand Down Expand Up @@ -107,6 +101,12 @@ export const $lib = createTypeSpecLibrary({
default: "Unexpected non-scalar type when trying to extract Scalar data",
},
},
"unknown-scalar": {
severity: "error",
messages: {
default: "Unknown scalar type",
},
},
},
});

Expand Down
Loading
Loading