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

perf(ivy): support simple generic type constraints in local type ctors #34021

Closed
wants to merge 2 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Nov 24, 2019

In Ivy's template type checker, type constructors are created for all
directive types to allow for accurate type inference to work. The type
checker has two strategies for dealing with such type constructors:

  1. They can be emitted local to the type check block/type check file.
  2. They can be emitted as static ngTypeCtor field into the directive
    itself.

The first strategy is preferred, as it avoids having to update the
directive type which would cause a more expensive rebuild. However, this
strategy is not suitable for directives that have constrained generic
types, as those constraints would need to be present on the local type
constructor declaration. This is not trivial, as it requires that any
type references within a type parameter's constraint are imported into
the local context of the type check block.

For example, lets consider the NgForOf directive from '@angular/core'
looks as follows:

import {NgIterable} from '@angular/core';

export class NgForOf<T, U extends NgIterable<T>> {}

The type constructor will then have the signature:
(o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U>

Notice how this refers to the type parameters T and U, so the type
constructor needs to be emitted into a scope where those types are
available, and have the correct constraints.

Previously, the template type checker would detect the situation where a
type parameter is constrained, and would emit the type constructor
using strategy 2; within the directive type itself. This approach makes
any type references with the generic type constraints lexically
available:

export class NgForOf<T, U extends NgIterable<T>> {
  static ngTypeCtor<T = any, U extends NgIterable<T> = any>
    (o: Pick<NgForOf<T, U>, 'ngForOf'>): NgForOf<T, U> { return null!; }
}

This commit introduces the ability to reify a type parameter with
constraints into a different context, under the condition that it can
be imported from an absolute module. This allows a generic type
constructor to be emitted into a type check block or type check file
according to strategy 1, as its generic types can be reconstructed using
the reification mechanism:

import * as i0 from '@angular/core';
import * as i1 from '@angular/common';

const _ctor1: <T = any, U extends i0.NgIterable<T> = any>
  (o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U> = null!;

Notice how the generic type constraint of U has resulted in an import
of @angular/core, and the NgIterable is transformed into a qualified
name during the reification process.

Resolves FW-1739

@JoostK JoostK added target: patch This PR is targeted for the next patch release comp: ivy area: compiler Issues related to `ngc`, Angular's template compiler labels Nov 24, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 24, 2019
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 24, 2019
@JoostK JoostK marked this pull request as ready for review November 24, 2019 16:55
@JoostK JoostK requested a review from a team as a code owner November 24, 2019 16:55
Copy link
Member

@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.

This is awesome work @JoostK . I couldn't find anything wrong with the implementation. Only nice tidy code, full tests and an excellent commit message!


I am sorry to bike-shed but "reify" is not the correct term to use here - and it actually made it much harder for me to work out what this PR was about.

Reifying is taking something abstract and making it concrete. This is not what this code is doing.

What this is really about is moving a type to a new file, when it is possible. I.E. the type that is being used in the file where a directive is being defined needs to be moved to the file containing the type checking code.

I believe that if you change all instances of reify to relocate then this makes a lot more sense. E.g.

  private relocateTypeParameters(declaration: ClassDeclaration<ts.ClassDeclaration>):
      ts.TypeParameterDeclaration[]|undefined {
    const relocator = new TypeParameterRelocator(declaration.typeParameters, this.reflector);
    return relocator.relocate(ref => this.referenceType(ref));
  }


// If the declaration has not been imported from an external module, it cannot be reified. The
// reason being that it's unlikely that an import can be written to the declaration from
// anywhere.
Copy link
Member

Choose a reason for hiding this comment

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

When you say anywhere, isn't it true that the type check file will be local to the application being built, so if the original application source file can import the local declaration then the type check file ought to be able to as well?

Copy link
Member

Choose a reason for hiding this comment

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

Can you give an example of when it is not possible to import the dependency?

Copy link
Member

Choose a reason for hiding this comment

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

@petebacondarwin in general, yes, we should be able to import such identifiers. However, I'd like to limit the scope of this change until we've thought through all the ramifications regarding rootDirs and the other quirks of TS module resolution. I'm mostly concerned that the template type-checking file is somewhat randomly placed right now.

@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 Nov 25, 2019
@JoostK JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 16, 2019
@@ -225,6 +226,11 @@ export class IvyDeclarationDtsTransform implements DtsTransform {
}
}

function emitAsSingleLine(node: ts.Node) {
ts.setEmitFlags(node, ts.EmitFlags.SingleLine);
Copy link
Member

Choose a reason for hiding this comment

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

Whoa, cool!

node: ClassDeclaration<ts.ClassDeclaration>, host: ReflectionHost): boolean {
// The class requires an inline type constructor if it has generic type bounds that can not be
// reified into a different context.
return !checkIfGenericTypeBoundsAreReifiable(node, host);
Copy link
Member

@alxhub alxhub Dec 18, 2019

Choose a reason for hiding this comment

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

Change the "reified" terminology here :)

One term we might consider using is "context-free". This means that the type nodes and their associated imports can be reproduced in the type-checking file without mutation - they're not sensitive to the specific file (context) in which they appear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hahaha, the very first iteration of the code started with a function named isTypeContextFree, which became isTypeReifiable as it became more complete. I'll have a look at changing the canEmit terminology into isContextFree to see how clear that would be.

@JoostK JoostK 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 Dec 18, 2019
In Ivy's template type checker, type constructors are created for all
directive types to allow for accurate type inference to work. The type
checker has two strategies for dealing with such type constructors:

1. They can be emitted local to the type check block/type check file.
2. They can be emitted as static `ngTypeCtor` field into the directive
itself.

The first strategy is preferred, as it avoids having to update the
directive type which would cause a more expensive rebuild. However, this
strategy is not suitable for directives that have constrained generic
types, as those constraints would need to be present on the local type
constructor declaration. This is not trivial, as it requires that any
type references within a type parameter's constraint are imported into
the local context of the type check block.

For example, lets consider the `NgForOf` directive from '@angular/core'
looks as follows:

```typescript
import {NgIterable} from '@angular/core';

export class NgForOf<T, U extends NgIterable<T>> {}
```

The type constructor will then have the signature:
`(o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U>`

Notice how this refers to the type parameters `T` and `U`, so the type
constructor needs to be emitted into a scope where those types are
available, _and_ have the correct constraints.

Previously, the template type checker would detect the situation where a
type parameter is constrained, and would emit the type constructor
using strategy 2; within the directive type itself. This approach makes
any type references within the generic type constraints lexically
available:

```typescript
export class NgForOf<T, U extends NgIterable<T>> {
  static ngTypeCtor<T = any, U extends NgIterable<T> = any>
    (o: Pick<NgForOf<T, U>, 'ngForOf'>): NgForOf<T, U> { return null!; }
}
```

This commit introduces the ability to emit a type parameter with
constraints into a different context, under the condition that it can
be imported from an absolute module. This allows a generic type
constructor to be emitted into a type check block or type check file
according to strategy 1, as imports have been generated for all type
references within generic type constraints. For example:

```typescript
import * as i0 from '@angular/core';
import * as i1 from '@angular/common';

const _ctor1: <T = any, U extends i0.NgIterable<T> = any>
  (o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U> = null!;
```

Notice how the generic type constraint of `U` has resulted in an import
of `@angular/core`, and the `NgIterable` is transformed into a qualified
name during the emitting process.

Resolves FW-1739
The compiler has a translation mechanism to convert from an Angular
`Type` to a `ts.TypeNode`, as appropriate. Prior to this change, it
would translate certain Angular expressions into their value equivalent
in TypeScript, instead of the correct type equivalent. This was possible
as the `ExpressionVisitor` interface is not strictly typed, with `any`s
being used for return values.

For example, a literal object was translated into a
`ts.ObjectLiteralExpression`, containing `ts.PropertyAssignment` nodes
as its entries. This has worked without issues as their printed
representation is identical, however it was incorrect from a semantic
point of view. Instead, a `ts.TypeLiteralNode` is created with
`ts.PropertySignature` as its members, which corresponds with the type
declaration of an object literal.
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 19, 2019
@JoostK JoostK dismissed petebacondarwin’s stale review December 19, 2019 21:41

Feedback has been addressed

@alxhub
Copy link
Member

alxhub commented Dec 20, 2019

Updated:

Presubmit
Ivy Presubmit

@alxhub alxhub removed action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 6, 2020
alxhub pushed a commit that referenced this pull request Jan 6, 2020
#34021)

In Ivy's template type checker, type constructors are created for all
directive types to allow for accurate type inference to work. The type
checker has two strategies for dealing with such type constructors:

1. They can be emitted local to the type check block/type check file.
2. They can be emitted as static `ngTypeCtor` field into the directive
itself.

The first strategy is preferred, as it avoids having to update the
directive type which would cause a more expensive rebuild. However, this
strategy is not suitable for directives that have constrained generic
types, as those constraints would need to be present on the local type
constructor declaration. This is not trivial, as it requires that any
type references within a type parameter's constraint are imported into
the local context of the type check block.

For example, lets consider the `NgForOf` directive from '@angular/core'
looks as follows:

```typescript
import {NgIterable} from '@angular/core';

export class NgForOf<T, U extends NgIterable<T>> {}
```

The type constructor will then have the signature:
`(o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U>`

Notice how this refers to the type parameters `T` and `U`, so the type
constructor needs to be emitted into a scope where those types are
available, _and_ have the correct constraints.

Previously, the template type checker would detect the situation where a
type parameter is constrained, and would emit the type constructor
using strategy 2; within the directive type itself. This approach makes
any type references within the generic type constraints lexically
available:

```typescript
export class NgForOf<T, U extends NgIterable<T>> {
  static ngTypeCtor<T = any, U extends NgIterable<T> = any>
    (o: Pick<NgForOf<T, U>, 'ngForOf'>): NgForOf<T, U> { return null!; }
}
```

This commit introduces the ability to emit a type parameter with
constraints into a different context, under the condition that it can
be imported from an absolute module. This allows a generic type
constructor to be emitted into a type check block or type check file
according to strategy 1, as imports have been generated for all type
references within generic type constraints. For example:

```typescript
import * as i0 from '@angular/core';
import * as i1 from '@angular/common';

const _ctor1: <T = any, U extends i0.NgIterable<T> = any>
  (o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U> = null!;
```

Notice how the generic type constraint of `U` has resulted in an import
of `@angular/core`, and the `NgIterable` is transformed into a qualified
name during the emitting process.

Resolves FW-1739

PR Close #34021
alxhub pushed a commit that referenced this pull request Jan 6, 2020
The compiler has a translation mechanism to convert from an Angular
`Type` to a `ts.TypeNode`, as appropriate. Prior to this change, it
would translate certain Angular expressions into their value equivalent
in TypeScript, instead of the correct type equivalent. This was possible
as the `ExpressionVisitor` interface is not strictly typed, with `any`s
being used for return values.

For example, a literal object was translated into a
`ts.ObjectLiteralExpression`, containing `ts.PropertyAssignment` nodes
as its entries. This has worked without issues as their printed
representation is identical, however it was incorrect from a semantic
point of view. Instead, a `ts.TypeLiteralNode` is created with
`ts.PropertySignature` as its members, which corresponds with the type
declaration of an object literal.

PR Close #34021
@alxhub alxhub closed this in f27187c Jan 6, 2020
alxhub pushed a commit that referenced this pull request Jan 6, 2020
The compiler has a translation mechanism to convert from an Angular
`Type` to a `ts.TypeNode`, as appropriate. Prior to this change, it
would translate certain Angular expressions into their value equivalent
in TypeScript, instead of the correct type equivalent. This was possible
as the `ExpressionVisitor` interface is not strictly typed, with `any`s
being used for return values.

For example, a literal object was translated into a
`ts.ObjectLiteralExpression`, containing `ts.PropertyAssignment` nodes
as its entries. This has worked without issues as their printed
representation is identical, however it was incorrect from a semantic
point of view. Instead, a `ts.TypeLiteralNode` is created with
`ts.PropertySignature` as its members, which corresponds with the type
declaration of an object literal.

PR Close #34021
JoostK added a commit to JoostK/angular that referenced this pull request Jan 18, 2020
In angular#34021 the ngtsc compiler gained the ability to emit type parameter
constraints, which would generate imports for any type reference that
is used within the constraint. However, the `AbsoluteModuleStrategy`
reference emitter strategy did not consider interface declarations as a
valid declaration it can generate an import for, throwing an error
instead.

This commit fixes the issue by including interface declarations in the
logic that determines whether something is a declaration.

Fixes angular#34837
JoostK added a commit to JoostK/angular that referenced this pull request Feb 3, 2020
In angular#34021 the ngtsc compiler gained the ability to emit type parameter
constraints, which would generate imports for any type reference that
is used within the constraint. However, the `AbsoluteModuleStrategy`
reference emitter strategy did not consider interface declarations as a
valid declaration it can generate an import for, throwing an error
instead.

This commit fixes the issue by including interface declarations in the
logic that determines whether something is a declaration.

Fixes angular#34837
mhevery pushed a commit that referenced this pull request Feb 4, 2020
)

In #34021 the ngtsc compiler gained the ability to emit type parameter
constraints, which would generate imports for any type reference that
is used within the constraint. However, the `AbsoluteModuleStrategy`
reference emitter strategy did not consider interface declarations as a
valid declaration it can generate an import for, throwing an error
instead.

This commit fixes the issue by including interface declarations in the
logic that determines whether something is a declaration.

Fixes #34837

PR Close #34849
mhevery pushed a commit that referenced this pull request Feb 4, 2020
)

In #34021 the ngtsc compiler gained the ability to emit type parameter
constraints, which would generate imports for any type reference that
is used within the constraint. However, the `AbsoluteModuleStrategy`
reference emitter strategy did not consider interface declarations as a
valid declaration it can generate an import for, throwing an error
instead.

This commit fixes the issue by including interface declarations in the
logic that determines whether something is a declaration.

Fixes #34837

PR Close #34849
@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 Feb 6, 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants