Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ivy): avoid infinite recursion when evaluation source files #33772

Closed
wants to merge 1 commit into from

Conversation

@JoostK
Copy link
Member

JoostK commented Nov 12, 2019

When ngtsc comes across a source file during partial evaluation, it
would determine all exported symbols from that module and evaluate their
values greedily. This greedy evaluation strategy introduces unnecessary
work and can fall into infinite recursion when the evaluation result of
an exported expression would circularly depend on the source file. This
would primarily occur in CommonJS code, where the exports variable can
be used to refer to an exported variable. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the exports variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending u
evaluating the exports variable again. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the exports variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending up
evaluating the exports variable again. This went on for some time
until all stack frames were exhausted.

This commit introduces a ResolvedModule that delays the evaluation of
its exports until they are actually requested. This avoids the circular
dependency when evaluating exports, thereby fixing the issue.

Fix #33734

When ngtsc comes across a source file during partial evaluation, it
would determine all exported symbols from that module and evaluate their
values greedily. This greedy evaluation strategy introduces unnecessary
work and can fall into infinite recursion when the evaluation result of
an exported expression would circularly depend on the source file. This
would primarily occur in CommonJS code, where the `exports` variable can
be used to refer to an exported variable. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the `exports` variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending u
evaluating the `exports` variable again. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the `exports` variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending up
evaluating the `exports` variable again. This went on for some time
until all stack frames were exhausted.

This commit introduces a `ResolvedModule` that delays the evaluation of
its exports until they are actually requested. This avoids the circular
dependency when evaluating `exports`, thereby fixing the issue.

Fix #33734
@VladimirAmiorkov

This comment has been minimized.

Copy link

VladimirAmiorkov commented Nov 18, 2019

What is the simplest way of building an Angular with this PR? I want to test if #33734 is resolved by it.

@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Nov 18, 2019

@VladimirAmiorkov You can find instructions here on using artifacts from CircleCI for this PR:

https://github.com/angular/angular/blob/master/docs/DEVELOPER.md#getting-packages-from-build-artifacts

@VladimirAmiorkov

This comment has been minimized.

Copy link

VladimirAmiorkov commented Nov 18, 2019

@JoostK

Thanks for this info. Looking at the builds and the note that the artefacts may not be averrable I feel like they already are not. I checked here https://circleci.com/gh/angular/angular/524826#artifacts but cant seem to find any artefacts, that build was 6 days ago so maybe I am late to get them :) . Or maybe I dont have access to the "Artefacts" tab, this is what I see on this PR and all others I have checked.
Screenshot 2019-11-18 at 17 32 09

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 18, 2019

@VladimirAmiorkov, you can find the latest artifacts from this PR here: https://circleci.com/gh/angular/angular/524805#artifacts

(Hint: It's the publish_packages_as_artifacts job, not the publish_snapshot job 😉)

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 18, 2019

(FWIW, I tried this with #33734 and I got a different error, so I guess the "Maximum call stack size exceeded" error is fixed 🎉)

@alxhub
alxhub approved these changes Nov 19, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 19, 2019

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 20, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Nov 20, 2019

merge-assistance: size failure is unrelated.

alxhub added a commit that referenced this pull request Nov 20, 2019
When ngtsc comes across a source file during partial evaluation, it
would determine all exported symbols from that module and evaluate their
values greedily. This greedy evaluation strategy introduces unnecessary
work and can fall into infinite recursion when the evaluation result of
an exported expression would circularly depend on the source file. This
would primarily occur in CommonJS code, where the `exports` variable can
be used to refer to an exported variable. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the `exports` variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending u
evaluating the `exports` variable again. This variable would be
resolved to the source file itself, thereby greedily evaluating all
exported symbols and thus ending up evaluating the `exports` variable
again. This variable would be resolved to the source file itself,
thereby greedily evaluating all exported symbols and thus ending up
evaluating the `exports` variable again. This went on for some time
until all stack frames were exhausted.

This commit introduces a `ResolvedModule` that delays the evaluation of
its exports until they are actually requested. This avoids the circular
dependency when evaluating `exports`, thereby fixing the issue.

Fix #33734

PR Close #33772
@alxhub alxhub closed this in b07b6f1 Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.