Skip to content

Commit

Permalink
fix(compiler-cli): fix crash during type-checking of library builds (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
JoostK authored and atscott committed Jan 6, 2022
1 parent d13e054 commit a26afce
Show file tree
Hide file tree
Showing 8 changed files with 165 additions and 150 deletions.
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Expand Up @@ -235,7 +235,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;

if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector)) {
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector, shimData.file)) {
// inlining not required
continue;
}
Expand Down Expand Up @@ -263,7 +263,8 @@ export class TypeCheckContextImpl implements TypeCheckContext {
templateDiagnostics,
});

const inliningRequirement = requiresInlineTypeCheckBlock(ref.node, pipes, this.reflector);
const inliningRequirement =
requiresInlineTypeCheckBlock(ref.node, shimData.file, pipes, this.reflector);

// If inlining is not supported, but is required for either the TCB or one of its directive
// dependencies, then exit here with an error.
Expand Down
16 changes: 11 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts
Expand Up @@ -9,11 +9,12 @@
import {ExpressionType, ExternalExpr, Type, WrappedNodeExpr} from '@angular/compiler';
import ts from 'typescript';

import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports';
import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitKind, ReferenceEmitter} from '../../imports';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ImportManager, translateExpression, translateType} from '../../translator';
import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from '../api';

import {ReferenceEmitEnvironment} from './tcb_util';
import {tsDeclareVariable} from './ts_util';
import {generateTypeCtorDeclarationFn, requiresInlineTypeCtor} from './type_constructor';
import {TypeParameterEmitter} from './type_parameter_emitter';
Expand All @@ -29,7 +30,7 @@ import {TypeParameterEmitter} from './type_parameter_emitter';
* `Environment` can be used in a standalone fashion, or can be extended to support more specialized
* usage.
*/
export class Environment {
export class Environment implements ReferenceEmitEnvironment {
private nextIds = {
pipeInst: 1,
typeCtor: 1,
Expand Down Expand Up @@ -59,7 +60,7 @@ export class Environment {
return this.typeCtors.get(node)!;
}

if (requiresInlineTypeCtor(node, this.reflector)) {
if (requiresInlineTypeCtor(node, this.reflector, this)) {
// The constructor has already been created inline, we just need to construct a reference to
// it.
const ref = this.reference(dirRef);
Expand All @@ -84,8 +85,7 @@ export class Environment {
coercedInputFields: dir.coercedInputFields,
};
const typeParams = this.emitTypeParameters(node);
const typeCtor = generateTypeCtorDeclarationFn(
node, meta, nodeTypeRef.typeName, typeParams, this.reflector);
const typeCtor = generateTypeCtorDeclarationFn(node, meta, nodeTypeRef.typeName, typeParams);
this.typeCtorStatements.push(typeCtor);
const fnId = ts.createIdentifier(fnName);
this.typeCtors.set(node, fnId);
Expand Down Expand Up @@ -127,6 +127,12 @@ export class Environment {
return translateExpression(ngExpr.expression, this.importManager);
}

canReferenceType(ref: Reference): boolean {
const result = this.refEmitter.emit(
ref, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports);
return result.kind === ReferenceEmitKind.Success;
}

/**
* Generate a `ts.TypeNode` that references the given node as a type.
*
Expand Down
21 changes: 16 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts
Expand Up @@ -18,6 +18,15 @@ import {hasIgnoreForDiagnosticsMarker, readSpanComment} from './comments';
import {checkIfClassIsExported} from './ts_util';
import {TypeParameterEmitter} from './type_parameter_emitter';

/**
* Represents the origin environment from where reference will be emitted. This interface exists
* as an indirection for the `Environment` type, which would otherwise introduce a (type-only)
* import cycle.
*/
export interface ReferenceEmitEnvironment {
canReferenceType(ref: Reference): boolean;
}

/**
* Adapter interface which allows the template type-checking diagnostics code to interpret offsets
* in a TCB and map them back to original locations in the template.
Expand Down Expand Up @@ -64,7 +73,7 @@ export enum TcbInliningRequirement {
}

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

export function checkIfGenericTypeBoundsAreContextFree(
node: ClassDeclaration<ts.ClassDeclaration>, reflector: ReflectionHost): boolean {
export function checkIfGenericTypeBoundsCanBeEmitted(
node: ClassDeclaration<ts.ClassDeclaration>, reflector: ReflectionHost,
env: ReferenceEmitEnvironment): boolean {
// Generic type parameters are considered context free if they can be emitted into any context.
return new TypeParameterEmitter(node.typeParameters, reflector).canEmit();
const emitter = new TypeParameterEmitter(node.typeParameters, reflector);
return emitter.canEmit(ref => env.canReferenceType(ref));
}
Expand Up @@ -11,7 +11,7 @@ import ts from 'typescript';

import {Reference} from '../../imports';
import {ClassPropertyName} from '../../metadata';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ClassDeclaration} from '../../reflection';
import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api';

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

import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {TypeCtorMetadata} from '../api';
import {checkIfGenericTypeBoundsAreContextFree} from './tcb_util';

import {checkIfGenericTypeBoundsCanBeEmitted, ReferenceEmitEnvironment} from './tcb_util';
import {tsCreateTypeQueryForCoercedInput} from './ts_util';

export function generateTypeCtorDeclarationFn(
node: ClassDeclaration<ts.ClassDeclaration>, meta: TypeCtorMetadata, nodeTypeRef: ts.EntityName,
typeParams: ts.TypeParameterDeclaration[]|undefined, reflector: ReflectionHost): ts.Statement {
if (requiresInlineTypeCtor(node, reflector)) {
throw new Error(`${node.name.text} requires an inline type constructor`);
}

typeParams: ts.TypeParameterDeclaration[]|undefined): ts.Statement {
const rawTypeArgs = typeParams !== undefined ? generateGenericArgs(typeParams) : undefined;
const rawType = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs);

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

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

/**
Expand Down
62 changes: 11 additions & 51 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts
Expand Up @@ -6,20 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/
import ts from 'typescript';
import {Reference} from '../../imports';

/**
* A resolved type reference can either be a `Reference`, the original `ts.TypeReferenceNode` itself
* or null. A value of null indicates that no reference could be resolved or that the reference can
* not be emitted.
* A type reference resolver function is responsible for translating a type reference from the
* origin source file into a type reference that is valid in the desired source file. If the type
* cannot be translated to the desired source file, then null can be returned.
*/
export type ResolvedTypeReference = Reference|ts.TypeReferenceNode|null;

/**
* A type reference resolver function is responsible for finding the declaration of the type
* reference and verifying whether it can be emitted.
*/
export type TypeReferenceResolver = (type: ts.TypeReferenceNode) => ResolvedTypeReference;
export type TypeReferenceTranslator = (type: ts.TypeReferenceNode) => ts.TypeReferenceNode|null;

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

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

function canEmitTypeReference(type: ts.TypeReferenceNode): boolean {
const reference = resolver(type);

// If the type could not be resolved, it can not be emitted.
if (reference === null) {
if (!canEmit(type)) {
return false;
}

// If the type is a reference, consider the type to be eligible for emitting.
if (reference instanceof Reference) {
return true;
}

// The type can be emitted if either it does not have any type arguments, or all of them can be
// emitted.
return type.typeArguments === undefined || type.typeArguments.every(canEmitTypeWorker);
Expand Down Expand Up @@ -117,21 +103,7 @@ export function canEmitType(type: ts.TypeNode, resolver: TypeReferenceResolver):
* referring to the namespace import that was created.
*/
export class TypeEmitter {
/**
* Resolver function that computes a `Reference` corresponding with a `ts.TypeReferenceNode`.
*/
private resolver: TypeReferenceResolver;

/**
* Given a `Reference`, this function is responsible for the actual emitting work. It should
* produce a `ts.TypeNode` that is valid within the desired context.
*/
private emitReference: (ref: Reference) => ts.TypeNode;

constructor(resolver: TypeReferenceResolver, emitReference: (ref: Reference) => ts.TypeNode) {
this.resolver = resolver;
this.emitReference = emitReference;
}
constructor(private translator: TypeReferenceTranslator) {}

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

private emitTypeReference(type: ts.TypeReferenceNode): ts.TypeNode {
// Determine the reference that the type corresponds with.
const reference = this.resolver(type);
if (reference === null) {
const translatedType = this.translator(type);
if (translatedType === null) {
throw new Error('Unable to emit an unresolved reference');
}

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

// Emit the type name.
let typeName = type.typeName;
if (reference instanceof Reference) {
const emittedType = this.emitReference(reference);
if (!ts.isTypeReferenceNode(emittedType)) {
throw new Error(`Expected TypeReferenceNode for emitted reference, got ${
ts.SyntaxKind[emittedType.kind]}`);
}

typeName = emittedType.typeName;
}

return ts.updateTypeReferenceNode(type, typeName, typeArguments);
return ts.updateTypeReferenceNode(type, translatedType.typeName, typeArguments);
}
}
Expand Up @@ -10,7 +10,7 @@ import ts from 'typescript';
import {OwningModule, Reference} from '../../imports';
import {DeclarationNode, ReflectionHost} from '../../reflection';

import {canEmitType, ResolvedTypeReference, TypeEmitter} from './type_emitter';
import {canEmitType, TypeEmitter} from './type_emitter';


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

return this.typeParameters.every(typeParam => {
return this.canEmitType(typeParam.constraint) && this.canEmitType(typeParam.default);
return this.canEmitType(typeParam.constraint, canEmitReference) &&
this.canEmitType(typeParam.default, canEmitReference);
});
}

private canEmitType(type: ts.TypeNode|undefined): boolean {
private canEmitType(type: ts.TypeNode|undefined, canEmitReference: (ref: Reference) => boolean):
boolean {
if (type === undefined) {
return true;
}

return canEmitType(type, typeReference => this.resolveTypeReference(typeReference));
return canEmitType(type, typeReference => {
const reference = this.resolveTypeReference(typeReference);
if (reference === null) {
return false;
}

if (reference instanceof Reference) {
return canEmitReference(reference);
}

return true;
});
}

/**
Expand All @@ -52,7 +65,7 @@ export class TypeParameterEmitter {
return undefined;
}

const emitter = new TypeEmitter(type => this.resolveTypeReference(type), emitReference);
const emitter = new TypeEmitter(type => this.translateTypeReference(type, emitReference));

return this.typeParameters.map(typeParam => {
const constraint =
Expand All @@ -68,7 +81,7 @@ export class TypeParameterEmitter {
});
}

private resolveTypeReference(type: ts.TypeReferenceNode): ResolvedTypeReference {
private resolveTypeReference(type: ts.TypeReferenceNode): Reference|ts.TypeReferenceNode|null {
const target = ts.isIdentifier(type.typeName) ? type.typeName : type.typeName.right;
const declaration = this.reflector.getDeclarationOfIdentifier(target);

Expand All @@ -92,23 +105,27 @@ export class TypeParameterEmitter {
};
}

// The declaration needs to be exported as a top-level export to be able to emit an import
// statement for it. If the declaration is not exported, null is returned to prevent emit.
if (!this.isTopLevelExport(declaration.node)) {
return null;
}

return new Reference(declaration.node, owningModule);
}

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

return this.reflector.isStaticallyExported(decl);
const typeNode = emitReference(reference);
if (typeNode === null) {
return null;
}

if (!ts.isTypeReferenceNode(typeNode)) {
throw new Error(
`Expected TypeReferenceNode for emitted reference, got ${ts.SyntaxKind[typeNode.kind]}.`);
}
return typeNode;
}

private isLocalTypeParameter(decl: DeclarationNode): boolean {
Expand Down

0 comments on commit a26afce

Please sign in to comment.