Skip to content

Commit 09aefd2

Browse files
zarendmhevery
zarend
authored and
mhevery
committed
fix(compiler-cli): add useInlining option to type check config (#41043)
This commit fixes the behavior when creating a type constructor for a directive when the following conditions are met. 1. The directive has bound generic parameters. 2. Inlining is not available. (This happens for language service compiles). Previously, we would throw an error saying 'Inlining is not supported in this environment.' The compiler would stop type checking, and the developer could lose out on getting errors after the compiler gives up. This commit adds a useInlineTypeConstructors to the type check config. When set to false, we use `any` type for bound generic parameters to avoid crashing. When set to true, we inline the type constructor when inlining is required. Addresses #40963 PR Close #41043
1 parent c267c68 commit 09aefd2

File tree

10 files changed

+254
-101
lines changed

10 files changed

+254
-101
lines changed

‎packages/compiler-cli/src/ngtsc/core/src/compiler.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,8 @@ export class NgCompiler {
658658
// is not disabled when `strictTemplates` is enabled.
659659
const strictTemplates = !!this.options.strictTemplates;
660660

661+
const useInlineTypeConstructors = this.typeCheckingProgramStrategy.supportsInlineOperations;
662+
661663
// First select a type-checking configuration, based on whether full template type-checking is
662664
// requested.
663665
let typeCheckingConfig: TypeCheckingConfig;
@@ -689,6 +691,7 @@ export class NgCompiler {
689691
useContextGenericType: strictTemplates,
690692
strictLiteralTypes: true,
691693
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
694+
useInlineTypeConstructors,
692695
};
693696
} else {
694697
typeCheckingConfig = {
@@ -713,6 +716,7 @@ export class NgCompiler {
713716
useContextGenericType: false,
714717
strictLiteralTypes: false,
715718
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
719+
useInlineTypeConstructors,
716720
};
717721
}
718722

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,21 @@ export interface TypeCheckingConfig {
260260
* literals are cast to `any` when declared.
261261
*/
262262
strictLiteralTypes: boolean;
263+
264+
/**
265+
* Whether to use inline type constructors.
266+
*
267+
* If this is `true`, create inline type constructors when required. For example, if a type
268+
* constructor's parameters has private types, it cannot be created normally, so we inline it in
269+
* the directives definition file.
270+
*
271+
* If false, do not create inline type constructors. Fall back to using `any` type for
272+
* constructors that normally require inlining.
273+
*
274+
* This option requires the environment to support inlining. If the environment does not support
275+
* inlining, this must be set to `false`.
276+
*/
277+
useInlineTypeConstructors: boolean;
263278
}
264279

265280

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

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
179179
private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
180180
private componentMappingStrategy: ComponentToShimMappingStrategy,
181181
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
182-
private host: TypeCheckingHost, private inlining: InliningMode) {}
182+
private host: TypeCheckingHost, private inlining: InliningMode) {
183+
if (inlining === InliningMode.Error && config.useInlineTypeConstructors) {
184+
// We cannot use inlining for type checking since this environment does not support it.
185+
throw new Error(`AssertionError: invalid inlining configuration.`);
186+
}
187+
}
183188

