Skip to content

Commit a26afce

Browse files
JoostKatscott
JoostK
authored and
atscott
committed
fix(compiler-cli): fix crash during type-checking of library builds (#44587)
When building a library, the `rootDir` option is configured to ensure that all source files are present within the entry-point that is being build. This imposes an extra constraint on the reference emit logic, which does not allow emitting a reference into a source file outside of this `rootDir`. During the generation of type-check blocks we used to make a best-effort estimation of whether a type reference can be emitted into the type-check file. This check was relaxed in #42492 to support emitting more syntax forms and type references, but this change did not consider the `rootDir` constraint that is present in library builds. As such, the compiler might conclude that a type reference is eligible for emit into the type-check file, whereas in practice this would cause a failure. This commit changes the best-effort estimation into a "preflight" reference emit that is fully accurate as to whether emitting a type reference is possible. Fixes #43624 PR Close #44587
1 parent d13e054 commit a26afce

File tree

8 files changed

+165
-150
lines changed

8 files changed

+165
-150
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/src/context.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
235235
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
236236
const dirNode = dirRef.node;
237237

238-
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector)) {
238+
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector, shimData.file)) {
239239
// inlining not required
240240
continue;
241241
}
@@ -263,7 +263,8 @@ export class TypeCheckContextImpl implements TypeCheckContext {
263263
templateDiagnostics,
264264
});
265265

266-
const inliningRequirement = requiresInlineTypeCheckBlock(ref.node, pipes, this.reflector);
266+
const inliningRequirement =
267+
requiresInlineTypeCheckBlock(ref.node, shimData.file, pipes, this.reflector);
267268

268269
// If inlining is not supported, but is required for either the TCB or one of its directive
269270
// dependencies, then exit here with an error.

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@
99
import {ExpressionType, ExternalExpr, Type, WrappedNodeExpr} from '@angular/compiler';
1010
import ts from 'typescript';
1111

12-
import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports';
12+
import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitKind, ReferenceEmitter} from '../../imports';
1313
import {ClassDeclaration, ReflectionHost} from '../../reflection';
1414
import {ImportManager, translateExpression, translateType} from '../../translator';
1515
import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from '../api';
1616

