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

JoostK · 2021-01-09T14:13:46Z

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

…large number of files 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 angular#40276

…rm for large number of files

petebacondarwin · 2021-01-09T18:39:25Z

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

+      program.emit(undefined, (fileName, outputText) => {
+        written++;
+
+        if (!outputText.includes(`import { Foo } from './foo';`)) {


Is this string match a bit brittle to rendering changes? Perhaps a regex might be a bit less fragile?

I wouldn't consider it brittle, as a change in rendering would not incorrectly allow the test to pass even if the bug was reintroduced. Regarding stability I wouldn't worry about it in this case.

No but it could cause the test to incorrectly fail. For example if the whitespace around the braces changed or the quote character used for the string. How about:

Suggested change

if (!outputText.includes(`import { Foo } from './foo';`)) {

if (!/import\s*{\s*Foo\s*}\s*from\s*['"]\.\/foo['"];/.test(outputText)) {

Although I accept that is pretty ugly.

…rm for large number of files

JoostK · 2021-01-09T22:05:05Z

Note for reviewer: I went through several iterations of this fix after CI feedback; the individual fixup commit still contain the history but it's advised to review this in full, as all intermediate commits are no longer relevant.

petebacondarwin

Thanks for fixing this @JoostK.

I found the patch code quite difficult to reason about, and I think it might have been easier if there was more consistency in the naming of the variables, symbol, functions etc. So I offered a few suggestions that you might like to consider.

petebacondarwin · 2021-01-10T11:57:56Z

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

@@ -17,9 +17,12 @@ interface TransformationContextWithResolver extends ts.TransformationContext {
  getEmitResolver: () => EmitResolver;
 }

+const patchedReferencesAliasesSymbol = Symbol('patchedAliasedReferences');


For consistency...

Suggested change

const patchedReferencesAliasesSymbol = Symbol('patchedAliasedReferences');

const patchedReferencedAliasesSymbol = Symbol('patchedAliasedReferences');

and

Suggested change

const patchedReferencesAliasesSymbol = Symbol('patchedAliasedReferences');

const patchedReferencesAliasesSymbol = Symbol('patchedReferencedAliases');

petebacondarwin · 2021-01-10T12:02:14Z

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

+  }
+
+  const referencedAliases = new Set<ts.Declaration>();
+
  const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration;


For consistency...

Suggested change

const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration;

const originalIsReferencedAliasDeclaration = emitResolver.isReferencedAliasDeclaration;

petebacondarwin · 2021-01-10T12:04:00Z

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

  const originalReferenceResolution = 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) {
    throwIncompatibleTransformationContextError();
-    return;
  }
  emitResolver.isReferencedAliasDeclaration = function(node, ...args) {


We should also change this function declaration to avoid the unnecessary variable argument:

Suggested change

emitResolver.isReferencedAliasDeclaration = function(node, ...args) {

emitResolver.isReferencedAliasDeclaration = function(node, checkChildren) {

I kept it as is on purpose, to avoid potential issues with TS updates which could introduce additional parameters without us being aware (as this is internal API, we don't have accurate type-checking of this call).

In that case I would propose that we change the EmitResolver interface above to have the same signature.

Good idea, done.

petebacondarwin · 2021-01-10T12:05:00Z

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

+  emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;
+  return referencedAliases;


FWIW this could be:

Suggested change

emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;

return referencedAliases;

return emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;

I know; I was trying to avoid that succinctness here but don't feel too strongly about it, so changed as suggested.

petebacondarwin · 2021-01-10T12:12:02Z

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

+    // 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`.


Suggested change

// about this in the description for `patchAliasReferenceResolution`.

// about this in the description for `loadAliasReferenceResolutionPatchOrDie `.

petebacondarwin · 2021-01-10T12:14:11Z

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

 * 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(
-    context: ts.TransformationContext, referencedAliases: Set<ts.Declaration>): void {
+export function loadAliasReferenceResolutionPatchOrDie(context: ts.TransformationContext):


For consistency:

Suggested change

export function loadAliasReferenceResolutionPatchOrDie(context: ts.TransformationContext):

export function loadIsReferencedAliasDeclarationPatchOrDie(context: ts.TransformationContext):

And I would probably drop the OrDie postfix as it is just an implementation aspect.

petebacondarwin · 2021-01-10T12:19:28Z

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

+      program.emit(undefined, (fileName, outputText) => {
+        written++;
+
+        if (!outputText.includes(`import { Foo } from './foo';`)) {


No but it could cause the test to incorrectly fail. For example if the whitespace around the braces changed or the quote character used for the string. How about:

Suggested change

if (!outputText.includes(`import { Foo } from './foo';`)) {

if (!/import\s*{\s*Foo\s*}\s*from\s*['"]\.\/foo['"];/.test(outputText)) {

Although I accept that is pretty ugly.

petebacondarwin · 2021-01-10T13:52:07Z

Actually I just had another thought about memory leaking... What is the life-time of the context and also of its emitScope?
The old implementation created a new referencedParameterTypes set for each transformation run, but the new implementation will create a set only for each emitScope. My concern is that this set could grow very large if the emitScope is used over and over. Perhaps we need to clear the set at the end of each transformation run?

JoostK · 2021-01-10T14:02:36Z

Actually I just had another thought about memory leaking... What is the life-time of the context and also of its emitScope?
The old implementation created a new referencedParameterTypes set for each transformation run, but the new implementation will create a set only for each emitScope. My concern is that this set could grow very large if the emitScope is used over and over. Perhaps we need to clear the set at the end of each transformation run?

So I thought about this as well, and my understanding is that we're not collecting any more data than before. We're now storing everything in a single set, whereas before we'd have individual sets but they would all be retained in the captured closure of a previously installed patch (exactly what this PR is fixing). I did consider clearing the set at the start of the transformer but did not end up doing it (nor trying it) as we'd be assuming that EmitResolver.isReferencedAliasDeclaration is actually called before the next source file is transformed, which is not necessarily a safe assumption to make. Since we were not reclaiming the set's memory before this fix, I didn't want to introduce this potential risk.

…rm for large number of files

AndrewKushnir · 2021-01-11T07:27:15Z

Presubmit.

…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

Follow the fix angular/angular#40374

angular-automatic-lock-bot · 2021-02-11T16:34:44Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

JoostK added type: bug/fix target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Jan 9, 2021

ngbot bot modified the milestone: Backlog Jan 9, 2021

google-cla bot added the cla: yes label Jan 9, 2021

JoostK mentioned this pull request Jan 9, 2021

@angular/compiler-cli downleveling decorators possibly causing RangeError: Maximum call stack size exceeded with isReferencedAliasDeclaration #40276

Closed

JoostK force-pushed the decorator-transform-stackoverflow branch from 9235c03 to f14a953 Compare January 9, 2021 17:21

JoostK added 2 commits January 9, 2021 20:01

fixup! fix(compiler-cli): prevent stack overflow in decorator transfo…

5434613

…rm for large number of files

JoostK force-pushed the decorator-transform-stackoverflow branch from f14a953 to 5434613 Compare January 9, 2021 19:02

fixup! fix(compiler-cli): prevent stack overflow in decorator transfo…

888997f

…rm for large number of files

petebacondarwin reviewed Jan 9, 2021

View reviewed changes

fixup! fix(compiler-cli): prevent stack overflow in decorator transfo…

1a3ecd0

…rm for large number of files

JoostK marked this pull request as ready for review January 9, 2021 22:03

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 9, 2021

JoostK requested a review from petebacondarwin January 9, 2021 22:05

petebacondarwin approved these changes Jan 10, 2021

View reviewed changes

JoostK added 2 commits January 10, 2021 15:31

fixup! fix(compiler-cli): prevent stack overflow in decorator transfo…

4ab0772

…rm for large number of files

fixup! fix(compiler-cli): prevent stack overflow in decorator transfo…

9624cec

…rm for large number of files

JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 10, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 11, 2021

atscott closed this in 27d0e54 Jan 11, 2021

ahnpnl mentioned this pull request Jan 14, 2021

refactor(transformers): update downlevel ctor transformer thymikee/jest-preset-angular#730

Merged

2 tasks

ahnpnl added a commit to thymikee/jest-preset-angular that referenced this pull request Jan 14, 2021

refactor(transformers): update downlevel ctor transformer (#730)

c124286

Follow the fix angular/angular#40374

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JoostK commented Jan 9, 2021

petebacondarwin Jan 9, 2021

JoostK Jan 9, 2021

petebacondarwin Jan 10, 2021

JoostK commented Jan 9, 2021

petebacondarwin left a comment

petebacondarwin Jan 10, 2021

petebacondarwin Jan 10, 2021

petebacondarwin Jan 10, 2021

JoostK Jan 10, 2021

petebacondarwin Jan 10, 2021

JoostK Jan 10, 2021

petebacondarwin Jan 10, 2021

JoostK Jan 10, 2021

petebacondarwin Jan 10, 2021

petebacondarwin Jan 10, 2021

petebacondarwin Jan 10, 2021

petebacondarwin commented Jan 10, 2021

JoostK commented Jan 10, 2021

AndrewKushnir commented Jan 11, 2021

angular-automatic-lock-bot bot commented Feb 11, 2021

	if (!outputText.includes(`import { Foo } from './foo';`)) {
	if (!/import\s{\sFoo\s}\sfrom\s*['"]\.\/foo['"];/.test(outputText)) {

	const patchedReferencesAliasesSymbol = Symbol('patchedAliasedReferences');
	const patchedReferencedAliasesSymbol = Symbol('patchedAliasedReferences');

	const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration;
	const originalIsReferencedAliasDeclaration = emitResolver.isReferencedAliasDeclaration;

	emitResolver.isReferencedAliasDeclaration = function(node, ...args) {
	emitResolver.isReferencedAliasDeclaration = function(node, checkChildren) {

		emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;
		return referencedAliases;

	emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;
	return referencedAliases;
	return emitResolver[patchedReferencesAliasesSymbol] = referencedAliases;

	// about this in the description for `patchAliasReferenceResolution`.
	// about this in the description for `loadAliasReferenceResolutionPatchOrDie `.

	export function loadAliasReferenceResolutionPatchOrDie(context: ts.TransformationContext):
	export function loadIsReferencedAliasDeclarationPatchOrDie(context: ts.TransformationContext):

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

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

Conversation

JoostK commented Jan 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK commented Jan 9, 2021

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 10, 2021

JoostK commented Jan 10, 2021

AndrewKushnir commented Jan 11, 2021

angular-automatic-lock-bot bot commented Feb 11, 2021