184189
/**
185190
* A `Map` of `ts.SourceFile`s that the context has seen to the operations (additions of methods
@@ -219,23 +224,21 @@ export class TypeCheckContextImpl implements TypeCheckContext {
219224
...this.getTemplateDiagnostics(parseErrors, templateId, sourceMapping));
220225
}
221226

222-
// Accumulate a list of any directives which could not have type constructors generated due to
223-
// unsupported inlining operations.
224-
let missingInlines: ClassDeclaration[] = [];
225-
226227
const boundTarget = binder.bind({template});
227228

228-
// Get all of the directives used in the template and record type constructors for all of them.
229-
for (const dir of boundTarget.getUsedDirectives()) {
230-
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
231-
const dirNode = dirRef.node;
229+
if (this.inlining === InliningMode.InlineOps) {
230+
// Get all of the directives used in the template and record inline type constructors when
231+
// required.
232+
for (const dir of boundTarget.getUsedDirectives()) {
233+
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
234+
const dirNode = dirRef.node;
232235

233-
if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) {
234-
if (this.inlining === InliningMode.Error) {
235-
missingInlines.push(dirNode);
236+
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector)) {
237+
// inlining not required
236238
continue;
237239
}
238-
// Add a type constructor operation for the directive.
240+
241+
// Add an inline type constructor operation for the directive.
239242
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
240243
fnName: 'ngTypeCtor',
241244
// The constructor should have a body if the directive comes from a .ts file, but not if
@@ -262,18 +265,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
262265

263266
// If inlining is not supported, but is required for either the TCB or one of its directive
264267
// dependencies, then exit here with an error.
265-
if (this.inlining === InliningMode.Error && (tcbRequiresInline || missingInlines.length > 0)) {
268+
if (this.inlining === InliningMode.Error && tcbRequiresInline) {
266269
// This template cannot be supported because the underlying strategy does not support inlining
267270
// and inlining would be required.
268271

269272
// Record diagnostics to indicate the issues with this template.
270-
if (tcbRequiresInline) {
271-
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
272-
}
273-
274-
if (missingInlines.length > 0) {
275-
shimData.oobRecorder.requiresInlineTypeConstructors(templateId, ref.node, missingInlines);
276-
}
273+
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
277274

278275
// Checking this template would be unsupported, so don't try.
279276
return;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export class Environment {
4343

4444
constructor(
4545
readonly config: TypeCheckingConfig, protected importManager: ImportManager,
46-
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
46+
private refEmitter: ReferenceEmitter, readonly reflector: ReflectionHost,
4747
protected contextFile: ts.SourceFile) {}
4848

4949
/**

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

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

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

1717
import {addExpressionIdentifier, ExpressionIdentifier, markIgnoreDiagnostics} from './comments';
@@ -21,7 +21,8 @@ import {Environment} from './environment';
2121
import {astToTypescript, NULL_AS_ANY} from './expression';
2222
import {OutOfBandDiagnosticRecorder} from './oob';
2323
import {ExpressionSemanticVisitor} from './template_semantics';
24-
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
24+
import {checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
25+
import {requiresInlineTypeCtor} from './type_constructor';
2526

2627
/**
2728
* Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a
@@ -357,18 +358,13 @@ class TcbTextInterpolationOp extends TcbOp {
357358
}
358359

359360
/**
360-
* A `TcbOp` which constructs an instance of a directive _without_ setting any of its inputs. Inputs
361-
* are later set in the `TcbDirectiveInputsOp`. Type checking was found to be faster when done in
362-
* this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the directive is
363-
* generic.
364-
*
365-
* Executing this operation returns a reference to the directive instance variable with its inferred
366-
* type.
361+
* A `TcbOp` which constructs an instance of a directive. For generic directives, generic
362+
* parameters are set to `any` type.
367363
*/
368-
class TcbDirectiveTypeOp extends TcbOp {
364+
abstract class TcbDirectiveTypeOpBase extends TcbOp {
369365
constructor(
370-
private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement,
371-
private dir: TypeCheckableDirectiveMeta) {
366+
protected tcb: Context, protected scope: Scope,
367+
protected node: TmplAstTemplate|TmplAstElement, protected dir: TypeCheckableDirectiveMeta) {
372368
super();
373369
}
374370

@@ -380,16 +376,74 @@ class TcbDirectiveTypeOp extends TcbOp {
380376
}
381377

382378
execute(): ts.Identifier {
383-
const id = this.tcb.allocateId();
379+
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
380+
381+
const rawType = this.tcb.env.referenceType(this.dir.ref);
382+
383+
let type: ts.TypeNode;
384+
if (this.dir.isGeneric === false || dirRef.node.typeParameters === undefined) {
385+
type = rawType;
386+
} else {
387+
if (!ts.isTypeReferenceNode(rawType)) {
388+
throw new Error(
389+
`Expected TypeReferenceNode when referencing the type for ${this.dir.ref.debugName}`);
390+
}
391+
const typeArguments = dirRef.node.typeParameters.map(
392+
() => ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
393+
type = ts.factory.createTypeReferenceNode(rawType.typeName, typeArguments);
394+
}
384395

385-
const type = this.tcb.env.referenceType(this.dir.ref);
396+
const id = this.tcb.allocateId();
386397
addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE);
387398
addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan);
388399
this.scope.addStatement(tsDeclareVariable(id, type));
389400
return id;
390401
}
391402
}
392403

