The Wayback Machine - https://web.archive.org/web/20210908171643/https://github.com/angular/angular/pull/42492
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

Ngtsc/smarter type emitter #42492

Closed
wants to merge 5 commits into from
Closed

Conversation

@JoostK
Copy link
Member

@JoostK JoostK commented Jun 5, 2021

See individual commits

@ngbot ngbot bot modified the milestone: Backlog Jun 5, 2021
@google-cla google-cla bot added the cla: yes label Jun 5, 2021
@JoostK JoostK marked this pull request as ready for review Jun 7, 2021
@JoostK JoostK requested a review from alxhub Jun 7, 2021
Copy link
Member

@petebacondarwin petebacondarwin left a comment

From first commit message:

The template type checker is capable of recreating generic type bounds
in a different, ...

It looks like this phrase was not completed?

when the type
parameter emitting logic was introduces

typo

fail('canEmit must be true when emitting succeeds');
}

const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 11, 2021
Member

Is this to help when running on Windows?

This comment has been minimized.

@JoostK

JoostK Jun 12, 2021
Author Member

Indeed. I haven't seen them fail but remembered from earlier that Windows would have used \r\n by default, causing assertions using \n to fail.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

From the second commit message:

Type parameters with a default clause would incorrectly be emitted into
the typecheck file using the original `ts.TypeNode` for the default
clause, such that `ts.TypeReferenceNode`s within the default clause
would be invalid.

I didn't quite understand this sentence. Are you saying that it is not valid to have a ts.TypeNode inside a ts.TypeReferenceNode?

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Nice work @JoostK.
If this PR means that the fallback is no longer necessary, should it be removed too?

@@ -10,7 +10,7 @@ import {Reference} from '../../imports';

/**
* A resolved type reference can either be a `Reference`, the original `ts.TypeReferenceNode` itself
* or null to indicate the no reference could be resolved.
* or null to indicate the no reference could be resolved, or that the reference can not be emitted.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 11, 2021
Member
Suggested change
* or null to indicate the no reference could be resolved, or that the reference can not be emitted.
* or null to indicate that no reference could be resolved, or that the reference can not be emitted.

Or even better start a new sentence?

Suggested change
* or null to indicate the no reference could be resolved, or that the reference can not be emitted.
* or null.
* A value of null indicates that no reference could be resolved or that the reference can not be emitted.

// emitting.
if (reference instanceof Reference && !reference.hasOwningModuleGuess) {
return false;
// If the type is a reference, consider the type not to be eligible for emitting.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 11, 2021
Member
Suggested change
// If the type is a reference, consider the type not to be eligible for emitting.
// If the type is a reference, consider the type to be eligible for emitting.

???

});

it('cannot emit references to local declarations as nested type arguments', () => {
const emitter = createEmitter(`
import {NgIterable} from '@angular/core';
export class Local {};
class Local {};

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 11, 2021
Member

So would this test pass if Local was exported?
If so, should we add a test to demonstrate that?

This comment has been minimized.

@JoostK

JoostK Jun 12, 2021
Author Member

If Local were exported, then the type emitter would indeed have succeeded.

There's a test here: https://github.com/angular/angular/pull/42492/files#diff-66249b018abab813af0a8497c3308ede071069a30ebb22dbb4c860559ef1b406R119-R127

expect(() => emit(emitter))
.toThrowError('A type reference to emit must be imported from an absolute module');
expect(emitter.canEmit()).toBe(true);
expect(emit(emitter)).toEqual('<T extends test.Internal>');

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 11, 2021
Member

Where does this test namespace come from and how would that look in practice?

This comment has been minimized.

@JoostK

JoostK Jun 12, 2021
Author Member

The test namespace comes from the test setup that creates a fake type reference node:

const emitted = emitter.emit(ref => {
const typeName = ts.createQualifiedName(ts.createIdentifier('test'), ref.debugName!);
return ts.createTypeReferenceNode(typeName, /* typeArguments */ undefined);
});

In the actual code, the rewritten type reference is created by ngtsc's ReferenceEmitter from the type-check's environment:

referenceType(ref: Reference): ts.TypeNode {
const ngExpr = this.refEmitter.emit(
ref, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports);
// Create an `ExpressionType` from the `Expression` and translate it via `translateType`.
// TODO(alxhub): support references to types with generic arguments in a clean way.
return translateType(new ExpressionType(ngExpr.expression), this.importManager);
}