17+
import {ReferenceEmitEnvironment} from './tcb_util';
1718
import {tsDeclareVariable} from './ts_util';
1819
import {generateTypeCtorDeclarationFn, requiresInlineTypeCtor} from './type_constructor';
1920
import {TypeParameterEmitter} from './type_parameter_emitter';
@@ -29,7 +30,7 @@ import {TypeParameterEmitter} from './type_parameter_emitter';
2930
* `Environment` can be used in a standalone fashion, or can be extended to support more specialized
3031
* usage.
3132
*/
32-
export class Environment {
33+
export class Environment implements ReferenceEmitEnvironment {
3334
private nextIds = {
3435
pipeInst: 1,
3536
typeCtor: 1,
@@ -59,7 +60,7 @@ export class Environment {
5960
return this.typeCtors.get(node)!;
6061
}
6162

62-
if (requiresInlineTypeCtor(node, this.reflector)) {
63+
if (requiresInlineTypeCtor(node, this.reflector, this)) {
6364
// The constructor has already been created inline, we just need to construct a reference to
6465
// it.
6566
const ref = this.reference(dirRef);
@@ -84,8 +85,7 @@ export class Environment {
8485
coercedInputFields: dir.coercedInputFields,
8586
};
8687
const typeParams = this.emitTypeParameters(node);
87-
const typeCtor = generateTypeCtorDeclarationFn(
88-
node, meta, nodeTypeRef.typeName, typeParams, this.reflector);
88+
const typeCtor = generateTypeCtorDeclarationFn(node, meta, nodeTypeRef.typeName, typeParams);
8989
this.typeCtorStatements.push(typeCtor);
9090
const fnId = ts.createIdentifier(fnName);
9191
this.typeCtors.set(node, fnId);
@@ -127,6 +127,12 @@ export class Environment {
127127
return translateExpression(ngExpr.expression, this.importManager);
128128
}
129129

130+
canReferenceType(ref: Reference): boolean {
131+
const result = this.refEmitter.emit(
132+
ref, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports);
133+
return result.kind === ReferenceEmitKind.Success;
134+
}
135+
130136
/**
131137
* Generate a `ts.TypeNode` that references the given node as a type.
132138
*

‎packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ import {hasIgnoreForDiagnosticsMarker, readSpanComment} from './comments';
1818
import {checkIfClassIsExported} from './ts_util';
1919
import {TypeParameterEmitter} from './type_parameter_emitter';
2020

21+
/**
22+
* Represents the origin environment from where reference will be emitted. This interface exists
23+
* as an indirection for the `Environment` type, which would otherwise introduce a (type-only)
24+
* import cycle.
25+
*/
26+
export interface ReferenceEmitEnvironment {
27+
canReferenceType(ref: Reference): boolean;
28+
}
29+
2130
/**
2231
* Adapter interface which allows the template type-checking diagnostics code to interpret offsets
2332
* in a TCB and map them back to original locations in the template.
@@ -64,7 +73,7 @@ export enum TcbInliningRequirement {
6473
}
6574

6675
export function requiresInlineTypeCheckBlock(
67-
node: ClassDeclaration<ts.ClassDeclaration>,
76+
node: ClassDeclaration<ts.ClassDeclaration>, env: ReferenceEmitEnvironment,
6877
usedPipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>,
6978
reflector: ReflectionHost): TcbInliningRequirement {
7079
// In order to qualify for a declared TCB (not inline) two conditions must be met:
@@ -73,7 +82,7 @@ export function requiresInlineTypeCheckBlock(
7382
if (!checkIfClassIsExported(node)) {
7483
// Condition 1 is false, the class is not exported.
7584
return TcbInliningRequirement.MustInline;
76-
} else if (!checkIfGenericTypeBoundsAreContextFree(node, reflector)) {
85+
} else if (!checkIfGenericTypeBoundsCanBeEmitted(node, reflector, env)) {
7786
// Condition 2 is false, the class has constrained generic types. It should be checked with an
7887
// inline TCB if possible, but can potentially use fallbacks to avoid inlining if not.
7988
return TcbInliningRequirement.ShouldInlineForGenericBounds;
@@ -175,8 +184,10 @@ function getTemplateId(
175184
}) as TemplateId || null;
176185
}
177186

178-
export function checkIfGenericTypeBoundsAreContextFree(
179-
node: ClassDeclaration<ts.ClassDeclaration>, reflector: ReflectionHost): boolean {
187+
export function checkIfGenericTypeBoundsCanBeEmitted(
188+
node: ClassDeclaration<ts.ClassDeclaration>, reflector: ReflectionHost,
189+
env: ReferenceEmitEnvironment): boolean {
180190
// Generic type parameters are considered context free if they can be emitted into any context.
181-
return new TypeParameterEmitter(node.typeParameters, reflector).canEmit();
191+
const emitter = new TypeParameterEmitter(node.typeParameters, reflector);
192+
return emitter.canEmit(ref => env.canReferenceType(ref));
182193
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import ts from 'typescript';
1111

1212
import {Reference} from '../../imports';
1313
import {ClassPropertyName} from '../../metadata';
14-
import {ClassDeclaration, ReflectionHost} from '../../reflection';
14+
import {ClassDeclaration} from '../../reflection';
1515
import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api';
1616

1717
import {addExpressionIdentifier, ExpressionIdentifier, markIgnoreDiagnostics} from './comments';
@@ -1509,7 +1509,7 @@ class Scope {
15091509
// `TcbNonDirectiveTypeOp`.
15101510
directiveOp = new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir);
15111511
} else if (
1512-
!requiresInlineTypeCtor(dirRef.node, host) ||
1512+
!requiresInlineTypeCtor(dirRef.node, host, this.tcb.env) ||
15131513
this.tcb.env.config.useInlineTypeConstructors) {
15141514
// For generic directives, we use a type constructor to infer types. If a directive requires
15151515
// an inline type constructor, then inlining must be available to use the

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,13 @@ import ts from 'typescript';
1010

1111
import {ClassDeclaration, ReflectionHost} from '../../reflection';
1212
import {TypeCtorMetadata} from '../api';
13-
import {checkIfGenericTypeBoundsAreContextFree} from './tcb_util';
1413

14+
import {checkIfGenericTypeBoundsCanBeEmitted, ReferenceEmitEnvironment} from './tcb_util';
1515
import {tsCreateTypeQueryForCoercedInput} from './ts_util';
1616

1717
export function generateTypeCtorDeclarationFn(
1818
node: ClassDeclaration<ts.ClassDeclaration>, meta: TypeCtorMetadata, nodeTypeRef: ts.EntityName,
19-
typeParams: ts.TypeParameterDeclaration[]|undefined, reflector: ReflectionHost): ts.Statement {
20-
if (requiresInlineTypeCtor(node, reflector)) {
21-
throw new Error(`${node.name.text} requires an inline type constructor`);
22-
}
23-
19+
typeParams: ts.TypeParameterDeclaration[]|undefined): ts.Statement {
2420
const rawTypeArgs = typeParams !== undefined ? generateGenericArgs(typeParams) : undefined;
2521
const rawType = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs);
2622

@@ -190,10 +186,11 @@ function generateGenericArgs(params: ReadonlyArray<ts.TypeParameterDeclaration>)
190186
}
191187

192188
export function requiresInlineTypeCtor(
193-
node: ClassDeclaration<ts.ClassDeclaration>, host: ReflectionHost): boolean {
189+
node: ClassDeclaration<ts.ClassDeclaration>, host: ReflectionHost,
190+
env: ReferenceEmitEnvironment): boolean {
194191
// The class requires an inline type constructor if it has generic type bounds that can not be
195-
// emitted into a different context.
196-
return !checkIfGenericTypeBoundsAreContextFree(node, host);
192+
// emitted into the provided type-check environment.
193+
return !checkIfGenericTypeBoundsCanBeEmitted(node, host, env);
197194
}
198195

199196
/**

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

Lines changed: 11 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,13 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88
import ts from 'typescript';
9-
import {Reference} from '../../imports';
109

1110
/**
12-
* A resolved type reference can either be a `Reference`, the original `ts.TypeReferenceNode` itself
13-
* or null. A value of null indicates that no reference could be resolved or that the reference can
14-
* not be emitted.
11+
* A type reference resolver function is responsible for translating a type reference from the
12+
* origin source file into a type reference that is valid in the desired source file. If the type
13+
* cannot be translated to the desired source file, then null can be returned.
1514
*/
16-
export type ResolvedTypeReference = Reference|ts.TypeReferenceNode|null;
17-
18-
/**
19-
* A type reference resolver function is responsible for finding the declaration of the type
20-
* reference and verifying whether it can be emitted.
21-
*/
22-
export type TypeReferenceResolver = (type: ts.TypeReferenceNode) => ResolvedTypeReference;
15+
export type TypeReferenceTranslator = (type: ts.TypeReferenceNode) => ts.TypeReferenceNode|null;
2316

2417
/**
2518
* A marker to indicate that a type reference is ineligible for emitting. This needs to be truthy
@@ -38,7 +31,8 @@ const INELIGIBLE: INELIGIBLE = {} as INELIGIBLE;
3831
* function returns false, then using the `TypeEmitter` should not be attempted as it is known to
3932
* fail.
4033
*/
41-
export function canEmitType(type: ts.TypeNode, resolver: TypeReferenceResolver): boolean {
34+
export function canEmitType(
35+
type: ts.TypeNode, canEmit: (type: ts.TypeReferenceNode) => boolean): boolean {
4236
return canEmitTypeWorker(type);
4337

4438
function canEmitTypeWorker(type: ts.TypeNode): boolean {
@@ -69,18 +63,10 @@ export function canEmitType(type: ts.TypeNode, resolver: TypeReferenceResolver):
6963
}
7064

7165
function canEmitTypeReference(type: ts.TypeReferenceNode): boolean {
72-
const reference = resolver(type);
73-
74-
// If the type could not be resolved, it can not be emitted.
75-
if (reference === null) {
66+
if (!canEmit(type)) {
7667
return false;
7768
}
7869

79-
// If the type is a reference, consider the type to be eligible for emitting.
80-
if (reference instanceof Reference) {
81-
return true;
82-
}
83-
8470
// The type can be emitted if either it does not have any type arguments, or all of them can be
8571
// emitted.
8672
return type.typeArguments === undefined || type.typeArguments.every(canEmitTypeWorker);
@@ -117,21 +103,7 @@ export function canEmitType(type: ts.TypeNode, resolver: TypeReferenceResolver):
117103
* referring to the namespace import that was created.
118104
*/
119105
export class TypeEmitter {
120-
/**
121-
* Resolver function that computes a `Reference` corresponding with a `ts.TypeReferenceNode`.
122-
*/
123-
private resolver: TypeReferenceResolver;
124-
125-
/**
126-
* Given a `Reference`, this function is responsible for the actual emitting work. It should
127-
* produce a `ts.TypeNode` that is valid within the desired context.
128-
*/
129-
private emitReference: (ref: Reference) => ts.TypeNode;
130-
131-
constructor(resolver: TypeReferenceResolver, emitReference: (ref: Reference) => ts.TypeNode) {
132-
this.resolver = resolver;
133-
this.emitReference = emitReference;
134-
}
106+
constructor(private translator: TypeReferenceTranslator) {}
135107

136108
emitType(type: ts.TypeNode): ts.TypeNode {
137109
const typeReferenceTransformer: ts.TransformerFactory<ts.TypeNode> = context => {
@@ -163,8 +135,8 @@ export class TypeEmitter {
163135

164136
private emitTypeReference(type: ts.TypeReferenceNode): ts.TypeNode {
165137
// Determine the reference that the type corresponds with.
166-
const reference = this.resolver(type);
167-
if (reference === null) {
138+
const translatedType = this.translator(type);
139+
if (translatedType === null) {
168140
throw new Error('Unable to emit an unresolved reference');
169141
}
170142

@@ -174,18 +146,6 @@ export class TypeEmitter {
174146
typeArguments = ts.createNodeArray(type.typeArguments.map(typeArg => this.emitType(typeArg)));
175147
}
176148

177-
// Emit the type name.
178-
let typeName = type.typeName;
179-
if (reference instanceof Reference) {
180-
const emittedType = this.emitReference(reference);
181-
if (!ts.isTypeReferenceNode(emittedType)) {
182-
throw new Error(`Expected TypeReferenceNode for emitted reference, got ${
183-
ts.SyntaxKind[emittedType.kind]}`);
184-
}
185-
186-
typeName = emittedType.typeName;
187-
}
188-
189-
return ts.updateTypeReferenceNode(type, typeName, typeArguments);
149+
return ts.updateTypeReferenceNode(type, translatedType.typeName, typeArguments);
190150
}
191151
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts‎

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import ts from 'typescript';
1010
import {OwningModule, Reference} from '../../imports';
1111
import {DeclarationNode, ReflectionHost} from '../../reflection';
1212

13-
import {canEmitType, ResolvedTypeReference, TypeEmitter} from './type_emitter';
13+
import {canEmitType, TypeEmitter} from './type_emitter';
1414

1515

1616
/**
@@ -26,22 +26,35 @@ export class TypeParameterEmitter {
2626
* `emit` is known to succeed. Vice versa, if false is returned then `emit` should not be
2727
* called, as it would fail.
2828
*/
29-
canEmit(): boolean {
29+
canEmit(canEmitReference: (ref: Reference) => boolean): boolean {
3030
if (this.typeParameters === undefined) {
3131
return true;
3232
}
3333

3434
return this.typeParameters.every(typeParam => {
35-
return this.canEmitType(typeParam.constraint) && this.canEmitType(typeParam.default);
35+
return this.canEmitType(typeParam.constraint, canEmitReference) &&
36+
this.canEmitType(typeParam.default, canEmitReference);
3637
});
3738
}
3839

39-
private canEmitType(type: ts.TypeNode|undefined): boolean {
40+
private canEmitType(type: ts.TypeNode|undefined, canEmitReference: (ref: Reference) => boolean):
41+
boolean {
4042
if (type === undefined) {
4143
return true;
4244
}
4345

44-
return canEmitType(type, typeReference => this.resolveTypeReference(typeReference));
46+
return canEmitType(type, typeReference => {
47+
const reference = this.resolveTypeReference(typeReference);
48+
if (reference === null) {
49+
return false;
50+
}
51+
52+
if (reference instanceof Reference) {
53+
return canEmitReference(reference);
54+
}
55+
56+
return true;
57+
});
4558
}
4659

4760
/**
@@ -52,7 +65,7 @@ export class TypeParameterEmitter {
5265
return undefined;
5366
}
5467

55-
const emitter = new TypeEmitter(type => this.resolveTypeReference(type), emitReference);
68+
const emitter = new TypeEmitter(type => this.translateTypeReference(type, emitReference));
5669

5770
return this.typeParameters.map(typeParam => {
5871
const constraint =
@@ -68,7 +81,7 @@ export class TypeParameterEmitter {
6881
});
6982
}
7083

71-
private resolveTypeReference(type: ts.TypeReferenceNode): ResolvedTypeReference {
84+
private resolveTypeReference(type: ts.TypeReferenceNode): Reference|ts.TypeReferenceNode|null {
7285
const target = ts.isIdentifier(type.typeName) ? type.typeName : type.typeName.right;
7386
const declaration = this.reflector.getDeclarationOfIdentifier(target);
7487

@@ -92,23 +105,27 @@ export class TypeParameterEmitter {
92105
};
93106
}
94107

95-
// The declaration needs to be exported as a top-level export to be able to emit an import
96-
// statement for it. If the declaration is not exported, null is returned to prevent emit.
97-
if (!this.isTopLevelExport(declaration.node)) {
98-
return null;
99-
}
100-
101108
return new Reference(declaration.node, owningModule);
102109
}
103110

104-
private isTopLevelExport(decl: DeclarationNode): boolean {
105-
if (decl.parent === undefined || !ts.isSourceFile(decl.parent)) {
106-
// The declaration has to exist at the top-level, as the reference emitters are not capable of
107-
// generating imports to classes declared in a namespace.
108-
return false;
111+
private translateTypeReference(
112+
type: ts.TypeReferenceNode,
113+
emitReference: (ref: Reference) => ts.TypeNode | null): ts.TypeReferenceNode|null {
114+
const reference = this.resolveTypeReference(type);
115+
if (!(reference instanceof Reference)) {
116+
return reference;
109117
}
110118

111-
return this.reflector.isStaticallyExported(decl);
119+
const typeNode = emitReference(reference);
120+
if (typeNode === null) {
121+
return null;
122+
}
123+
124+
if (!ts.isTypeReferenceNode(typeNode)) {
125+
throw new Error(
126+
`Expected TypeReferenceNode for emitted reference, got ${ts.SyntaxKind[typeNode.kind]}.`);
127+
}
128+
return typeNode;
112129
}
113130

114131
private isLocalTypeParameter(decl: DeclarationNode): boolean {

0 commit comments

Comments
 (0)