404+
/**
405+
* A `TcbOp` which constructs an instance of a non-generic directive _without_ setting any of its
406+
* inputs. Inputs are later set in the `TcbDirectiveInputsOp`. Type checking was found to be
407+
* faster when done in this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the
408+
* directive is generic.
409+
*
410+
* Executing this operation returns a reference to the directive instance variable with its inferred
411+
* type.
412+
*/
413+
class TcbNonGenericDirectiveTypeOp extends TcbDirectiveTypeOpBase {
414+
/**
415+
* Creates a variable declaration for this op's directive of the argument type. Returns the id of
416+
* the newly created variable.
417+
*/
418+
execute(): ts.Identifier {
419+
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
420+
if (this.dir.isGeneric) {
421+
throw new Error(`Assertion Error: expected ${dirRef.debugName} not to be generic.`);
422+
}
423+
return super.execute();
424+
}
425+
}
426+
427+
/**
428+
* A `TcbOp` which constructs an instance of a generic directive with its generic parameters set
429+
* to `any` type. This op is like `TcbDirectiveTypeOp`, except that generic parameters are set to
430+
* `any` type. This is used for situations where we want to avoid inlining.
431+
*
432+
* Executing this operation returns a reference to the directive instance variable with its generic
433+
* type parameters set to `any`.
434+
*/
435+
class TcbGenericDirectiveTypeWithAnyParamsOp extends TcbDirectiveTypeOpBase {
436+
execute(): ts.Identifier {
437+
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
438+
if (dirRef.node.typeParameters === undefined) {
439+
throw new Error(`Assertion Error: expected typeParameters when creating a declaration for ${
440+
dirRef.debugName}`);
441+
}
442+
443+
return super.execute();
444+
}
445+
}
446+
393447
/**
394448
* A `TcbOp` which creates a variable for a local ref in a template.
395449
* The initializer for the variable is the variable expression for the directive, template, or
@@ -1383,8 +1437,27 @@ class Scope {
13831437

13841438
const dirMap = new Map<TypeCheckableDirectiveMeta, number>();
13851439
for (const dir of directives) {
1386-
const directiveOp = dir.isGeneric ? new TcbDirectiveCtorOp(this.tcb, this, node, dir) :
1387-
new TcbDirectiveTypeOp(this.tcb, this, node, dir);
1440+
let directiveOp: TcbOp;
1441+
const host = this.tcb.env.reflector;
1442+
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
1443+
1444+
if (!dir.isGeneric) {
1445+
// The most common case is that when a directive is not generic, we use the normal
1446+
// `TcbNonDirectiveTypeOp`.
1447+
directiveOp = new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir);
1448+
} else if (
1449+
!requiresInlineTypeCtor(dirRef.node, host) ||
1450+
this.tcb.env.config.useInlineTypeConstructors) {
1451+
// For generic directives, we use a type constructor to infer types. If a directive requires
1452+
// an inline type constructor, then inlining must be available to use the
1453+
// `TcbDirectiveCtorOp`. If not we, we fallback to using `any` – see below.
1454+
directiveOp = new TcbDirectiveCtorOp(this.tcb, this, node, dir);
1455+
} else {
1456+
// If inlining is not available, then we give up on infering the generic params, and use
1457+
// `any` type for the directive's generic parameters.
1458+
directiveOp = new TcbGenericDirectiveTypeWithAnyParamsOp(this.tcb, this, node, dir);
1459+
}
1460+
13881461
const dirIndex = this.opQueue.push(directiveOp) - 1;
13891462
dirMap.set(dir, dirIndex);
13901463

‎packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ export function ngForTypeCheckTarget(): TypeCheckingTarget {
166166
};
167167
}
168168

169-
export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
169+
export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
170170
applyTemplateContextGuards: true,
171171
checkQueries: false,
172172
checkTemplateBodies: true,
@@ -188,37 +188,55 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
188188
useContextGenericType: true,
189189
strictLiteralTypes: true,
190190
enableTemplateTypeChecker: false,
191+
useInlineTypeConstructors: true
191192
};
192193

193194
// Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
194-
export type TestDirective = Partial<Pick<
195+
export interface TestDirective extends Partial<Pick<
195196
TypeCheckableDirectiveMeta,
196197
Exclude<
197198
keyof TypeCheckableDirectiveMeta,
198199
'ref'|'coercedInputFields'|'restrictedInputFields'|'stringLiteralInputFields'|
199-
'undeclaredInputFields'|'inputs'|'outputs'>>>&{
200-
selector: string, name: string, file?: AbsoluteFsPath, type: 'directive',
201-
inputs?: {[fieldName: string]: string}, outputs?: {[fieldName: string]: string},
202-
coercedInputFields?: string[], restrictedInputFields?: string[],
203-
stringLiteralInputFields?: string[], undeclaredInputFields?: string[], isGeneric?: boolean;
204-
};
200+
'undeclaredInputFields'|'inputs'|'outputs'>>> {
201+
selector: string;
202+
name: string;
203+
file?: AbsoluteFsPath;
204+
type: 'directive';
205+
inputs?: {[fieldName: string]: string};
206+
outputs?: {[fieldName: string]: string};
207+
coercedInputFields?: string[];
208+
restrictedInputFields?: string[];
209+
stringLiteralInputFields?: string[];
210+
undeclaredInputFields?: string[];
211+
isGeneric?: boolean;
212+
code?: string;
213+
}
205214

206-
export type TestPipe = {
207-
name: string,
208-
file?: AbsoluteFsPath, pipeName: string, type: 'pipe',
209-
};
215+
export interface TestPipe {
216+
name: string;
217+
file?: AbsoluteFsPath;
218+
pipeName: string;
219+
type: 'pipe';
220+
code?: string;
221+
}
210222

211223
export type TestDeclaration = TestDirective|TestPipe;
212224

213225
export function tcb(
214-
template: string, declarations: TestDeclaration[] = [], config?: TypeCheckingConfig,
226+
template: string, declarations: TestDeclaration[] = [], config?: Partial<TypeCheckingConfig>,
215227
options?: {emitSpans?: boolean}): string {
216-
const classes = ['Test', ...declarations.map(decl => decl.name)];
217-
const code = classes.map(name => `export class ${name}<T extends string> {}`).join('\n');
228+
const codeLines = [`export class Test<T extends string> {}`];
218229

230+
for (const decl of declarations) {
231+
if (decl.code !== undefined) {
232+
codeLines.push(decl.code);
233+
} else {
234+
codeLines.push(`export class ${decl.name}<T extends string> {}`);
235+
}
236+
}
219237
const rootFilePath = absoluteFrom('/synthetic.ts');
220238
const {program, host} = makeProgram([
221-
{name: rootFilePath, contents: code, isRoot: true},
239+
{name: rootFilePath, contents: codeLines.join('\n'), isRoot: true},
222240
]);
223241

224242
const sf = getSourceFileOrError(program, rootFilePath);
@@ -233,7 +251,7 @@ export function tcb(
233251
const id = 'tcb' as TemplateId;
234252
const meta: TypeCheckBlockMetadata = {id, boundTarget, pipes, schemas: []};
235253

236-
config = config || {
254+
const fullConfig: TypeCheckingConfig = {
237255
applyTemplateContextGuards: true,
238256
checkQueries: false,
239257
checkTypeOfInputBindings: true,
@@ -253,6 +271,8 @@ export function tcb(
253271
useContextGenericType: true,
254272
strictLiteralTypes: true,
255273
enableTemplateTypeChecker: false,
274+
useInlineTypeConstructors: true,
275+
...config
256276
};
257277
options = options || {
258278
emitSpans: false,
@@ -265,7 +285,7 @@ export function tcb(
265285
const refEmmiter: ReferenceEmitter = new ReferenceEmitter(
266286
[new LocalIdentifierStrategy(), new RelativePathStrategy(reflectionHost)]);
267287

268-
const env = new TypeCheckFile(fileName, config, refEmmiter, reflectionHost, host);
288+
const env = new TypeCheckFile(fileName, fullConfig, refEmmiter, reflectionHost, host);
269289

270290
const ref = new Reference(clazz);
271291

@@ -373,7 +393,14 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
373393
program, checker, moduleResolver, new TypeScriptReflectionHost(checker)),
374394
new LogicalProjectStrategy(reflectionHost, logicalFs),
375395
]);
376-
const fullConfig = {...ALL_ENABLED_CONFIG, ...config};
396+
397+
const fullConfig = {
398+
...ALL_ENABLED_CONFIG,
399+
useInlineTypeConstructors: overrides.inlining !== undefined ?
400+
overrides.inlining :
401+
ALL_ENABLED_CONFIG.useInlineTypeConstructors,
402+
...config
403+
};
377404

378405
// Map out the scope of each target component, which is needed for the ComponentScopeReader.
379406
const scopeMap = new Map<ClassDeclaration, ScopeData>();

0 commit comments

Comments
 (0)