fix(jsii): build succeeds using Omit<T, K>

When using the `Omit<T, K>` helper type (a mapped type) on an exported API, `jsii` should detect this and fail, instead of allowing the application to compile. An additional bug caused use of this type to result in a successful exit code from `jsii`, while the `.jsii` file was never written. This has also been fixed. Fixes #1707
aws · Jun 2, 2020 · 842baa8 · 842baa8
1 parent b2aa424
commit 842baa8
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 2 deletions.
diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts
@@ -758,12 +758,14 @@ export class Assembler implements Emitter {
 
     if (ts.isClassDeclaration(node) && _isExported(node)) {
       // export class Name { ... }
+      this._validateHeritageClauses(node.heritageClauses);
       jsiiType = await this._visitClass(
         this._typeChecker.getTypeAtLocation(node),
         context,
       );
     } else if (ts.isInterfaceDeclaration(node) && _isExported(node)) {
       // export interface Name { ... }
+      this._validateHeritageClauses(node.heritageClauses);
       jsiiType = await this._visitInterface(
         this._typeChecker.getTypeAtLocation(node),
         context,
@@ -892,6 +894,45 @@ export class Assembler implements Emitter {
     return [jsiiType];
   }
 
+  private _validateHeritageClauses(clauses?: ts.NodeArray<ts.HeritageClause>) {
+    if (clauses == null || clauses.length === 0) {
+      // Nothing to do.
+      return;
+    }
+    for (const clause of clauses) {
+      for (const node of clause.types) {
+        const parentType = this._typeChecker.getTypeAtLocation(node);
+        // For some reason, we cannot trust parentType.isClassOrInterface()
+        const badDecl = parentType.symbol.declarations.find(
+          (decl) =>
+            !ts.isClassDeclaration(decl) && // <-- local classes
+            !ts.isInterfaceDeclaration(decl) && // <-- local interfaces
+            !ts.isModuleDeclaration(decl), // <-- imported types
+        );
+        if (badDecl != null) {
+          this._diagnostic(
+            node,
+            ts.DiagnosticCategory.Error,
+            `Illegal "${clauseType(clause.token)}" value for an exported API: ${
+              ts.SyntaxKind[badDecl.kind]
+            }`,
+          );
+        }
+      }
+    }
+
+    function clauseType(token: ts.SyntaxKind): string {
+      switch (token) {
+        case ts.SyntaxKind.ExtendsKeyword:
+          return 'extends';
+        case ts.SyntaxKind.ImplementsKeyword:
+          return 'implements';
+        default:
+          return ts.SyntaxKind[token];
+      }
+    }
+  }
+
   private declarationLocation(node: ts.Declaration): spec.SourceLocation {
     const file = node.getSourceFile();
     const line = ts.getLineAndCharacterOfPosition(file, node.getStart()).line;
@@ -1587,7 +1628,7 @@ export class Assembler implements Emitter {
       for (const member of declaringType.getProperties()) {
         if (
           !(declaringType.symbol.getDeclarations() ?? []).find(
-            (decl) => decl === member.valueDeclaration.parent,
+            (decl) => decl === member.valueDeclaration?.parent,
           )
         ) {
           continue;
@@ -2104,7 +2145,7 @@ export class Assembler implements Emitter {
         this._diagnostic(
           declaration,
           ts.DiagnosticCategory.Error,
-          'Only string index maps are supported',
+          'Only string-indexed map types are supported',
         );
         elementtype = spec.CANONICAL_ANY;
       }

diff --git a/packages/jsii/lib/compiler.ts b/packages/jsii/lib/compiler.ts
@@ -246,6 +246,7 @@ export class Compiler implements Emitter {
       diagnostics.push(...assmEmit.diagnostics);
     } catch (e) {
       LOG.error(`Error during type model analysis: ${e}\n${e.stack}`);
+      hasErrors = true;
     }
 
     return {

diff --git a/packages/jsii/test/negatives/neg.omit.1.ts b/packages/jsii/test/negatives/neg.omit.1.ts
@@ -0,0 +1,11 @@
+///!MATCH_ERROR: Illegal "extends" value for an exported API
+
+export interface FooBar {
+  readonly foo: string;
+  readonly bar: string;
+}
+
+// Illegal attempt to extend Omit<T, Key>
+export interface BarBaz extends Omit<FooBar, 'foo'> {
+  readonly baz: number;
+}
diff --git a/packages/jsii/test/negatives/neg.omit.2.ts b/packages/jsii/test/negatives/neg.omit.2.ts
@@ -0,0 +1,12 @@
+///!MATCH_ERROR: Illegal "implements" value for an exported API
+
+export interface FooBar {
+  readonly foo: string;
+  readonly bar: string;
+}
+
+// Illegal attempt to implement Omit<T, Key>
+export class BarBaz implements Omit<FooBar, 'foo'> {
+  public readonly bar = 'BAR!';
+  public readonly baz: number = 1337;
+}
diff --git a/packages/jsii/test/negatives/neg.omit.3.ts b/packages/jsii/test/negatives/neg.omit.3.ts
@@ -0,0 +1,11 @@
+///!MATCH_ERROR: Only string-indexed map types are supported
+
+export interface FooBar {
+  readonly foo: string;
+  readonly bar: string;
+}
+
+// Illegal attempt to return Omit<T, Key>
+export interface IReturner {
+  bar(): Omit<FooBar, 'foo'>;
+}
diff --git a/packages/jsii/test/negatives/neg.omit.4.ts b/packages/jsii/test/negatives/neg.omit.4.ts
@@ -0,0 +1,11 @@
+///!MATCH_ERROR: Only string-indexed map types are supported
+
+export interface FooBar {
+  readonly foo: string;
+  readonly bar: string;
+}
+
+// Illegal attempt to accept Omit<T, Key>
+export interface IReturner {
+  bar(opts: Omit<FooBar, 'foo'>): void;
+}