refactor(compiler): add `@nocollapse` annotation using a synthetic comment (#35932)

In Ivy, Angular decorators are compiled into static fields that are
inserted into a class declaration in a TypeScript transform. When
targeting Closure compiler such fields need to be annotated with
`@nocollapse` to prevent them from being lifted from a static field into
a variable, as that would prevent the Ivy runtime from being able to
find the compiled definitions.

Previously, there was a bug in TypeScript where synthetic comments added
in a transform would not be emitted at all, so as a workaround a global
regex-replace was done in the emit's `writeFile` callback that would add
the `@nocollapse` annotation to all static Ivy definition fields. This
approach is no longer possible when ngtsc is running as TypeScript
plugin, as a plugin cannot control emit behavior.

The workaround is no longer necessary, as synthetic comments are now
properly emitted, likely as of
https://github.com/microsoft/TypeScript/pull/22141 which has been
released with TypeScript 2.8.

This change is required for running ngtsc as TypeScript plugin in
Bazel's `ts_library` rule, to move away from the custom `ngc_wrapped`
approach.

Resolves FW-1952

PR Close #35932

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

@@ -10,7 +10,6 @@ import {GeneratedFile} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import * as api from '../transformers/api';
-import {nocollapseHack} from '../transformers/nocollapse_hack';
 import {verifySupportedTypeScriptVersion} from '../typescript_support';
 
 import {NgCompilerHost} from './core';
@@ -193,16 +192,6 @@ export class NgtscProgram implements api.Program {
               this.compiler.incrementalDriver.recordSuccessfulEmit(writtenSf);
             }
           }
-
-          // If Closure annotations are being produced, tsickle should be adding `@nocollapse` to
-          // any static fields present. However, tsickle doesn't yet handle synthetic fields added
-          // during other transformations, so this hack is in place to ensure Ivy definitions get
-          // properly annotated, pending an upstream fix in tsickle.
-          //
-          // TODO(alxhub): remove when tsickle properly annotates synthetic fields.
-          if (this.closureCompilerEnabled && fileName.endsWith('.js')) {
-            data = nocollapseHack(data);
-          }
           this.host.writeFile(fileName, data, writeByteOrderMark, onError, sourceFiles);
         };
 

packages/compiler-cli/src/ngtsc/transform/src/transform.ts