@JoostK JoostK force-pushed the JoostK:ngtsc/smarter-type-emitter branch from a4cee78 to 1b610d0 Jun 12, 2021
@JoostK
Copy link
Member Author

@JoostK JoostK commented Jun 12, 2021

I didn't quite understand this sentence. Are you saying that it is not valid to have a ts.TypeNode inside a ts.TypeReferenceNode?

I added a note to clarify that an untranslated ts.TypeReferenceNode would likely be invalid in the type-check file, as there won't be an import statement available in the type-check file if it isn't translated.

If this PR means that the fallback is no longer necessary, should it be removed too?

The fallback continues to be necessary, as non-exported declarations cannot be translated by the type-emitter.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jun 14, 2021

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jun 15, 2021

@JoostK FYI presubmits went well for the changes in this PR, but it looks like it'd need an LGTM from @alxhub.

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jun 17, 2021

Since this is still awaiting @alxhub review, I will go ahead and remove it from the merge queue for now. Please re-add action: merge when ready, or let me know if you want to proceed now.

@alxhub
alxhub approved these changes Jun 18, 2021
if (ts.isTypeReferenceNode(node) && !canEmitTypeReference(node)) {
return INELIGIBLE;
} else {
return ts.forEachChild(node, visitNode);

This comment has been minimized.

@alxhub

alxhub Jun 18, 2021
Contributor

This is getting a bit too clever... it seems we're relying on the fact that this returns undefined for all other kinds of type nodes, correct? At the very least, we should have a comment here explaining:

  1. what kinds of nodes we want to return INELIGIBLE for, and why.
  2. the mechanism by which eligible nodes end up getting selected.

return visitNode(type) !== INELIGIBLE;
}

