fix(compiler-cli): fix and re-enble expression lowering (#18570)

Fixes issue uncovered by #18388 and re-enables expression
lowering disabled by #18513.

packages/compiler-cli/src/transformers/lower_expressions.ts

@@ -32,6 +32,24 @@ function toMap<T, K>(items: T[], select: (item: T) => K): Map<K, T> {
   return new Map(items.map<[K, T]>(i => [select(i), i]));
 }
 
+// We will never lower expressions in a nested lexical scope so avoid entering them.
+// This also avoids a bug in TypeScript 2.3 where the lexical scopes get out of sync
+// when using visitEachChild.
+function isLexicalScope(node: ts.Node): boolean {
+  switch (node.kind) {
+    case ts.SyntaxKind.ArrowFunction:
+    case ts.SyntaxKind.FunctionExpression:
+    case ts.SyntaxKind.FunctionDeclaration:
+    case ts.SyntaxKind.ClassExpression:
+    case ts.SyntaxKind.ClassDeclaration:
+    case ts.SyntaxKind.FunctionType:
+    case ts.SyntaxKind.TypeLiteral:
+    case ts.SyntaxKind.ArrayType:
+      return true;
+  }
+  return false;
+}
+
 function transformSourceFile(
     sourceFile: ts.SourceFile, requests: RequestLocationMap,
     context: ts.TransformationContext): ts.SourceFile {
@@ -56,11 +74,15 @@ function transformSourceFile(
           declarations.push({name, node});
           return ts.createIdentifier(name);
         }
-        if (node.pos <= max && node.end >= min) return ts.visitEachChild(node, visitNode, context);
+        let result = node;
-        return node;
+        if (node.pos <= max && node.end >= min && !isLexicalScope(node)) {
+          result = ts.visitEachChild(node, visitNode, context);
+        }
+        return result;
       }
 
-      const result = ts.visitEachChild(node, visitNode, context);
+      const result =
+          (node.pos <= max && node.end >= min) ? ts.visitEachChild(node, visitNode, context) : node;
 
       if (declarations.length) {
         inserts.push({priorTo: result, declarations});
@@ -126,6 +148,29 @@ interface MetadataAndLoweringRequests {
   requests: RequestLocationMap;
 }
 
+function shouldLower(node: ts.Node | undefined): boolean {
+  if (node) {
+    switch (node.kind) {
+      case ts.SyntaxKind.SourceFile:
+      case ts.SyntaxKind.Decorator:
+        // Lower expressions that are local to the module scope or
+        // in a decorator.
+        return true;
+      case ts.SyntaxKind.ClassDeclaration:
+      case ts.SyntaxKind.InterfaceDeclaration:
+      case ts.SyntaxKind.EnumDeclaration:
+      case ts.SyntaxKind.FunctionDeclaration:
+        // Don't lower expressions in a declaration.
+        return false;
+      case ts.SyntaxKind.VariableDeclaration:
+        // Avoid lowering expressions already in an exported variable declaration
+        return (ts.getCombinedModifierFlags(node) & ts.ModifierFlags.Export) == 0;
+    }
+    return shouldLower(node.parent);
+  }
+  return true;
+}
+
 export class LowerMetadataCache implements RequestsMap {
   private collector: MetadataCollector;
   private metadataCache = new Map<string, MetadataAndLoweringRequests>();
@@ -162,8 +207,9 @@ export class LowerMetadataCache implements RequestsMap {
     };
 
     const substituteExpression = (value: MetadataValue, node: ts.Node): MetadataValue => {
-      if (node.kind === ts.SyntaxKind.ArrowFunction ||
+      if ((node.kind === ts.SyntaxKind.ArrowFunction ||
-          node.kind === ts.SyntaxKind.FunctionExpression) {
+           node.kind === ts.SyntaxKind.FunctionExpression) &&
+          shouldLower(node)) {
         return replaceNode(node);
       }
       return value;

packages/compiler-cli/src/transformers/program.ts

@@ -187,8 +187,7 @@ class AngularCompilerProgram implements Program {
     const before: ts.TransformerFactory<ts.SourceFile>[] = [];
     const after: ts.TransformerFactory<ts.SourceFile>[] = [];
     if (!this.options.disableExpressionLowering) {
-      // TODO(chuckj): fix and re-enable + tests - see https://github.com/angular/angular/pull/18388
+      before.push(getExpressionLoweringTransformFactory(this.metadataCache));
-      // before.push(getExpressionLoweringTransformFactory(this.metadataCache));
     }
     if (!this.options.skipTemplateCodegen) {
       after.push(getAngularEmitterTransformFactory(this.generatedFiles));

packages/compiler-cli/test/ngc_spec.ts

@@ -12,7 +12,7 @@ import * as path from 'path';
 import * as ts from 'typescript';
 
 import {main} from '../src/ngc';
-import {performCompilation} from '../src/perform-compile';
+import {performCompilation, readConfiguration} from '../src/perform-compile';
 
 function getNgRootDir() {
   const moduleFilename = module.filename.replace(/\\/g, '/');
@@ -316,7 +316,7 @@ describe('ngc command-line', () => {
           .toBe(true);
     });
 
-    xdescribe('expression lowering', () => {
+    describe('expression lowering', () => {
       beforeEach(() => {
         writeConfig(`{
             "extends": "./tsconfig-base.json",
@@ -424,13 +424,35 @@ describe('ngc command-line', () => {
           })
           export class MyModule {}
         `);
-        expect(compile()).toEqual(0);
+        expect(compile()).toEqual(0, 'Compile failed');
 
         const mymodulejs = path.resolve(outDir, 'mymodule.js');
         const mymoduleSource = fs.readFileSync(mymodulejs, 'utf8');
         expect(mymoduleSource).toContain('var ɵ0 = function () { return new Foo(); }');
         expect(mymoduleSource).toContain('export { ɵ0');
       });
+
+      it('should not lower a lambda that is already exported', () => {
+        write('mymodule.ts', `
+          import {CommonModule} from '@angular/common';
+          import {NgModule} from '@angular/core';
+
+          class Foo {}
+
+          export const factory = () => new Foo();
+
+          @NgModule({
+            imports: [CommonModule],
+            providers: [{provide: 'someToken', useFactory: factory}]
+          })
+          export class MyModule {}
+        `);
+        expect(compile()).toEqual(0);
+
+        const mymodulejs = path.resolve(outDir, 'mymodule.js');
+        const mymoduleSource = fs.readFileSync(mymodulejs, 'utf8');
+        expect(mymoduleSource).not.toContain('ɵ0');
+      });
     });
 
     const shouldExist = (fileName: string) => {

packages/compiler-cli/test/transformers/lower_expressions_spec.ts

@@ -51,12 +51,10 @@ function convert(annotatedSource: string) {
 
   for (const annotation of annotations) {
     const node = findNode(sourceFile, annotation.start, annotation.length);
-    expect(node).toBeDefined();
+    if (!node) throw new Error('Invalid test specification. Could not find the node to substitute');
-    if (node) {
     const location = node.pos;
     requests.set(location, {name: annotation.name, kind: node.kind, location, end: node.end});
   }
-  }
 
   const program = ts.createProgram(
       [fileName], {module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2017}, host);