From 7cd59fc180648b6c4b873364ef301b3228e69885 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Wed, 10 Nov 2021 12:44:51 +0100 Subject: [PATCH] fix(jsii): incorrectly allowed unexported type in constructor params (#3147) The compiler incorrectly allowed un-epxorted types to be used in a constructor parameter, resulting in invalid assemblies (where a reference to the un-exported type exists, but the type itself does not). Added a test that validates this is now checked against, and improved the error experience by providing source anchors for where the problematic type is being used. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --- packages/jsii/lib/node-bindings.ts | 9 +++ packages/jsii/lib/validator.ts | 74 +++++++++++++++---- .../test/__snapshots__/negatives.test.ts.snap | 36 ++++++++- packages/jsii/test/negatives/_unexported.ts | 6 ++ ...xpose-unexported-type-constructor-param.ts | 9 +++ 5 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 packages/jsii/test/negatives/_unexported.ts create mode 100644 packages/jsii/test/negatives/neg.expose-unexported-type-constructor-param.ts diff --git a/packages/jsii/lib/node-bindings.ts b/packages/jsii/lib/node-bindings.ts index a92984b2f7..ae7721577f 100644 --- a/packages/jsii/lib/node-bindings.ts +++ b/packages/jsii/lib/node-bindings.ts @@ -99,6 +99,15 @@ export const getParameterRelatedNode: ( | ts.PropertySignature | undefined = getRelatedNode; +export const getPropertyRelatedNode: ( + object: spec.Parameter, +) => + | ts.AccessorDeclaration + | ts.ParameterPropertyDeclaration + | ts.PropertyDeclaration + | ts.PropertySignature + | undefined = getRelatedNode; + export const getTypeRelatedNode: ( object: spec.Type, ) => diff --git a/packages/jsii/lib/validator.ts b/packages/jsii/lib/validator.ts index 998c6b8fb7..3442e0793a 100644 --- a/packages/jsii/lib/validator.ts +++ b/packages/jsii/lib/validator.ts @@ -7,6 +7,7 @@ import * as ts from 'typescript'; import { Emitter } from './emitter'; import { JsiiDiagnostic } from './jsii-diagnostic'; import { getRelatedNode } from './node-bindings'; +import * as bindings from './node-bindings'; import { ProjectInfo } from './project-info'; export class Validator implements Emitter { @@ -187,7 +188,8 @@ function _defaultValidations(): ValidationFunction[] { if (assembly.name === assm) { if (!(typeRef.fqn in (assembly.types ?? {}))) { diagnostic( - JsiiDiagnostic.JSII_3000_EXPORTED_API_USES_HIDDEN_TYPE.createDetached( + JsiiDiagnostic.JSII_3000_EXPORTED_API_USES_HIDDEN_TYPE.create( + typeRef.node!, // Pretend there is always a value typeRef.fqn, ), ); @@ -607,39 +609,85 @@ function _allMembers( return [..._allMethods(assm), ..._allProperties(assm)]; } -function _allTypeReferences(assm: spec.Assembly): spec.NamedTypeReference[] { - const typeReferences = new Array(); +interface AnnotatedTypeReference extends spec.NamedTypeReference { + readonly node: ts.Node | undefined; +} + +function _allTypeReferences( + assm: spec.Assembly, +): readonly AnnotatedTypeReference[] { + const typeReferences = new Array(); for (const type of _allTypes(assm)) { if (!spec.isClassOrInterfaceType(type)) { continue; } - if (spec.isClassType(type) && type.base) { - typeReferences.push({ fqn: type.base }); + if (spec.isClassType(type)) { + const node = bindings.getClassRelatedNode(type); + if (type.base) { + typeReferences.push({ + fqn: type.base, + node: node?.heritageClauses?.find( + (hc) => hc.token === ts.SyntaxKind.ExtendsKeyword, + )?.types[0], + }); + } + if (type.initializer?.parameters) { + for (const param of type.initializer.parameters) { + _collectTypeReferences( + param.type, + bindings.getParameterRelatedNode(param)?.type, + ); + } + } } if (type.interfaces) { - type.interfaces.forEach((iface) => typeReferences.push({ fqn: iface })); + const node = bindings.getClassOrInterfaceRelatedNode(type); + type.interfaces.forEach((iface) => + typeReferences.push({ + fqn: iface, + node: node?.heritageClauses?.find( + (hc) => + hc.token === + (spec.isInterfaceType(type) + ? ts.SyntaxKind.ImplementsKeyword + : ts.SyntaxKind.ExtendsKeyword), + ), + }), + ); } } for (const { member: prop } of _allProperties(assm)) { - _collectTypeReferences(prop.type); + _collectTypeReferences( + prop.type, + bindings.getPropertyRelatedNode(prop)?.type, + ); } for (const { member: meth } of _allMethods(assm)) { if (meth.returns) { - _collectTypeReferences(meth.returns.type); + _collectTypeReferences( + meth.returns.type, + bindings.getMethodRelatedNode(meth)?.type, + ); } for (const param of meth.parameters ?? []) { - _collectTypeReferences(param.type); + _collectTypeReferences( + param.type, + bindings.getParameterRelatedNode(param)?.type, + ); } } return typeReferences; - function _collectTypeReferences(type: spec.TypeReference): void { + function _collectTypeReferences( + type: spec.TypeReference, + node: ts.Node | undefined, + ): void { if (spec.isNamedTypeReference(type)) { - typeReferences.push(type); + typeReferences.push({ ...type, node }); } else if (spec.isCollectionTypeReference(type)) { - _collectTypeReferences(type.collection.elementtype); + _collectTypeReferences(type.collection.elementtype, node); } else if (spec.isUnionTypeReference(type)) { - type.union.types.forEach(_collectTypeReferences); + type.union.types.forEach((type) => _collectTypeReferences(type, node)); } } } diff --git a/packages/jsii/test/__snapshots__/negatives.test.ts.snap b/packages/jsii/test/__snapshots__/negatives.test.ts.snap index 3ed8b6469c..9eaa50f870 100644 --- a/packages/jsii/test/__snapshots__/negatives.test.ts.snap +++ b/packages/jsii/test/__snapshots__/negatives.test.ts.snap @@ -107,10 +107,23 @@ error JSII8000: Type names must be CamelCased. Rename "myEnum" to "MyEnum" exports[`enum-name.2 1`] = ` error JSII8000: Type names must be CamelCased. Rename "My_Enum" to "MyEnum" +`; + +exports[`expose-unexported-type-constructor-param 1`] = ` +neg.expose-unexported-type-constructor-param.ts:6:29 - error JSII3000: Exported APIs cannot use un-exported type "jsii.UnexportedProps" + +6 public constructor(props: UnexportedProps) { + ~~~~~~~~~~~~~~~ + + `; exports[`expose-unexported-type-external 1`] = ` -error JSII3000: Exported APIs cannot use un-exported type "jsii.UnexportedType" +neg.expose-unexported-type-external.ts:7:14 - error JSII3000: Exported APIs cannot use un-exported type "jsii.UnexportedType" + +7 public p?: UnexportedType; + ~~~~~~~~~~~~~~ + `; @@ -641,6 +654,11 @@ error JSII8002: Method and property (unless they are static readonly) names must `; exports[`stripped-deprecated 1`] = ` +neg.stripped-deprecated.ts:11:25 - error JSII3000: Exported APIs cannot use un-exported type "jsii.DeprecatedInterface" + +11 public bad(parameter: DeprecatedInterface): DeprecatedInterface { + ~~~~~~~~~~~~~~~~~~~ + neg.stripped-deprecated.ts:11:25 - error JSII3999: Parameter has @deprecated type jsii.DeprecatedInterface, and it is erased by --strip-deprecated. 11 public bad(parameter: DeprecatedInterface): DeprecatedInterface { @@ -650,6 +668,11 @@ neg.stripped-deprecated.ts:11:25 - error JSII3999: Parameter has @deprecated typ 4 export interface DeprecatedInterface {} ~~~~~~~~~~~~~~~~~~~ The @deprecated type is declared here +neg.stripped-deprecated.ts:11:47 - error JSII3000: Exported APIs cannot use un-exported type "jsii.DeprecatedInterface" + +11 public bad(parameter: DeprecatedInterface): DeprecatedInterface { + ~~~~~~~~~~~~~~~~~~~ + neg.stripped-deprecated.ts:11:47 - error JSII3999: Method has @deprecated type jsii.DeprecatedInterface, and it is erased by --strip-deprecated. 11 public bad(parameter: DeprecatedInterface): DeprecatedInterface { @@ -659,6 +682,11 @@ neg.stripped-deprecated.ts:11:47 - error JSII3999: Method has @deprecated type j 4 export interface DeprecatedInterface {} ~~~~~~~~~~~~~~~~~~~ The @deprecated type is declared here +neg.stripped-deprecated.ts:7:17 - error JSII3000: Exported APIs cannot use un-exported type "jsii.DeprecatedInterface" + +7 public ouch?: DeprecatedInterface; + ~~~~~~~~~~~~~~~~~~~ + neg.stripped-deprecated.ts:7:17 - error JSII3999: Property has @deprecated type jsii.DeprecatedInterface, and it is erased by --strip-deprecated. 7 public ouch?: DeprecatedInterface; @@ -668,6 +696,11 @@ neg.stripped-deprecated.ts:7:17 - error JSII3999: Property has @deprecated type 4 export interface DeprecatedInterface {} ~~~~~~~~~~~~~~~~~~~ The @deprecated type is declared here +neg.stripped-deprecated.ts:9:42 - error JSII3000: Exported APIs cannot use un-exported type "jsii.DeprecatedInterface" + +9 public constructor(public readonly no: DeprecatedInterface) {} + ~~~~~~~~~~~~~~~~~~~ + neg.stripped-deprecated.ts:9:42 - error JSII3999: Parameter has @deprecated type jsii.DeprecatedInterface, and it is erased by --strip-deprecated. 9 public constructor(public readonly no: DeprecatedInterface) {} @@ -686,7 +719,6 @@ neg.stripped-deprecated.ts:9:42 - error JSII3999: Property has @deprecated type 4 export interface DeprecatedInterface {} ~~~~~~~~~~~~~~~~~~~ The @deprecated type is declared here -error JSII3000: Exported APIs cannot use un-exported type "jsii.DeprecatedInterface" `; diff --git a/packages/jsii/test/negatives/_unexported.ts b/packages/jsii/test/negatives/_unexported.ts new file mode 100644 index 0000000000..94a7c28bde --- /dev/null +++ b/packages/jsii/test/negatives/_unexported.ts @@ -0,0 +1,6 @@ +/** + * This should never be exported by the modules that import it. + */ +export interface UnexportedProps { + readonly name: string; +} diff --git a/packages/jsii/test/negatives/neg.expose-unexported-type-constructor-param.ts b/packages/jsii/test/negatives/neg.expose-unexported-type-constructor-param.ts new file mode 100644 index 0000000000..3d2350ead1 --- /dev/null +++ b/packages/jsii/test/negatives/neg.expose-unexported-type-constructor-param.ts @@ -0,0 +1,9 @@ +import { UnexportedProps } from './_unexported'; + +export class ExportedClass { + public readonly name: string; + + public constructor(props: UnexportedProps) { + this.name = props.name; + } +}