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

feat(compiler-cli): TemplateTypeChecker -- Get Symbol Of Node In Component Template #38618

Closed
wants to merge 7 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Aug 27, 2020

See individual commits

@atscott atscott added the area: compiler Issues related to `ngc`, Angular's template compiler label Aug 27, 2020
@ngbot ngbot bot added this to the needsTriage milestone Aug 27, 2020
@atscott atscott force-pushed the getSymbolsOfTemplateNode branch 2 times, most recently from 465aa45 to bd15900 Compare August 27, 2020 21:21
@atscott atscott added the target: minor This PR is targeted for the next minor release label Aug 27, 2020
@atscott atscott marked this pull request as ready for review August 27, 2020 22:54
@atscott atscott requested review from alxhub and JoostK August 27, 2020 22:54
@atscott atscott requested review from kyliau and removed request for AndrewKushnir August 27, 2020 22:54
*
* @see Symbol
*/
getSymbolOfNodeInComponentTemplate(node: AST|TmplAstNode, component: ts.ClassDeclaration): Symbol
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any requirement that the component must have a name?
In the current language service and many part of the compiler codebase the anonymous class is not supported.
I wonder if the same applies here.

Copy link
Member

Choose a reason for hiding this comment

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

The compiler will not "detect" components for classes that don't have a name, yes. We have an interface ClassDeclaration that requires name: ts.Identifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a similar note, must it be the case that the class is exported?
In the existing language service, we will not "detect" a class if it does not have a name and/or it's not exported.
This often catches user by surprise, since they get no signal whatsoever why intellisense is not working for the template.

Copy link
Member

Choose a reason for hiding this comment

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

ngtsc itself can compile unexported classes. The issue is that type-checking them requires the "inline" type-checking blocks that the Language Service is not supported. We should probably produce a diagnostic in that case.

packages/language-service/ivy/language_service.ts Outdated Show resolved Hide resolved
packages/language-service/ivy/language_service.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/core/src/compiler.ts Outdated Show resolved Hide resolved
*
* @see Symbol
*/
getSymbolOfNodeInComponentTemplate(node: AST|TmplAstNode, component: ts.ClassDeclaration): Symbol
Copy link
Member

Choose a reason for hiding this comment

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

The compiler will not "detect" components for classes that don't have a name, yes. We have an interface ClassDeclaration that requires name: ts.Identifier.

packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts Outdated Show resolved Hide resolved
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

This is looking really good and super exciting for the LS!

@atscott atscott force-pushed the getSymbolsOfTemplateNode branch 4 times, most recently from b4d8f6c to 5577ce2 Compare August 31, 2020 19:11
/** The `DirectiveSymbol` for the Directive or Component with the binding. */
directive: DirectiveSymbol;

/** The location in the shim file where this binding is declared. */
Copy link
Member

Choose a reason for hiding this comment

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

Expand to include the kind of expression being pointed to - this is a reference to the target member (LHS of the input assignment)

@atscott atscott removed the request for review from devversion September 1, 2020 20:07
@atscott atscott added the action: presubmit The PR is in need of a google3 presubmit label Sep 8, 2020
@atscott
Copy link
Contributor Author

atscott commented Sep 8, 2020

@JoostK @alxhub - Could you review this fixup commit 41509c1? A failure was detected in presubmits where the TcbReferenceOp could not resolve an initializer when checkTemplateBodies was set to false.

I updated the TCB to always add a TcbTemplateContextOp to the queue (which is always var _t1: any = (null!);) but still generate the TcbTemplateBodyOp conditionally based on checkTemplateBodies. This was needed because resolveLocal depends on there being something in the templateCtxOpMap to resolve a TmplAstTemplate ref.

This commit defines the interfaces which outline the information the
`TemplateTypeChecker` can return when requesting a Symbol for an item in the
`TemplateAst`.
Rather than providing the `ts.Symbol`, `ts.Type`, etc.
information in several separate functions, the `TemplateTypeChecker` can
instead provide all the useful information it knows about a particular
node in the `TemplateAst` and allow the callers to determine what to do
with it.
…Checker` Symbol retrieval

The statements generated in the TCB are optimized for performance and producing diagnostics.
These optimizations can result in generating a TCB that does not have all the information
needed by the `TemplateTypeChecker` for retrieving `Symbol`s. For example, as an optimization,
the TCB will not generate variable declaration statements for directives that have no
references, inputs, or outputs. However, the `TemplateTypeChecker` always needs these
statements to be present in order to provide `ts.Symbol`s and `ts.Type`s for the directives.

This commit adds logic to the TCB generation to ensure the required
information is available in a form that the `TemplateTypeChecker` can
consume. It also adds an option to the `NgCompiler` that makes this
generation configurable.
…rom a template node

Specifically, this commit adds support for retrieving a `Symbol` from a
`TmplAstBoundEvent` or `TmplAstBoundAttribute`. Other template nodes
will be supported in following commits.
…lement`s in component template

Adds support to the `TemplateTypeChecker` for retrieving a `Symbol` for
`TmplAstTemplate` and `TmplAstElement` nodes in a component template.
…component template

Adds support to the `TemplateTypeChecker` to get a `Symbol` of an AST
expression in a component template.
Not all expressions will have `ts.Symbol`s (e.g. there is no `ts.Symbol`
associated with the expression `a + b`, but there are for both the a and b
nodes individually).
Adds `TemplateTypeChecker` operation to retrieve the `Symbol` of a
`TmplAstVariable` or `TmplAstReference` in a template.

Sometimes we need to traverse an intermediate variable declaration to arrive at
the correct `ts.Symbol`. For example, loop variables are declared using an intermediate:
```
<div *ngFor="let user of users">
  {{user.name}}
</div>
```
Getting the symbol of user here (from the expression) is tricky, because the TCB looks like:

```
var _t0 = ...; // type of NgForOf
var _t1: any; // context of embedded view for NgForOf structural directive
if (NgForOf.ngTemplateContextGuard(_t0, _t1)) {
  // _t1 is now NgForOfContext<...>
  var _t2 = _t1.$implicit; // let user = '$implicit'
  _t2.name; // user.name expression
}
```
Just getting the `ts.Expression` for the `AST` node `PropRead(ImplicitReceiver, 'user')`
via the sourcemaps will yield the `_t2` expression.  This function recognizes that `_t2`
is a variable declared locally in the TCB, and actually fetch the `ts.Symbol` of its initializer.

These special handlings show the versatility of the `Symbol`
interface defined in the API. With this, when we encounter a template variable,
we can provide the declaration node, as well as specific information
about the variable instance, such as the `ts.Type` and `ts.Symbol`.
@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 9, 2020
@atscott
Copy link
Contributor Author

atscott commented Sep 10, 2020

presubmit

AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
…Checker` Symbol retrieval (#38618)

The statements generated in the TCB are optimized for performance and producing diagnostics.
These optimizations can result in generating a TCB that does not have all the information
needed by the `TemplateTypeChecker` for retrieving `Symbol`s. For example, as an optimization,
the TCB will not generate variable declaration statements for directives that have no
references, inputs, or outputs. However, the `TemplateTypeChecker` always needs these
statements to be present in order to provide `ts.Symbol`s and `ts.Type`s for the directives.

This commit adds logic to the TCB generation to ensure the required
information is available in a form that the `TemplateTypeChecker` can
consume. It also adds an option to the `NgCompiler` that makes this
generation configurable.

PR Close #38618
AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
…rom a template node (#38618)

Specifically, this commit adds support for retrieving a `Symbol` from a
`TmplAstBoundEvent` or `TmplAstBoundAttribute`. Other template nodes
will be supported in following commits.

PR Close #38618
AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
…lement`s in component template (#38618)

Adds support to the `TemplateTypeChecker` for retrieving a `Symbol` for
`TmplAstTemplate` and `TmplAstElement` nodes in a component template.

PR Close #38618
AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
…component template (#38618)

Adds support to the `TemplateTypeChecker` to get a `Symbol` of an AST
expression in a component template.
Not all expressions will have `ts.Symbol`s (e.g. there is no `ts.Symbol`
associated with the expression `a + b`, but there are for both the a and b
nodes individually).

PR Close #38618
AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
#38618)

Adds `TemplateTypeChecker` operation to retrieve the `Symbol` of a
`TmplAstVariable` or `TmplAstReference` in a template.

Sometimes we need to traverse an intermediate variable declaration to arrive at
the correct `ts.Symbol`. For example, loop variables are declared using an intermediate:
```
<div *ngFor="let user of users">
  {{user.name}}
</div>
```
Getting the symbol of user here (from the expression) is tricky, because the TCB looks like:

```
var _t0 = ...; // type of NgForOf
var _t1: any; // context of embedded view for NgForOf structural directive
if (NgForOf.ngTemplateContextGuard(_t0, _t1)) {
  // _t1 is now NgForOfContext<...>
  var _t2 = _t1.$implicit; // let user = '$implicit'
  _t2.name; // user.name expression
}
```
Just getting the `ts.Expression` for the `AST` node `PropRead(ImplicitReceiver, 'user')`
via the sourcemaps will yield the `_t2` expression.  This function recognizes that `_t2`
is a variable declared locally in the TCB, and actually fetch the `ts.Symbol` of its initializer.

These special handlings show the versatility of the `Symbol`
interface defined in the API. With this, when we encounter a template variable,
we can provide the declaration node, as well as specific information
about the variable instance, such as the `ts.Type` and `ts.Symbol`.

PR Close #38618
@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 Oct 11, 2020
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
No open projects
6 participants