From 2ca9cbee57ff79f067d6831d9e10954dc65f7b34 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 21 Nov 2023 13:05:06 -0800 Subject: [PATCH] Json Schema: Stop throwing errors for user errors (#2685) fix [#2535](https://github.com/microsoft/typespec/issues/2535) Converted most of the `throw error` to either: - `compileAssert` if it is somethign that shouldn't happen - `reportDiagnositc` if this is something actionable by the user There is still 2 `throw error` as I'm not sure if those are things that could happen or not. I'll leave them until someone maybe gets a crash and we can then convert. --------- Co-authored-by: Mark Cowlishaw --- ...json-schema-no-throw_2023-11-21-16-21.json | 10 +++ .../json-schema/src/json-schema-emitter.ts | 76 ++++++++++--------- packages/json-schema/src/lib.ts | 12 +++ packages/json-schema/test/ids.test.ts | 19 +++-- packages/json-schema/test/utils.ts | 33 +++++--- 5 files changed, 101 insertions(+), 49 deletions(-) create mode 100644 common/changes/@typespec/json-schema/json-schema-no-throw_2023-11-21-16-21.json diff --git a/common/changes/@typespec/json-schema/json-schema-no-throw_2023-11-21-16-21.json b/common/changes/@typespec/json-schema/json-schema-no-throw_2023-11-21-16-21.json new file mode 100644 index 0000000000..8ffa5e2da2 --- /dev/null +++ b/common/changes/@typespec/json-schema/json-schema-no-throw_2023-11-21-16-21.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/json-schema", + "comment": "Report diagnostic instead of throwing errors in the case of duplicate ids or unknown scalar", + "type": "none" + } + ], + "packageName": "@typespec/json-schema" +} \ No newline at end of file diff --git a/packages/json-schema/src/json-schema-emitter.ts b/packages/json-schema/src/json-schema-emitter.ts index 97d3bcfce5..1c498f851d 100644 --- a/packages/json-schema/src/json-schema-emitter.ts +++ b/packages/json-schema/src/json-schema-emitter.ts @@ -1,6 +1,8 @@ import { BooleanLiteral, compilerAssert, + DiagnosticTarget, + DuplicateTracker, emitFile, Enum, EnumMember, @@ -69,7 +71,7 @@ import { } from "./index.js"; import { JSONSchemaEmitterOptions, reportDiagnostic } from "./lib.js"; export class JsonSchemaEmitter extends TypeEmitter, JSONSchemaEmitterOptions> { - #seenIds = new Set(); + #idDuplicateTracker = new DuplicateTracker(); #typeForSourceFile = new Map, JsonSchemaDeclaration>(); #refToDecl = new Map>>(); @@ -164,10 +166,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche modelPropertyLiteral(property: ModelProperty): EmitterOutput { const propertyType = this.emitter.emitTypeReference(property.type); - - if (propertyType.kind !== "code") { - throw new Error("Unexpected non-code result from emit reference"); - } + compilerAssert(propertyType.kind === "code", "Unexpected non-code result from emit reference"); const result = new ObjectBuilder(propertyType.value); @@ -321,9 +320,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche // could be made easier, as it's a bit subtle. const refSchema = this.emitter.emitTypeReference(property.type); - if (refSchema.kind !== "code") { - throw new Error("Unexpected non-code result from emit reference"); - } + compilerAssert(refSchema.kind === "code", "Unexpected non-code result from emit reference"); const schema = new ObjectBuilder(refSchema.value); this.#applyConstraints(property, schema); return schema; @@ -411,7 +408,12 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche } else if (scalar.baseScalar) { result = this.#getSchemaForScalar(scalar.baseScalar); } else { - throw new Error(`Can't emit custom scalar type ${scalar.name}`); + reportDiagnostic(this.emitter.getProgram(), { + code: "unknown-scalar", + format: { name: scalar.name }, + target: scalar, + }); + return {}; } const objectBuilder = new ObjectBuilder(result); @@ -489,7 +491,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche case "bytes": return { type: "string", contentEncoding: "base64" }; default: - throw new Error("Unknown scalar type " + baseBuiltIn.name); + compilerAssert(false, `Unknown built-in scalar type ${baseBuiltIn.name}`); } } @@ -508,9 +510,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche const constraintType = fn(this.emitter.getProgram(), type); if (constraintType) { const ref = this.emitter.emitTypeReference(constraintType); - if (ref.kind !== "code") { - throw new Error("Couldn't get reference to contains type"); - } + compilerAssert(ref.kind === "code", "Unexpected non-code result from emit reference"); schema.set(key, ref.value); } }; @@ -591,7 +591,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche } intrinsic(intrinsic: IntrinsicType, name: string): EmitterOutput { - switch (name) { + switch (intrinsic.name) { case "null": return { type: "null" }; case "unknown": @@ -599,12 +599,27 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche case "never": case "void": return { not: {} }; + case "ErrorType": + return {}; + default: + const _assertNever: never = intrinsic.name; + compilerAssert(false, "Unreachable"); } - - throw new Error("Unknown intrinsic type " + name); } + #reportDuplicateIds() { + for (const [id, targets] of this.#idDuplicateTracker.entries()) { + for (const target of targets) { + reportDiagnostic(this.emitter.getProgram(), { + code: "duplicate-id", + format: { id }, + target: target, + }); + } + } + } async writeOutput(sourceFiles: SourceFile>[]): Promise { + this.#reportDuplicateIds(); const toEmit: EmittedSourceFile[] = []; for (const sf of sourceFiles) { @@ -644,9 +659,7 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche ), }; } else { - if (decls.length > 1) { - throw new Error("Emit error - multiple decls in single schema per file mode"); - } + compilerAssert(decls.length === 1, "Multiple decls in single schema per file mode"); content = { ...decls[0].value }; @@ -682,13 +695,12 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche #getCurrentSourceFile() { let scope: Scope = this.emitter.getContext().scope; - if (!scope) throw new Error("need scope"); + compilerAssert(scope, "Scope should exists"); while (scope && scope.kind !== "sourceFile") { scope = scope.parentScope; } - - if (!scope) throw new Error("Didn't find source file scope"); + compilerAssert(scope, "Top level scope should be a source file"); return scope.sourceFile; } @@ -697,15 +709,13 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche const baseUri = findBaseUri(this.emitter.getProgram(), type); const explicitId = getId(this.emitter.getProgram(), type); if (explicitId) { - return this.#checkForDuplicateId(idWithBaseURI(explicitId, baseUri)); + return this.#trackId(idWithBaseURI(explicitId, baseUri), type); } // generate an id if (this.emitter.getOptions().bundleId) { - if (!type.name) { - throw new Error("Type needs a name to emit a declaration id"); - } - return this.#checkForDuplicateId(idWithBaseURI(name, baseUri)); + compilerAssert(type.name, "Type needs a name to emit a declaration id"); + return this.#trackId(idWithBaseURI(name, baseUri), type); } else { // generate the ID based on the file path const base = this.emitter.getOptions().emitterOutputDir; @@ -713,9 +723,9 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche const relative = getRelativePathFromDirectory(base, file, false); if (baseUri) { - return this.#checkForDuplicateId(new URL(relative, baseUri).href); + return this.#trackId(new URL(relative, baseUri).href, type); } else { - return this.#checkForDuplicateId(relative); + return this.#trackId(relative, type); } } @@ -728,12 +738,8 @@ export class JsonSchemaEmitter extends TypeEmitter, JSONSche } } - #checkForDuplicateId(id: string) { - if (this.#seenIds.has(id)) { - throw new Error(`Duplicate id: ${id}`); - } - - this.#seenIds.add(id); + #trackId(id: string, target: DiagnosticTarget) { + this.#idDuplicateTracker.track(id, target); return id; } diff --git a/packages/json-schema/src/lib.ts b/packages/json-schema/src/lib.ts index d8d4455692..d6c87d1e47 100644 --- a/packages/json-schema/src/lib.ts +++ b/packages/json-schema/src/lib.ts @@ -89,6 +89,18 @@ export const libDef = { default: paramMessage`Invalid type '${"type"}' for a default value`, }, }, + "duplicate-id": { + severity: "error", + messages: { + default: paramMessage`There are multiple types with the same id "${"id"}".`, + }, + }, + "unknown-scalar": { + severity: "warning", + messages: { + default: paramMessage`Scalar '${"name"}' is not a known scalar type and doesn't extend a known scalar type.`, + }, + }, }, emitter: { options: EmitterOptionsSchema as JSONSchemaType, diff --git a/packages/json-schema/test/ids.test.ts b/packages/json-schema/test/ids.test.ts index 776b90f972..b90f7f542b 100644 --- a/packages/json-schema/test/ids.test.ts +++ b/packages/json-schema/test/ids.test.ts @@ -1,5 +1,6 @@ +import { expectDiagnostics } from "@typespec/compiler/testing"; import assert from "assert"; -import { emitSchema } from "./utils.js"; +import { emitSchema, emitSchemaWithDiagnostics } from "./utils.js"; describe("implicit ids", () => { it("when bundling, sets the id based on the declaration name", async () => { @@ -32,9 +33,8 @@ describe("implicit ids", () => { assert.strictEqual(schemas["Foo.json"].$id, "http://example.org/Foo.json"); }); - it("throws errors on duplicate IDs", async () => { - await assert.rejects(async () => { - await emitSchema(` + it("emit diagnostic on duplicate IDs", async () => { + const [_, diagnostics] = await emitSchemaWithDiagnostics(` namespace Test1 { model Foo {} } @@ -42,7 +42,16 @@ describe("implicit ids", () => { model Foo {} } `); - }); + expectDiagnostics(diagnostics, [ + { + code: "@typespec/json-schema/duplicate-id", + message: `There are multiple types with the same id "Foo.json".`, + }, + { + code: "@typespec/json-schema/duplicate-id", + message: `There are multiple types with the same id "Foo.json".`, + }, + ]); }); }); diff --git a/packages/json-schema/test/utils.ts b/packages/json-schema/test/utils.ts index d2a1d36c18..cdcdd73ceb 100644 --- a/packages/json-schema/test/utils.ts +++ b/packages/json-schema/test/utils.ts @@ -1,5 +1,6 @@ +import { Diagnostic } from "@typespec/compiler"; import { createAssetEmitter } from "@typespec/compiler/emitter-framework"; -import { createTestHost } from "@typespec/compiler/testing"; +import { createTestHost, expectDiagnosticEmpty } from "@typespec/compiler/testing"; import { parse } from "yaml"; import { JsonSchemaEmitter } from "../src/json-schema-emitter.js"; import { JSONSchemaEmitterOptions } from "../src/lib.js"; @@ -10,21 +11,21 @@ export async function getHostForCadlFile(contents: string, decorators?: Record, readonly Diagnostic[]]> { if (!options["file-type"]) { options["file-type"] = "json"; } @@ -33,10 +34,14 @@ export async function emitSchema( ? `import "@typespec/json-schema"; using TypeSpec.JsonSchema; @jsonSchema namespace test; ${code}` : `import "@typespec/json-schema"; using TypeSpec.JsonSchema; ${code}`; const host = await getHostForCadlFile(code); - const emitter = createAssetEmitter(host.program, JsonSchemaEmitter, { - emitterOutputDir: "cadl-output", - options, - } as any); + const emitter = createAssetEmitter( + host.program, + JsonSchemaEmitter as any, + { + emitterOutputDir: "cadl-output", + options, + } as any + ); if (testOptions.emitTypes === undefined) { emitter.emitType(host.program.resolveTypeReference("test")[0]!); } else { @@ -58,5 +63,15 @@ export async function emitSchema( } } + return [schemas, host.program.diagnostics]; +} + +export async function emitSchema( + code: string, + options: JSONSchemaEmitterOptions = {}, + testOptions: { emitNamespace?: boolean; emitTypes?: string[] } = { emitNamespace: true } +) { + const [schemas, diagnostics] = await emitSchemaWithDiagnostics(code, options, testOptions); + expectDiagnosticEmpty(diagnostics); return schemas; }