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

several cleanups in template type checker #34417

Closed
wants to merge 3 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Dec 15, 2019

Review per commit.

@ngbot ngbot bot added this to the needsTriage milestone Dec 15, 2019
@googlebot googlebot added the cla: yes label Dec 15, 2019
@JoostK JoostK changed the title refactor(ivy): several cleanups in template type checker several cleanups in template type checker Dec 15, 2019
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-absolute-span-cleanup branch from 524a5aa to 027b2f0 Dec 15, 2019
@JoostK JoostK marked this pull request as ready for review Dec 15, 2019
@JoostK JoostK requested a review from angular/fw-compiler as a code owner Dec 15, 2019
@alxhub
alxhub approved these changes Dec 17, 2019
Copy link
Contributor

alxhub left a comment

👍 nice work! The deduplication of error messages is a huge dx improvement.

captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): string {
const id = `tcb${this.nextTcbId++}`;
captureSource(mapping: TemplateSourceMapping, file: ParseSourceFile): TemplateId {
const id = `tcb${this.nextTemplateId++}` as unknown as TemplateId;

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Contributor

Just a nit: the as unknown intermediate cast is unnecessary as TemplateId is already a subtype of string, so a direct cast is legal.

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Dec 17, 2019

JoostK added 3 commits Dec 15, 2019
Previously, the type checker would compute an absolute source span by
combining an expression AST node's `ParseSpan` (relative to the start of
the expression) together with the absolute offset of the expression as
represented in a `ParseSourceSpan`, to arrive at a span relative to the
start of the file. This information is now directly available on an
expression AST node in the `AST.sourceSpan` property, which can be used
instead.
This commit cleans up the template type checker regarding how
diagnostics are produced.
The template type checker generates TypeScript expressions for any
expression that occurs in a template, so that TypeScript can check it
and produce errors. Some expressions as they occur in a template may be
translated into TypeScript code multiple times, for instance a binding
to a directive input that has a template guard.

One example would be the `NgIf` directive, which has a template guard to
narrow the type in the template as appropriate. Given the following
template:

```typescript
@component({
  template: '<div *ngIf="person">{{ person.name }}</div>'
})
class AppComponent {
  person?: { name: string };
}
```

A type check block (TCB) with roughly the following structure is
created:

```typescript
function tcb(ctx: AppComponent) {
  const t1 = NgIf.ngTypeCtor({ ngIf: ctx.person });
  if (ctx.person) {
    "" + ctx.person.name;
  }
}
```

Notice how the `*ngIf="person"` binding is present twice: once in the
type constructor call and once in the `if` guard. As such, TypeScript
will check both instances and would produce duplicate errors, if any
were found.

Another instance is when the safe navigation operator is used, where an
expression such as `person?.name` is emitted into the TCB as
`person != null ? person!.name : undefined`. As can be seen, the
left-hand side expression `person` occurs twice in the TCB.

This commit adds the ability to insert markers into the TCB that
indicate that any errors within the expression should be ignored. This
is similar to `@ts-ignore`, however it can be applied more granularly.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-absolute-span-cleanup branch from 027b2f0 to 9c1026c Dec 17, 2019
@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Dec 18, 2019

merge-assistance: presubmit results should be available in Alex's run yesterday.

kara added a commit that referenced this pull request Dec 18, 2019
Previously, the type checker would compute an absolute source span by
combining an expression AST node's `ParseSpan` (relative to the start of
the expression) together with the absolute offset of the expression as
represented in a `ParseSourceSpan`, to arrive at a span relative to the
start of the file. This information is now directly available on an
expression AST node in the `AST.sourceSpan` property, which can be used
instead.

PR Close #34417
kara added a commit that referenced this pull request Dec 18, 2019
…34417)

This commit cleans up the template type checker regarding how
diagnostics are produced.

PR Close #34417
kara added a commit that referenced this pull request Dec 18, 2019
…rds (#34417)

The template type checker generates TypeScript expressions for any
expression that occurs in a template, so that TypeScript can check it
and produce errors. Some expressions as they occur in a template may be
translated into TypeScript code multiple times, for instance a binding
to a directive input that has a template guard.

One example would be the `NgIf` directive, which has a template guard to
narrow the type in the template as appropriate. Given the following
template:

```typescript
@component({
  template: '<div *ngIf="person">{{ person.name }}</div>'
})
class AppComponent {
  person?: { name: string };
}
```

A type check block (TCB) with roughly the following structure is
created:

```typescript
function tcb(ctx: AppComponent) {
  const t1 = NgIf.ngTypeCtor({ ngIf: ctx.person });
  if (ctx.person) {
    "" + ctx.person.name;
  }
}
```

Notice how the `*ngIf="person"` binding is present twice: once in the
type constructor call and once in the `if` guard. As such, TypeScript
will check both instances and would produce duplicate errors, if any
were found.

Another instance is when the safe navigation operator is used, where an
expression such as `person?.name` is emitted into the TCB as
`person != null ? person!.name : undefined`. As can be seen, the
left-hand side expression `person` occurs twice in the TCB.

This commit adds the ability to insert markers into the TCB that
indicate that any errors within the expression should be ignored. This
is similar to `@ts-ignore`, however it can be applied more granularly.

PR Close #34417
@kara kara closed this in 8c6468a Dec 18, 2019
kara added a commit that referenced this pull request Dec 18, 2019
…34417)

This commit cleans up the template type checker regarding how
diagnostics are produced.

PR Close #34417
kara added a commit that referenced this pull request Dec 18, 2019
…rds (#34417)

The template type checker generates TypeScript expressions for any
expression that occurs in a template, so that TypeScript can check it
and produce errors. Some expressions as they occur in a template may be
translated into TypeScript code multiple times, for instance a binding
to a directive input that has a template guard.

One example would be the `NgIf` directive, which has a template guard to
narrow the type in the template as appropriate. Given the following
template:

```typescript
@component({
  template: '<div *ngIf="person">{{ person.name }}</div>'
})
class AppComponent {
  person?: { name: string };
}
```

A type check block (TCB) with roughly the following structure is
created:

```typescript
function tcb(ctx: AppComponent) {
  const t1 = NgIf.ngTypeCtor({ ngIf: ctx.person });
  if (ctx.person) {
    "" + ctx.person.name;
  }
}
```

Notice how the `*ngIf="person"` binding is present twice: once in the
type constructor call and once in the `if` guard. As such, TypeScript
will check both instances and would produce duplicate errors, if any
were found.

Another instance is when the safe navigation operator is used, where an
expression such as `person?.name` is emitted into the TCB as
`person != null ? person!.name : undefined`. As can be seen, the
left-hand side expression `person` occurs twice in the TCB.

This commit adds the ability to insert markers into the TCB that
indicate that any errors within the expression should be ignored. This
is similar to `@ts-ignore`, however it can be applied more granularly.

PR Close #34417
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 18, 2020

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 Jan 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.