From 842baa8023d4c32456458a0a2b6df2439eb207b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Tue, 2 Jun 2020 14:57:15 +0200 Subject: [PATCH] fix(jsii): build succeeds using Omit When using the `Omit` 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 --- packages/jsii/lib/assembler.ts | 45 +++++++++++++++++++++- packages/jsii/lib/compiler.ts | 1 + packages/jsii/test/negatives/neg.omit.1.ts | 11 ++++++ packages/jsii/test/negatives/neg.omit.2.ts | 12 ++++++ packages/jsii/test/negatives/neg.omit.3.ts | 11 ++++++ packages/jsii/test/negatives/neg.omit.4.ts | 11 ++++++ 6 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 packages/jsii/test/negatives/neg.omit.1.ts create mode 100644 packages/jsii/test/negatives/neg.omit.2.ts create mode 100644 packages/jsii/test/negatives/neg.omit.3.ts create mode 100644 packages/jsii/test/negatives/neg.omit.4.ts diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 8bda394bc4..0300d283f2 100644 --- 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) { + 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 index fd5ef2fa9b..a7d445c2bf 100644 --- 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 new file mode 100644 index 0000000000..694b3416a7 --- /dev/null +++ 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 +export interface BarBaz extends Omit { + readonly baz: number; +} diff --git a/packages/jsii/test/negatives/neg.omit.2.ts b/packages/jsii/test/negatives/neg.omit.2.ts new file mode 100644 index 0000000000..9ac411f164 --- /dev/null +++ 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 +export class BarBaz implements Omit { + 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 new file mode 100644 index 0000000000..80da33d44c --- /dev/null +++ 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 +export interface IReturner { + bar(): Omit; +} diff --git a/packages/jsii/test/negatives/neg.omit.4.ts b/packages/jsii/test/negatives/neg.omit.4.ts new file mode 100644 index 0000000000..b8365c48a8 --- /dev/null +++ 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 +export interface IReturner { + bar(opts: Omit): void; +}