From fc805f9fa03ff1b369330beb4042cd61561e5c0b Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Wed, 12 Sep 2018 17:41:43 +0300 Subject: [PATCH 1/2] Fix #221: improve diagnostics when dereference fails In some cases, when a type could not be referenced, the diagnostic message doesn't indicate that (the undefined return value will still cause an error, but we also want to know that we couldn't deref the type) --- packages/jsii/lib/assembler.ts | 33 ++++++++++++------- .../jsii/test/negatives/neg.deref-error.ts | 10 ++++++ 2 files changed, 31 insertions(+), 12 deletions(-) create mode 100644 packages/jsii/test/negatives/neg.deref-error.ts diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 672db268af..5c6f29afae 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -162,14 +162,21 @@ export class Assembler implements Emitter { * * @returns the de-referenced type, if it was found, otherwise ``undefined``. */ - private _dereference(ref: spec.NamedTypeReference): spec.Type | undefined { + private _dereference(ref: spec.NamedTypeReference, referencingNode: ts.Node): spec.Type | undefined { const [assm, ] = ref.fqn.split('.'); + let type; if (assm === this.projectInfo.name) { - return this._types[ref.fqn]; + type = this._types[ref.fqn]; } else { const assembly = this.projectInfo.transitiveDependencies.find(dep => dep.name === assm); - return assembly && assembly.types && assembly.types[ref.fqn]; + type = assembly && assembly.types && assembly.types[ref.fqn]; } + + if (!type) { + this._diagnostic(referencingNode, ts.DiagnosticCategory.Error, `Unable to resolve referenced type '${ref.fqn}'. Missing export?`); + } + + return type; } private _diagnostic(node: ts.Node | null, category: ts.DiagnosticCategory, messageText: string) { @@ -206,7 +213,7 @@ export class Assembler implements Emitter { return `unknown.${typeName}`; } const fqn = `${pkg.name}.${typeName}`; - if (pkg.name !== this.projectInfo.name && !this._dereference({ fqn })) { + if (pkg.name !== this.projectInfo.name && !this._dereference({ fqn }, type.symbol.valueDeclaration)) { this._diagnostic(type.symbol.valueDeclaration, ts.DiagnosticCategory.Error, `Use of foreign type not present in the ${pkg.name}'s assembly: ${fqn}`); @@ -314,10 +321,10 @@ export class Assembler implements Emitter { continue; } this._defer(() => { - if (!spec.isClassType(this._dereference(ref))) { + if (!spec.isClassType(this._dereference(ref, base.symbol.valueDeclaration))) { this._diagnostic(base.symbol.valueDeclaration, ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} is not a class (${spec.describeTypeReference(ref)})`); + `Base type of ${jsiiType.fqn} is not a class or cannot be dereferenced (${spec.describeTypeReference(ref)})`); } }); jsiiType.base = ref; @@ -340,7 +347,7 @@ export class Assembler implements Emitter { continue; } this._defer(() => { - if (!spec.isInterfaceType(this._dereference(typeRef))) { + if (!spec.isInterfaceType(this._dereference(typeRef, expression))) { this._diagnostic(expression, ts.DiagnosticCategory.Error, `Implements clause of ${jsiiType.fqn} uses ${spec.describeTypeReference(typeRef)} as an interface`); @@ -393,7 +400,7 @@ export class Assembler implements Emitter { } } else if (jsiiType.base) { this._defer(() => { - const baseType = this._dereference(jsiiType.base!); + const baseType = this._dereference(jsiiType.base!, type.symbol.valueDeclaration); if (!baseType) { this._diagnostic(type.symbol.valueDeclaration, ts.DiagnosticCategory.Error, @@ -403,7 +410,7 @@ export class Assembler implements Emitter { } else { this._diagnostic(type.symbol.valueDeclaration, ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} (${jsiiType.base!.fqn}) is not a class`); + `Base type of ${jsiiType.fqn} (${jsiiType.base!.fqn}) is not a class or cannot be dereferenced`); } }); } else { @@ -499,12 +506,14 @@ export class Assembler implements Emitter { continue; } this._defer(() => { - if (!spec.isInterfaceType(this._dereference(ref))) { - const baseType = this._dereference(ref); + const baseType = this._dereference(ref, base.symbol.valueDeclaration); + if (!spec.isInterfaceType(baseType)) { if (baseType) { + // tslint:disable:max-line-length this._diagnostic(base.symbol.valueDeclaration, ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} is not an interface (${baseType.kind} ${spec.describeTypeReference(ref)})`); + `Base type of ${jsiiType.fqn} is not an interface or cannot be dereferenced (${baseType.kind} ${spec.describeTypeReference(ref)})`); + // tslint:enable:max-line-length } else { this._diagnostic(base.symbol.valueDeclaration, ts.DiagnosticCategory.Error, diff --git a/packages/jsii/test/negatives/neg.deref-error.ts b/packages/jsii/test/negatives/neg.deref-error.ts new file mode 100644 index 0000000000..2eb9976ea9 --- /dev/null +++ b/packages/jsii/test/negatives/neg.deref-error.ts @@ -0,0 +1,10 @@ +///!MATCH_ERROR: Unable to resolve referenced type 'Base'. Missing export? +///!MATCH_ERROR: Base type of jsii.Derived is not a class or cannot be dereferenced (Base) + +class Base { + +} + +export class Derived extends Base { + +} \ No newline at end of file From df1435fd3207ecf0ac1e52f4257608e383fa62bd Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Wed, 12 Sep 2018 17:59:44 +0300 Subject: [PATCH 2/2] If deref fails, do not emit further errors --- packages/jsii/lib/assembler.ts | 44 ++++++++----------- .../jsii/test/negatives/neg.deref-error.ts | 1 - 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 5c6f29afae..082c525ee9 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -321,10 +321,11 @@ export class Assembler implements Emitter { continue; } this._defer(() => { - if (!spec.isClassType(this._dereference(ref, base.symbol.valueDeclaration))) { + const deref = this._dereference(ref, base.symbol.valueDeclaration); + if (deref && !spec.isClassType(deref)) { this._diagnostic(base.symbol.valueDeclaration, ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} is not a class or cannot be dereferenced (${spec.describeTypeReference(ref)})`); + `Base type of ${jsiiType.fqn} is not a class (${spec.describeTypeReference(ref)})`); } }); jsiiType.base = ref; @@ -347,7 +348,8 @@ export class Assembler implements Emitter { continue; } this._defer(() => { - if (!spec.isInterfaceType(this._dereference(typeRef, expression))) { + const deref = this._dereference(typeRef, expression); + if (deref && !spec.isInterfaceType(deref)) { this._diagnostic(expression, ts.DiagnosticCategory.Error, `Implements clause of ${jsiiType.fqn} uses ${spec.describeTypeReference(typeRef)} as an interface`); @@ -401,16 +403,14 @@ export class Assembler implements Emitter { } else if (jsiiType.base) { this._defer(() => { const baseType = this._dereference(jsiiType.base!, type.symbol.valueDeclaration); - if (!baseType) { - this._diagnostic(type.symbol.valueDeclaration, - ts.DiagnosticCategory.Error, - `Unable to resolve type ${jsiiType.base!.fqn} (base type of ${jsiiType.fqn})`); - } else if (spec.isClassType(baseType)) { - jsiiType.initializer = baseType.initializer; - } else { - this._diagnostic(type.symbol.valueDeclaration, - ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} (${jsiiType.base!.fqn}) is not a class or cannot be dereferenced`); + if (baseType) { + if (spec.isClassType(baseType)) { + jsiiType.initializer = baseType.initializer; + } else { + this._diagnostic(type.symbol.valueDeclaration, + ts.DiagnosticCategory.Error, + `Base type of ${jsiiType.fqn} (${jsiiType.base!.fqn}) is not a class`); + } } }); } else { @@ -507,18 +507,12 @@ export class Assembler implements Emitter { } this._defer(() => { const baseType = this._dereference(ref, base.symbol.valueDeclaration); - if (!spec.isInterfaceType(baseType)) { - if (baseType) { - // tslint:disable:max-line-length - this._diagnostic(base.symbol.valueDeclaration, - ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} is not an interface or cannot be dereferenced (${baseType.kind} ${spec.describeTypeReference(ref)})`); - // tslint:enable:max-line-length - } else { - this._diagnostic(base.symbol.valueDeclaration, - ts.DiagnosticCategory.Error, - `Base type of ${jsiiType.fqn} could not be resolved (${spec.describeTypeReference(ref)})`); - } + if (baseType && !spec.isInterfaceType(baseType)) { + // tslint:disable:max-line-length + this._diagnostic(base.symbol.valueDeclaration, + ts.DiagnosticCategory.Error, + `Base type of ${jsiiType.fqn} is not an interface (${baseType.kind} ${spec.describeTypeReference(ref)})`); + // tslint:enable:max-line-length } }); if (jsiiType.interfaces) { diff --git a/packages/jsii/test/negatives/neg.deref-error.ts b/packages/jsii/test/negatives/neg.deref-error.ts index 2eb9976ea9..a98d479ee3 100644 --- a/packages/jsii/test/negatives/neg.deref-error.ts +++ b/packages/jsii/test/negatives/neg.deref-error.ts @@ -1,5 +1,4 @@ ///!MATCH_ERROR: Unable to resolve referenced type 'Base'. Missing export? -///!MATCH_ERROR: Base type of jsii.Derived is not a class or cannot be dereferenced (Base) class Base {