From fba276d3d1c21b4c252335f5361b9d15cf534f0a Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 27 Jun 2018 08:41:11 -0700 Subject: [PATCH] fix(ivy): use a single constant pool per source file (#25392) Previously, ngtsc used a new ConstantPool for each decorator compilation. This could result in collisions between constants in the top-level scope. Now, ngtsc uses a single ConstantPool for each source file being compiled, and merges the constant statements into the file after the import section. PR Close #25392 --- .../compiler-cli/src/ngcc/src/analyzer.ts | 14 +++++--- .../ngcc/src/rendering/esm2015_renderer.ts | 14 ++++++++ .../src/ngcc/src/rendering/renderer.ts | 19 +++++++++- .../test/rendering/esm2015_renderer_spec.ts | 30 ++++++++++++++++ .../src/ngcc/test/rendering/renderer_spec.ts | 10 ++++-- .../src/ngtsc/annotations/src/component.ts | 7 ++-- .../src/ngtsc/transform/src/api.ts | 6 ++-- .../src/ngtsc/transform/src/compilation.ts | 5 +-- .../src/ngtsc/transform/src/transform.ts | 35 ++++++++++++++----- 9 files changed, 115 insertions(+), 25 deletions(-) diff --git a/packages/compiler-cli/src/ngcc/src/analyzer.ts b/packages/compiler-cli/src/ngcc/src/analyzer.ts index 4dca46ae67..ed7b678efb 100644 --- a/packages/compiler-cli/src/ngcc/src/analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analyzer.ts @@ -5,11 +5,14 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +import {ConstantPool} from '@angular/compiler'; import * as fs from 'fs'; import * as ts from 'typescript'; + import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ResourceLoader, SelectorScopeRegistry} from '../../ngtsc/annotations'; import {Decorator} from '../../ngtsc/host'; import {CompileResult, DecoratorHandler} from '../../ngtsc/transform'; + import {NgccReflectionHost} from './host/ngcc_host'; import {ParsedClass} from './parsing/parsed_class'; import {ParsedFile} from './parsing/parsed_file'; @@ -25,6 +28,7 @@ export interface AnalyzedClass extends ParsedClass { export interface AnalyzedFile { analyzedClasses: AnalyzedClass[]; sourceFile: ts.SourceFile; + constantPool: ConstantPool; } export interface MatchingHandler { @@ -59,17 +63,19 @@ export class Analyzer { * @param file The file to be analysed for decorated classes. */ analyzeFile(file: ParsedFile): AnalyzedFile { + const constantPool = new ConstantPool(); const analyzedClasses = - file.decoratedClasses.map(clazz => this.analyzeClass(file.sourceFile, clazz)) + file.decoratedClasses.map(clazz => this.analyzeClass(file.sourceFile, constantPool, clazz)) .filter(isDefined); return { analyzedClasses, - sourceFile: file.sourceFile, + sourceFile: file.sourceFile, constantPool, }; } - protected analyzeClass(file: ts.SourceFile, clazz: ParsedClass): AnalyzedClass|undefined { + protected analyzeClass(file: ts.SourceFile, pool: ConstantPool, clazz: ParsedClass): AnalyzedClass + |undefined { const matchingHandlers = this.handlers.map(handler => ({handler, decorator: handler.detect(clazz.decorators)})) .filter(isMatchingHandler); @@ -84,7 +90,7 @@ export class Analyzer { const {handler, decorator} = matchingHandlers[0]; const {analysis, diagnostics} = handler.analyze(clazz.declaration, decorator); - let compilation = handler.compile(clazz.declaration, analysis); + let compilation = handler.compile(clazz.declaration, analysis, pool); if (!Array.isArray(compilation)) { compilation = [compilation]; } diff --git a/packages/compiler-cli/src/ngcc/src/rendering/esm2015_renderer.ts b/packages/compiler-cli/src/ngcc/src/rendering/esm2015_renderer.ts index a841b0c616..10db8b292e 100644 --- a/packages/compiler-cli/src/ngcc/src/rendering/esm2015_renderer.ts +++ b/packages/compiler-cli/src/ngcc/src/rendering/esm2015_renderer.ts @@ -22,6 +22,20 @@ export class Esm2015Renderer extends Renderer { imports.forEach(i => { output.appendLeft(0, `import * as ${i.as} from '${i.name}';\n`); }); } + addConstants(output: MagicString, constants: string, file: ts.SourceFile): void { + if (constants === '') { + return; + } + const insertionPoint = file.statements.reduce((prev, stmt) => { + if (ts.isImportDeclaration(stmt) || ts.isImportEqualsDeclaration(stmt) || + ts.isNamespaceImport(stmt)) { + return stmt.getEnd(); + } + return prev; + }, 0); + output.appendLeft(insertionPoint, '\n' + constants + '\n'); + } + /** * Add the definitions to each decorated class */ diff --git a/packages/compiler-cli/src/ngcc/src/rendering/renderer.ts b/packages/compiler-cli/src/ngcc/src/rendering/renderer.ts index 55a8b9d5fe..974d428fb8 100644 --- a/packages/compiler-cli/src/ngcc/src/rendering/renderer.ts +++ b/packages/compiler-cli/src/ngcc/src/rendering/renderer.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import MagicString from 'magic-string'; import {commentRegex, mapFileCommentRegex, fromJSON, fromSource, fromMapFileSource, fromObject, generateMapFileComment, removeComments, removeMapFileComments, SourceMapConverter} from 'convert-source-map'; import {SourceMapConsumer, SourceMapGenerator, RawSourceMap} from 'source-map'; -import {Expression, Statement, WrappedNodeExpr, WritePropExpr} from '@angular/compiler'; +import {ConstantPool, Expression, Statement, WrappedNodeExpr, WritePropExpr} from '@angular/compiler'; import {AnalyzedClass, AnalyzedFile} from '../analyzer'; import {Decorator} from '../../../ngtsc/host'; import {ImportManager, translateStatement} from '../../../ngtsc/transform'; @@ -79,6 +79,10 @@ export abstract class Renderer { this.trackDecorators(clazz.decorators, decoratorsToRemove); }); + this.addConstants( + outputText, renderConstantPool(file.sourceFile, file.constantPool, importManager), + file.sourceFile); + this.addImports(outputText, importManager.getAllImports(file.sourceFile.fileName, null)); // QUESTION: do we need to remove contructor param metadata and property decorators? this.removeDecorators(outputText, decoratorsToRemove); @@ -86,6 +90,8 @@ export abstract class Renderer { return this.renderSourceAndMap(file, input, outputText, targetPath); } + protected abstract addConstants(output: MagicString, constants: string, file: ts.SourceFile): + void; protected abstract addImports(output: MagicString, imports: {name: string, as: string}[]): void; protected abstract addDefinitions( output: MagicString, analyzedClass: AnalyzedClass, definitions: string): void; @@ -207,6 +213,17 @@ export function mergeSourceMaps( return merged; } +/** + * Render the constant pool as source code for the given class. + */ +export function renderConstantPool( + sourceFile: ts.SourceFile, constantPool: ConstantPool, imports: ImportManager): string { + const printer = ts.createPrinter(); + return constantPool.statements.map(stmt => translateStatement(stmt, imports)) + .map(stmt => printer.printNode(ts.EmitHint.Unspecified, stmt, sourceFile)) + .join('\n'); +} + /** * Render the definitions as source code for the given class. * @param sourceFile The file containing the class to process. diff --git a/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts index 002921aa37..9bcd127c7f 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts @@ -56,6 +56,36 @@ A.decorators = [ }); + describe('addConstants', () => { + it('should insert the given constants after imports in the source file', () => { + const PROGRAM = { + name: 'some/file.js', + contents: ` +/* A copyright notice */ +import {Directive} from '@angular/core'; +export class A {} +A.decorators = [ + { type: Directive, args: [{ selector: '[a]' }] }, + { type: Other } +]; +// Some other content` + }; + const {renderer, program} = setup(PROGRAM); + const file = program.getSourceFile('some/file.js'); + if (file === undefined) { + throw new Error(`Could not find source file`); + } + const output = new MagicString(PROGRAM.contents); + renderer.addConstants(output, 'const x = 3;', file); + expect(output.toString()).toContain(` +import {Directive} from '@angular/core'; +const x = 3; + +export class A {}`); + }); + }); + + describe('addDefinitions', () => { it('should insert the definitions directly after the class declaration', () => { const PROGRAM = { diff --git a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts index 982e2e7b49..46cf87b6e8 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts @@ -20,6 +20,9 @@ class TestRenderer extends Renderer { addImports(output: MagicString, imports: {name: string, as: string}[]) { output.prepend('\n// ADD IMPORTS\n'); } + addConstants(output: MagicString, constants: string, file: ts.SourceFile): void { + output.prepend('\n// ADD CONSTANTS\n'); + } addDefinitions(output: MagicString, analyzedClass: AnalyzedClass, definitions: string) { output.prepend('\n// ADD DEFINITIONS\n'); } @@ -65,7 +68,8 @@ describe('Renderer', () => { ] }); const RENDERED_CONTENTS = - `\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD DEFINITIONS\n` + INPUT_PROGRAM.contents; + `\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` + + INPUT_PROGRAM.contents; const OUTPUT_PROGRAM_MAP = fromObject({ 'version': 3, 'file': '/output_file.js', @@ -74,14 +78,14 @@ describe('Renderer', () => { 'import { Directive } from \'@angular/core\';\nexport class A {\n foo(x) {\n return x;\n }\n}\nA.decorators = [\n { type: Directive, args: [{ selector: \'[a]\' }] }\n];\n' ], 'names': [], - 'mappings': ';;;;;;AAAA;;;;;;;;;' + 'mappings': ';;;;;;;;AAAA;;;;;;;;;' }); const MERGED_OUTPUT_PROGRAM_MAP = fromObject({ 'version': 3, 'sources': ['/file.ts'], 'names': [], - 'mappings': ';;;;;;AAAA', + 'mappings': ';;;;;;;;AAAA', 'file': '/output_file.js', 'sourcesContent': [ 'import { Directive } from \'@angular/core\';\nexport class A {\n foo(x: string): string {\n return x;\n }\n static decorators = [\n { type: Directive, args: [{ selector: \'[a]\' }] }\n ];\n}' diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 989152dcea..9219755110 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -146,9 +146,8 @@ export class ComponentDecoratorHandler implements DecoratorHandler { * Generate a description of the field which should be added to the class, including any * initialization code to be generated. */ - compile(node: ts.Declaration, analysis: A): CompileResult|CompileResult[]; + compile(node: ts.Declaration, analysis: A, constantPool: ConstantPool): CompileResult + |CompileResult[]; } /** diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index bff07b681a..0dc0d44843 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; import {Decorator, ReflectionHost} from '../../host'; @@ -151,7 +152,7 @@ export class IvyCompilation { * Perform a compilation operation on the given class declaration and return instructions to an * AST transformer if any are available. */ - compileIvyFieldFor(node: ts.Declaration): CompileResult[]|undefined { + compileIvyFieldFor(node: ts.Declaration, constantPool: ConstantPool): CompileResult[]|undefined { // Look to see whether the original node was analyzed. If not, there's nothing to do. const original = ts.getOriginalNode(node) as ts.Declaration; if (!this.analysis.has(original)) { @@ -160,7 +161,7 @@ export class IvyCompilation { const op = this.analysis.get(original) !; // Run the actual compilation, which generates an Expression for the Ivy field. - let res: CompileResult|CompileResult[] = op.adapter.compile(node, op.analysis); + let res: CompileResult|CompileResult[] = op.adapter.compile(node, op.analysis, constantPool); if (!Array.isArray(res)) { res = [res]; } diff --git a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts index 8af657aa49..19a9da5b69 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {WrappedNodeExpr} from '@angular/compiler'; +import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; import {Decorator, ReflectionHost} from '../../host'; @@ -32,7 +32,8 @@ export function ivyTransformFactory( class IvyVisitor extends Visitor { constructor( private compilation: IvyCompilation, private reflector: ReflectionHost, - private importManager: ImportManager, private isCore: boolean) { + private importManager: ImportManager, private isCore: boolean, + private constantPool: ConstantPool) { super(); } @@ -40,7 +41,7 @@ class IvyVisitor extends Visitor { VisitListEntryResult { // Determine if this class has an Ivy field that needs to be added, and compile the field // to an expression if so. - const res = this.compilation.compileIvyFieldFor(node); + const res = this.compilation.compileIvyFieldFor(node, this.constantPool); if (res !== undefined) { // There is at least one field to add. @@ -189,24 +190,35 @@ class IvyVisitor extends Visitor { function transformIvySourceFile( compilation: IvyCompilation, context: ts.TransformationContext, reflector: ReflectionHost, coreImportsFrom: ts.SourceFile | null, file: ts.SourceFile): ts.SourceFile { + const constantPool = new ConstantPool(); const importManager = new ImportManager(coreImportsFrom !== null); // Recursively scan through the AST and perform any updates requested by the IvyCompilation. - const sf = visit( - file, new IvyVisitor(compilation, reflector, importManager, coreImportsFrom !== null), - context); + const visitor = + new IvyVisitor(compilation, reflector, importManager, coreImportsFrom !== null, constantPool); + const sf = visit(file, visitor, context); + + // Generate the constant statements first, as they may involve adding additional imports + // to the ImportManager. + const constants = constantPool.statements.map(stmt => translateStatement(stmt, importManager)); // Generate the import statements to prepend. - const imports = importManager.getAllImports(file.fileName, coreImportsFrom).map(i => { + const addedImports = importManager.getAllImports(file.fileName, coreImportsFrom).map(i => { return ts.createImportDeclaration( undefined, undefined, ts.createImportClause(undefined, ts.createNamespaceImport(ts.createIdentifier(i.as))), ts.createLiteral(i.name)); }); + // Filter out the existing imports and the source file body. All new statements + // will be inserted between them. + const existingImports = sf.statements.filter(stmt => isImportStatement(stmt)); + const body = sf.statements.filter(stmt => !isImportStatement(stmt)); + // Prepend imports if needed. - if (imports.length > 0) { - sf.statements = ts.createNodeArray([...imports, ...sf.statements]); + if (addedImports.length > 0) { + sf.statements = + ts.createNodeArray([...existingImports, ...addedImports, ...constants, ...body]); } return sf; } @@ -227,3 +239,8 @@ function maybeFilterDecorator( function isFromAngularCore(decorator: Decorator): boolean { return decorator.import !== null && decorator.import.from === '@angular/core'; } + +function isImportStatement(stmt: ts.Statement): boolean { + return ts.isImportDeclaration(stmt) || ts.isImportEqualsDeclaration(stmt) || + ts.isNamespaceImport(stmt); +}