@@ -47,7 +47,8 @@ class IvyVisitor extends Visitor {
   constructor(
       private compilation: TraitCompiler, private reflector: ReflectionHost,
       private importManager: ImportManager, private defaultImportRecorder: DefaultImportRecorder,
-      private isCore: boolean, private constantPool: ConstantPool) {
+      private isClosureCompilerEnabled: boolean, private isCore: boolean,
+      private constantPool: ConstantPool) {
     super();
   }
 
@@ -73,6 +74,16 @@ class IvyVisitor extends Visitor {
             undefined, [ts.createToken(ts.SyntaxKind.StaticKeyword)], field.name, undefined,
             undefined, exprNode);
 
+        if (this.isClosureCompilerEnabled) {
+          // Closure compiler transforms the form `Service.ɵprov = X` into `Service$ɵprov = X`. To
+          // prevent this transformation, such assignments need to be annotated with @nocollapse.
+          // Note that tsickle is typically responsible for adding such annotations, however it
+          // doesn't yet handle synthetic fields added during other transformations.
+          ts.addSyntheticLeadingComment(
+              property, ts.SyntaxKind.MultiLineCommentTrivia, '* @nocollapse ',
+              /* hasTrailingNewLine */ false);
+        }
+
         field.statements
             .map(
                 stmt => translateStatement(
@@ -215,7 +226,8 @@ function transformIvySourceFile(
 
   // Recursively scan through the AST and perform any updates requested by the IvyCompilation.
   const visitor = new IvyVisitor(
-      compilation, reflector, importManager, defaultImportRecorder, isCore, constantPool);
+      compilation, reflector, importManager, defaultImportRecorder, isClosureCompilerEnabled,
+      isCore, constantPool);
   let sf = visit(file, visitor, context);
 
   // Generate the constant statements first, as they may involve adding additional imports

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

@@ -1,58 +0,0 @@
-/**
- * @license
- * Copyright Google Inc. All Rights Reserved.
- *
- * 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
- */
-
-// Closure compiler transforms the form `Service.ɵprov = X` into
-// `Service$ɵprov = X`. To prevent this transformation, such assignments need to be
-// annotated with @nocollapse. Unfortunately, a bug in Typescript where comments aren't propagated
-// through the TS transformations precludes adding the comment via the AST. This workaround detects
-// the static assignments to R3 properties such as ɵprov using a regex, as output files
-// are written, and applies the annotation through regex replacement.
-//
-// TODO(alxhub): clean up once fix for TS transformers lands in upstream
-//
-// Typescript reference issue: https://github.com/Microsoft/TypeScript/issues/22497
-
-// Pattern matching all Render3 property names.
-const R3_DEF_NAME_PATTERN = [
-  'ɵcmp',
-  'ɵdir',
-  'ɵprov',
-  'ɵinj',
-  'ɵmod',
-  'ɵpipe',
-  'ɵfac',
-].join('|');
-
-// Pattern matching `Identifier.property` where property is a Render3 property.
-const R3_DEF_ACCESS_PATTERN = `[^\\s\\.()[\\]]+\.(${R3_DEF_NAME_PATTERN})`;
-
-// Pattern matching a source line that contains a Render3 static property assignment.
-// It declares two matching groups - one for the preceding whitespace, the second for the rest
-// of the assignment expression.
-const R3_DEF_LINE_PATTERN = `^(\\s*)(${R3_DEF_ACCESS_PATTERN} = .*)$`;
-
-// Regex compilation of R3_DEF_LINE_PATTERN. Matching group 1 yields the whitespace preceding the
-// assignment, matching group 2 gives the rest of the assignment expressions.
-const R3_MATCH_DEFS = new RegExp(R3_DEF_LINE_PATTERN, 'gmu');
-
-const R3_TSICKLE_DECL_PATTERN =
-    `(\\/\\*\\*[*\\s]*)(@[^*]+\\*\\/\\s+[^.]+\\.(?:${R3_DEF_NAME_PATTERN});)`;
-
-const R3_MATCH_TSICKLE_DECL = new RegExp(R3_TSICKLE_DECL_PATTERN, 'gmu');
-
-// Replacement string that complements R3_MATCH_DEFS. It inserts `/** @nocollapse */` before the
-// assignment but after any indentation. Note that this will mess up any sourcemaps on this line
-// (though there shouldn't be any, since Render3 properties are synthetic).
-const R3_NOCOLLAPSE_DEFS = '$1\/** @nocollapse *\/ $2';
-
-const R3_NOCOLLAPSE_TSICKLE_DECL = '$1@nocollapse $2';
-
-export function nocollapseHack(contents: string): string {
-  return contents.replace(R3_MATCH_DEFS, R3_NOCOLLAPSE_DEFS)
-      .replace(R3_MATCH_TSICKLE_DECL, R3_NOCOLLAPSE_TSICKLE_DECL);
-}

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

@@ -20,9 +20,11 @@ const CATCH_STACK_NAME = 'stack';
 const _VALID_IDENTIFIER_RE = /^[$A-Z_][0-9A-Z_$]*$/i;
 
 export class TypeScriptNodeEmitter {
+  constructor(private annotateForClosureCompiler: boolean) {}
+
   updateSourceFile(sourceFile: ts.SourceFile, stmts: Statement[], preamble?: string):
       [ts.SourceFile, Map<ts.Node, Node>] {
-    const converter = new NodeEmitterVisitor();
+    const converter = new NodeEmitterVisitor(this.annotateForClosureCompiler);
     // [].concat flattens the result so that each `visit...` method can also return an array of
     // stmts.
     const statements: any[] = [].concat(
@@ -63,8 +65,8 @@ export class TypeScriptNodeEmitter {
  */
 export function updateSourceFile(
     sourceFile: ts.SourceFile, module: PartialModule,
-    context: ts.TransformationContext): [ts.SourceFile, Map<ts.Node, Node>] {
+    annotateForClosureCompiler: boolean): [ts.SourceFile, Map<ts.Node, Node>] {
-  const converter = new NodeEmitterVisitor();
+  const converter = new NodeEmitterVisitor(annotateForClosureCompiler);
   converter.loadExportedVariableIdentifiers(sourceFile);
 
   const prefixStatements = module.statements.filter(statement => !(statement instanceof ClassStmt));
@@ -192,6 +194,8 @@ export class NodeEmitterVisitor implements StatementVisitor, ExpressionVisitor {
   private _templateSources = new Map<ParseSourceFile, ts.SourceMapSource>();
   private _exportedVariableIdentifiers = new Map<string, ts.Identifier>();
 
+  constructor(private annotateForClosureCompiler: boolean) {}
+
   /**
    * Process the source file and collect exported identifiers that refer to variables.
    *
@@ -367,14 +371,27 @@ export class NodeEmitterVisitor implements StatementVisitor, ExpressionVisitor {
 
   visitDeclareClassStmt(stmt: ClassStmt) {
     const modifiers = this.getModifiers(stmt);
-    const fields = stmt.fields.map(
+    const fields = stmt.fields.map(field => {
-        field => ts.createProperty(
+      const property = ts.createProperty(
-            /* decorators */ undefined, /* modifiers */ translateModifiers(field.modifiers),
+          /* decorators */ undefined, /* modifiers */ translateModifiers(field.modifiers),
-            field.name,
+          field.name,
-            /* questionToken */ undefined,
+          /* questionToken */ undefined,
-            /* type */ undefined,
+          /* type */ undefined,
-            field.initializer == null ? ts.createNull() :
+          field.initializer == null ? ts.createNull() :
-                                        field.initializer.visitExpression(this, null)));
+                                      field.initializer.visitExpression(this, null));
+
+      if (this.annotateForClosureCompiler) {
+        // Closure compiler transforms the form `Service.ɵprov = X` into `Service$ɵprov = X`. To
+        // prevent this transformation, such assignments need to be annotated with @nocollapse.
+        // Note that tsickle is typically responsible for adding such annotations, however it
+        // doesn't yet handle synthetic fields added during other transformations.
+        ts.addSyntheticLeadingComment(
+            property, ts.SyntaxKind.MultiLineCommentTrivia, '* @nocollapse ',
+            /* hasTrailingNewLine */ false);
+      }
+
+      return property;
+    });
     const getters = stmt.getters.map(
         getter => ts.createGetAccessor(
             /* decorators */ undefined, /* modifiers */ undefined, getter.name, /* parameters */[],

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

@@ -30,10 +30,10 @@ function getPreamble(original: string) {
  *   list of statements as their body.
  */
 export function getAngularEmitterTransformFactory(
-    generatedFiles: Map<string, GeneratedFile>, program: ts.Program): () =>
+    generatedFiles: Map<string, GeneratedFile>, program: ts.Program,
-    (sourceFile: ts.SourceFile) => ts.SourceFile {
+    annotateForClosureCompiler: boolean): () => (sourceFile: ts.SourceFile) => ts.SourceFile {
   return function() {
-    const emitter = new TypeScriptNodeEmitter();
+    const emitter = new TypeScriptNodeEmitter(annotateForClosureCompiler);
     return function(sourceFile: ts.SourceFile): ts.SourceFile {
       const g = generatedFiles.get(sourceFile.fileName);
       const orig = g && program.getSourceFile(g.srcFileUrl);

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

@@ -22,7 +22,6 @@ import {CodeGenerator, TsCompilerAotCompilerTypeCheckHostAdapter, getOriginalRef
 import {InlineResourcesMetadataTransformer, getInlineResourcesTransformFactory} from './inline_resources';
 import {LowerMetadataTransform, getExpressionLoweringTransformFactory} from './lower_expressions';
 import {MetadataCache, MetadataTransformer} from './metadata_cache';
-import {nocollapseHack} from './nocollapse_hack';
 import {getAngularEmitterTransformFactory} from './node_emitter_transform';
 import {PartialModuleMetadataTransformer} from './r3_metadata_transform';
 import {StripDecoratorsMetadataTransformer, getDecoratorStripTransformerFactory} from './r3_strip_decorators';
@@ -281,12 +280,6 @@ class AngularCompilerProgram implements Program {
 
     const writeTsFile: ts.WriteFileCallback =
         (outFileName, outData, writeByteOrderMark, onError?, sourceFiles?) => {
-          const sourceFile = sourceFiles && sourceFiles.length == 1 ? sourceFiles[0] : null;
-          let genFile: GeneratedFile|undefined;
-          if (this.options.annotateForClosureCompiler && sourceFile &&
-              TS.test(sourceFile.fileName)) {
-            outData = nocollapseHack(outData);
-          }
           this.writeFile(outFileName, outData, writeByteOrderMark, onError, undefined, sourceFiles);
         };
 
@@ -377,9 +370,6 @@ class AngularCompilerProgram implements Program {
                 emittedSourceFiles.push(originalFile);
               }
             }
-            if (this.options.annotateForClosureCompiler && TS.test(sourceFile.fileName)) {
-              outData = nocollapseHack(outData);
-            }
           }
           this.writeFile(outFileName, outData, writeByteOrderMark, onError, genFile, sourceFiles);
         };
@@ -572,11 +562,13 @@ class AngularCompilerProgram implements Program {
           getExpressionLoweringTransformFactory(this.loweringMetadataTransform, this.tsProgram));
       metadataTransforms.push(this.loweringMetadataTransform);
     }
+    const annotateForClosureCompiler = this.options.annotateForClosureCompiler || false;
     if (genFiles) {
-      beforeTs.push(getAngularEmitterTransformFactory(genFiles, this.getTsProgram()));
+      beforeTs.push(getAngularEmitterTransformFactory(
+          genFiles, this.getTsProgram(), annotateForClosureCompiler));
     }
     if (partialModules) {
-      beforeTs.push(getAngularClassTransformerFactory(partialModules));
+      beforeTs.push(getAngularClassTransformerFactory(partialModules, annotateForClosureCompiler));
 
       // If we have partial modules, the cached metadata might be incorrect as it doesn't reflect
       // the partial module transforms.

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

@@ -17,7 +17,8 @@ export type TransformerFactory = (context: ts.TransformationContext) => Transfor
 /**
  * Returns a transformer that adds the requested static methods specified by modules.
  */
-export function getAngularClassTransformerFactory(modules: PartialModule[]): TransformerFactory {
+export function getAngularClassTransformerFactory(
+    modules: PartialModule[], annotateForClosureCompiler: boolean): TransformerFactory {
   if (modules.length === 0) {
     // If no modules are specified, just return an identity transform.
     return () => sf => sf;
@@ -27,7 +28,7 @@ export function getAngularClassTransformerFactory(modules: PartialModule[]): Tra
     return function(sourceFile: ts.SourceFile): ts.SourceFile {
       const module = moduleMap.get(sourceFile.fileName);
       if (module && module.statements.length > 0) {
-        const [newSourceFile] = updateSourceFile(sourceFile, module, context);
+        const [newSourceFile] = updateSourceFile(sourceFile, module, annotateForClosureCompiler);
         return newSourceFile;
       }
       return sourceFile;

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

@@ -1,26 +0,0 @@
-/**
- * @license
- * Copyright Google Inc. All Rights Reserved.
- *
- * 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 {nocollapseHack} from '../../src/transformers/nocollapse_hack';
-
-describe('@nocollapse hack', () => {
-  it('should add @nocollapse to a basic class', () => {
-    const decl = `Foo.ɵinj = define(...);`;
-    expect(nocollapseHack(decl)).toEqual('/** @nocollapse */ ' + decl);
-  });
-
-  it('should add nocollapse to an if (false) declaration of the kind generated by tsickle', () => {
-    const decl = `
-      if (false) {
-        /** @type {?} */
-        Foo.ɵinj;
-      }
-    `;
-    expect(nocollapseHack(decl)).toContain('/** @nocollapse @type {?} */');
-  });
-});

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

@@ -33,7 +33,7 @@ describe('TypeScriptNodeEmitter', () => {
   beforeEach(() => {
     context = new MockAotContext('/', FILES);
     host = new MockCompilerHost(context);
-    emitter = new TypeScriptNodeEmitter();
+    emitter = new TypeScriptNodeEmitter(false);
     someVar = o.variable('someVar', null, null);
   });
 

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

@@ -40,14 +40,16 @@ describe('r3_transform_spec', () => {
   });
 
   it('should be able to modify multiple classes in the same module', () => {
-    const result = emit(getAngularClassTransformerFactory([{
+    const result = emit(getAngularClassTransformerFactory(
-      fileName: someGenFileName,
+        [{
-      statements: [
+          fileName: someGenFileName,
-        classMethod(new o.ReturnStatement(o.variable('v')), ['v'], 'someMethod', 'SomeClass'),
+          statements: [
-        classMethod(
+            classMethod(new o.ReturnStatement(o.variable('v')), ['v'], 'someMethod', 'SomeClass'),
-            new o.ReturnStatement(o.variable('v')), ['v'], 'someOtherMethod', 'SomeOtherClass')
+            classMethod(
-      ]
+                new o.ReturnStatement(o.variable('v')), ['v'], 'someOtherMethod', 'SomeOtherClass')
-    }]));
+          ]
+        }],
+        false));
     expect(result).toContain('static someMethod(v) { return v; }');
     expect(result).toContain('static someOtherMethod(v) { return v; }');
   });
@@ -94,7 +96,7 @@ describe('r3_transform_spec', () => {
       fileName: someGenFileName,
       statements: [classMethod(stmt, parameters, methodName, className)]
     };
-    return emit(getAngularClassTransformerFactory([module]));
+    return emit(getAngularClassTransformerFactory([module], false));
   }
 
   function emitStaticField(
@@ -104,7 +106,7 @@ describe('r3_transform_spec', () => {
       fileName: someGenFileName,
       statements: [classField(initializer, fieldName, className)]
     };
-    return emit(getAngularClassTransformerFactory([module]));
+    return emit(getAngularClassTransformerFactory([module], false));
   }
 });