From fbc9df181ea50527cb755382b54b8b45d0f9ef39 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 26 Feb 2021 11:55:19 -0800 Subject: [PATCH] feat(compiler-cli): support producing Closure-specific PURE annotations (#41021) For certain generated function calls, the compiler emits a 'PURE' annotation which informs Terser (the optimizer) about the purity of a specific function call. This commit expands that system to produce a new Closure-specific 'pureOrBreakMyCode' annotation when targeting the Closure optimizer instead of Terser. PR Close #41021 --- .../linker/test/ast/ast_value_spec.ts | 2 +- .../emit_scopes/emit_scope_spec.ts | 8 +++--- .../emit_scopes/iief_emit_scope_spec.ts | 8 +++--- .../test/file_linker/file_linker_spec.ts | 5 ++-- .../partial_linker_selector_spec.ts | 3 ++- .../test/file_linker/translator_spec.ts | 2 +- .../src/ngtsc/transform/src/transform.ts | 17 +++++++------ .../src/ngtsc/translator/src/translator.ts | 1 + .../translator/src/typescript_ast_factory.ts | 21 +++++++++++++++- .../translator/src/typescript_translator.ts | 4 +-- .../test/typescript_ast_factory_spec.ts | 10 +++++++- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 25 +++++++++++++++++++ 12 files changed, 81 insertions(+), 25 deletions(-) diff --git a/packages/compiler-cli/linker/test/ast/ast_value_spec.ts b/packages/compiler-cli/linker/test/ast/ast_value_spec.ts index 08f11ac3a7..2893e80722 100644 --- a/packages/compiler-cli/linker/test/ast/ast_value_spec.ts +++ b/packages/compiler-cli/linker/test/ast/ast_value_spec.ts @@ -23,7 +23,7 @@ interface TestObject { } const host: AstHost = new TypeScriptAstHost(); -const factory = new TypeScriptAstFactory(); +const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const nestedObj = factory.createObjectLiteral([ {propertyName: 'x', quoted: false, value: factory.createLiteral(42)}, {propertyName: 'y', quoted: false, value: factory.createLiteral('X')}, diff --git a/packages/compiler-cli/linker/test/file_linker/emit_scopes/emit_scope_spec.ts b/packages/compiler-cli/linker/test/file_linker/emit_scopes/emit_scope_spec.ts index fe9b82ebfb..3e0568a740 100644 --- a/packages/compiler-cli/linker/test/file_linker/emit_scopes/emit_scope_spec.ts +++ b/packages/compiler-cli/linker/test/file_linker/emit_scopes/emit_scope_spec.ts @@ -16,7 +16,7 @@ import {generate} from '../helpers'; describe('EmitScope', () => { describe('translateDefinition()', () => { it('should translate the given output AST into a TExpression', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = new EmitScope(ngImport, translator); @@ -26,7 +26,7 @@ describe('EmitScope', () => { }); it('should use the `ngImport` idenfifier for imports when translating', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = new EmitScope(ngImport, translator); @@ -37,7 +37,7 @@ describe('EmitScope', () => { }); it('should not emit any shared constants in the replacement expression', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = new EmitScope(ngImport, translator); @@ -54,7 +54,7 @@ describe('EmitScope', () => { describe('getConstantStatements()', () => { it('should return any constant statements that were added to the `constantPool`', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = new EmitScope(ngImport, translator); diff --git a/packages/compiler-cli/linker/test/file_linker/emit_scopes/iief_emit_scope_spec.ts b/packages/compiler-cli/linker/test/file_linker/emit_scopes/iief_emit_scope_spec.ts index 4873fd97b9..408d396dea 100644 --- a/packages/compiler-cli/linker/test/file_linker/emit_scopes/iief_emit_scope_spec.ts +++ b/packages/compiler-cli/linker/test/file_linker/emit_scopes/iief_emit_scope_spec.ts @@ -16,7 +16,7 @@ import {generate} from '../helpers'; describe('IifeEmitScope', () => { describe('translateDefinition()', () => { it('should translate the given output AST into a TExpression, wrapped in an IIFE', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = @@ -27,7 +27,7 @@ describe('IifeEmitScope', () => { }); it('should use the `ngImport` idenfifier for imports when translating', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = @@ -39,7 +39,7 @@ describe('IifeEmitScope', () => { }); it('should emit any shared constants in the replacement expression IIFE', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = @@ -58,7 +58,7 @@ describe('IifeEmitScope', () => { describe('getConstantStatements()', () => { it('should throw an error', () => { - const factory = new TypeScriptAstFactory(); + const factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false); const translator = new Translator(factory); const ngImport = factory.createIdentifier('core'); const emitScope = diff --git a/packages/compiler-cli/linker/test/file_linker/file_linker_spec.ts b/packages/compiler-cli/linker/test/file_linker/file_linker_spec.ts index 39b0e2f78c..cb684a8edc 100644 --- a/packages/compiler-cli/linker/test/file_linker/file_linker_spec.ts +++ b/packages/compiler-cli/linker/test/file_linker/file_linker_spec.ts @@ -23,7 +23,7 @@ import {generate} from './helpers'; describe('FileLinker', () => { let factory: TypeScriptAstFactory; - beforeEach(() => factory = new TypeScriptAstFactory()); + beforeEach(() => factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false)); describe('isPartialDeclaration()', () => { it('should return true if the callee is recognized', () => { @@ -154,7 +154,8 @@ describe('FileLinker', () => { const fs = new MockFileSystemNative(); const logger = new MockLogger(); const linkerEnvironment = LinkerEnvironment.create( - fs, logger, new TypeScriptAstHost(), new TypeScriptAstFactory(), DEFAULT_LINKER_OPTIONS); + fs, logger, new TypeScriptAstHost(), + new TypeScriptAstFactory(/* annotateForClosureCompiler */ false), DEFAULT_LINKER_OPTIONS); const fileLinker = new FileLinker( linkerEnvironment, fs.resolve('/test.js'), '// test code'); return {host: linkerEnvironment.host, fileLinker}; diff --git a/packages/compiler-cli/linker/test/file_linker/partial_linkers/partial_linker_selector_spec.ts b/packages/compiler-cli/linker/test/file_linker/partial_linkers/partial_linker_selector_spec.ts index 7d8cc687e8..31561d4d50 100644 --- a/packages/compiler-cli/linker/test/file_linker/partial_linkers/partial_linker_selector_spec.ts +++ b/packages/compiler-cli/linker/test/file_linker/partial_linkers/partial_linker_selector_spec.ts @@ -33,7 +33,8 @@ describe('PartialLinkerSelector', () => { fs = new MockFileSystemNative(); const logger = new MockLogger(); environment = LinkerEnvironment.create( - fs, logger, new TypeScriptAstHost(), new TypeScriptAstFactory(), options); + fs, logger, new TypeScriptAstHost(), + new TypeScriptAstFactory(/* annotateForClosureCompiler */ false), options); }); describe('supportsDeclaration()', () => { diff --git a/packages/compiler-cli/linker/test/file_linker/translator_spec.ts b/packages/compiler-cli/linker/test/file_linker/translator_spec.ts index 31d0401f0b..233cd38e9a 100644 --- a/packages/compiler-cli/linker/test/file_linker/translator_spec.ts +++ b/packages/compiler-cli/linker/test/file_linker/translator_spec.ts @@ -14,7 +14,7 @@ import {generate} from './helpers'; describe('Translator', () => { let factory: TypeScriptAstFactory; - beforeEach(() => factory = new TypeScriptAstFactory()); + beforeEach(() => factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false)); describe('translateExpression()', () => { it('should generate expression specific output', () => { diff --git a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts index 76ebcef30f..9dbeb1279f 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {DefaultImportRecorder, ImportRewriter} from '../../imports'; import {Decorator, ReflectionHost} from '../../reflection'; -import {ImportManager, RecordWrappedNodeExprFn, translateExpression, translateStatement} from '../../translator'; +import {ImportManager, RecordWrappedNodeExprFn, translateExpression, translateStatement, TranslatorOptions} from '../../translator'; import {visit, VisitListEntryResult, Visitor} from '../../util/src/visitor'; import {CompileResult} from './api'; @@ -91,15 +91,18 @@ class IvyTransformationVisitor extends Visitor { return {node}; } + const translateOptions: TranslatorOptions = { + recordWrappedNodeExpr: this.recordWrappedNodeExpr, + annotateForClosureCompiler: this.isClosureCompilerEnabled, + }; + // There is at least one field to add. const statements: ts.Statement[] = []; const members = [...node.members]; for (const field of this.classCompilationMap.get(node)!) { // Translate the initializer for the field into TS nodes. - const exprNode = translateExpression( - field.initializer, this.importManager, - {recordWrappedNodeExpr: this.recordWrappedNodeExpr}); + const exprNode = translateExpression(field.initializer, this.importManager, translateOptions); // Create a static property declaration for the new field. const property = ts.createProperty( @@ -116,10 +119,7 @@ class IvyTransformationVisitor extends Visitor { /* hasTrailingNewLine */ false); } - field.statements - .map( - stmt => translateStatement( - stmt, this.importManager, {recordWrappedNodeExpr: this.recordWrappedNodeExpr})) + field.statements.map(stmt => translateStatement(stmt, this.importManager, translateOptions)) .forEach(stmt => statements.push(stmt)); members.push(property); @@ -282,6 +282,7 @@ function transformIvySourceFile( recordWrappedNodeExpr, downlevelTaggedTemplates: downlevelTranslatedCode, downlevelVariableDeclarations: downlevelTranslatedCode, + annotateForClosureCompiler: isClosureCompilerEnabled, })); // Preserve @fileoverview comments required by Closure, since the location might change as a diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts index 9f9a37c1e1..f2d71023a6 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts @@ -42,6 +42,7 @@ export interface TranslatorOptions { downlevelTaggedTemplates?: boolean; downlevelVariableDeclarations?: boolean; recordWrappedNodeExpr?: RecordWrappedNodeExprFn; + annotateForClosureCompiler?: boolean; } export class ExpressionTranslatorVisitor implements o.ExpressionVisitor, diff --git a/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts b/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts index a75c03c324..6de1302c31 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts @@ -9,6 +9,21 @@ import * as ts from 'typescript'; import {AstFactory, BinaryOperator, LeadingComment, ObjectLiteralProperty, SourceMapRange, TemplateLiteral, UnaryOperator, VariableDeclarationType} from './api/ast_factory'; +/** + * Different optimizers use different annotations on a function or method call to indicate its pure + * status. + */ +enum PureAnnotation { + /** + * Closure's annotation for purity is `@pureOrBreakMyCode`, but this needs to be in a semantic + * (jsdoc) enabled comment. Thus, the actual comment text for Closure must include the `*` that + * turns a `/*` comment into a `/**` comment, as well as surrounding whitespace. + */ + CLOSURE = '* @pureOrBreakMyCode ', + + TERSER = '@__PURE__', +} + const UNARY_OPERATORS: Record = { '+': ts.SyntaxKind.PlusToken, '-': ts.SyntaxKind.MinusToken, @@ -46,6 +61,8 @@ const VAR_TYPES: Record = { export class TypeScriptAstFactory implements AstFactory { private externalSourceFiles = new Map(); + constructor(private annotateForClosureCompiler: boolean) {} + attachComments = attachComments; createArrayLiteral = ts.createArrayLiteral; @@ -68,7 +85,9 @@ export class TypeScriptAstFactory implements AstFactory = {}): ts.Expression { return expression.visitExpression( new ExpressionTranslatorVisitor( - new TypeScriptAstFactory(), imports, options), + new TypeScriptAstFactory(options.annotateForClosureCompiler === true), imports, options), new Context(false)); } @@ -28,6 +28,6 @@ export function translateStatement( options: TranslatorOptions = {}): ts.Statement { return statement.visitStatement( new ExpressionTranslatorVisitor( - new TypeScriptAstFactory(), imports, options), + new TypeScriptAstFactory(options.annotateForClosureCompiler === true), imports, options), new Context(true)); } diff --git a/packages/compiler-cli/src/ngtsc/translator/test/typescript_ast_factory_spec.ts b/packages/compiler-cli/src/ngtsc/translator/test/typescript_ast_factory_spec.ts index a341535f60..f213c2abc2 100644 --- a/packages/compiler-cli/src/ngtsc/translator/test/typescript_ast_factory_spec.ts +++ b/packages/compiler-cli/src/ngtsc/translator/test/typescript_ast_factory_spec.ts @@ -12,7 +12,7 @@ import {TypeScriptAstFactory} from '../src/typescript_ast_factory'; describe('TypeScriptAstFactory', () => { let factory: TypeScriptAstFactory; - beforeEach(() => factory = new TypeScriptAstFactory()); + beforeEach(() => factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ false)); describe('attachComments()', () => { it('should add the comments to the given statement', () => { @@ -77,6 +77,14 @@ describe('TypeScriptAstFactory', () => { const call = factory.createCallExpression(callee, [arg1, arg2], true); expect(generate(call)).toEqual('/*@__PURE__*/ foo(42, "moo")'); }); + + it('should create a call marked with a closure-style pure comment if `pure` is true', () => { + factory = new TypeScriptAstFactory(/* annotateForClosureCompiler */ true); + + const {items: [callee, arg1, arg2], generate} = setupExpressions(`foo`, `42`, `"moo"`); + const call = factory.createCallExpression(callee, [arg1, arg2], true); + expect(generate(call)).toEqual('/** @pureOrBreakMyCode */ foo(42, "moo")'); + }); }); describe('createConditional()', () => { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 1a6761191d..12077c768b 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -393,6 +393,31 @@ runInEachFileSystem(os => { // that start with `C:`. if (os !== 'Windows' || platform() === 'win32') { describe('when closure annotations are requested', () => { + it('should add @pureOrBreakMyCode to getInheritedFactory calls', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write(`test.ts`, ` + import {Directive} from '@angular/core'; + + @Directive({ + selector: '[base]', + }) + class Base {} + + @Directive({ + selector: '[test]', + }) + class Dir extends Base { + } + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('/** @pureOrBreakMyCode */ i0.ɵɵgetInheritedFactory(Dir)'); + }); + it('should add @nocollapse to static fields', () => { env.tsconfig({ 'annotateForClosureCompiler': true,