fix(ngcc): ensure that "inline exports" can be interpreted correctly (#39267)

petebacondarwin · atscott · commit 822b838fbcb3 · 2020-10-14T14:11:45.000-07:00
Previously, inline exports of the form `exports.foo = <implementation>;` were being interpreted (by the ngtsc `PartialInterpeter`) as `Reference` objects. This is not what is desired since it prevents the value of the export from being unpacked, such as when analyzing `NgModule` declarations: ``` exports.directives = [Directive1, Directive2]; @NgImport({declarations: [exports.directives]}) class AppModule {} ``` In this example the interpreter would think that `exports.directives` was a reference rather than an array that needs to be unpacked. This bug was picked up by the ngcc-validation repository. See angular/ngcc-validation#1990 and https://circleci.com/gh/angular/ngcc-validation/17130 PR Close #39267
diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts
@@ -14,7 +14,7 @@ import {Declaration, DeclarationKind, Import} from '../../../src/ngtsc/reflectio
 import {BundleProgram} from '../packages/bundle_program';
 import {FactoryMap, isDefined} from '../utils';
 
-import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, WildcardReexportStatement} from './commonjs_umd_utils';
+import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils';
 import {Esm5ReflectionHost} from './esm5_host';
 import {NgccClassSymbol} from './ngcc_host';
 
@@ -117,9 +117,16 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
   }
 
   private extractBasicCommonJsExportDeclaration(statement: ExportsStatement): ExportDeclaration {
-    const exportExpression = statement.expression.right;
-    const name = statement.expression.left.name.text;
-    return this.extractCommonJsExportDeclaration(name, exportExpression);
+    const exportExpression = skipAliases(statement.expression.right);
+    const node = statement.expression.left;
+    const declaration = this.getDeclarationOfExpression(exportExpression) ?? {
+      kind: DeclarationKind.Inline,
+      node,
+      implementation: exportExpression,
+      known: null,
+      viaModule: null,
+    };
+    return {name: node.name.text, declaration};
   }
 
   private extractCommonJsWildcardReexports(
@@ -163,7 +170,22 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
     if (getterFnExpression === null) {
       return null;
     }
-    return this.extractCommonJsExportDeclaration(name, getterFnExpression);
+
+    const declaration = this.getDeclarationOfExpression(getterFnExpression);
+    if (declaration !== null) {
+      return {name, declaration};
+    }
+
+    return {
+      name,
+      declaration: {
+        kind: DeclarationKind.Inline,
+        node: args[1],
+        implementation: getterFnExpression,
+        known: null,
+        viaModule: null,
+      },
+    };
   }
 
   private findCommonJsImport(id: ts.Identifier): RequireCall|null {
@@ -173,19 +195,6 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
     return nsIdentifier && findRequireCallReference(nsIdentifier, this.checker);
   }
 
-  private extractCommonJsExportDeclaration(name: string, expression: ts.Expression):
-      ExportDeclaration {
-    const declaration = this.getDeclarationOfExpression(expression);
-    if (declaration !== null) {
-      return {name, declaration};
-    } else {
-      return {
-        name,
-        declaration: {node: expression, known: null, kind: DeclarationKind.Inline, viaModule: null},
-      };
-    }
-  }
-
   /**
    * Handle the case where the identifier represents a reference to a whole CommonJS
    * module, i.e. the result of a call to `require(...)`.
diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts
@@ -264,3 +264,20 @@ export interface ExportsStatement extends ts.ExpressionStatement {
 export function isExportsStatement(stmt: ts.Node): stmt is ExportsStatement {
   return ts.isExpressionStatement(stmt) && isExportsAssignment(stmt.expression);
 }
+
+/**
+ * Find the far right hand side of a sequence of aliased assignements of the form
+ *
+ * ```
+ * exports.MyClass = alias1 = alias2 = <<declaration>>
+ * ```
+ *
+ * @param node the expression to parse
+ * @returns the original `node` or the far right expression of a series of assignments.
+ */
+export function skipAliases(node: ts.Expression): ts.Expression {
+  while (isAssignment(node)) {
+    node = node.right;
+  }
+  return node;
+}
diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts
@@ -14,7 +14,7 @@ import {Declaration, DeclarationKind, Import, isNamedFunctionDeclaration} from '
 import {BundleProgram} from '../packages/bundle_program';
 import {FactoryMap, getTsHelperFnFromIdentifier, stripExtension} from '../utils';
 
-import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsAssignment, isExportsDeclaration, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, WildcardReexportStatement} from './commonjs_umd_utils';
+import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsAssignment, isExportsDeclaration, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils';
 import {getInnerClassDeclaration, getOuterNodeFromInnerDeclaration, isAssignment} from './esm2015_host';
 import {Esm5ReflectionHost} from './esm5_host';
 import {NgccClassSymbol} from './ngcc_host';
@@ -77,6 +77,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
     return {
       kind: DeclarationKind.Inline,
       node: outerNode.left,
+      implementation: outerNode.right,
       known: null,
       viaModule: null,
     };
@@ -278,6 +279,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
     const declaration = this.getDeclarationOfExpression(exportExpression) ?? {
       kind: DeclarationKind.Inline,
       node: statement.expression.left,
+      implementation: statement.expression.right,
       known: null,
       viaModule: null,
     };
@@ -340,7 +342,8 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
       name,
       declaration: {
         kind: DeclarationKind.Inline,
-        node: getterFnExpression,
+        node: args[1],
+        implementation: getterFnExpression,
         known: null,
         viaModule: null,
       },
@@ -564,20 +567,3 @@ function getRequiredModulePath(wrapperFn: ts.FunctionExpression, paramIndex: num
 function isExportsIdentifier(node: ts.Node): node is ts.Identifier {
   return ts.isIdentifier(node) && node.text === 'exports';
 }
-
-/**
- * Find the far right hand side of a sequence of aliased assignements of the form
- *
- * ```
- * exports.MyClass = alias1 = alias2 = <<declaration>>
- * ```
- *
- * @param node the expression to parse
- * @returns the original `node` or the far right expression of a series of assignments.
- */
-function skipAliases(node: ts.Expression): ts.Expression {
-  while (isAssignment(node)) {
-    node = node.right;
-  }
-  return node;
-}
diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
@@ -2422,9 +2422,10 @@ exports.MissingClass2 = MissingClass2;
           const file = getSourceFileOrError(bundle.program, _('/inline_export.js'));
           const exportDeclarations = host.getExportsOfModule(file);
           expect(exportDeclarations).not.toBeNull();
-          const decl = exportDeclarations!.get('directives')!;
+          const decl = exportDeclarations!.get('directives') as InlineDeclaration;
           expect(decl).toBeDefined();
-          expect(decl.node).toBeDefined();
+          expect(decl.node.getText()).toEqual('exports.directives');
+          expect(decl.implementation!.getText()).toEqual('[foo]');
           expect(decl.kind).toEqual(DeclarationKind.Inline);
         });
 
diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
@@ -2750,13 +2750,13 @@ runInEachFileSystem(() => {
           const exportDeclarations = host.getExportsOfModule(file);
           expect(exportDeclarations).not.toBe(null);
           expect(exportDeclarations!.size).toEqual(1);
-          const classDecl = exportDeclarations!.get('DecoratedClass')!;
+          const classDecl = exportDeclarations!.get('DecoratedClass') as InlineDeclaration;
           expect(classDecl).toBeDefined();
           expect(classDecl.kind).toEqual(DeclarationKind.Inline);
           expect(classDecl.known).toBe(null);
           expect(classDecl.viaModule).toBe(null);
           expect(classDecl.node.getText()).toEqual('exports.DecoratedClass');
-          expect(classDecl.node.parent.parent.getText()).toContain('function DecoratedClass() {');
+          expect(classDecl.implementation!.getText()).toContain('function DecoratedClass() {');
         });
 
         it('should handle wildcard re-exports of other modules (with emitted helpers)', () => {
@@ -2824,9 +2824,10 @@ runInEachFileSystem(() => {
           const file = getSourceFileOrError(bundle.program, INLINE_EXPORT_FILE.name);
           const exportDeclarations = host.getExportsOfModule(file);
           expect(exportDeclarations).not.toBe(null);
-          const decl = exportDeclarations!.get('directives')!;
+          const decl = exportDeclarations!.get('directives') as InlineDeclaration;
           expect(decl).toBeDefined();
-          expect(decl.node).toBeDefined();
+          expect(decl.node.getText()).toEqual('exports.directives');
+          expect(decl.implementation!.getText()).toEqual('[foo]');
           expect(decl.kind).toEqual(DeclarationKind.Inline);
         });
 
