Ngtsc/smarter type emitter #42492
Ngtsc/smarter type emitter #42492
Conversation
From first commit message:
It looks like this phrase was not completed?
typo |
fail('canEmit must be true when emitting succeeds'); | ||
} | ||
|
||
const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed}); |
petebacondarwin
Jun 11, 2021
Member
Is this to help when running on Windows?
Is this to help when running on Windows?
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.
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.
From the second commit message:
I didn't quite understand this sentence. Are you saying that it is not valid to have a |
Nice work @JoostK. |
@@ -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. |
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.
* 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?
* 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. |
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.
???
// 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 {}; |
petebacondarwin
Jun 11, 2021
Member
So would this test pass if Local
was exported?
If so, should we add a test to demonstrate that?
So would this test pass if Local
was exported?
If so, should we add a test to demonstrate that?
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
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>'); |
petebacondarwin
Jun 11, 2021
Member
Where does this test
namespace come from and how would that look in practice?
Where does this test
namespace come from and how would that look in practice?
JoostK
Jun 12, 2021
Author
Member
The test
namespace comes from the test setup that creates a fake type reference node:
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
The test
namespace comes from the test setup that creates a fake type reference node:
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
a4cee78
to
1b610d0
I added a note to clarify that an untranslated
The fallback continues to be necessary, as non-exported declarations cannot be translated by the type-emitter. |
Since this is still awaiting @alxhub review, I will go ahead and remove it from the merge queue for now. Please re-add |
if (ts.isTypeReferenceNode(node) && !canEmitTypeReference(node)) { | ||
return INELIGIBLE; | ||
} else { | ||
return ts.forEachChild(node, visitNode); |
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:
- what kinds of nodes we want to return
INELIGIBLE
for, and why.
- the mechanism by which eligible nodes end up getting selected.
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.
return visitNode(type) !== INELIGIBLE; | ||
} | ||
|
||
function visitNode(node: ts.Node): typeof INELIGIBLE|undefined { |
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.
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.
JoostK
Jun 19, 2021
Author
Member
Good point, updated!
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.
…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.
1b610d0
to
1b961a6
…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 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
…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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
See individual commits