Skip to content

fix(compiler-cli): ensure the compiler tracks ts.Programs correctly#41291

Closed
alxhub wants to merge 1 commit intoangular:masterfrom
alxhub:ngtsc/what-is-the-current-program-really
Closed

fix(compiler-cli): ensure the compiler tracks ts.Programs correctly#41291
alxhub wants to merge 1 commit intoangular:masterfrom
alxhub:ngtsc/what-is-the-current-program-really

Conversation

@alxhub
Copy link
Copy Markdown
Member

@alxhub alxhub commented Mar 20, 2021

NgCompiler previously had a notion of the "next" ts.Program, which
served two purposes:

  • it allowed a client using the ts.createProgram API to query for the
    latest program produced by the previous NgCompiler, as a starting
    point for building the next program that incorporated any new user
    changes.

  • it allowed the old NgCompiler to be queried for the ts.Program on
    which all prior state is based, which is needed to compute the delta
    from the new program to ultimately determine how much of the prior
    state can be reused.

This system contained a flaw: it relied on the NgCompiler knowing when
the ts.Program would be changed. This works fine for changes that
originate in NgCompiler APIs, but a client of the TemplateTypeChecker
may use that API in ways that create new ts.Programs without the
NgCompiler's knowledge. This caused the NgCompiler's concept of the
"next" program to get out of sync, causing incorrectness in future
incremental analysis.

This refactoring cleans up the compiler's ts.Program management in
several ways:

  • TypeCheckingProgramStrategy, the API which controls ts.Program
    updating, is renamed to the ProgramDriver and extracted to a separate
    ngtsc package.

  • It loses its responsibility of determining component shim filenames. That
    functionality now lives exclusively in the template type-checking package.

  • The "next" ts.Program concept is renamed to the "current" program, as
    the "next" name was misleading in several ways.

  • NgCompiler now wraps the ProgramDriver used in the
    TemplateTypeChecker to know when a new ts.Program is created,
    regardless of which API drove the creation, which actually fixes the bug.

@google-cla google-cla Bot added the cla: yes label Mar 20, 2021
@pullapprove pullapprove Bot requested review from atscott and josephperrott March 20, 2021 00:27
@alxhub alxhub force-pushed the ngtsc/what-is-the-current-program-really branch from 19bc8e2 to 74d4ca6 Compare March 22, 2021 18:28
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the input program (program#1) hasn't changed, but there are some operations that cause additional private TCB code to be generated after step 2, is the new program generated based on (1) or (2)?
Could you please clarify this in the example above?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is describing "under a normal compilation", meaning the normal functioning of ngc with no usage of TemplateTypeChecker APIs directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a section to clarify how such a client might consume TsCreateProgramDriver and manage programs that way.

@josephperrott josephperrott added the area: compiler Issues related to `ngc`, Angular's template compiler label Mar 23, 2021
@ngbot ngbot Bot added this to the Backlog milestone Mar 23, 2021
@alxhub alxhub force-pushed the ngtsc/what-is-the-current-program-really branch from 74d4ca6 to 2f62e5d Compare March 24, 2021 17:54
@alxhub alxhub added the target: minor This PR is targeted for the next minor release label Mar 24, 2021
@alxhub alxhub force-pushed the ngtsc/what-is-the-current-program-really branch from 2f62e5d to 51ac5d9 Compare March 24, 2021 21:23
Copy link
Copy Markdown
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: dev-infra

`NgCompiler` previously had a notion of the "next" `ts.Program`, which
served two purposes:

* it allowed a client using the `ts.createProgram` API to query for the
  latest program produced by the previous `NgCompiler`, as a starting
  point for building the _next_ program that incorporated any new user
  changes.

* it allowed the old `NgCompiler` to be queried for the `ts.Program` on
  which all prior state is based, which is needed to compute the delta
  from the new program to ultimately determine how much of the prior
  state can be reused.

This system contained a flaw: it relied on the `NgCompiler` knowing when
the `ts.Program` would be changed. This works fine for changes that
originate in `NgCompiler` APIs, but a client of the `TemplateTypeChecker`
may use that API in ways that create new `ts.Program`s without the
`NgCompiler`'s knowledge. This caused the `NgCompiler`'s concept of the
"next" program to get out of sync, causing incorrectness in future
incremental analysis.

This refactoring cleans up the compiler's `ts.Program` management in
several ways:

* `TypeCheckingProgramStrategy`, the API which controls `ts.Program`
  updating, is renamed to the `ProgramDriver` and extracted to a separate
  ngtsc package.

* It loses its responsibility of determining component shim filenames. That
  functionality now lives exclusively in the template type-checking package.

* The "next" `ts.Program` concept is renamed to the "current" program, as
  the "next" name was misleading in several ways.

* `NgCompiler` now wraps the `ProgramDriver` used in the
  `TemplateTypeChecker` to know when a new `ts.Program` is created,
  regardless of which API drove the creation, which actually fixes the bug.
@alxhub alxhub force-pushed the ngtsc/what-is-the-current-program-really branch from 7246174 to 9019cc2 Compare April 7, 2021 19:30
@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Apr 8, 2021
@alxhub alxhub removed the request for review from atscott April 8, 2021 13:26
@zarend zarend closed this in deacc74 Apr 8, 2021
kyliau added a commit to kyliau/angular that referenced this pull request Apr 8, 2021
…rFactory

With the work done in angular#41291, the compiler always tracks the last known
program, so there's no need to track the program in the compiler factory
anymore.
zarend pushed a commit that referenced this pull request Apr 9, 2021
…rFactory (#41517)

With the work done in #41291, the compiler always tracks the last known
program, so there's no need to track the program in the compiler factory
anymore.

PR Close #41517
@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators May 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants