Skip to content

feat(ivy): more accurate type narrowing for ngIf directive - #30248

Closed
JoostK wants to merge 1 commit into
angular:masterfrom
JoostK:ngtsc-template-guards
Closed

JoostK wants to merge 1 commit into
angular:masterfrom
JoostK:ngtsc-template-guards

Conversation

@JoostK

@JoostK JoostK commented May 2, 2019

Copy link
Copy Markdown
Member

A structural directive can specify a template guard for an input, such that
the type of that input's binding can be narrowed based on the guard's return
type. Previously, such template guards could only be methods, of which an
invocation would be inserted into the type-check block (TCB). For NgIf,
the template guard narrowed the type of its expression to be NonNullable
using the following declaration:

export declare class NgIf {
  static ngTemplateGuard_ngIf<E>(dir: NgIf, expr: E): expr is NonNullable<E>
}

This works fine for usages such as *ngIf="person" but starts to introduce
false-positives when e.g. an explicit non-null check like
*ngIf="person !== null" is used, as the method invocation in the TCB
would not have the desired effect of narrowing person to become
non-nullable:

if (NgIf.ngTemplateGuard_ngIf(directive, ctx.person !== null)) {
  // Usages of `ctx.person` within this block would
  // not have been narrowed to be non-nullable.
}

This commit introduces a new strategy for template guards to allow for the
binding expression itself to be used as template guard in the TCB. Now,
the TCB generated for *ngIf="person !== null" would look as follows:

if (ctx.person !== null) {
  // This time `ctx.person` will successfully have
  // been narrowed to be non-nullable.
}

This strategy can be activated by declaring the template guard as a
property declaration with 'binding' as literal return type.

See #30235 for an example where this led to a false positive.

@JoostK JoostK added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels May 2, 2019
@JoostK
JoostK requested review from a team May 2, 2019 22:37
@ngbot ngbot Bot added this to the needsTriage milestone May 2, 2019
@JoostK
JoostK force-pushed the ngtsc-template-guards branch 2 times, most recently from 7c743c1 to 5cf51bf Compare May 3, 2019 19:08
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 15, 2019
@ngbot

ngbot Bot commented May 15, 2019

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    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.

@alxhub

alxhub commented May 15, 2019

Copy link
Copy Markdown
Member

Presubmit

A structural directive can specify a template guard for an input, such that
the type of that input's binding can be narrowed based on the guard's return
type. Previously, such template guards could only be methods, of which an
invocation would be inserted into the type-check block (TCB). For `NgIf`,
the template guard narrowed the type of its expression to be `NonNullable`
using the following declaration:

```typescript
export declare class NgIf {
  static ngTemplateGuard_ngIf<E>(dir: NgIf, expr: E): expr is NonNullable<E>
}
```

This works fine for usages such as `*ngIf="person"` but starts to introduce
false-positives when e.g. an explicit non-null check like
`*ngIf="person !== null"` is used, as the method invocation in the TCB
would not have the desired effect of narrowing `person` to become
non-nullable:

```typescript
if (NgIf.ngTemplateGuard_ngIf(directive, ctx.person !== null)) {
  // Usages of `ctx.person` within this block would
  // not have been narrowed to be non-nullable.
}
```

This commit introduces a new strategy for template guards to allow for the
binding expression itself to be used as template guard in the TCB. Now,
the TCB generated for `*ngIf="person !== null"` would look as follows:

```typescript
if (ctx.person !== null) {
  // This time `ctx.person` will successfully have
  // been narrowed to be non-nullable.
}
```

This strategy can be activated by declaring the template guard as a
property declaration with `'binding'` as literal return type.

See angular#30235 for an example where this led to a false positive.
@alxhub
alxhub force-pushed the ngtsc-template-guards branch from 5cf51bf to 80e5412 Compare May 15, 2019 22:16
@alxhub

alxhub commented May 15, 2019

Copy link
Copy Markdown
Member

@JoostK I rebased this on top of #30362 :)

@alxhub
alxhub requested a review from IgorMinar May 15, 2019 22:18
@alxhub alxhub added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 16, 2019
@alxhub

alxhub commented May 16, 2019

Copy link
Copy Markdown
Member

Caretaker: the public API change in this PR is Ivy-only and not user facing.

@jasonaden jasonaden closed this in e9ead2b May 16, 2019
@JoostK
JoostK deleted the ngtsc-template-guards branch May 18, 2019 20:55

@IgorMinar IgorMinar left a comment

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.

post-submit lgtm, except that this shouldn't have been documented as an "Ivy" api because we don't want "Ivy" to be a thing after it's on by default. At that point it will be just "Angular"

BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…#30248)

A structural directive can specify a template guard for an input, such that
the type of that input's binding can be narrowed based on the guard's return
type. Previously, such template guards could only be methods, of which an
invocation would be inserted into the type-check block (TCB). For `NgIf`,
the template guard narrowed the type of its expression to be `NonNullable`
using the following declaration:

```typescript
export declare class NgIf {
  static ngTemplateGuard_ngIf<E>(dir: NgIf, expr: E): expr is NonNullable<E>
}
```

This works fine for usages such as `*ngIf="person"` but starts to introduce
false-positives when e.g. an explicit non-null check like
`*ngIf="person !== null"` is used, as the method invocation in the TCB
would not have the desired effect of narrowing `person` to become
non-nullable:

```typescript
if (NgIf.ngTemplateGuard_ngIf(directive, ctx.person !== null)) {
  // Usages of `ctx.person` within this block would
  // not have been narrowed to be non-nullable.
}
```

This commit introduces a new strategy for template guards to allow for the
binding expression itself to be used as template guard in the TCB. Now,
the TCB generated for `*ngIf="person !== null"` would look as follows:

```typescript
if (ctx.person !== null) {
  // This time `ctx.person` will successfully have
  // been narrowed to be non-nullable.
}
```

This strategy can be activated by declaring the template guard as a
property declaration with `'binding'` as literal return type.

See angular#30235 for an example where this led to a false positive.

PR Close angular#30248
@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 Sep 15, 2019
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 cla: yes feature Label used to distinguish feature request from other issues merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants