Skip to content

Commit fa01040

Browse files
JoostKAndrewKushnir
authored andcommitted
refactor(compiler-cli): only use type constructors for directives with generic types (angular#38249)
Prior to this change, the template type checker would always use a type-constructor to instantiate a directive. This type-constructor call serves two purposes: 1. Infer any generic types for the directive instance from the inputs that are passed in. 2. Type check the inputs that are passed into the directive's inputs. The first purpose is only relevant when the directive actually has any generic types and using a type-constructor for these cases inhibits a type-check performance penalty, as a type-constructor's signature is quite complex and needs to be generated for each directive. This commit refactors the generated type-check blocks to only generate a type-constructor call for directives that have generic types. Type checking of inputs is achieved by generating individual statements for all inputs, using assignments into the directive's fields. Even if a type-constructor is used for type-inference of generic types will the input checking also be achieved using the individual assignment statements. This is done to support the rework of the language service, which will start to extract symbol information from the type-check blocks. As a future optimization, it may be possible to reduce the number of inputs passed into a type-constructor to only those inputs that contribute the the type-inference of the generics. As this is not a necessity at the moment this is left as follow-up work. Closes angular#38185 PR Close angular#38249
1 parent 80b67e0 commit fa01040

File tree

18 files changed

+951
-180
lines changed

18 files changed

+951
-180
lines changed

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {absoluteFrom, relative} from '../../file_system';
1515
import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
1616
import {DependencyTracker} from '../../incremental/api';
1717
import {IndexingContext} from '../../indexer';
18-
import {DirectiveMeta, extractDirectiveGuards, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
18+
import {DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
1919
import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance';
2020
import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
2121
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
@@ -51,7 +51,7 @@ export interface ComponentAnalysisData {
5151
*/
5252
meta: Omit<R3ComponentMetadata, ComponentMetadataResolvedFields>;
5353
baseClass: Reference<ClassDeclaration>|'dynamic'|null;
54-
guards: ReturnType<typeof extractDirectiveGuards>;
54+
typeCheckMeta: DirectiveTypeCheckMeta;
5555
template: ParsedTemplateWithSource;
5656
metadataStmt: Statement|null;
5757

@@ -327,7 +327,7 @@ export class ComponentDecoratorHandler implements
327327
i18nUseExternalIds: this.i18nUseExternalIds,
328328
relativeContextFilePath,
329329
},
330-
guards: extractDirectiveGuards(node, this.reflector),
330+
typeCheckMeta: extractDirectiveTypeCheckMeta(node, metadata.inputs, this.reflector),
331331
metadataStmt: generateSetClassMetadataCall(
332332
node, this.reflector, this.defaultImportRecorder, this.isCore,
333333
this.annotateForClosureCompiler),
@@ -356,7 +356,7 @@ export class ComponentDecoratorHandler implements
356356
queries: analysis.meta.queries.map(query => query.propertyName),
357357
isComponent: true,
358358
baseClass: analysis.baseClass,
359-
...analysis.guards,
359+
...analysis.typeCheckMeta,
360360
});
361361

362362
this.injectableRegistry.registerInjectable(node);

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

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

1212
import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
1313
import {DefaultImportRecorder, Reference} from '../../imports';
14-
import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
15-
import {extractDirectiveGuards} from '../../metadata/src/util';
14+
import {DirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
15+
import {extractDirectiveTypeCheckMeta} from '../../metadata/src/util';
1616
import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator';
1717
import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, filterToMembersWithDecorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
1818
import {LocalModuleScopeRegistry} from '../../scope';
@@ -35,7 +35,7 @@ const LIFECYCLE_HOOKS = new Set([
3535

3636
export interface DirectiveHandlerData {
3737
baseClass: Reference<ClassDeclaration>|'dynamic'|null;
38-
guards: ReturnType<typeof extractDirectiveGuards>;
38+
typeCheckMeta: DirectiveTypeCheckMeta;
3939
meta: R3DirectiveMetadata;
4040
metadataStmt: Statement|null;
4141
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
@@ -102,7 +102,7 @@ export class DirectiveDecoratorHandler implements
102102
node, this.reflector, this.defaultImportRecorder, this.isCore,
103103
this.annotateForClosureCompiler),
104104
baseClass: readBaseClass(node, this.reflector, this.evaluator),
105-
guards: extractDirectiveGuards(node, this.reflector),
105+
typeCheckMeta: extractDirectiveTypeCheckMeta(node, analysis.inputs, this.reflector),
106106
providersRequiringFactory
107107
}
108108
};
@@ -122,7 +122,7 @@ export class DirectiveDecoratorHandler implements
122122
queries: analysis.meta.queries.map(query => query.propertyName),
123123
isComponent: false,
124124
baseClass: analysis.baseClass,
125-
...analysis.guards,
125+
...analysis.typeCheckMeta,
126126
});
127127

128128
this.injectableRegistry.registerInjectable(node);

packages/compiler-cli/src/ngtsc/metadata/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,4 @@
99
export * from './src/api';
1010
export {DtsMetadataReader} from './src/dts';
1111
export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry';
12-
export {extractDirectiveGuards, CompoundMetadataReader} from './src/util';
12+
export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util';

packages/compiler-cli/src/ngtsc/metadata/src/api.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,19 +32,55 @@ export interface NgModuleMeta {
3232
rawDeclarations: ts.Expression|null;
3333
}
3434

35+
/**
36+
* Typing metadata collected for a directive within an NgModule's scope.
37+
*/
38+
export interface DirectiveTypeCheckMeta {
39+
/**
40+
* List of static `ngTemplateGuard_xx` members found on the Directive's class.
41+
* @see `TemplateGuardMeta`
42+
*/
43+
ngTemplateGuards: TemplateGuardMeta[];
44+
45+
/**
46+
* Whether the Directive's class has a static ngTemplateContextGuard function.
47+
*/
48+
hasNgTemplateContextGuard: boolean;
49+
50+
/**
51+
* The set of input fields which have a corresponding static `ngAcceptInputType_` on the
52+
* Directive's class. This allows inputs to accept a wider range of types and coerce the input to
53+
* a narrower type with a getter/setter. See https://angular.io/guide/template-typecheck.
54+
*/
55+
coercedInputFields: Set<string>;
56+
57+
/**
58+
* The set of input fields which map to `readonly`, `private`, or `protected` members in the
59+
* Directive's class.
60+
*/
61+
restrictedInputFields: Set<string>;
62+
63+
/**
64+
* The set of input fields which do not have corresponding members in the Directive's class.
65+
*/
66+
undeclaredInputFields: Set<string>;
67+
68+
/**
69+
* Whether the Directive's class is generic, i.e. `class MyDir<T> {...}`.
70+
*/
71+
isGeneric: boolean;
72+
}
73+
3574
/**
3675
* Metadata collected for a directive within an NgModule's scope.
3776
*/
38-
export interface DirectiveMeta extends T2DirectiveMeta {
77+
export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta {
3978
ref: Reference<ClassDeclaration>;
4079
/**
4180
* Unparsed selector of the directive, or null if the directive does not have a selector.
4281
*/
4382
selector: string|null;
4483
queries: string[];
45-
ngTemplateGuards: TemplateGuardMeta[];
46-
hasNgTemplateContextGuard: boolean;
47-
coercedInputFields: Set<string>;
4884

4985
/**
5086
* A `Reference` to the base class for the directive, if one was detected.

packages/compiler-cli/src/ngtsc/metadata/src/dts.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {Reference} from '../../imports';
1212
import {ClassDeclaration, isNamedClassDeclaration, ReflectionHost} from '../../reflection';
1313

1414
import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api';
15-
import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util';
15+
import {extractDirectiveTypeCheckMeta, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util';
1616

1717
/**
1818
* A `MetadataReader` that can read metadata from `.d.ts` files, which have static Ivy properties
@@ -76,16 +76,17 @@ export class DtsMetadataReader implements MetadataReader {
7676
return null;
7777
}
7878

79+
const inputs = readStringMapType(def.type.typeArguments[3]);
7980
return {
8081
ref,
8182
name: clazz.name.text,
8283
isComponent: def.name === 'ɵcmp',
8384
selector: readStringType(def.type.typeArguments[1]),
8485
exportAs: readStringArrayType(def.type.typeArguments[2]),
85-
inputs: readStringMapType(def.type.typeArguments[3]),
86+
inputs,
8687
outputs: readStringMapType(def.type.typeArguments[4]),
8788
queries: readStringArrayType(def.type.typeArguments[5]),
88-
...extractDirectiveGuards(clazz, this.reflector),
89+
...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
8990
baseClass: readBaseClass(clazz, this.checker, this.reflector),
9091
};
9192
}

packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ export function flattenInheritedDirectiveMetadata(
2828
let inputs: {[key: string]: string|[string, string]} = {};
2929
let outputs: {[key: string]: string} = {};
3030
let coercedInputFields = new Set<string>();
31+
let undeclaredInputFields = new Set<string>();
32+
let restrictedInputFields = new Set<string>();
3133
let isDynamic = false;
3234

3335
const addMetadata = (meta: DirectiveMeta): void => {
@@ -48,6 +50,12 @@ export function flattenInheritedDirectiveMetadata(
4850
for (const coercedInputField of meta.coercedInputFields) {
4951
coercedInputFields.add(coercedInputField);
5052
}
53+
for (const undeclaredInputField of meta.undeclaredInputFields) {
54+
undeclaredInputFields.add(undeclaredInputField);
55+
}
56+
for (const restrictedInputField of meta.restrictedInputFields) {
57+
restrictedInputFields.add(restrictedInputField);
58+
}
5159
};
5260

5361
addMetadata(topMeta);
@@ -57,6 +65,8 @@ export function flattenInheritedDirectiveMetadata(
5765
inputs,
5866
outputs,
5967
coercedInputFields,
68+
undeclaredInputFields,
69+
restrictedInputFields,
6070
baseClass: isDynamic ? 'dynamic' : null,
6171
};
6272
}

packages/compiler-cli/src/ngtsc/metadata/src/util.ts

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {Reference} from '../../imports';
1212
import {ClassDeclaration, ClassMember, ClassMemberKind, isNamedClassDeclaration, ReflectionHost, reflectTypeEntityToDeclaration} from '../../reflection';
1313
import {nodeDebugInfo} from '../../util/src/typescript';
1414

15-
import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api';
15+
import {DirectiveMeta, DirectiveTypeCheckMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api';
1616

1717
export function extractReferencesFromType(
1818
checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string|null,
@@ -78,13 +78,16 @@ export function readStringArrayType(type: ts.TypeNode): string[] {
7878
return res;
7979
}
8080

81-
82-
export function extractDirectiveGuards(node: ClassDeclaration, reflector: ReflectionHost): {
83-
ngTemplateGuards: TemplateGuardMeta[],
84-
hasNgTemplateContextGuard: boolean,
85-
coercedInputFields: Set<string>,
86-
} {
87-
const staticMembers = reflector.getMembersOfClass(node).filter(member => member.isStatic);
81+
/**
82+
* Inspects the class' members and extracts the metadata that is used when type-checking templates
83+
* that use the directive. This metadata does not contain information from a base class, if any,
84+
* making this metadata invariant to changes of inherited classes.
85+
*/
86+
export function extractDirectiveTypeCheckMeta(
87+
node: ClassDeclaration, inputs: {[fieldName: string]: string|[string, string]},
88+
reflector: ReflectionHost): DirectiveTypeCheckMeta {
89+
const members = reflector.getMembersOfClass(node);
90+
const staticMembers = members.filter(member => member.isStatic);
8891
const ngTemplateGuards = staticMembers.map(extractTemplateGuard)
8992
.filter((guard): guard is TemplateGuardMeta => guard !== null);
9093
const hasNgTemplateContextGuard = staticMembers.some(
@@ -93,7 +96,40 @@ export function extractDirectiveGuards(node: ClassDeclaration, reflector: Reflec
9396
const coercedInputFields =
9497
new Set(staticMembers.map(extractCoercedInput)
9598
.filter((inputName): inputName is string => inputName !== null));
96-
return {hasNgTemplateContextGuard, ngTemplateGuards, coercedInputFields};
99+
100+
const restrictedInputFields = new Set<string>();
101+
const undeclaredInputFields = new Set<string>();
102+
103+
for (const fieldName of Object.keys(inputs)) {
104+
const field = members.find(member => member.name === fieldName);
105+
if (field === undefined || field.node === null) {
106+
undeclaredInputFields.add(fieldName);
107+
} else if (isRestricted(field.node)) {
108+
restrictedInputFields.add(fieldName);
109+
}
110+
}
111+
112+
const arity = reflector.getGenericArityOfClass(node);
113+
114+
return {
115+
hasNgTemplateContextGuard,
116+
ngTemplateGuards,
117+
coercedInputFields,
118+
restrictedInputFields,
119+
undeclaredInputFields,
120+
isGeneric: arity !== null && arity > 0,
121+
};
122+
}
123+
124+
function isRestricted(node: ts.Node): boolean {
125+
if (node.modifiers === undefined) {
126+
return false;
127+
}
128+
129+
return node.modifiers.some(
130+
modifier => modifier.kind === ts.SyntaxKind.PrivateKeyword ||
131+
modifier.kind === ts.SyntaxKind.ProtectedKeyword ||
132+
modifier.kind === ts.SyntaxKind.ReadonlyKeyword);
97133
}
98134

99135
function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null {

packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,9 @@ function fakeDirective(ref: Reference<ClassDeclaration>): DirectiveMeta {
243243
hasNgTemplateContextGuard: false,
244244
ngTemplateGuards: [],
245245
coercedInputFields: new Set<string>(),
246+
restrictedInputFields: new Set<string>(),
247+
undeclaredInputFields: new Set<string>(),
248+
isGeneric: false,
246249
baseClass: null,
247250
};
248251
}

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

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

1212
import {AbsoluteFsPath} from '../../file_system';
1313
import {Reference} from '../../imports';
14-
import {TemplateGuardMeta} from '../../metadata';
14+
import {DirectiveTypeCheckMeta} from '../../metadata';
1515
import {ClassDeclaration} from '../../reflection';
1616

1717

1818
/**
1919
* Extension of `DirectiveMeta` that includes additional information required to type-check the
2020
* usage of a particular directive.
2121
*/
22-
export interface TypeCheckableDirectiveMeta extends DirectiveMeta {
22+
export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveTypeCheckMeta {
2323
ref: Reference<ClassDeclaration>;
2424
queries: string[];
25-
ngTemplateGuards: TemplateGuardMeta[];
26-
coercedInputFields: Set<string>;
27-
hasNgTemplateContextGuard: boolean;
2825
}
2926

3027
export type TemplateId = string&{__brand: 'TemplateId'};

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,12 +199,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
199199
for (const dir of boundTarget.getUsedDirectives()) {
200200
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
201201
const dirNode = dirRef.node;
202-
if (requiresInlineTypeCtor(dirNode, this.reflector)) {
202+
203+
if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) {
203204
if (this.inlining === InliningMode.Error) {
204205
missingInlines.push(dirNode);
205206
continue;
206207
}
207-
208208
// Add a type constructor operation for the directive.
209209
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
210210
fnName: 'ngTypeCtor',

packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,21 @@ export function tsDeclareVariable(id: ts.Identifier, type: ts.TypeNode): ts.Vari
8686
/* declarationList */[decl]);
8787
}
8888

89+
/**
90+
* Creates a `ts.TypeQueryNode` for a coerced input.
91+
*
92+
* For example: `typeof MatInput.ngAcceptInputType_value`, where MatInput is `typeName` and `value`
93+
* is the `coercedInputName`.
94+
*
95+
* @param typeName The `EntityName` of the Directive where the static coerced input is defined.
96+
* @param coercedInputName The field name of the coerced input.
97+
*/
98+
export function tsCreateTypeQueryForCoercedInput(
99+
typeName: ts.EntityName, coercedInputName: string): ts.TypeQueryNode {
100+
return ts.createTypeQueryNode(
101+
ts.createQualifiedName(typeName, `ngAcceptInputType_${coercedInputName}`));
102+
}
103+
89104
/**
90105
* Create a `ts.VariableStatement` that initializes a variable with a given expression.
91106
*

0 commit comments

Comments
 (0)