Ngtsc/smarter type emitter by JoostK · Pull Request #42492 · angular/angular

JoostK · 2021-06-05T21:49:52Z

See individual commits

petebacondarwin

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

petebacondarwin · 2021-06-11T04:51:01Z

packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts

Is this to help when running on Windows?

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.

petebacondarwin

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?

petebacondarwin

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

petebacondarwin · 2021-06-11T09:04:20Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts

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.

petebacondarwin · 2021-06-11T09:05:26Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts

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.

???

petebacondarwin · 2021-06-11T09:11:26Z

packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts

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

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

petebacondarwin · 2021-06-11T09:19:34Z

packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts

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

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

angular/packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts

Lines 36 to 39 in 5161084

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:

angular/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts

Lines 134 to 141 in 5161084

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 · 2021-06-12T15:05:59Z

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 · 2021-06-14T19:08:19Z

Presubmit.

AndrewKushnir · 2021-06-15T18:28:38Z

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

dylhunn · 2021-06-17T18:13:47Z

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 · 2021-06-18T23:24:09Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts

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:

what kinds of nodes we want to return INELIGIBLE for, and why.

the mechanism by which eligible nodes end up getting selected.

alxhub · 2021-06-18T23:26:12Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts

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.

Good point, updated!

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.

…parameters

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

…rameter emitter

…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

…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

… 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

…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

… 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

…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

…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 · 2021-07-22T16:10:35Z

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

JoostK added refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Jun 5, 2021

ngbot bot modified the milestone: Backlog Jun 5, 2021

google-cla bot added the cla: yes label Jun 5, 2021

JoostK marked this pull request as ready for review June 7, 2021 17:51

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 7, 2021

JoostK requested a review from alxhub June 7, 2021 17:52

petebacondarwin reviewed Jun 11, 2021

View reviewed changes

petebacondarwin approved these changes Jun 11, 2021

View reviewed changes

JoostK force-pushed the ngtsc/smarter-type-emitter branch from a4cee78 to 1b610d0 Compare June 12, 2021 15:03

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: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 14, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 15, 2021

dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Jun 17, 2021

alxhub approved these changes Jun 18, 2021

View reviewed changes

JoostK added 5 commits June 19, 2021 20:39

fixup! refactor(compiler-cli): use TypeScript transform to emit type …

3198d76

…parameters

fixup! refactor(compiler-cli): enable relative imports in the type pa…

1b961a6

…rameter emitter

JoostK force-pushed the ngtsc/smarter-type-emitter branch from 1b610d0 to 1b961a6 Compare June 19, 2021 18:39

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: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 19, 2021

dylhunn closed this in 16aaa23 Jun 21, 2021

This was referenced Jun 21, 2021

Language Service doesn't work with custom generics #42145

Closed

Incremental build - ngtypecheck - error TS1110: Type expected. #42667

Closed

JoostK mentioned this pull request Jul 3, 2021

fix(compiler-cli): properly emit literal types when recreating type parameters in a different file #42761

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 22, 2021

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

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

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

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

Conversation

JoostK commented Jun 5, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK commented Jun 12, 2021

Uh oh!

AndrewKushnir commented Jun 14, 2021

Uh oh!

AndrewKushnir commented Jun 15, 2021

Uh oh!

dylhunn commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants