From c685cc2f0a1e9a382051eceb4632504972dc95ab Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Wed, 23 Aug 2017 10:22:17 -0700 Subject: [PATCH] feat(compiler-cli): lower metadata `useValue` and `data` literal fields (#18905) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this commit the compiler will "lower" expressions into exported variables for values the compiler does not need to know statically in order to be able to generate a factory. For example: ``` providers: [{provider: 'token', useValue: calculated()}] ``` produced an error as the expression `calculated()` is not supported by the compiler because `calculated` is not a [known function](https://angular.io/guide/metadata#annotationsdecorators) With this commit this is rewritten, during emit of the .js file, into something like: ``` export var ɵ0 = calculated(); ... provdiers: [{provider: 'token', useValue: ɵ0}] ``` The compiler then will now generate a reference to the exported `ɵ0` instead of failing to evaluate `calculated()`. PR Close #18905 --- .../src/diagnostics/check_types.ts | 8 +- .../src/transformers/lower_expressions.ts | 100 +++++++++++++++-- .../test/diagnostics/check_types_spec.ts | 41 ++++++- packages/compiler-cli/test/ngc_spec.ts | 74 +++++++++++++ .../transformers/lower_expressions_spec.ts | 103 ++++++++++++++++-- packages/compiler/src/output/ts_emitter.ts | 13 ++- packages/compiler/test/aot/test_util.ts | 9 +- packages/tsc-wrapped/src/evaluator.ts | 4 +- 8 files changed, 324 insertions(+), 28 deletions(-) diff --git a/packages/compiler-cli/src/diagnostics/check_types.ts b/packages/compiler-cli/src/diagnostics/check_types.ts index 48584a8493..f71fd85ea3 100644 --- a/packages/compiler-cli/src/diagnostics/check_types.ts +++ b/packages/compiler-cli/src/diagnostics/check_types.ts @@ -166,9 +166,13 @@ function diagnosticMessageToString(message: ts.DiagnosticMessageChain | string): return ts.flattenDiagnosticMessageText(message, '\n'); } +const REWRITE_PREFIX = /^\u0275[0-9]+$/; + function createFactoryInfo(emitter: TypeScriptEmitter, file: GeneratedFile): FactoryInfo { - const {sourceText, context} = - emitter.emitStatementsAndContext(file.srcFileUrl, file.genFileUrl, file.stmts !); + const {sourceText, context} = emitter.emitStatementsAndContext( + file.srcFileUrl, file.genFileUrl, file.stmts !, + /* preamble */ undefined, /* emitSourceMaps */ undefined, + /* referenceFilter */ reference => !!(reference.name && REWRITE_PREFIX.test(reference.name))); const source = ts.createSourceFile( file.genFileUrl, sourceText, ts.ScriptTarget.Latest, /* setParentNodes */ true); return {source, context}; diff --git a/packages/compiler-cli/src/transformers/lower_expressions.ts b/packages/compiler-cli/src/transformers/lower_expressions.ts index 1a2edb825e..a6a1e5e47d 100644 --- a/packages/compiler-cli/src/transformers/lower_expressions.ts +++ b/packages/compiler-cli/src/transformers/lower_expressions.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {CollectorOptions, MetadataCollector, MetadataValue, ModuleMetadata} from '@angular/tsc-wrapped'; +import {CollectorOptions, MetadataCollector, MetadataValue, ModuleMetadata, isMetadataGlobalReferenceExpression} from '@angular/tsc-wrapped'; import * as ts from 'typescript'; export interface LoweringRequest { @@ -181,6 +181,30 @@ function shouldLower(node: ts.Node | undefined): boolean { return true; } +const REWRITE_PREFIX = '\u0275'; + +function isPrimitive(value: any): boolean { + return Object(value) !== value; +} + +function isRewritten(value: any): boolean { + return isMetadataGlobalReferenceExpression(value) && value.name.startsWith(REWRITE_PREFIX); +} + +function isLiteralFieldNamed(node: ts.Node, names: Set): boolean { + if (node.parent && node.parent.kind == ts.SyntaxKind.PropertyAssignment) { + const property = node.parent as ts.PropertyAssignment; + if (property.parent && property.parent.kind == ts.SyntaxKind.ObjectLiteralExpression && + property.name && property.name.kind == ts.SyntaxKind.Identifier) { + const propertyName = property.name as ts.Identifier; + return names.has(propertyName.text); + } + } + return false; +} + +const LOWERABLE_FIELD_NAMES = new Set(['useValue', 'useFactory', 'data']); + export class LowerMetadataCache implements RequestsMap { private collector: MetadataCollector; private metadataCache = new Map(); @@ -208,8 +232,24 @@ export class LowerMetadataCache implements RequestsMap { private getMetadataAndRequests(sourceFile: ts.SourceFile): MetadataAndLoweringRequests { let identNumber = 0; - const freshIdent = () => '\u0275' + identNumber++; + const freshIdent = () => REWRITE_PREFIX + identNumber++; const requests = new Map(); + + const isExportedSymbol = (() => { + let exportTable: Set; + return (node: ts.Node) => { + if (node.kind == ts.SyntaxKind.Identifier) { + const ident = node as ts.Identifier; + + if (!exportTable) { + exportTable = createExportTableFor(sourceFile); + } + return exportTable.has(ident.text); + } + return false; + }; + })(); + const replaceNode = (node: ts.Node) => { const name = freshIdent(); requests.set(node.pos, {name, kind: node.kind, location: node.pos, end: node.end}); @@ -217,10 +257,16 @@ export class LowerMetadataCache implements RequestsMap { }; const substituteExpression = (value: MetadataValue, node: ts.Node): MetadataValue => { - if ((node.kind === ts.SyntaxKind.ArrowFunction || - node.kind === ts.SyntaxKind.FunctionExpression) && - shouldLower(node)) { - return replaceNode(node); + if (!isPrimitive(value) && !isRewritten(value)) { + if ((node.kind === ts.SyntaxKind.ArrowFunction || + node.kind === ts.SyntaxKind.FunctionExpression) && + shouldLower(node)) { + return replaceNode(node); + } + if (isLiteralFieldNamed(node, LOWERABLE_FIELD_NAMES) && shouldLower(node) && + !isExportedSymbol(node)) { + return replaceNode(node); + } } return value; }; @@ -229,4 +275,44 @@ export class LowerMetadataCache implements RequestsMap { return {metadata, requests}; } -} \ No newline at end of file +} + +function createExportTableFor(sourceFile: ts.SourceFile): Set { + const exportTable = new Set(); + // Lazily collect all the exports from the source file + ts.forEachChild(sourceFile, function scan(node) { + switch (node.kind) { + case ts.SyntaxKind.ClassDeclaration: + case ts.SyntaxKind.FunctionDeclaration: + case ts.SyntaxKind.InterfaceDeclaration: + if ((ts.getCombinedModifierFlags(node) & ts.ModifierFlags.Export) != 0) { + const classDeclaration = + node as(ts.ClassDeclaration | ts.FunctionDeclaration | ts.InterfaceDeclaration); + const name = classDeclaration.name; + if (name) exportTable.add(name.text); + } + break; + case ts.SyntaxKind.VariableStatement: + const variableStatement = node as ts.VariableStatement; + for (const declaration of variableStatement.declarationList.declarations) { + scan(declaration); + } + break; + case ts.SyntaxKind.VariableDeclaration: + const variableDeclaration = node as ts.VariableDeclaration; + if ((ts.getCombinedModifierFlags(node) & ts.ModifierFlags.Export) != 0 && + variableDeclaration.name.kind == ts.SyntaxKind.Identifier) { + const name = variableDeclaration.name as ts.Identifier; + exportTable.add(name.text); + } + break; + case ts.SyntaxKind.ExportDeclaration: + const exportDeclaration = node as ts.ExportDeclaration; + const {moduleSpecifier, exportClause} = exportDeclaration; + if (!moduleSpecifier && exportClause) { + exportClause.elements.forEach(spec => { exportTable.add(spec.name.text); }); + } + } + }); + return exportTable; +} diff --git a/packages/compiler-cli/test/diagnostics/check_types_spec.ts b/packages/compiler-cli/test/diagnostics/check_types_spec.ts index ec24d60ebf..9f0b16fcf0 100644 --- a/packages/compiler-cli/test/diagnostics/check_types_spec.ts +++ b/packages/compiler-cli/test/diagnostics/check_types_spec.ts @@ -12,6 +12,7 @@ import * as ts from 'typescript'; import {TypeChecker} from '../../src/diagnostics/check_types'; import {Diagnostic} from '../../src/transformers/api'; +import {LowerMetadataCache} from '../../src/transformers/lower_expressions'; function compile( rootDirs: MockData, options: AotCompilerOptions = {}, @@ -19,7 +20,7 @@ function compile( const rootDirArr = toMockFileArray(rootDirs); const scriptNames = rootDirArr.map(entry => entry.fileName).filter(isSource); const host = new MockCompilerHost(scriptNames, arrayToMockDir(rootDirArr)); - const aotHost = new MockAotCompilerHost(host); + const aotHost = new MockAotCompilerHost(host, new LowerMetadataCache({})); const tsSettings = {...settings, ...tsOptions}; const program = ts.createProgram(host.scriptNames.slice(0), tsSettings, host); const ngChecker = new TypeChecker(program, tsSettings, host, aotHost, options); @@ -80,6 +81,12 @@ describe('ng type checker', () => { it('should accept a safe property access of a nullable field reference of a method result', () => { a('{{getMaybePerson()?.name}}'); }); }); + + describe('with lowered expressions', () => { + it('should not report lowered expressions as errors', () => { + expectNoDiagnostics(compile([angularFiles, LOWERING_QUICKSTART])); + }); + }); }); function appComponentSource(template: string): string { @@ -134,8 +141,38 @@ const QUICKSTART: MockDirectory = { } }; +const LOWERING_QUICKSTART: MockDirectory = { + quickstart: { + app: { + 'app.component.ts': appComponentSource('

Hello {{name}}

'), + 'app.module.ts': ` + import { NgModule, Component } from '@angular/core'; + import { toString } from './utils'; + + import { AppComponent } from './app.component'; + + class Foo {} + + @Component({ + template: '', + providers: [ + {provide: 'someToken', useFactory: () => new Foo()} + ] + }) + export class Bar {} + + @NgModule({ + declarations: [ AppComponent, Bar ], + bootstrap: [ AppComponent ] + }) + export class AppModule { } + ` + } + } +}; + function expectNoDiagnostics(diagnostics: Diagnostic[]) { if (diagnostics && diagnostics.length) { throw new Error(diagnostics.map(d => `${d.span}: ${d.message}`).join('\n')); } -} \ No newline at end of file +} diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index 8b3fe0629a..13b616c291 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -848,4 +848,78 @@ describe('ngc transformer command-line', () => { shouldExist('app/main.js'); }); }); + + describe('expression lowering', () => { + const shouldExist = (fileName: string) => { + if (!fs.existsSync(path.resolve(basePath, fileName))) { + throw new Error(`Expected ${fileName} to be emitted (basePath: ${basePath})`); + } + }; + + it('should be able to lower supported expressions', () => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "files": ["module.ts"] + }`); + write('module.ts', ` + import {NgModule, InjectionToken} from '@angular/core'; + import {AppComponent} from './app'; + + export interface Info { + route: string; + data: string; + } + + export const T1 = new InjectionToken('t1'); + export const T2 = new InjectionToken('t2'); + export const T3 = new InjectionToken('t3'); + export const T4 = new InjectionToken('t4'); + + enum SomeEnum { + OK, + Cancel + } + + function calculateString() { + return 'someValue'; + } + + const routeLikeData = [{ + route: '/home', + data: calculateString() + }]; + + @NgModule({ + declarations: [AppComponent], + providers: [ + { provide: T1, useValue: calculateString() }, + { provide: T2, useFactory: () => 'someValue' }, + { provide: T3, useValue: SomeEnum.OK }, + { provide: T4, useValue: routeLikeData } + ] + }) + export class MyModule {} + `); + write('app.ts', ` + import {Component, Inject} from '@angular/core'; + import * as m from './module'; + + @Component({ + selector: 'my-app', + template: '' + }) + export class AppComponent { + constructor( + @Inject(m.T1) private t1: string, + @Inject(m.T2) private t2: string, + @Inject(m.T3) private t3: number, + @Inject(m.T4) private t4: m.Info[], + ) {} + } + `); + + expect(mainSync(['-p', basePath], s => {})).toBe(0); + shouldExist('built/module.js'); + }); + }); }); diff --git a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts index 6b923ed671..fdb5c92c18 100644 --- a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts +++ b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts @@ -6,29 +6,92 @@ * found in the LICENSE file at https://angular.io/license */ +import {ModuleMetadata} from '@angular/tsc-wrapped'; import * as ts from 'typescript'; -import {LoweringRequest, RequestLocationMap, getExpressionLoweringTransformFactory} from '../../src/transformers/lower_expressions'; +import {LowerMetadataCache, LoweringRequest, RequestLocationMap, getExpressionLoweringTransformFactory} from '../../src/transformers/lower_expressions'; import {Directory, MockAotContext, MockCompilerHost} from '../mocks'; describe('Expression lowering', () => { - it('should be able to lower a simple expression', () => { - expect(convert('const a = 1 +◊b: 2◊;')).toBe('const b = 2; const a = 1 + b; export { b };'); + describe('transform', () => { + it('should be able to lower a simple expression', () => { + expect(convert('const a = 1 +◊b: 2◊;')).toBe('const b = 2; const a = 1 + b; export { b };'); + }); + + it('should be able to lower an expression in a decorator', () => { + expect(convert(` + import {Component} from '@angular/core'; + + @Component({ + provider: [{provide: 'someToken', useFactory:◊l: () => null◊}] + }) + class MyClass {} + `)).toContain('const l = () => null; exports.l = l;'); + }); }); - it('should be able to lower an expression in a decorator', () => { - expect(convert(` + describe('collector', () => { + it('should request a lowering for useValue', () => { + const collected = collect(` import {Component} from '@angular/core'; - + + enum SomeEnum { + OK, + NotOK + } + @Component({ - provider: [{provide: 'someToken', useFactory:◊l: () => null◊}] + provider: [{provide: 'someToken', useValue:◊enum: SomeEnum.OK◊}] }) - class MyClass {} - `)).toContain('const l = () => null; exports.l = l;'); + export class MyClass {} + `); + expect(collected.requests.has(collected.annotations[0].start)) + .toBeTruthy('did not find the useValue'); + }); + + it('should request a lowering for useFactory', () => { + const collected = collect(` + import {Component} from '@angular/core'; + + @Component({ + provider: [{provide: 'someToken', useFactory:◊lambda: () => null◊}] + }) + export class MyClass {} + `); + expect(collected.requests.has(collected.annotations[0].start)) + .toBeTruthy('did not find the useFactory'); + }); + + it('should request a lowering for data', () => { + const collected = collect(` + import {Component} from '@angular/core'; + + enum SomeEnum { + OK, + NotOK + } + + @Component({ + provider: [{provide: 'someToken', data:◊enum: SomeEnum.OK◊}] + }) + export class MyClass {} + `); + expect(collected.requests.has(collected.annotations[0].start)) + .toBeTruthy('did not find the data field'); + }); }); }); -function convert(annotatedSource: string) { +// Helpers + +interface Annotation { + start: number; + length: number; + name: string; +} + +function getAnnotations(annotatedSource: string): + {unannotatedSource: string, annotations: Annotation[]} { const annotations: {start: number, length: number, name: string}[] = []; let adjustment = 0; const unannotatedSource = annotatedSource.replace( @@ -38,6 +101,13 @@ function convert(annotatedSource: string) { adjustment -= text.length - source.length; return source; }); + return {unannotatedSource, annotations}; +} + +// Transform helpers + +function convert(annotatedSource: string) { + const {annotations, unannotatedSource} = getAnnotations(annotatedSource); const baseFileName = 'someFile'; const moduleName = '/' + baseFileName; @@ -103,3 +173,16 @@ function normalizeResult(result: string): string { .replace(/^ /g, '') .replace(/ $/g, ''); } + +// Collector helpers + +function collect(annotatedSource: string) { + const {annotations, unannotatedSource} = getAnnotations(annotatedSource); + const cache = new LowerMetadataCache({}); + const sourceFile = ts.createSourceFile( + 'someName.ts', unannotatedSource, ts.ScriptTarget.Latest, /* setParentNodes */ true); + return { + metadata: cache.getMetadata(sourceFile), + requests: cache.getRequests(sourceFile), annotations + }; +} \ No newline at end of file diff --git a/packages/compiler/src/output/ts_emitter.ts b/packages/compiler/src/output/ts_emitter.ts index 74f2b8410f..86ca3d0017 100644 --- a/packages/compiler/src/output/ts_emitter.ts +++ b/packages/compiler/src/output/ts_emitter.ts @@ -35,12 +35,14 @@ export function debugOutputAstAsTypeScript(ast: o.Statement | o.Expression | o.T return ctx.toSource(); } +export type ReferenceFilter = (reference: o.ExternalReference) => boolean; export class TypeScriptEmitter implements OutputEmitter { emitStatementsAndContext( srcFilePath: string, genFilePath: string, stmts: o.Statement[], preamble: string = '', - emitSourceMaps: boolean = true): {sourceText: string, context: EmitterVisitorContext} { - const converter = new _TsEmitterVisitor(); + emitSourceMaps: boolean = true, + referenceFilter?: ReferenceFilter): {sourceText: string, context: EmitterVisitorContext} { + const converter = new _TsEmitterVisitor(referenceFilter); const ctx = EmitterVisitorContext.createRoot(); @@ -78,10 +80,11 @@ export class TypeScriptEmitter implements OutputEmitter { } } + class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor { private typeExpression = 0; - constructor() { super(false); } + constructor(private referenceFilter?: ReferenceFilter) { super(false); } importsWithPrefixes = new Map(); reexports = new Map(); @@ -377,6 +380,10 @@ class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor private _visitIdentifier( value: o.ExternalReference, typeParams: o.Type[]|null, ctx: EmitterVisitorContext): void { const {name, moduleName} = value; + if (this.referenceFilter && this.referenceFilter(value)) { + ctx.print(null, '(null as any)'); + return; + } if (moduleName) { let prefix = this.importsWithPrefixes.get(moduleName); if (prefix == null) { diff --git a/packages/compiler/test/aot/test_util.ts b/packages/compiler/test/aot/test_util.ts index f38b2ed897..d9a0a18847 100644 --- a/packages/compiler/test/aot/test_util.ts +++ b/packages/compiler/test/aot/test_util.ts @@ -12,6 +12,8 @@ import * as fs from 'fs'; import * as path from 'path'; import * as ts from 'typescript'; +export interface MetadataProvider { getMetadata(source: ts.SourceFile): ModuleMetadata|undefined; } + let nodeModulesPath: string; let angularSourcePath: string; let rootPath: string; @@ -327,12 +329,13 @@ const DTS = /\.d\.ts$/; const GENERATED_FILES = /\.ngfactory\.ts$|\.ngstyle\.ts$/; export class MockAotCompilerHost implements AotCompilerHost { - private metadataCollector = new MetadataCollector(); private metadataVisible: boolean = true; private dtsAreSource: boolean = true; private resolveModuleNameHost: ts.ModuleResolutionHost; - constructor(private tsHost: MockCompilerHost) { + constructor( + private tsHost: MockCompilerHost, + private metadataProvider: MetadataProvider = new MetadataCollector()) { this.resolveModuleNameHost = Object.create(tsHost); this.resolveModuleNameHost.fileExists = (fileName) => { fileName = stripNgResourceSuffix(fileName); @@ -359,7 +362,7 @@ export class MockAotCompilerHost implements AotCompilerHost { } } else { const sf = this.tsHost.getSourceFile(modulePath, ts.ScriptTarget.Latest); - const metadata = this.metadataCollector.getMetadata(sf); + const metadata = this.metadataProvider.getMetadata(sf); return metadata ? [metadata] : []; } return undefined; diff --git a/packages/tsc-wrapped/src/evaluator.ts b/packages/tsc-wrapped/src/evaluator.ts index 27e5afc9af..db02169a2b 100644 --- a/packages/tsc-wrapped/src/evaluator.ts +++ b/packages/tsc-wrapped/src/evaluator.ts @@ -284,7 +284,9 @@ export class Evaluator { error = propertyValue; return true; // Stop the forEachChild. } else { - obj[propertyName] = propertyValue; + obj[propertyName] = isPropertyAssignment(assignment) ? + recordEntry(propertyValue, assignment.initializer) : + propertyValue; } } });