From 6f2038cc852fb31e5cdbab2f15cd3ab80bf3dac7 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Tue, 8 Aug 2017 12:40:08 -0700 Subject: [PATCH] fix(compiler-cli): fix and re-enble expression lowering (#18570) Fixes issue uncovered by #18388 and re-enables expression lowering disabled by #18513. --- .../src/transformers/lower_expressions.ts | 56 +++++++++++++++++-- .../compiler-cli/src/transformers/program.ts | 3 +- packages/compiler-cli/test/ngc_spec.ts | 28 +++++++++- .../transformers/lower_expressions_spec.ts | 8 +-- 4 files changed, 80 insertions(+), 15 deletions(-) diff --git a/packages/compiler-cli/src/transformers/lower_expressions.ts b/packages/compiler-cli/src/transformers/lower_expressions.ts index 286ce3b4fa..65c4cd3aff 100644 --- a/packages/compiler-cli/src/transformers/lower_expressions.ts +++ b/packages/compiler-cli/src/transformers/lower_expressions.ts @@ -32,6 +32,24 @@ function toMap(items: T[], select: (item: T) => K): Map { 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); - return node; + let result = 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(); @@ -162,8 +207,9 @@ export class LowerMetadataCache implements RequestsMap { }; const substituteExpression = (value: MetadataValue, node: ts.Node): MetadataValue => { - if (node.kind === ts.SyntaxKind.ArrowFunction || - node.kind === ts.SyntaxKind.FunctionExpression) { + if ((node.kind === ts.SyntaxKind.ArrowFunction || + node.kind === ts.SyntaxKind.FunctionExpression) && + shouldLower(node)) { return replaceNode(node); } return value; diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index b2753555fa..3970aeb015 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -187,8 +187,7 @@ class AngularCompilerProgram implements Program { const before: ts.TransformerFactory[] = []; const after: ts.TransformerFactory[] = []; 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)); diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index 0a994cf6e8..53320cc186 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/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) => { diff --git a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts index 7588982b03..6b923ed671 100644 --- a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts +++ b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts @@ -51,11 +51,9 @@ function convert(annotatedSource: string) { for (const annotation of annotations) { const node = findNode(sourceFile, annotation.start, annotation.length); - expect(node).toBeDefined(); - if (node) { - const location = node.pos; - requests.set(location, {name: annotation.name, kind: node.kind, location, end: node.end}); - } + if (!node) throw new Error('Invalid test specification. Could not find the node to substitute'); + const location = node.pos; + requests.set(location, {name: annotation.name, kind: node.kind, location, end: node.end}); } const program = ts.createProgram(