fix(compiler-cli): prevent stack overflow in decorator transform for large number of files (#40374)

The decorator downleveling transform patches `ts.EmitResolver.isReferencedAliasDeclaration`
to prevent elision of value imports that occur only in a type-position, which would
inadvertently install the patch repeatedly for each source file in the program.
This could potentially result in a stack overflow when a very large number of files is
present in the program.

This commit fixes the issue by ensuring that the patch is only applied once.
This is also a slight performance improvement, as `isReferencedAliasDeclaration`
is no longer repeatedly calling into all prior installed patch functions.

Fixes #40276

PR Close #40374

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

@@ -8,7 +8,7 @@
 
 import * as ts from 'typescript';
 import {Decorator, ReflectionHost} from '../ngtsc/reflection';
-import {isAliasImportDeclaration, patchAliasReferenceResolutionOrDie} from './patch_alias_reference_resolution';
+import {isAliasImportDeclaration, loadIsReferencedAliasDeclarationPatch} from './patch_alias_reference_resolution';
 
 /**
  * Whether a given decorator should be treated as an Angular decorator.
@@ -347,7 +347,12 @@ export function getDownlevelDecoratorsTransform(
     isCore: boolean, isClosureCompilerEnabled: boolean,
     skipClassDecorators: boolean): ts.TransformerFactory<ts.SourceFile> {
   return (context: ts.TransformationContext) => {
-    let referencedParameterTypes = new Set<ts.Declaration>();
+    // Ensure that referenced type symbols are not elided by TypeScript. Imports for
+    // such parameter type symbols previously could be type-only, but now might be also
+    // used in the `ctorParameters` static property as a value. We want to make sure
+    // that TypeScript does not elide imports for such type references. Read more
+    // about this in the description for `loadIsReferencedAliasDeclarationPatch`.
+    const referencedParameterTypes = loadIsReferencedAliasDeclarationPatch(context);
 
     /**
      * Converts an EntityName (from a type annotation) to an expression (accessing a value).
@@ -595,12 +600,6 @@ export function getDownlevelDecoratorsTransform(
     }
 
     return (sf: ts.SourceFile) => {
-      // Ensure that referenced type symbols are not elided by TypeScript. Imports for
-      // such parameter type symbols previously could be type-only, but now might be also
-      // used in the `ctorParameters` static property as a value. We want to make sure
-      // that TypeScript does not elide imports for such type references. Read more
-      // about this in the description for `patchAliasReferenceResolution`.
-      patchAliasReferenceResolutionOrDie(context, referencedParameterTypes);
       // Downlevel decorators and constructor parameter types. We will keep track of all
       // referenced constructor parameter types so that we can instruct TypeScript to
       // not elide their imports if they previously were only type-only.

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

@@ -17,9 +17,12 @@ interface TransformationContextWithResolver extends ts.TransformationContext {
   getEmitResolver: () => EmitResolver;
 }
 
+const patchedReferencedAliasesSymbol = Symbol('patchedReferencedAliases');
+
 /** Describes a subset of the TypeScript internal emit resolver. */
 interface EmitResolver {
-  isReferencedAliasDeclaration?(node: ts.Node, checkChildren?: boolean): void;
+  isReferencedAliasDeclaration?(node: ts.Node, ...args: unknown[]): void;
+  [patchedReferencedAliasesSymbol]?: Set<ts.Declaration>;
 }
 
 /**
@@ -40,10 +43,10 @@ interface EmitResolver {
  * This is a trick the CLI used in the past  for constructor parameter downleveling in JIT:
  * https://github.com/angular/angular-cli/blob/b3f84cc5184337666ce61c07b7b9df418030106f/packages/ngtools/webpack/src/transformers/ctor-parameters.ts#L323-L325
  * The trick is not ideal though as it preserves the full import (as outlined before), and it
- * results in a slow-down due to the type checker being involved multiple times. The CLI
+ * results in a slow-down due to the type checker being involved multiple times. The CLI worked
- * worked around this import preserving issue by having another complex post-process step that
+ * around this import preserving issue by having another complex post-process step that detects and
- * detects and elides unused imports. Note that these unused imports could cause unused chunks
+ * elides unused imports. Note that these unused imports could cause unused chunks being generated
- * being generated by Webpack if the application or library is not marked as side-effect free.
+ * by Webpack if the application or library is not marked as side-effect free.
  *
  * This is not ideal though, as we basically re-implement the complex import usage resolution
  * from TypeScript. We can do better by letting TypeScript do the import eliding, but providing
@@ -60,32 +63,46 @@ interface EmitResolver {
  * `emitDecoratorMetadata` flag is enabled. TypeScript basically surfaces the same problem and
  * solves it conceptually the same way, but obviously doesn't need to access an `@internal` API.
  *
+ * The set that is returned by this function is meant to be filled with import declaration nodes
+ * that have been referenced in a value-position by the transform, such the the installed patch can
+ * ensure that those import declarations are not elided.
+ *
  * See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on
  * Github.
  * https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257
  */
-export function patchAliasReferenceResolutionOrDie(
+export function loadIsReferencedAliasDeclarationPatch(context: ts.TransformationContext):
-    context: ts.TransformationContext, referencedAliases: Set<ts.Declaration>): void {
+    Set<ts.Declaration> {
   // If the `getEmitResolver` method is not available, TS most likely changed the
   // internal structure of the transformation context. We will abort gracefully.
   if (!isTransformationContextWithEmitResolver(context)) {
     throwIncompatibleTransformationContextError();
-    return;
   }
   const emitResolver = context.getEmitResolver();
-  const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration;
+
+  // The emit resolver may have been patched already, in which case we return the set of referenced
+  // aliases that was created when the patch was first applied.
+  // See https://github.com/angular/angular/issues/40276.
+  const existingReferencedAliases = emitResolver[patchedReferencedAliasesSymbol];
+  if (existingReferencedAliases !== undefined) {
+    return existingReferencedAliases;
+  }
+
+  const originalIsReferencedAliasDeclaration = emitResolver.isReferencedAliasDeclaration;
   // If the emit resolver does not have a function called `isReferencedAliasDeclaration`, then
   // we abort gracefully as most likely TS changed the internal structure of the emit resolver.
-  if (originalReferenceResolution === undefined) {
+  if (originalIsReferencedAliasDeclaration === undefined) {
     throwIncompatibleTransformationContextError();
-    return;
   }
+
+  const referencedAliases = new Set<ts.Declaration>();
   emitResolver.isReferencedAliasDeclaration = function(node, ...args) {
     if (isAliasImportDeclaration(node) && referencedAliases.has(node)) {
       return true;
     }
-    return originalReferenceResolution.call(emitResolver, node, ...args);
+    return originalIsReferencedAliasDeclaration.call(emitResolver, node, ...args);
   };
+  return emitResolver[patchedReferencedAliasesSymbol] = referencedAliases;
 }
 
 /**
@@ -110,7 +127,7 @@ function isTransformationContextWithEmitResolver(context: ts.TransformationConte
  * declaration reference resolution could not be monkey-patched. The error will
  * also propose potential solutions that can be applied by developers.
  */
-function throwIncompatibleTransformationContextError() {
+function throwIncompatibleTransformationContextError(): never {
   throw Error(
       'Unable to downlevel Angular decorators due to an incompatible TypeScript ' +
       'version.\nIf you recently updated TypeScript and this issue surfaces now, consider ' +

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

@@ -740,6 +740,107 @@ describe('downlevel decorator transform', () => {
       `);
     });
   });
+
+  describe('transforming multiple files', () => {
+    it('should work correctly for multiple files that import distinct declarations', () => {
+      context.writeFile('foo_service.d.ts', `
+        export declare class Foo {};
+      `);
+      context.writeFile('foo.ts', `
+        import {Injectable} from '@angular/core';
+        import {Foo} from './foo_service';
+
+        @Injectable()
+        export class MyService {
+          constructor(foo: Foo) {}
+        }
+      `);
+
+      context.writeFile('bar_service.d.ts', `
+        export declare class Bar {};
+      `);
+      context.writeFile('bar.ts', `
+        import {Injectable} from '@angular/core';
+        import {Bar} from './bar_service';
+
+        @Injectable()
+        export class MyService {
+          constructor(bar: Bar) {}
+        }
+      `);
+
+      const {program, transformers} = createProgramWithTransform(['/foo.ts', '/bar.ts']);
+      program.emit(undefined, undefined, undefined, undefined, transformers);
+
+      expect(context.readFile('/foo.js')).toContain(`import { Foo } from './foo_service';`);
+      expect(context.readFile('/bar.js')).toContain(`import { Bar } from './bar_service';`);
+    });
+
+    it('should not result in a stack overflow for a large number of files', () => {
+      // The decorators transform used to patch `ts.EmitResolver.isReferencedAliasDeclaration`
+      // repeatedly for each source file in the program, causing a stack overflow once a large
+      // number of source files was reached. This test verifies that emit succeeds even when there's
+      // lots of source files. See https://github.com/angular/angular/issues/40276.
+      context.writeFile('foo.d.ts', `
+        export declare class Foo {};
+      `);
+
+      // A somewhat minimal number of source files that used to trigger a stack overflow.
+      const numberOfTestFiles = 6500;
+      const files: string[] = [];
+      for (let i = 0; i < numberOfTestFiles; i++) {
+        const file = `/${i}.ts`;
+        files.push(file);
+        context.writeFile(file, `
+          import {Injectable} from '@angular/core';
+          import {Foo} from './foo';
+
+          @Injectable()
+          export class MyService {
+            constructor(foo: Foo) {}
+          }
+        `);
+      }
+
+      const {program, transformers} = createProgramWithTransform(files);
+
+      let written = 0;
+      program.emit(undefined, (fileName, outputText) => {
+        written++;
+
+        // The below assertion throws an explicit error instead of using a Jasmine expectation,
+        // as we want to abort on the first failure, if any. This avoids as many as `numberOfFiles`
+        // expectation failures, which would bloat the test output.
+        if (!outputText.includes(`import { Foo } from './foo';`)) {
+          throw new Error(`Transform failed to preserve the import in ${fileName}:\n${outputText}`);
+        }
+      }, undefined, undefined, transformers);
+      expect(written).toBe(numberOfTestFiles);
+    });
+
+    function createProgramWithTransform(files: string[]) {
+      const program = ts.createProgram(
+          files, {
+            moduleResolution: ts.ModuleResolutionKind.NodeJs,
+            importHelpers: true,
+            lib: [],
+            module: ts.ModuleKind.ESNext,
+            target: ts.ScriptTarget.Latest,
+            declaration: false,
+            experimentalDecorators: true,
+            emitDecoratorMetadata: false,
+          },
+          host);
+      const typeChecker = program.getTypeChecker();
+      const reflectionHost = new TypeScriptReflectionHost(typeChecker);
+      const transformers: ts.CustomTransformers = {
+        before: [getDownlevelDecoratorsTransform(
+            program.getTypeChecker(), reflectionHost, diagnostics,
+            /* isCore */ false, isClosureEnabled, skipClassDecorators)]
+      };
+      return {program, transformers};
+    }
+  });
 });
 
 /** Template string function that can be used to dedent a given string literal. */