refactor(compiler-cli): enable relative imports in the type parameter 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
This commit is contained in:
JoostK 2021-06-05 23:48:10 +02:00 committed by Dylan Hunn
parent 729eea5716
commit a9dd7e21a2
3 changed files with 37 additions and 26 deletions

View File

@ -10,7 +10,8 @@ import {Reference} from '../../imports';
/** /**
* A resolved type reference can either be a `Reference`, the original `ts.TypeReferenceNode` itself * 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. A value of null indicates that no reference could be resolved or that the reference can
* not be emitted.
*/ */
export type ResolvedTypeReference = Reference|ts.TypeReferenceNode|null; export type ResolvedTypeReference = Reference|ts.TypeReferenceNode|null;
@ -69,10 +70,9 @@ export function canEmitType(type: ts.TypeNode, resolver: TypeReferenceResolver):
return false; return false;
} }
// If the type is a reference without a owning module, consider the type not to be eligible for // If the type is a reference, consider the type to be eligible for emitting.
// emitting. if (reference instanceof Reference) {
if (reference instanceof Reference && !reference.hasOwningModuleGuess) { return true;
return false;
} }
// The type can be emitted if either it does not have any type arguments, or all of them can be // The type can be emitted if either it does not have any type arguments, or all of them can be
@ -157,10 +157,6 @@ export class TypeEmitter {
// Emit the type name. // Emit the type name.
let typeName = type.typeName; let typeName = type.typeName;
if (reference instanceof Reference) { if (reference instanceof Reference) {
if (!reference.hasOwningModuleGuess) {
throw new Error('A type reference to emit must be imported from an absolute module');
}
const emittedType = this.emitReference(reference); const emittedType = this.emitReference(reference);
if (!ts.isTypeReferenceNode(emittedType)) { if (!ts.isTypeReferenceNode(emittedType)) {
throw new Error(`Expected TypeReferenceNode for emitted reference, got ${ throw new Error(`Expected TypeReferenceNode for emitted reference, got ${

View File

@ -8,7 +8,7 @@
import * as ts from 'typescript'; import * as ts from 'typescript';
import {OwningModule, Reference} from '../../imports'; import {OwningModule, Reference} from '../../imports';
import {DeclarationNode, ReflectionHost} from '../../reflection'; import {DeclarationNode, isNamedClassDeclaration, ReflectionHost} from '../../reflection';
import {canEmitType, ResolvedTypeReference, TypeEmitter} from './type_emitter'; import {canEmitType, ResolvedTypeReference, TypeEmitter} from './type_emitter';
@ -92,9 +92,19 @@ export class TypeParameterEmitter {
}; };
} }
// If no owning module is known, the reference needs to be exported to be able to emit an import
// statement for it. If the declaration is not exported, null is returned to prevent emit.
if (owningModule === null && !this.isStaticallyExported(declaration.node)) {
return null;
}
return new Reference(declaration.node, owningModule); return new Reference(declaration.node, owningModule);
} }
private isStaticallyExported(decl: DeclarationNode): boolean {
return isNamedClassDeclaration(decl) && this.reflector.isStaticallyExported(decl);
}
private isLocalTypeParameter(decl: DeclarationNode): boolean { private isLocalTypeParameter(decl: DeclarationNode): boolean {
// Checking for local type parameters only occurs during resolution of type parameters, so it is // Checking for local type parameters only occurs during resolution of type parameters, so it is
// guaranteed that type parameters are present. // guaranteed that type parameters are present.

View File

@ -116,26 +116,34 @@ runInEachFileSystem(() => {
expect(emit(emitter)).toEqual('<T, U extends test.NgIterable<T>>'); expect(emit(emitter)).toEqual('<T, U extends test.NgIterable<T>>');
}); });
it('cannot emit references to local declarations', () => { it('can emit references to local, exported declarations', () => {
const emitter = createEmitter(` const emitter = createEmitter(`
export class Local {}; class Local {};
export {Local};
export class TestClass<T extends Local> {}`);
expect(emitter.canEmit()).toBe(true);
expect(emit(emitter)).toEqual('<T extends test.Local>');
});
it('cannot emit references to non-exported local declarations', () => {
const emitter = createEmitter(`
class Local {};
export class TestClass<T extends Local> {}`); export class TestClass<T extends Local> {}`);
expect(emitter.canEmit()).toBe(false); expect(emitter.canEmit()).toBe(false);
expect(() => emit(emitter)) expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
.toThrowError('A type reference to emit must be imported from an absolute module');
}); });
it('cannot emit references to local declarations as nested type arguments', () => { it('cannot emit references to local declarations as nested type arguments', () => {
const emitter = createEmitter(` const emitter = createEmitter(`
import {NgIterable} from '@angular/core'; import {NgIterable} from '@angular/core';
export class Local {}; class Local {};
export class TestClass<T extends NgIterable<Local>> {}`); export class TestClass<T extends NgIterable<Local>> {}`);
expect(emitter.canEmit()).toBe(false); expect(emitter.canEmit()).toBe(false);
expect(() => emit(emitter)) expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
.toThrowError('A type reference to emit must be imported from an absolute module');
}); });
it('can emit references into external modules within array types', () => { it('can emit references into external modules within array types', () => {
@ -150,15 +158,14 @@ runInEachFileSystem(() => {
it('cannot emit references to local declarations within array types', () => { it('cannot emit references to local declarations within array types', () => {
const emitter = createEmitter(` const emitter = createEmitter(`
export class Local {}; class Local {};
export class TestClass<T extends Local[]> {}`); export class TestClass<T extends Local[]> {}`);
expect(emitter.canEmit()).toBe(false); expect(emitter.canEmit()).toBe(false);
expect(() => emit(emitter)) expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
.toThrowError('A type reference to emit must be imported from an absolute module');
}); });
it('cannot emit references into relative files', () => { it('can emit references into relative files', () => {
const additionalFiles: TestFile[] = [{ const additionalFiles: TestFile[] = [{
name: absoluteFrom('/internal.ts'), name: absoluteFrom('/internal.ts'),
contents: `export class Internal {}`, contents: `export class Internal {}`,
@ -170,9 +177,8 @@ runInEachFileSystem(() => {
export class TestClass<T extends Internal> {}`, export class TestClass<T extends Internal> {}`,
additionalFiles); additionalFiles);
expect(emitter.canEmit()).toBe(false); expect(emitter.canEmit()).toBe(true);
expect(() => emit(emitter)) expect(emit(emitter)).toEqual('<T extends test.Internal>');
.toThrowError('A type reference to emit must be imported from an absolute module');
}); });
it('can emit references to interfaces', () => { it('can emit references to interfaces', () => {
@ -246,8 +252,7 @@ runInEachFileSystem(() => {
export class TestClass<T extends object = Local> {}`); export class TestClass<T extends object = Local> {}`);
expect(emitter.canEmit()).toBe(false); expect(emitter.canEmit()).toBe(false);
expect(() => emit(emitter)) expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
.toThrowError('A type reference to emit must be imported from an absolute module');
}); });
}); });
}); });