function visitNode(node: ts.Node): typeof INELIGIBLE|undefined {

This comment has been minimized.

@alxhub

alxhub Jun 18, 2021
Contributor

typeof INELIGIBLE here is {}, the empty interface type. Lots of things are assignable to it, including false.

If we want to make a singleton value, it'd be better to use a branded type instead:

type INELIGIBLE = {__brand: 'ineligible'};
const INELIGIBLE: INELIGIBLE = {} as INELIGIBLE;

This ensures we don't accidentally create other values of this type besides the constant.

This comment has been minimized.

@JoostK

JoostK Jun 19, 2021
Author Member

Good point, updated!

JoostK added 5 commits Jun 5, 2021
The template type checker is capable of recreating generic type bounds
in a different context, rewriting type references along the way (if
possible). This was previously done using a visitor that only supported
a limited set of types, resulting in the inability to emit all sorts of
types (even if they don't contain type references at all).

The inability to emit generic type bounds was not critical when the type
parameter emitting logic was introduced, as the compiler also has a
fallback strategy of creating inline type constructors. However, this
fallback is not available to the language service, resulting in
inaccurate types when components/directives use a complex generic type.

To mitigate this problem, the specialized visitor has been replaced with
a generalized TypeScript transform, where only type references get
special treatment. This allows for more complex types to be emitted,
such as union and intersection types, object literal types and tuple
types.
…r default

When a component/directive has a generic type parameter, the template
type checker attempts to translate the type parameter such that the
type parameters can be replicated in the type constructor that is
emitted into the typecheck file.

Type parameters with a default clause would incorrectly be emitted into
the typecheck file using the original `ts.TypeNode` for the default
clause, such that `ts.TypeReferenceNode`s within the default clause
would likely be invalid (i.e. referencing a type for which no import is
present in the typecheck file). This did not result in user-facing
type-check errors as errors reported in type constructors are not
translated into template positions Regardless, this commit ensures that
`ts.TypeReferenceNode`s within defaults are properly translated into the
typecheck file.
… emitter

Previously, the template type checker would only opt-in to inline type
constructors if it could import all type references from absolute module
specifiers. This limitation was put into place in an abundance of
caution as there was a safe, but less performant, fallback available.

The language service is not capable of using this fallback, which now
means that the limitation of absolute module specifiers limits the
language service's ability to use accurate types for component/directive
classes that have generic type parameters.

This commit loosens the restriction such that type references are now
eligible for emit as long as they are exported.
@JoostK JoostK force-pushed the JoostK:ngtsc/smarter-type-emitter branch from 1b610d0 to 1b961a6 Jun 19, 2021
dylhunn added a commit that referenced this pull request Jun 21, 2021
…ers (#42492)

The template type checker is capable of recreating generic type bounds
in a different context, rewriting type references along the way (if
possible). This was previously done using a visitor that only supported
a limited set of types, resulting in the inability to emit all sorts of
types (even if they don't contain type references at all).

The inability to emit generic type bounds was not critical when the type
parameter emitting logic was introduced, as the compiler also has a
fallback strategy of creating inline type constructors. However, this
fallback is not available to the language service, resulting in
inaccurate types when components/directives use a complex generic type.

To mitigate this problem, the specialized visitor has been replaced with
a generalized TypeScript transform, where only type references get
special treatment. This allows for more complex types to be emitted,
such as union and intersection types, object literal types and tuple
types.

PR Close #42492
dylhunn added a commit that referenced this pull request Jun 21, 2021
…r default (#42492)

When a component/directive has a generic type parameter, the template
type checker attempts to translate the type parameter such that the
type parameters can be replicated in the type constructor that is
emitted into the typecheck file.

Type parameters with a default clause would incorrectly be emitted into
the typecheck file using the original `ts.TypeNode` for the default
clause, such that `ts.TypeReferenceNode`s within the default clause
would likely be invalid (i.e. referencing a type for which no import is
present in the typecheck file). This did not result in user-facing
type-check errors as errors reported in type constructors are not
translated into template positions Regardless, this commit ensures that
`ts.TypeReferenceNode`s within defaults are properly translated into the
typecheck file.

PR Close #42492
dylhunn added a commit that referenced this pull request Jun 21, 2021
… emitter (#42492)

Previously, the template type checker would only opt-in to inline type
constructors if it could import all type references from absolute module
specifiers. This limitation was put into place in an abundance of
caution as there was a safe, but less performant, fallback available.

The language service is not capable of using this fallback, which now
means that the limitation of absolute module specifiers limits the
language service's ability to use accurate types for component/directive
classes that have generic type parameters.

This commit loosens the restriction such that type references are now
eligible for emit as long as they are exported.

PR Close #42492
@dylhunn dylhunn closed this in 16aaa23 Jun 21, 2021
dylhunn added a commit that referenced this pull request Jun 21, 2021
…r default (#42492)

When a component/directive has a generic type parameter, the template
type checker attempts to translate the type parameter such that the
type parameters can be replicated in the type constructor that is
emitted into the typecheck file.

Type parameters with a default clause would incorrectly be emitted into
the typecheck file using the original `ts.TypeNode` for the default
clause, such that `ts.TypeReferenceNode`s within the default clause
would likely be invalid (i.e. referencing a type for which no import is
present in the typecheck file). This did not result in user-facing
type-check errors as errors reported in type constructors are not
translated into template positions Regardless, this commit ensures that
`ts.TypeReferenceNode`s within defaults are properly translated into the
typecheck file.

PR Close #42492
dylhunn added a commit that referenced this pull request Jun 21, 2021
… emitter (#42492)

Previously, the template type checker would only opt-in to inline type
constructors if it could import all type references from absolute module
specifiers. This limitation was put into place in an abundance of
caution as there was a safe, but less performant, fallback available.

The language service is not capable of using this fallback, which now
means that the limitation of absolute module specifiers limits the
language service's ability to use accurate types for component/directive
classes that have generic type parameters.

This commit loosens the restriction such that type references are now
eligible for emit as long as they are exported.

PR Close #42492
JoostK added a commit to JoostK/angular that referenced this pull request Jul 3, 2021
…arameters in a different file

In angular#42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes angular#42667
JoostK added a commit to JoostK/angular that referenced this pull request Jul 10, 2021
…arameters in a different file

In angular#42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes angular#42667
AndrewKushnir added a commit that referenced this pull request Jul 12, 2021
…arameters in a different file (#42761)

In #42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes #42667

PR Close #42761
AndrewKushnir added a commit that referenced this pull request Jul 12, 2021
…arameters in a different file (#42761)

In #42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes #42667

PR Close #42761
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Jul 22, 2021

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 Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants