chore(jsii): better errors around use of hidden types

When a hidden (that is, `@internal` or not exported) type is used in a visible (`public` or `protected` on an exported type) API, the error produced would refer to the unusable type, but would not give any indication of where it was being used from. This makes several enhancements to this process: - Qualify the kind of use for the type (return, parameter, ...) - Attach the error to the resolving node (usage location) - Provide a related message with the unusable type's declaration - Specifically message around "this" (used or inferred as a return type) This is going to particularly enhance the experience of folks extending internal base types, where those internal base types declare members that return hidden types (or "this"). Fixes #1860
aws · Aug 7, 2020 · 17a551e · 17a551e
1 parent 2cc4628
commit 17a551e
Show file tree

Hide file tree

Showing 3 changed files with 150 additions and 41 deletions.
diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts
@@ -377,10 +377,18 @@ export class Assembler implements Emitter {
    * inserted in the assembler context.
    *
    * @param type the type for which a JSII fully qualified name is neede.
+   * @param resolvingNode the node for which the type was being resolved (for locality of error reporting)
+   * @param typeUse the reason why this type was resolved (e.g: "return type")
+   * @param isThisType whether this type was specified or inferred as "this" or not
    *
    * @returns the FQN of the type, or some "unknown" marker.
    */
-  private async _getFQN(type: ts.Type): Promise<string> {
+  private async _getFQN(
+    type: ts.Type,
+    resolvingNode: ts.Node,
+    typeUse: TypeUseKind,
+    isThisType: boolean,
+  ): Promise<string> {
     const singleValuedEnum = isSingleValuedEnum(type, this._typeChecker);
 
     const tsFullName = this._typeChecker.getFullyQualifiedName(type.symbol);
@@ -397,23 +405,49 @@ export class Assembler implements Emitter {
       node = type.symbol.declarations[0];
     }
 
-    const groups = /^"([^"]+)"\.(.*)$/.exec(tsName);
-    if (!groups) {
+    // Set to true to prevent further adding of Error diagnostics for known-bad reference
+    let hasError = false;
+
+    if (this._isPrivateOrInternal(type.symbol)) {
+      // Check if this type is "this" (explicit or inferred method return type).
+      const commonMessage = `cannot be used as the ${typeUse} because it is private or @internal`;
       this._diagnostic(
-        node,
+        resolvingNode,
         ts.DiagnosticCategory.Error,
-        `Cannot use private type ${tsName} in exported declarations`,
+        isThisType
+          ? `Type "this" (aka: "${type.symbol.name}") ${commonMessage}`
+          : `Type "${type.symbol.name}" ${commonMessage}`,
+        makeCause(node),
       );
+
+      hasError = true;
+    }
+
+    const groups = /^"([^"]+)"\.(.*)$/.exec(tsName);
+    if (!groups) {
+      if (!hasError) {
+        this._diagnostic(
+          resolvingNode,
+          ts.DiagnosticCategory.Error,
+          `Cannot internal type ${tsName} as a ${typeUse} in exported declarations`,
+          makeCause(node),
+        );
+        hasError = true;
+      }
       return tsName;
     }
     const [, modulePath, typeName] = groups;
     const pkg = await findPackageInfo(modulePath);
     if (!pkg) {
-      this._diagnostic(
-        node,
-        ts.DiagnosticCategory.Error,
-        `Could not find module for ${modulePath}`,
-      );
+      if (!hasError) {
+        this._diagnostic(
+          resolvingNode,
+          ts.DiagnosticCategory.Error,
+          `Could not find module corresponding to ${modulePath}`,
+          makeCause(node),
+        );
+        hasError = true;
+      }
       return `unknown.${typeName}`;
     }
 
@@ -428,13 +462,38 @@ export class Assembler implements Emitter {
       pkg.name !== this.projectInfo.name &&
       !this._dereference({ fqn }, type.symbol.valueDeclaration)
     ) {
-      this._diagnostic(
-        node,
-        ts.DiagnosticCategory.Error,
-        `Use of foreign type not present in the ${pkg.name}'s assembly: ${fqn}`,
-      );
+      if (!hasError) {
+        this._diagnostic(
+          resolvingNode,
+          ts.DiagnosticCategory.Error,
+          `Type "${fqn}" cannot be used as a ${typeUse} because it is not exported from ${pkg.name}`,
+          makeCause(node),
+        );
+        hasError = true;
+      }
     }
     return fqn;
+
+    function makeCause(node: ts.Node): ts.DiagnosticRelatedInformation[] {
+      const declNode =
+        ts.isClassDeclaration(node) ||
+        ts.isEnumDeclaration(node) ||
+        ts.isInterfaceDeclaration(node) ||
+        ts.isTypeAliasDeclaration(node)
+          ? node.name ?? node
+          : node;
+      return [
+        {
+          category: ts.DiagnosticCategory.Message,
+          code: JSII_DIAGNOSTICS_CODE,
+          file: declNode.getSourceFile(),
+          start: declNode.getStart(declNode.getSourceFile()),
+          length:
+            declNode.getEnd() - declNode.getStart(declNode.getSourceFile()),
+          messageText: `The referenced type is declared here`,
+        },
+      ];
+    }
   }
 
   /**
@@ -487,7 +546,7 @@ export class Assembler implements Emitter {
   ): Promise<void> {
     const declaration = symbol.valueDeclaration ?? symbol.declarations[0];
     if (declaration == null) {
-      // Nothing to do here...
+      // Nothing to do here..
       return;
     }
     if (ts.isModuleDeclaration(declaration)) {
@@ -506,7 +565,7 @@ export class Assembler implements Emitter {
       return;
     }
     if (!ts.isNamespaceExport(declaration)) {
-      // Nothing to do here...
+      // Nothing to do here..
       return;
     }
 
@@ -993,7 +1052,7 @@ export class Assembler implements Emitter {
 
     const typeRefs = Array.from(baseInterfaces).map(async (iface) => {
       const decl = iface.symbol.valueDeclaration;
-      const typeRef = await this._typeReference(iface, decl);
+      const typeRef = await this._typeReference(iface, decl, 'base interface');
       return { decl, typeRef };
     });
     for (const { decl, typeRef } of await Promise.all(typeRefs)) {
@@ -1096,7 +1155,11 @@ export class Assembler implements Emitter {
       }
 
       // eslint-disable-next-line no-await-in-loop
-      const ref = await this._typeReference(base, type.symbol.valueDeclaration);
+      const ref = await this._typeReference(
+        base,
+        type.symbol.valueDeclaration,
+        'base class',
+      );
 
       if (!spec.isNamedTypeReference(ref)) {
         this._diagnostic(
@@ -1214,7 +1277,7 @@ export class Assembler implements Emitter {
       }
 
       for (const memberDecl of classDecl.members) {
-        // The "??" is to get to the __constructor symbol (getSymbolAtLocation wouldn't work there...)
+        // The "??" is to get to the __constructor symbol (getSymbolAtLocation wouldn't work there..)
         const member =
           this._typeChecker.getSymbolAtLocation(memberDecl.name!) ??
           ((memberDecl as any).symbol as ts.Symbol);
@@ -1749,7 +1812,7 @@ export class Assembler implements Emitter {
           // This is *NOT* a data type, so it may not extend something that is one.
           for (const base of bases) {
             if (!spec.isInterfaceType(base)) {
-              // Invalid type we already warned about earlier, just ignoring it here...
+              // Invalid type we already warned about earlier, just ignoring it here..
               continue;
             }
             if (base.datatype) {
@@ -1851,7 +1914,11 @@ export class Assembler implements Emitter {
       protected: _isProtected(symbol) || undefined,
       returns: _isVoid(returnType)
         ? undefined
-        : await this._optionalValue(returnType, declaration),
+        : await this._optionalValue(
+            returnType,
+            declaration.name,
+            'return type',
+          ),
       async: _isPromise(returnType) || undefined,
       static: _isStatic(symbol) || undefined,
       locationInModule: this.declarationLocation(declaration),
@@ -1974,7 +2041,8 @@ export class Assembler implements Emitter {
     const property: spec.Property = {
       ...(await this._optionalValue(
         this._typeChecker.getTypeOfSymbolAtLocation(symbol, signature),
-        signature,
+        signature.name,
+        'property type',
       )),
       abstract: _isAbstract(symbol, type) || undefined,
       name: symbol.name,
@@ -2038,8 +2106,9 @@ export class Assembler implements Emitter {
 
     const parameter: spec.Parameter = {
       ...(await this._optionalValue(
-        this._typeChecker.getTypeAtLocation(paramSymbol.valueDeclaration),
-        paramSymbol.valueDeclaration,
+        this._typeChecker.getTypeAtLocation(paramDeclaration),
+        paramDeclaration.name,
+        'parameter type',
       )),
       name: paramSymbol.name,
       variadic: paramDeclaration.dotDotDotToken && true,
@@ -2063,9 +2132,10 @@ export class Assembler implements Emitter {
 
   private async _typeReference(
     type: ts.Type,
-    declaration: ts.Declaration,
+    declaration: ts.Node,
+    purpose: TypeUseKind,
   ): Promise<spec.TypeReference> {
-    const optionalValue = await this._optionalValue(type, declaration);
+    const optionalValue = await this._optionalValue(type, declaration, purpose);
     if (optionalValue.optional) {
       this._diagnostic(
         declaration,
@@ -2078,8 +2148,11 @@ export class Assembler implements Emitter {
 
   private async _optionalValue(
     type: ts.Type,
-    declaration: ts.Declaration,
+    declaration: ts.Node,
+    purpose: TypeUseKind,
   ): Promise<spec.OptionalValue> {
+    const isThisType = _isThisType(type, this._typeChecker);
+
     if (type.isLiteral() && _isEnumLike(type)) {
       type = this._typeChecker.getBaseTypeOfLiteralType(type);
     } else {
@@ -2123,11 +2196,17 @@ export class Assembler implements Emitter {
         return { type: spec.CANONICAL_ANY };
       }
       return {
-        type: await this._typeReference(typeRef.typeArguments[0], declaration),
+        type: await this._typeReference(
+          typeRef.typeArguments[0],
+          declaration,
+          purpose,
+        ),
       };
     }
 
-    return { type: { fqn: await this._getFQN(type) } };
+    return {
+      type: { fqn: await this._getFQN(type, declaration, purpose, isThisType) },
+    };
 
     async function _arrayType(
       this: Assembler,
@@ -2139,6 +2218,7 @@ export class Assembler implements Emitter {
         elementtype = await this._typeReference(
           typeRef.typeArguments[0],
           declaration,
+          'list element type',
         );
       } else {
         const count = typeRef.typeArguments
@@ -2166,7 +2246,11 @@ export class Assembler implements Emitter {
       let elementtype: spec.TypeReference;
       const objectType = type.getStringIndexType();
       if (objectType) {
-        elementtype = await this._typeReference(objectType, declaration);
+        elementtype = await this._typeReference(
+          objectType,
+          declaration,
+          'map element type',
+        );
       } else {
         this._diagnostic(
           declaration,
@@ -2230,7 +2314,11 @@ export class Assembler implements Emitter {
           continue;
         }
         // eslint-disable-next-line no-await-in-loop
-        const resolvedType = await this._typeReference(subType, declaration);
+        const resolvedType = await this._typeReference(
+          subType,
+          declaration,
+          purpose,
+        );
         if (types.find((ref) => deepEqual(ref, resolvedType)) != null) {
           continue;
         }
@@ -2806,3 +2894,27 @@ async function findPackageInfo(fromDir: string): Promise<any> {
   }
   return findPackageInfo(parent);
 }
+
+/**
+ * Checks is the provided type is "this" (as a type annotation).
+ *
+ * @param type        the validated type.
+ * @param typeChecker the type checker.
+ *
+ * @returns `true` iif the type is `this`
+ */
+function _isThisType(type: ts.Type, typeChecker: ts.TypeChecker): boolean {
+  return typeChecker.typeToTypeNode(type)?.kind === ts.SyntaxKind.ThisKeyword;
+}
+
+/**
+ * A location where a type can be used.
+ */
+type TypeUseKind =
+  | 'base class'
+  | 'base interface'
+  | 'list element type'
+  | 'map element type'
+  | 'parameter type'
+  | 'property type'
+  | 'return type';
diff --git a/packages/jsii/test/negatives/neg.expose-unexported-type-internal-in-namespace.ts b/packages/jsii/test/negatives/neg.expose-unexported-type-internal-in-namespace.ts
@@ -1,13 +1,12 @@
-///!MATCH_ERROR: Cannot use private type MyNamespace.UnexportedType in exported declarations
+///!MATCH_ERROR: Cannot internal type MyNamespace.UnexportedType as a property type in exported declarations
 
 // Attempt to expose an unexported type defined in this file should fail
 // because that type will not be available in the module spec.
 
 namespace MyNamespace {
-    export class UnexportedType {
-    }
+  export class UnexportedType {}
 }
 
 export class ExportedType {
-    public p?: MyNamespace.UnexportedType;
+  public p?: MyNamespace.UnexportedType;
 }
diff --git a/packages/jsii/test/negatives/neg.expose-unexported-type-internal.ts b/packages/jsii/test/negatives/neg.expose-unexported-type-internal.ts
@@ -1,12 +1,10 @@
-///!MATCH_ERROR: Cannot use private type UnexportedType in exported declarations
+///!MATCH_ERROR: Type "UnexportedType" cannot be used as the property type because it is private or @internal
 
 // Attempt to expose an unexported type defined in this file should fail
 // because that type will not be available in the module spec.
 
-class UnexportedType {
-
-}
+class UnexportedType {}
 
 export class ExportedType {
-    public p?: UnexportedType;
+  public p?: UnexportedType;
 }