Json Schema: Stop throwing errors for user errors (#2685)

fix [#2535](#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 <[email protected]>
microsoft · Nov 21, 2023 · 2ca9cbe · 2ca9cbe
1 parent d2e72f6
commit 2ca9cbe
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 49 deletions.
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
@@ -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"
+}
diff --git 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<Record<string, any>, JSONSchemaEmitterOptions> {
-  #seenIds = new Set();
+  #idDuplicateTracker = new DuplicateTracker<string, DiagnosticTarget>();
   #typeForSourceFile = new Map<SourceFile<any>, JsonSchemaDeclaration>();
   #refToDecl = new Map<string, Declaration<Record<string, unknown>>>();
 
@@ -164,10 +166,7 @@ export class JsonSchemaEmitter extends TypeEmitter<Record<string, any>, JSONSche
 
   modelPropertyLiteral(property: ModelProperty): EmitterOutput<object> {
     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<Record<string, any>, 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<Record<string, any>, 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<Record<string, any>, 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<Record<string, any>, 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,20 +591,35 @@ export class JsonSchemaEmitter extends TypeEmitter<Record<string, any>, JSONSche
   }
 
   intrinsic(intrinsic: IntrinsicType, name: string): EmitterOutput<object> {
-    switch (name) {
+    switch (intrinsic.name) {
       case "null":
         return { type: "null" };
       case "unknown":
         return {};
       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<Record<string, any>>[]): Promise<void> {
+    this.#reportDuplicateIds();
     const toEmit: EmittedSourceFile[] = [];
 
     for (const sf of sourceFiles) {
@@ -644,9 +659,7 @@ export class JsonSchemaEmitter extends TypeEmitter<Record<string, any>, 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<Record<string, any>, JSONSche
 
   #getCurrentSourceFile() {
     let scope: Scope<object> = 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,25 +709,23 @@ export class JsonSchemaEmitter extends TypeEmitter<Record<string, any>, 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;
       const file = this.#getCurrentSourceFile().path;
       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<Record<string, any>, 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
@@ -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<JSONSchemaEmitterOptions>,

diff --git 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,17 +33,25 @@ 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 {}
         }
         namespace Test2 {
           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
@@ -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<s
     libraries: [JsonSchemaTestLibrary],
   });
   if (decorators) {
-    await host.addJsFile("dec.js", decorators);
+    host.addJsFile("dec.js", decorators);
     contents = `import "./dec.js";\n` + contents;
   }
-  await host.addTypeSpecFile("main.cadl", contents);
+  host.addTypeSpecFile("main.cadl", contents);
   await host.compile("main.cadl", {
     outputDir: "cadl-output",
   });
   return host;
 }
 
-export async function emitSchema(
+export async function emitSchemaWithDiagnostics(
   code: string,
   options: JSONSchemaEmitterOptions = {},
   testOptions: { emitNamespace?: boolean; emitTypes?: string[] } = { emitNamespace: true }
-) {
+): Promise<[Record<string, any>, 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;
 }