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

@ngbot ngbot bot added this to the needsTriage milestone Nov 24, 2019
@googlebot googlebot added the cla: yes label Nov 24, 2019
@JoostK JoostK marked this pull request as ready for review Nov 24, 2019
@JoostK JoostK requested a review from angular/fw-compiler as a code owner Nov 24, 2019
Copy link
Member

petebacondarwin left a comment

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.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2019

Member

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?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2019

Member

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

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Contributor

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

@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-reify-types branch from c730126 to ec355b4 Dec 14, 2019
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-reify-types branch from ec355b4 to 41b0753 Dec 16, 2019
@@ -225,6 +226,11 @@ export class IvyDeclarationDtsTransform implements DtsTransform {
}
}

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

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 18, 2019

Contributor

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);

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 18, 2019

Contributor

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.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 18, 2019

Author Member

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.

@alxhub
alxhub approved these changes Dec 18, 2019
JoostK added 2 commits 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:

```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 force-pushed the JoostK:ngtsc-ttc-reify-types branch from 41b0753 to 04af6fe Dec 19, 2019
@JoostK JoostK dismissed petebacondarwin’s stale review Dec 19, 2019

Feedback has been addressed

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Dec 20, 2019

alxhub added 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 added 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 added 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.