Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Introduce the TemplateTypeChecker abstraction in prep for the Ivy Language Service #38105

Closed
wants to merge 7 commits into from
Closed

Introduce the TemplateTypeChecker abstraction in prep for the Ivy Language Service #38105

wants to merge 7 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jul 17, 2020

No description provided.

@alxhub alxhub added area: language-service Issues related to Angular's VS Code language service area: compiler Issues related to `ngc`, Angular's template compiler labels Jul 17, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jul 17, 2020
@alxhub alxhub force-pushed the ngtsc/ls/ngtypecheck-per-cmp branch 2 times, most recently from feea1b2 to 7057f57 Compare July 20, 2020 20:17
@alxhub
refactor(compiler-cli): make file/shim split 1:n instead of 1:1
b684082 
Previously in the template type-checking engine, it was assumed that every
input file would have an associated type-checking shim. The type check block
code for all components in the input file would be generated into this shim.

This is fine for whole-program type checking operations, but to support the
language service's requirements for low latency, it would be ideal to be
able to check a single component in isolation, especially if the component
is declared along with many others in a single file.

This commit removes the assumption that the file/shim mapping is 1:1, and
introduces the concept of component-to-shim mapping. Any
`TypeCheckingProgramStrategy` must provide such a mapping.

To achieve this:

 * type checking record information is now split into file-level data as
   well as per-shim data.
 * components are now assigned a stable `TemplateId` which is unique to the
   file in which they're declared.
@alxhub alxhub force-pushed the ngtsc/ls/ngtypecheck-per-cmp branch 2 times, most recently from f7419e3 to 91fcf67 Compare July 20, 2020 22:42
@alxhub alxhub requested a review from atscott July 20, 2020 22:57
@alxhub alxhub added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 20, 2020
@alxhub alxhub marked this pull request as ready for review July 20, 2020 23:02
@pullapprove pullapprove bot requested review from josephperrott and kyliau July 20, 2020 23:02
josephperrott
josephperrott approved these changes Jul 21, 2020
View reviewed changes
Copy link
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

Looks good overall for dev-infra, just one comment/request, which isn't really blocking just might be easier long term

packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel Outdated Show resolved Hide resolved
atscott
atscott reviewed Jul 21, 2020
View reviewed changes
packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/context.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/context.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/api/context.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/context.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts Show resolved Hide resolved
@alxhub alxhub force-pushed the ngtsc/ls/ngtypecheck-per-cmp branch from 91fcf67 to bd98066 Compare July 24, 2020 00:43
petebacondarwin
petebacondarwin previously requested changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

There are a bunch of valid CI failures. We should probably hold off on public-api reviews until this is stable.

@pullapprove pullapprove bot requested a review from petebacondarwin July 24, 2020 13:13
@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 24, 2020
alxhub added 2 commits July 24, 2020 17:39
@alxhub
refactor(compiler-cli): introduce the TemplateTypeChecker abstraction
8b1a240 
This commit significantly refactors the 'typecheck' package to introduce a
new abstraction, the `TemplateTypeChecker`. To achieve this:

* a 'typecheck:api' package is introduced, containing common interfaces that
  consumers of the template type-checking infrastructure can depend on
  without incurring a dependency on the template type-checking machinery as
  a whole.
* interfaces for `TemplateTypeChecker` and `TypeCheckContext` are introduced
  which contain the abstract operations supported by the implementation
  classes `TemplateTypeCheckerImpl` and `TypeCheckContextImpl` respectively.
* the `TemplateTypeChecker` interface supports diagnostics on a whole
  program basis to start with, but the implementation is purposefully
  designed to support incremental diagnostics at a per-file or per-component
  level.
* `TemplateTypeChecker` supports direct access to the type check block of a
  component.
* the testing utility is refactored to be a lot more useful, and new tests
  are added for the new abstraction.
@alxhub
refactor(compiler-cli): allow program strategies to opt out of inlining
6762025 
The template type-checking engine relies on the abstraction interface
`TypeCheckingProgramStrategy` to create updated `ts.Program`s for
template type-checking. The basic API is that the type-checking engine
requests changes to certain files in the program, and the strategy provides
an updated `ts.Program`.

Typically, such changes are made to 'ngtypecheck' shim files, but certain
conditions can cause template type-checking to require "inline" operations,
which change user .ts files instead. The strategy used by 'ngc' (the
`ReusedProgramStrategy`) supports these kinds of updates, but other clients
such as the language service might not always support modifying user files.

To accommodate this, the `TypeCheckingProgramStrategy` interface was
modified to include a `supportsInlineOperations` flag. If an implementation
specifies `false` for inline support, the template type-checking system will
return diagnostics on components which would otherwise require inline
operations.

Closes angular#38059
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Jul 30, 2020
@AndrewKushnir
test(compiler-cli): disable one TypeChecker test on Windows due to pa…
6479bce 
…th sensitivity issue

This commit disables one TypeChecker test (added as a part of
angular#38105) which make assertions about the filename while
running on Windows.

Such assertions are currently suffering from a case sensitivity issue.
@AndrewKushnir AndrewKushnir mentioned this pull request Jul 30, 2020
Closed
3 tasks
alxhub pushed a commit that referenced this pull request Jul 30, 2020
@AndrewKushnir @alxhub
test(compiler-cli): disable one TypeChecker test on Windows due to pa…
1eebb7f 
…th sensitivity issue (#38294)

This commit disables one TypeChecker test (added as a part of
#38105) which make assertions about the filename while
running on Windows.

Such assertions are currently suffering from a case sensitivity issue.

PR Close #38294
alxhub pushed a commit that referenced this pull request Jul 30, 2020
@AndrewKushnir @alxhub
test(compiler-cli): disable one TypeChecker test on Windows due to pa…
0cdb523 
…th sensitivity issue (#38294)

This commit disables one TypeChecker test (added as a part of
#38105) which make assertions about the filename while
running on Windows.

Such assertions are currently suffering from a case sensitivity issue.

PR Close #38294
kyliau added a commit to kyliau/angular that referenced this pull request Jul 31, 2020
@kyliau
refactor(language-service): [Ivy[ remove temporary compiler
4167511 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see angular#38105) we no longer need to
keep the temporary compiler implementation.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.
kyliau added a commit to kyliau/angular that referenced this pull request Jul 31, 2020
@kyliau
refactor(language-service): [Ivy[ remove temporary compiler
454d176 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see angular#38105) we no longer need to
keep the temporary compiler implementation.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.
@kyliau kyliau mentioned this pull request Jul 31, 2020
Closed
14 tasks
kyliau added a commit to kyliau/angular that referenced this pull request Jul 31, 2020
@kyliau
refactor(language-service): [Ivy[ remove temporary compiler
a0b5e5e 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see angular#38105) we no longer need to
keep the temporary compiler implementation.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): make file/shim split 1:n instead of 1:1 (angu…
b4b453d 
…lar#38105)

Previously in the template type-checking engine, it was assumed that every
input file would have an associated type-checking shim. The type check block
code for all components in the input file would be generated into this shim.

This is fine for whole-program type checking operations, but to support the
language service's requirements for low latency, it would be ideal to be
able to check a single component in isolation, especially if the component
is declared along with many others in a single file.

This commit removes the assumption that the file/shim mapping is 1:1, and
introduces the concept of component-to-shim mapping. Any
`TypeCheckingProgramStrategy` must provide such a mapping.

To achieve this:

 * type checking record information is now split into file-level data as
   well as per-shim data.
 * components are now assigned a stable `TemplateId` which is unique to the
   file in which they're declared.

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): introduce the TemplateTypeChecker abstraction (…
79b5e18 
…angular#38105)

This commit significantly refactors the 'typecheck' package to introduce a
new abstraction, the `TemplateTypeChecker`. To achieve this:

* a 'typecheck:api' package is introduced, containing common interfaces that
  consumers of the template type-checking infrastructure can depend on
  without incurring a dependency on the template type-checking machinery as
  a whole.
* interfaces for `TemplateTypeChecker` and `TypeCheckContext` are introduced
  which contain the abstract operations supported by the implementation
  classes `TemplateTypeCheckerImpl` and `TypeCheckContextImpl` respectively.
* the `TemplateTypeChecker` interface supports diagnostics on a whole
  program basis to start with, but the implementation is purposefully
  designed to support incremental diagnostics at a per-file or per-component
  level.
* `TemplateTypeChecker` supports direct access to the type check block of a
  component.
* the testing utility is refactored to be a lot more useful, and new tests
  are added for the new abstraction.

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): allow program strategies to opt out of inlini…
271e0f9 
…ng (angular#38105)

The template type-checking engine relies on the abstraction interface
`TypeCheckingProgramStrategy` to create updated `ts.Program`s for
template type-checking. The basic API is that the type-checking engine
requests changes to certain files in the program, and the strategy provides
an updated `ts.Program`.

Typically, such changes are made to 'ngtypecheck' shim files, but certain
conditions can cause template type-checking to require "inline" operations,
which change user .ts files instead. The strategy used by 'ngc' (the
`ReusedProgramStrategy`) supports these kinds of updates, but other clients
such as the language service might not always support modifying user files.

To accommodate this, the `TypeCheckingProgramStrategy` interface was
modified to include a `supportsInlineOperations` flag. If an implementation
specifies `false` for inline support, the template type-checking system will
return diagnostics on components which would otherwise require inline
operations.

Closes angular#38059

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): efficient single-file type checking diagnosti…
3f2257f 
…cs (angular#38105)

Previously, the `TemplateTypeChecker` abstraction allowed fetching
diagnostics for a single file, but under the hood would generate type
checking code for the entire program to satisfy the request.

With this commit, an `OptimizeFor` hint is passed to `getDiagnosticsForFile`
which indicates whether the user intends to request diagnostics for the
whole program or is truly interested in just the single file. If the latter,
the `TemplateTypeChecker` can perform only the work needed to produce
diagnostics for just that file, thus returning answers more efficiently.

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): allow overriding templates in the type checker (
0314fd4 
angular#38105)

This commit adds an `overrideComponentTemplate` operation to the template
type-checker. This operation changes the template used during template
type-checking operations.

Overriding a template causes any previous work for it to be discarded, and
the template type-checking engine will regenerate the TCB for that template
on the next request.

This operation can be used by a consumer such as the language service to
get rapid feedback or diagnostics as the user is editing a template file,
without the need for a full incremental build iteration.

Closes angular#38058

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): add TemplateId to template diagnostics (ang…
4096bbf 
…ular#38105)

Previously, a stable template id was implemented for each component in a
file. This commit adds this id to each `TemplateDiagnostic` generated from
the template type-checker, so it can potentially be used for filtration.

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@alxhub @Splaktar
refactor(compiler-cli): support type-checking a single component (ang…
2968435 
…ular#38105)

This commit adds a method `getDiagnosticsForComponent` to the
`TemplateTypeChecker`, which does the minimum amount of work to retrieve
diagnostics for a single component.

With the normal `ReusedProgramStrategy` this offers virtually no improvement
over the standard `getDiagnosticsForFile` operation, but if the
`TypeCheckingProgramStrategy` supports separate shims for each component,
this operation can yield a faster turnaround for components that are
declared in files with many other components.

PR Close angular#38105
Splaktar pushed a commit to angular-hispano/angular that referenced this pull request Aug 8, 2020
@AndrewKushnir @Splaktar
test(compiler-cli): disable one TypeChecker test on Windows due to pa…
6f54c28 
…th sensitivity issue (angular#38294)

This commit disables one TypeChecker test (added as a part of
angular#38105) which make assertions about the filename while
running on Windows.

Such assertions are currently suffering from a case sensitivity issue.

PR Close angular#38294
kyliau added a commit to kyliau/angular that referenced this pull request Aug 14, 2020
@kyliau
refactor(language-service): [Ivy[ remove temporary compiler
938a247 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see angular#38105) we no longer need to
keep the temporary compiler implementation.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.
kyliau added a commit to kyliau/angular that referenced this pull request Aug 17, 2020
@kyliau
refactor(language-service): [Ivy] remove temporary compiler
7bcd757 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see angular#38105) we no longer need to
keep the temporary compiler implementation.

The temporary compiler was created to enable testing infrastructure to
be developed for the Ivy language service.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.
atscott pushed a commit that referenced this pull request Aug 17, 2020
@kyliau @atscott
refactor(language-service): [Ivy] remove temporary compiler (#38310)
cfe424e 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see #38105) we no longer need to
keep the temporary compiler implementation.

The temporary compiler was created to enable testing infrastructure to
be developed for the Ivy language service.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.

PR Close #38310
atscott pushed a commit that referenced this pull request Aug 17, 2020
@kyliau @atscott
refactor(language-service): [Ivy] remove temporary compiler (#38310)
84d1ba7 
Now that Ivy compiler has a proper `TemplateTypeChecker` interface
(see #38105) we no longer need to
keep the temporary compiler implementation.

The temporary compiler was created to enable testing infrastructure to
be developed for the Ivy language service.

This commit removes the whole `ivy/compiler` directory and moves two
functions `createTypeCheckingProgramStrategy` and
`getOrCreateTypeCheckScriptInfo` to the `LanguageService` class.

Also re-enable the Ivy LS test since it's no longer blocking development.

PR Close #38310
@angular-automatic-lock-bot
Copy link

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 Aug 29, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): make file/shim split 1:n instead of 1:1 (angu…
ac8fcb5 
…lar#38105)

Previously in the template type-checking engine, it was assumed that every
input file would have an associated type-checking shim. The type check block
code for all components in the input file would be generated into this shim.

This is fine for whole-program type checking operations, but to support the
language service's requirements for low latency, it would be ideal to be
able to check a single component in isolation, especially if the component
is declared along with many others in a single file.

This commit removes the assumption that the file/shim mapping is 1:1, and
introduces the concept of component-to-shim mapping. Any
`TypeCheckingProgramStrategy` must provide such a mapping.

To achieve this:

 * type checking record information is now split into file-level data as
   well as per-shim data.
 * components are now assigned a stable `TemplateId` which is unique to the
   file in which they're declared.

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): introduce the TemplateTypeChecker abstraction (…
982dfc2 
…angular#38105)

This commit significantly refactors the 'typecheck' package to introduce a
new abstraction, the `TemplateTypeChecker`. To achieve this:

* a 'typecheck:api' package is introduced, containing common interfaces that
  consumers of the template type-checking infrastructure can depend on
  without incurring a dependency on the template type-checking machinery as
  a whole.
* interfaces for `TemplateTypeChecker` and `TypeCheckContext` are introduced
  which contain the abstract operations supported by the implementation
  classes `TemplateTypeCheckerImpl` and `TypeCheckContextImpl` respectively.
* the `TemplateTypeChecker` interface supports diagnostics on a whole
  program basis to start with, but the implementation is purposefully
  designed to support incremental diagnostics at a per-file or per-component
  level.
* `TemplateTypeChecker` supports direct access to the type check block of a
  component.
* the testing utility is refactored to be a lot more useful, and new tests
  are added for the new abstraction.

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): allow program strategies to opt out of inlini…
63f5167 
…ng (angular#38105)

The template type-checking engine relies on the abstraction interface
`TypeCheckingProgramStrategy` to create updated `ts.Program`s for
template type-checking. The basic API is that the type-checking engine
requests changes to certain files in the program, and the strategy provides
an updated `ts.Program`.

Typically, such changes are made to 'ngtypecheck' shim files, but certain
conditions can cause template type-checking to require "inline" operations,
which change user .ts files instead. The strategy used by 'ngc' (the
`ReusedProgramStrategy`) supports these kinds of updates, but other clients
such as the language service might not always support modifying user files.

To accommodate this, the `TypeCheckingProgramStrategy` interface was
modified to include a `supportsInlineOperations` flag. If an implementation
specifies `false` for inline support, the template type-checking system will
return diagnostics on components which would otherwise require inline
operations.

Closes angular#38059

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): efficient single-file type checking diagnosti…
d6b990d 
…cs (angular#38105)

Previously, the `TemplateTypeChecker` abstraction allowed fetching
diagnostics for a single file, but under the hood would generate type
checking code for the entire program to satisfy the request.

With this commit, an `OptimizeFor` hint is passed to `getDiagnosticsForFile`
which indicates whether the user intends to request diagnostics for the
whole program or is truly interested in just the single file. If the latter,
the `TemplateTypeChecker` can perform only the work needed to produce
diagnostics for just that file, thus returning answers more efficiently.

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): allow overriding templates in the type checker (
c538c3b 
angular#38105)

This commit adds an `overrideComponentTemplate` operation to the template
type-checker. This operation changes the template used during template
type-checking operations.

Overriding a template causes any previous work for it to be discarded, and
the template type-checking engine will regenerate the TCB for that template
on the next request.

This operation can be used by a consumer such as the language service to
get rapid feedback or diagnostics as the user is editing a template file,
without the need for a full incremental build iteration.

Closes angular#38058

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): add TemplateId to template diagnostics (ang…
22fdf96 
…ular#38105)

Previously, a stable template id was implemented for each component in a
file. This commit adds this id to each `TemplateDiagnostic` generated from
the template type-checker, so it can potentially be used for filtration.

PR Close angular#38105
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@alxhub @profanis
refactor(compiler-cli): support type-checking a single component (ang…
6b7811f 
…ular#38105)

This commit adds a method `getDiagnosticsForComponent` to the
`TemplateTypeChecker`, which does the minimum amount of work to retrieve
diagnostics for a single component.

With the normal `ReusedProgramStrategy` this offers virtually no improvement
over the standard `getDiagnosticsForFile` operation, but if the
`TypeCheckingProgramStrategy` supports separate shims for each component,
this operation can yield a faster turnaround for components that are
declared in files with many other components.

PR Close angular#38105
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@atscott atscott atscott approved these changes

@josephperrott josephperrott josephperrott approved these changes

@jelbourn jelbourn jelbourn approved these changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@kyliau kyliau kyliau approved these changes

@petebacondarwin petebacondarwin petebacondarwin left review comments

Assignees

@atscott atscott

Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: language-service Issues related to Angular's VS Code language service cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
8 participants
@alxhub @petebacondarwin @atscott @jelbourn @pkozlowski-opensource @kyliau @josephperrott @googlebot