From fa0104017a3f0f2d9d3e9bf0180290f589b137e4 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 23 Jul 2020 00:19:38 +0200 Subject: [PATCH] refactor(compiler-cli): only use type constructors for directives with generic types (#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 #38185 PR Close #38249 --- .../src/ngtsc/annotations/src/component.ts | 8 +- .../src/ngtsc/annotations/src/directive.ts | 10 +- .../compiler-cli/src/ngtsc/metadata/index.ts | 2 +- .../src/ngtsc/metadata/src/api.ts | 44 ++- .../src/ngtsc/metadata/src/dts.ts | 7 +- .../src/ngtsc/metadata/src/inheritance.ts | 10 + .../src/ngtsc/metadata/src/util.ts | 54 ++- .../src/ngtsc/scope/test/local_spec.ts | 3 + .../src/ngtsc/typecheck/api/api.ts | 7 +- .../src/ngtsc/typecheck/src/context.ts | 4 +- .../src/ngtsc/typecheck/src/ts_util.ts | 15 + .../ngtsc/typecheck/src/type_check_block.ts | 349 +++++++++++++----- .../ngtsc/typecheck/src/type_constructor.ts | 5 +- .../ngtsc/typecheck/test/diagnostics_spec.ts | 3 +- .../src/ngtsc/typecheck/test/test_utils.ts | 17 +- .../typecheck/test/type_check_block_spec.ts | 337 +++++++++++++++-- .../ngtsc/typecheck/test/type_checker_spec.ts | 8 +- .../test/ngtsc/template_typecheck_spec.ts | 250 ++++++++++++- 18 files changed, 952 insertions(+), 181 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index c2b5559f6a..5ea6aec532 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -15,7 +15,7 @@ import {absoluteFrom, relative} from '../../file_system'; import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; import {IndexingContext} from '../../indexer'; -import {DirectiveMeta, extractDirectiveGuards, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; +import {DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; @@ -51,7 +51,7 @@ export interface ComponentAnalysisData { */ meta: Omit; baseClass: Reference|'dynamic'|null; - guards: ReturnType; + typeCheckMeta: DirectiveTypeCheckMeta; template: ParsedTemplateWithSource; metadataStmt: Statement|null; @@ -327,7 +327,7 @@ export class ComponentDecoratorHandler implements i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath, }, - guards: extractDirectiveGuards(node, this.reflector), + typeCheckMeta: extractDirectiveTypeCheckMeta(node, metadata.inputs, this.reflector), metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), @@ -356,7 +356,7 @@ export class ComponentDecoratorHandler implements queries: analysis.meta.queries.map(query => query.propertyName), isComponent: true, baseClass: analysis.baseClass, - ...analysis.guards, + ...analysis.typeCheckMeta, }); this.injectableRegistry.registerInjectable(node); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 04c6f3f2c4..6bdd3398a7 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -11,8 +11,8 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder, Reference} from '../../imports'; -import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; -import {extractDirectiveGuards} from '../../metadata/src/util'; +import {DirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; +import {extractDirectiveTypeCheckMeta} from '../../metadata/src/util'; import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, filterToMembersWithDecorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {LocalModuleScopeRegistry} from '../../scope'; @@ -35,7 +35,7 @@ const LIFECYCLE_HOOKS = new Set([ export interface DirectiveHandlerData { baseClass: Reference|'dynamic'|null; - guards: ReturnType; + typeCheckMeta: DirectiveTypeCheckMeta; meta: R3DirectiveMetadata; metadataStmt: Statement|null; providersRequiringFactory: Set>|null; @@ -102,7 +102,7 @@ export class DirectiveDecoratorHandler implements node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), baseClass: readBaseClass(node, this.reflector, this.evaluator), - guards: extractDirectiveGuards(node, this.reflector), + typeCheckMeta: extractDirectiveTypeCheckMeta(node, analysis.inputs, this.reflector), providersRequiringFactory } }; @@ -122,7 +122,7 @@ export class DirectiveDecoratorHandler implements queries: analysis.meta.queries.map(query => query.propertyName), isComponent: false, baseClass: analysis.baseClass, - ...analysis.guards, + ...analysis.typeCheckMeta, }); this.injectableRegistry.registerInjectable(node); diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index 9061d03146..ee3f910b1b 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -9,4 +9,4 @@ export * from './src/api'; export {DtsMetadataReader} from './src/dts'; export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry'; -export {extractDirectiveGuards, CompoundMetadataReader} from './src/util'; +export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 45fac785ac..a9d58de7c7 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -32,19 +32,55 @@ export interface NgModuleMeta { rawDeclarations: ts.Expression|null; } +/** + * Typing metadata collected for a directive within an NgModule's scope. + */ +export interface DirectiveTypeCheckMeta { + /** + * List of static `ngTemplateGuard_xx` members found on the Directive's class. + * @see `TemplateGuardMeta` + */ + ngTemplateGuards: TemplateGuardMeta[]; + + /** + * Whether the Directive's class has a static ngTemplateContextGuard function. + */ + hasNgTemplateContextGuard: boolean; + + /** + * The set of input fields which have a corresponding static `ngAcceptInputType_` on the + * Directive's class. This allows inputs to accept a wider range of types and coerce the input to + * a narrower type with a getter/setter. See https://angular.io/guide/template-typecheck. + */ + coercedInputFields: Set; + + /** + * The set of input fields which map to `readonly`, `private`, or `protected` members in the + * Directive's class. + */ + restrictedInputFields: Set; + + /** + * The set of input fields which do not have corresponding members in the Directive's class. + */ + undeclaredInputFields: Set; + + /** + * Whether the Directive's class is generic, i.e. `class MyDir {...}`. + */ + isGeneric: boolean; +} + /** * Metadata collected for a directive within an NgModule's scope. */ -export interface DirectiveMeta extends T2DirectiveMeta { +export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta { ref: Reference; /** * Unparsed selector of the directive, or null if the directive does not have a selector. */ selector: string|null; queries: string[]; - ngTemplateGuards: TemplateGuardMeta[]; - hasNgTemplateContextGuard: boolean; - coercedInputFields: Set; /** * A `Reference` to the base class for the directive, if one was detected. diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index c8f2c8b2b1..faf588ab67 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -12,7 +12,7 @@ import {Reference} from '../../imports'; import {ClassDeclaration, isNamedClassDeclaration, ReflectionHost} from '../../reflection'; import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api'; -import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util'; +import {extractDirectiveTypeCheckMeta, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util'; /** * A `MetadataReader` that can read metadata from `.d.ts` files, which have static Ivy properties @@ -76,16 +76,17 @@ export class DtsMetadataReader implements MetadataReader { return null; } + const inputs = readStringMapType(def.type.typeArguments[3]); return { ref, name: clazz.name.text, isComponent: def.name === 'ɵcmp', selector: readStringType(def.type.typeArguments[1]), exportAs: readStringArrayType(def.type.typeArguments[2]), - inputs: readStringMapType(def.type.typeArguments[3]), + inputs, outputs: readStringMapType(def.type.typeArguments[4]), queries: readStringArrayType(def.type.typeArguments[5]), - ...extractDirectiveGuards(clazz, this.reflector), + ...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector), baseClass: readBaseClass(clazz, this.checker, this.reflector), }; } diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts b/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts index 119a7a0e39..ba775783e1 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts @@ -28,6 +28,8 @@ export function flattenInheritedDirectiveMetadata( let inputs: {[key: string]: string|[string, string]} = {}; let outputs: {[key: string]: string} = {}; let coercedInputFields = new Set(); + let undeclaredInputFields = new Set(); + let restrictedInputFields = new Set(); let isDynamic = false; const addMetadata = (meta: DirectiveMeta): void => { @@ -48,6 +50,12 @@ export function flattenInheritedDirectiveMetadata( for (const coercedInputField of meta.coercedInputFields) { coercedInputFields.add(coercedInputField); } + for (const undeclaredInputField of meta.undeclaredInputFields) { + undeclaredInputFields.add(undeclaredInputField); + } + for (const restrictedInputField of meta.restrictedInputFields) { + restrictedInputFields.add(restrictedInputField); + } }; addMetadata(topMeta); @@ -57,6 +65,8 @@ export function flattenInheritedDirectiveMetadata( inputs, outputs, coercedInputFields, + undeclaredInputFields, + restrictedInputFields, baseClass: isDynamic ? 'dynamic' : null, }; } diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index 457c8cab2c..4cf0e5eabd 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -12,7 +12,7 @@ import {Reference} from '../../imports'; import {ClassDeclaration, ClassMember, ClassMemberKind, isNamedClassDeclaration, ReflectionHost, reflectTypeEntityToDeclaration} from '../../reflection'; import {nodeDebugInfo} from '../../util/src/typescript'; -import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api'; +import {DirectiveMeta, DirectiveTypeCheckMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api'; export function extractReferencesFromType( checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string|null, @@ -78,13 +78,16 @@ export function readStringArrayType(type: ts.TypeNode): string[] { return res; } - -export function extractDirectiveGuards(node: ClassDeclaration, reflector: ReflectionHost): { - ngTemplateGuards: TemplateGuardMeta[], - hasNgTemplateContextGuard: boolean, - coercedInputFields: Set, -} { - const staticMembers = reflector.getMembersOfClass(node).filter(member => member.isStatic); +/** + * Inspects the class' members and extracts the metadata that is used when type-checking templates + * that use the directive. This metadata does not contain information from a base class, if any, + * making this metadata invariant to changes of inherited classes. + */ +export function extractDirectiveTypeCheckMeta( + node: ClassDeclaration, inputs: {[fieldName: string]: string|[string, string]}, + reflector: ReflectionHost): DirectiveTypeCheckMeta { + const members = reflector.getMembersOfClass(node); + const staticMembers = members.filter(member => member.isStatic); const ngTemplateGuards = staticMembers.map(extractTemplateGuard) .filter((guard): guard is TemplateGuardMeta => guard !== null); const hasNgTemplateContextGuard = staticMembers.some( @@ -93,7 +96,40 @@ export function extractDirectiveGuards(node: ClassDeclaration, reflector: Reflec const coercedInputFields = new Set(staticMembers.map(extractCoercedInput) .filter((inputName): inputName is string => inputName !== null)); - return {hasNgTemplateContextGuard, ngTemplateGuards, coercedInputFields}; + + const restrictedInputFields = new Set(); + const undeclaredInputFields = new Set(); + + for (const fieldName of Object.keys(inputs)) { + const field = members.find(member => member.name === fieldName); + if (field === undefined || field.node === null) { + undeclaredInputFields.add(fieldName); + } else if (isRestricted(field.node)) { + restrictedInputFields.add(fieldName); + } + } + + const arity = reflector.getGenericArityOfClass(node); + + return { + hasNgTemplateContextGuard, + ngTemplateGuards, + coercedInputFields, + restrictedInputFields, + undeclaredInputFields, + isGeneric: arity !== null && arity > 0, + }; +} + +function isRestricted(node: ts.Node): boolean { + if (node.modifiers === undefined) { + return false; + } + + return node.modifiers.some( + modifier => modifier.kind === ts.SyntaxKind.PrivateKeyword || + modifier.kind === ts.SyntaxKind.ProtectedKeyword || + modifier.kind === ts.SyntaxKind.ReadonlyKeyword); } function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index 7e98f45aa9..60302988a1 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -243,6 +243,9 @@ function fakeDirective(ref: Reference): DirectiveMeta { hasNgTemplateContextGuard: false, ngTemplateGuards: [], coercedInputFields: new Set(), + restrictedInputFields: new Set(), + undeclaredInputFields: new Set(), + isGeneric: false, baseClass: null, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index b7db3f0f6d..f0e241144a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; import {Reference} from '../../imports'; -import {TemplateGuardMeta} from '../../metadata'; +import {DirectiveTypeCheckMeta} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; @@ -19,12 +19,9 @@ import {ClassDeclaration} from '../../reflection'; * Extension of `DirectiveMeta` that includes additional information required to type-check the * usage of a particular directive. */ -export interface TypeCheckableDirectiveMeta extends DirectiveMeta { +export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveTypeCheckMeta { ref: Reference; queries: string[]; - ngTemplateGuards: TemplateGuardMeta[]; - coercedInputFields: Set; - hasNgTemplateContextGuard: boolean; } export type TemplateId = string&{__brand: 'TemplateId'}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 9bd6897fde..748ee71d74 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -199,12 +199,12 @@ export class TypeCheckContextImpl implements TypeCheckContext { for (const dir of boundTarget.getUsedDirectives()) { const dirRef = dir.ref as Reference>; const dirNode = dirRef.node; - if (requiresInlineTypeCtor(dirNode, this.reflector)) { + + if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) { if (this.inlining === InliningMode.Error) { missingInlines.push(dirNode); continue; } - // Add a type constructor operation for the directive. this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, { fnName: 'ngTypeCtor', diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts index 7c58444851..96ff8178a4 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts @@ -86,6 +86,21 @@ export function tsDeclareVariable(id: ts.Identifier, type: ts.TypeNode): ts.Vari /* declarationList */[decl]); } +/** + * Creates a `ts.TypeQueryNode` for a coerced input. + * + * For example: `typeof MatInput.ngAcceptInputType_value`, where MatInput is `typeName` and `value` + * is the `coercedInputName`. + * + * @param typeName The `EntityName` of the Directive where the static coerced input is defined. + * @param coercedInputName The field name of the coerced input. + */ +export function tsCreateTypeQueryForCoercedInput( + typeName: ts.EntityName, coercedInputName: string): ts.TypeQueryNode { + return ts.createTypeQueryNode( + ts.createQualifiedName(typeName, `ngAcceptInputType_${coercedInputName}`)); +} + /** * Create a `ts.VariableStatement` that initializes a variable with a given expression. * diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 014ff17e02..d2d56de68c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -19,7 +19,7 @@ import {Environment} from './environment'; import {astToTypescript, NULL_AS_ANY} from './expression'; import {OutOfBandDiagnosticRecorder} from './oob'; import {ExpressionSemanticVisitor} from './template_semantics'; -import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; +import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util'; @@ -197,6 +197,7 @@ class TcbTemplateBodyOp extends TcbOp { constructor(private tcb: Context, private scope: Scope, private template: TmplAstTemplate) { super(); } + execute(): null { // An `if` will be constructed, within which the template's children will be type checked. The // `if` is used for two reasons: it creates a new syntactic scope, isolating variables declared @@ -308,13 +309,15 @@ class TcbTextInterpolationOp extends TcbOp { } /** - * A `TcbOp` which constructs an instance of a directive with types inferred from its inputs, which - * also checks the bindings to the directive in the process. + * A `TcbOp` which constructs an instance of a directive _without_ setting any of its inputs. Inputs + * are later set in the `TcbDirectiveInputsOp`. Type checking was found to be faster when done in + * this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the directive is + * generic. * * Executing this operation returns a reference to the directive instance variable with its inferred * type. */ -class TcbDirectiveOp extends TcbOp { +class TcbDirectiveTypeOp extends TcbOp { constructor( private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement, private dir: TypeCheckableDirectiveMeta) { @@ -323,37 +326,190 @@ class TcbDirectiveOp extends TcbOp { execute(): ts.Identifier { const id = this.tcb.allocateId(); - // Process the directive and construct expressions for each of its bindings. - const inputs = tcbGetDirectiveInputs(this.node, this.dir, this.tcb, this.scope); + + const type = this.tcb.env.referenceType(this.dir.ref); + this.scope.addStatement(tsDeclareVariable(id, type)); + return id; + } +} + +/** + * A `TcbOp` which constructs an instance of a directive with types inferred from its inputs. The + * inputs themselves are not checked here; checking of inputs is achieved in `TcbDirectiveInputsOp`. + * Any errors reported in this statement are ignored, as the type constructor call is only present + * for type-inference. + * + * When a Directive is generic, it is required that the TCB generates the instance using this method + * in order to infer the type information correctly. + * + * Executing this operation returns a reference to the directive instance variable with its inferred + * type. + */ +class TcbDirectiveCtorOp extends TcbOp { + constructor( + private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement, + private dir: TypeCheckableDirectiveMeta) { + super(); + } + + execute(): ts.Identifier { + const id = this.tcb.allocateId(); + + const genericInputs = new Map(); + + const inputs = getBoundInputs(this.dir, this.node, this.tcb); + for (const input of inputs) { + for (const fieldName of input.fieldNames) { + // Skip the field if an attribute has already been bound to it; we can't have a duplicate + // key in the type constructor call. + if (genericInputs.has(fieldName)) { + continue; + } + + const expression = translateInput(input.attribute, this.tcb, this.scope); + genericInputs.set(fieldName, { + type: 'binding', + field: fieldName, + expression, + sourceSpan: input.attribute.sourceSpan + }); + } + } + + // Add unset directive inputs for each of the remaining unset fields. + for (const fieldName of Object.keys(this.dir.inputs)) { + if (!genericInputs.has(fieldName)) { + genericInputs.set(fieldName, {type: 'unset', field: fieldName}); + } + } // Call the type constructor of the directive to infer a type, and assign the directive // instance. - const typeCtor = tcbCallTypeCtor(this.dir, this.tcb, inputs); - addParseSpanInfo(typeCtor, this.node.sourceSpan); + const typeCtor = tcbCallTypeCtor(this.dir, this.tcb, Array.from(genericInputs.values())); + ignoreDiagnostics(typeCtor); this.scope.addStatement(tsCreateVariable(id, typeCtor)); return id; } circularFallback(): TcbOp { - return new TcbDirectiveCircularFallbackOp(this.tcb, this.scope, this.node, this.dir); + return new TcbDirectiveCtorCircularFallbackOp(this.tcb, this.scope, this.node, this.dir); + } +} + +/** + * A `TcbOp` which generates code to check input bindings on an element that correspond with the + * members of a directive. + * + * Executing this operation returns nothing. + */ +class TcbDirectiveInputsOp extends TcbOp { + constructor( + private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement, + private dir: TypeCheckableDirectiveMeta) { + super(); + } + + execute(): null { + const dirId = this.scope.resolve(this.node, this.dir); + + // TODO(joost): report duplicate properties + + const inputs = getBoundInputs(this.dir, this.node, this.tcb); + for (const input of inputs) { + // For bound inputs, the property is assigned the binding expression. + let expr = translateInput(input.attribute, this.tcb, this.scope); + if (!this.tcb.env.config.checkTypeOfInputBindings) { + // If checking the type of bindings is disabled, cast the resulting expression to 'any' + // before the assignment. + expr = tsCastToAny(expr); + } else if (!this.tcb.env.config.strictNullInputBindings) { + // If strict null checks are disabled, erase `null` and `undefined` from the type by + // wrapping the expression in a non-null assertion. + expr = ts.createNonNullExpression(expr); + } + + let assignment: ts.Expression = wrapForDiagnostics(expr); + + for (const fieldName of input.fieldNames) { + let target: ts.LeftHandSideExpression; + if (this.dir.coercedInputFields.has(fieldName)) { + // The input has a coercion declaration which should be used instead of assigning the + // expression into the input field directly. To achieve this, a variable is declared + // with a type of `typeof Directive.ngAcceptInputType_fieldName` which is then used as + // target of the assignment. + const dirTypeRef = this.tcb.env.referenceType(this.dir.ref); + if (!ts.isTypeReferenceNode(dirTypeRef)) { + throw new Error( + `Expected TypeReferenceNode from reference to ${this.dir.ref.debugName}`); + } + + const id = this.tcb.allocateId(); + const type = tsCreateTypeQueryForCoercedInput(dirTypeRef.typeName, fieldName); + this.scope.addStatement(tsDeclareVariable(id, type)); + + target = id; + } else if (this.dir.undeclaredInputFields.has(fieldName)) { + // If no coercion declaration is present nor is the field declared (i.e. the input is + // declared in a `@Directive` or `@Component` decorator's `inputs` property) there is no + // assignment target available, so this field is skipped. + continue; + } else if (this.dir.restrictedInputFields.has(fieldName)) { + // To ignore errors, assign to temp variable with type of the field + const id = this.tcb.allocateId(); + const dirTypeRef = this.tcb.env.referenceType(this.dir.ref); + if (!ts.isTypeReferenceNode(dirTypeRef)) { + throw new Error( + `Expected TypeReferenceNode from reference to ${this.dir.ref.debugName}`); + } + const type = ts.createIndexedAccessTypeNode( + ts.createTypeQueryNode(dirId as ts.Identifier), + ts.createLiteralTypeNode(ts.createStringLiteral(fieldName))); + const temp = tsCreateVariable(id, ts.createNonNullExpression(ts.createNull()), type); + addParseSpanInfo(temp, input.attribute.sourceSpan); + this.scope.addStatement(temp); + target = id; + + // TODO: To get errors assign directly to the fields on the instance, using dot access + // when possible + + } else { + // Otherwise, a declaration exists in which case the `dir["fieldName"]` syntax is used + // as assignment target. An element access is used instead of a property access to + // support input names that are not valid JavaScript identifiers. Additionally, using + // element access syntax does not produce + // TS2341 "Property $prop is private and only accessible within class $class." nor + // TS2445 "Property $prop is protected and only accessible within class $class and its + // subclasses." + target = ts.createElementAccess(dirId, ts.createStringLiteral(fieldName)); + } + + // Finally the assignment is extended by assigning it into the target expression. + assignment = ts.createBinary(target, ts.SyntaxKind.EqualsToken, assignment); + } + + addParseSpanInfo(assignment, input.attribute.sourceSpan); + this.scope.addStatement(ts.createExpressionStatement(assignment)); + } + + return null; } } /** * A `TcbOp` which is used to generate a fallback expression if the inference of a directive type - * via `TcbDirectiveOp` requires a reference to its own type. This can happen using a template + * via `TcbDirectiveCtorOp` requires a reference to its own type. This can happen using a template * reference: * * ```html * * ``` * - * In this case, `TcbDirectiveCircularFallbackOp` will add a second inference of the directive type - * to the type-check block, this time calling the directive's type constructor without any input - * expressions. This infers the widest possible supertype for the directive, which is used to + * In this case, `TcbDirectiveCtorCircularFallbackOp` will add a second inference of the directive + * type to the type-check block, this time calling the directive's type constructor without any + * input expressions. This infers the widest possible supertype for the directive, which is used to * resolve any recursive references required to infer the real type. */ -class TcbDirectiveCircularFallbackOp extends TcbOp { +class TcbDirectiveCtorCircularFallbackOp extends TcbOp { constructor( private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement, private dir: TypeCheckableDirectiveMeta) { @@ -694,8 +850,8 @@ class Scope { */ private elementOpMap = new Map(); /** - * A map of maps which tracks the index of `TcbDirectiveOp`s in the `opQueue` for each directive - * on a `TmplAstElement` or `TmplAstTemplate` node. + * A map of maps which tracks the index of `TcbDirectiveCtorOp`s in the `opQueue` for each + * directive on a `TmplAstElement` or `TmplAstTemplate` node. */ private directiveOpMap = new Map>(); @@ -957,8 +1113,12 @@ class Scope { const dirMap = new Map(); for (const dir of directives) { - const dirIndex = this.opQueue.push(new TcbDirectiveOp(this.tcb, this, node, dir)) - 1; + const directiveOp = dir.isGeneric ? new TcbDirectiveCtorOp(this.tcb, this, node, dir) : + new TcbDirectiveTypeOp(this.tcb, this, node, dir); + const dirIndex = this.opQueue.push(directiveOp) - 1; dirMap.set(dir, dirIndex); + + this.opQueue.push(new TcbDirectiveInputsOp(this.tcb, this, node, dir)); } this.directiveOpMap.set(node, dirMap); @@ -1016,6 +1176,11 @@ class Scope { } } +interface TcbBoundInput { + attribute: TmplAstBoundAttribute|TmplAstTextAttribute; + fieldNames: string[]; +} + /** * Create the `ctx` parameter to the top-level TCB function. * @@ -1269,53 +1434,13 @@ function tcbCallTypeCtor( /* argumentsArray */[ts.createObjectLiteral(members)]); } -type TcbDirectiveInput = { - type: 'binding'; field: string; expression: ts.Expression; sourceSpan: ParseSourceSpan; -}|{ - type: 'unset'; - field: string; -}; +function getBoundInputs( + directive: TypeCheckableDirectiveMeta, node: TmplAstTemplate|TmplAstElement, + tcb: Context): TcbBoundInput[] { + const boundInputs: TcbBoundInput[] = []; -function tcbGetDirectiveInputs( - el: TmplAstElement|TmplAstTemplate, dir: TypeCheckableDirectiveMeta, tcb: Context, - scope: Scope): TcbDirectiveInput[] { - // Only the first binding to a property is written. - // TODO(alxhub): produce an error for duplicate bindings to the same property, independently of - // this logic. - const directiveInputs = new Map(); - // `dir.inputs` is an object map of field names on the directive class to property names. - // This is backwards from what's needed to match bindings - a map of properties to field names - // is desired. Invert `dir.inputs` into `propMatch` to create this map. - const propMatch = new Map(); - const inputs = dir.inputs; - Object.keys(inputs).forEach(key => { - Array.isArray(inputs[key]) ? propMatch.set(inputs[key][0], key) : - propMatch.set(inputs[key] as string, key); - }); - - el.inputs.forEach(processAttribute); - el.attributes.forEach(processAttribute); - if (el instanceof TmplAstTemplate) { - el.templateAttrs.forEach(processAttribute); - } - - // Add unset directive inputs for each of the remaining unset fields. - // Note: it's actually important here that `propMatch.values()` isn't used, as there can be - // multiple fields which share the same property name and only one of them will be listed as a - // value in `propMatch`. - for (const field of Object.keys(inputs)) { - if (!directiveInputs.has(field)) { - directiveInputs.set(field, {type: 'unset', field}); - } - } - - return Array.from(directiveInputs.values()); - - /** - * Add a binding expression to the map for each input/template attribute of the directive that has - * a matching binding. - */ - function processAttribute(attr: TmplAstBoundAttribute|TmplAstTextAttribute): void { + const propertyToFieldNames = invertInputs(directive.inputs); + const processAttribute = (attr: TmplAstBoundAttribute|TmplAstTextAttribute) => { // Skip non-property bindings. if (attr instanceof TmplAstBoundAttribute && attr.type !== BindingType.Property) { return; @@ -1327,34 +1452,92 @@ function tcbGetDirectiveInputs( } // Skip the attribute if the directive does not have an input for it. - if (!propMatch.has(attr.name)) { + if (!propertyToFieldNames.has(attr.name)) { return; } - const field = propMatch.get(attr.name)!; + const fieldNames = propertyToFieldNames.get(attr.name)!; + boundInputs.push({attribute: attr, fieldNames}); + }; - // Skip the attribute if a previous binding also wrote to it. - if (directiveInputs.has(field)) { - return; - } + node.inputs.forEach(processAttribute); + node.attributes.forEach(processAttribute); + if (node instanceof TmplAstTemplate) { + node.templateAttrs.forEach(processAttribute); + } - let expr: ts.Expression; - if (attr instanceof TmplAstBoundAttribute) { - // Produce an expression representing the value of the binding. - expr = tcbExpression(attr.value, tcb, scope); - } else { - // For regular attributes with a static string value, use the represented string literal. - expr = ts.createStringLiteral(attr.value); - } + return boundInputs; +} - directiveInputs.set(field, { - type: 'binding', - field: field, - expression: expr, - sourceSpan: attr.sourceSpan, - }); +/** + * Translates the given attribute binding to a `ts.Expression`. + */ +function translateInput( + attr: TmplAstBoundAttribute|TmplAstTextAttribute, tcb: Context, scope: Scope): ts.Expression { + if (attr instanceof TmplAstBoundAttribute) { + // Produce an expression representing the value of the binding. + return tcbExpression(attr.value, tcb, scope); + } else { + // For regular attributes with a static string value, use the represented string literal. + return ts.createStringLiteral(attr.value); } } +/** + * Inverts the input-mapping from field-to-property name into property-to-field name, to be able + * to match a property in a template with the corresponding field on a directive. + */ +function invertInputs(inputs: {[fieldName: string]: string|[string, string]}): + Map { + const propertyToFieldNames = new Map(); + for (const fieldName of Object.keys(inputs)) { + const propertyNames = inputs[fieldName]; + const propertyName = Array.isArray(propertyNames) ? propertyNames[0] : propertyNames; + + if (propertyToFieldNames.has(propertyName)) { + propertyToFieldNames.get(propertyName)!.push(fieldName); + } else { + propertyToFieldNames.set(propertyName, [fieldName]); + } + } + return propertyToFieldNames; +} + +/** + * An input binding that corresponds with a field of a directive. + */ +interface TcbDirectiveBoundInput { + type: 'binding'; + + /** + * The name of a field on the directive that is set. + */ + field: string; + + /** + * The `ts.Expression` corresponding with the input binding expression. + */ + expression: ts.Expression; + + /** + * The source span of the full attribute binding. + */ + sourceSpan: ParseSourceSpan; +} + +/** + * Indicates that a certain field of a directive does not have a corresponding input binding. + */ +interface TcbDirectiveUnsetInput { + type: 'unset'; + + /** + * The name of a field on the directive for which no input binding is present. + */ + field: string; +} + +type TcbDirectiveInput = TcbDirectiveBoundInput|TcbDirectiveUnsetInput; + const EVENT_PARAMETER = '$event'; const enum EventParamType { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts index f80fd0fc69..8cb2114ddf 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts @@ -11,6 +11,7 @@ import * as ts from 'typescript'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; import {TypeCtorMetadata} from '../api'; +import {tsCreateTypeQueryForCoercedInput} from './ts_util'; import {TypeParameterEmitter} from './type_parameter_emitter'; export function generateTypeCtorDeclarationFn( @@ -150,9 +151,7 @@ function constructTypeCtorParameter( /* modifiers */ undefined, /* name */ key, /* questionToken */ undefined, - /* type */ - ts.createTypeQueryNode( - ts.createQualifiedName(rawType.typeName, `ngAcceptInputType_${key}`)), + /* type */ tsCreateTypeQueryForCoercedInput(rawType.typeName, key), /* initializer */ undefined)); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts index 081296f637..2c278a3d68 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -227,7 +227,8 @@ runInEachFileSystem(() => { name: 'GuardDir', selector: '[guard]', inputs: {'guard': 'guard'}, - ngTemplateGuards: [{inputName: 'guard', type: 'binding'}] + ngTemplateGuards: [{inputName: 'guard', type: 'binding'}], + undeclaredInputFields: ['guard'], }]); expect(messages).toEqual([ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 99043a6428..f1e285c25a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -120,8 +120,9 @@ export function ngForDeclaration(): TestDeclaration { file: absoluteFrom('/ngfor.d.ts'), selector: '[ngForOf]', name: 'NgForOf', - inputs: {ngForOf: 'ngForOf'}, + inputs: {ngForOf: 'ngForOf', ngForTrackBy: 'ngForTrackBy', ngForTemplate: 'ngForTemplate'}, hasNgTemplateContextGuard: true, + isGeneric: true, }; } @@ -175,11 +176,12 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = { // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead. export type TestDirective = Partial>>&{ - selector: string, - name: string, - file?: AbsoluteFsPath, type: 'directive', - coercedInputFields?: string[], + Exclude< + keyof TypeCheckableDirectiveMeta, + 'ref'|'coercedInputFields'|'restrictedInputFields'|'undeclaredInputFields'>>>&{ + selector: string, name: string, file?: AbsoluteFsPath, type: 'directive', + coercedInputFields?: string[], restrictedInputFields?: string[], + undeclaredInputFields?: string[], isGeneric?: boolean; }; export type TestPipe = { name: string, @@ -417,6 +419,9 @@ function prepareDeclarations( isComponent: decl.isComponent || false, ngTemplateGuards: decl.ngTemplateGuards || [], coercedInputFields: new Set(decl.coercedInputFields || []), + restrictedInputFields: new Set(decl.restrictedInputFields || []), + undeclaredInputFields: new Set(decl.undeclaredInputFields || []), + isGeneric: decl.isGeneric ?? false, outputs: decl.outputs || {}, queries: decl.queries || [], }; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index febe9c9e98..d4a6b14a55 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -55,7 +55,7 @@ describe('type check blocks', () => { selector: '[dir]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": ("value")'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('_t2: DirA = (null!); _t2["inputA"] = ("value");'); }); it('should handle multiple bindings to the same property', () => { @@ -67,8 +67,8 @@ describe('type check blocks', () => { inputs: {inputA: 'inputA'}, }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('"inputA": (1)'); - expect(block).not.toContain('"inputA": (2)'); + expect(block).toContain('_t2["inputA"] = (1);'); + expect(block).toContain('_t2["inputA"] = (2);'); }); it('should handle empty bindings', () => { @@ -79,7 +79,7 @@ describe('type check blocks', () => { selector: '[dir-a]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('_t2["inputA"] = (undefined);'); }); it('should handle bindings without value', () => { @@ -90,7 +90,7 @@ describe('type check blocks', () => { selector: '[dir-a]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('_t2["inputA"] = (undefined);'); }); it('should handle implicit vars on ng-template', () => { @@ -109,20 +109,148 @@ describe('type check blocks', () => { expect(tcb(TEMPLATE)).toContain('var _t2 = _t1.$implicit;'); }); - it('should handle missing property bindings', () => { - const TEMPLATE = `
`; - const DIRECTIVES: TestDeclaration[] = [{ - type: 'directive', - name: 'Dir', - selector: '[dir]', - inputs: { - fieldA: 'inputA', - fieldB: 'inputB', - }, - }]; - expect(tcb(TEMPLATE, DIRECTIVES)) - .toContain( - 'var _t2 = Dir.ngTypeCtor({ "fieldA": (((ctx).foo)), "fieldB": (null as any) });'); + describe('type constructors', () => { + it('should handle missing property bindings', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + fieldB: 'inputB', + }, + isGeneric: true, + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2 = Dir.ngTypeCtor({ "fieldA": (((ctx).foo)), "fieldB": (null as any) });'); + }); + + it('should handle multiple bindings to the same property', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + isGeneric: true, + }]; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('"fieldA": (1)'); + expect(block).not.toContain('"fieldA": (2)'); + }); + + + it('should only apply property bindings to directives', () => { + const TEMPLATE = ` +
+ `; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: {'color': 'color', 'strong': 'strong', 'enabled': 'enabled'}, + isGeneric: true, + }]; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain( + 'var _t2 = Dir.ngTypeCtor({ "color": (null as any), "strong": (null as any), "enabled": (null as any) });'); + expect(block).toContain('"blue"; false; true;'); + }); + + it('should generate a circular directive reference correctly', () => { + const TEMPLATE = ` +
+ `; + const DIRECTIVES: TestDirective[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + exportAs: ['dir'], + inputs: {input: 'input'}, + isGeneric: true, + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t3 = Dir.ngTypeCtor((null!)); ' + + 'var _t2 = Dir.ngTypeCtor({ "input": (_t3) });'); + }); + + it('should generate circular references between two directives correctly', () => { + const TEMPLATE = ` +
A
+
B
+`; + const DIRECTIVES: TestDirective[] = [ + { + type: 'directive', + name: 'DirA', + selector: '[dir-a]', + exportAs: ['dirA'], + inputs: {inputA: 'inputA'}, + isGeneric: true, + }, + { + type: 'directive', + name: 'DirB', + selector: '[dir-b]', + exportAs: ['dirB'], + inputs: {inputB: 'inputB'}, + isGeneric: true, + } + ]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t4 = DirA.ngTypeCtor((null!)); ' + + 'var _t3 = DirB.ngTypeCtor({ "inputB": (_t4) }); ' + + 'var _t2 = DirA.ngTypeCtor({ "inputA": (_t3) });'); + }); + + it('should handle empty bindings', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'DirA', + selector: '[dir-a]', + inputs: {inputA: 'inputA'}, + isGeneric: true, + }]; + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); + }); + + it('should handle bindings without value', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'DirA', + selector: '[dir-a]', + inputs: {inputA: 'inputA'}, + isGeneric: true, + }]; + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); + }); + + it('should use coercion types if declared', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + isGeneric: true, + coercedInputFields: ['fieldA'], + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2 = Dir.ngTypeCtor({ "fieldA": (((ctx).foo)) }); ' + + 'var _t3: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t3 = (((ctx).foo));'); + }); }); it('should generate a forward element reference correctly', () => { @@ -147,7 +275,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1 = Dir.ngTypeCtor({}); "" + ((_t1).value); var _t2 = document.createElement("div");'); + 'var _t1: Dir = (null!); "" + ((_t1).value); var _t2 = document.createElement("div");'); }); it('should handle style and class bindings specially', () => { @@ -173,8 +301,10 @@ describe('type check blocks', () => { inputs: {'color': 'color', 'strong': 'strong', 'enabled': 'enabled'}, }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain( - 'var _t2 = Dir.ngTypeCtor({ "color": (null as any), "strong": (null as any), "enabled": (null as any) });'); + expect(block).toContain('var _t2: Dir = (null!);'); + expect(block).not.toContain('"color"'); + expect(block).not.toContain('"strong"'); + expect(block).not.toContain('"enabled"'); expect(block).toContain('"blue"; false; true;'); }); @@ -191,8 +321,8 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t3 = Dir.ngTypeCtor((null!)); ' + - 'var _t2 = Dir.ngTypeCtor({ "input": (_t3) });'); + 'var _t2: Dir = (null!); ' + + '_t2["input"] = (_t2);'); }); it('should generate circular references between two directives correctly', () => { @@ -218,9 +348,139 @@ describe('type check blocks', () => { ]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t4 = DirA.ngTypeCtor((null!)); ' + - 'var _t3 = DirB.ngTypeCtor({ "inputA": (_t4) }); ' + - 'var _t2 = DirA.ngTypeCtor({ "inputA": (_t3) });'); + 'var _t2: DirA = (null!); ' + + 'var _t3: DirB = (null!); ' + + '_t2["inputA"] = (_t3); ' + + 'var _t4 = document.createElement("div"); ' + + '_t3["inputA"] = (_t2);'); + }); + + it('should handle undeclared properties', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + undeclaredInputFields: ['fieldA'] + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + '(((ctx).foo)); '); + }); + + it('should handle restricted properties', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + restrictedInputFields: ['fieldA'] + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + 'var _t3: typeof _t2["fieldA"] = (null!); ' + + '_t3 = (((ctx).foo)); '); + }); + + it('should handle a single property bound to multiple fields', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + field1: 'inputA', + field2: 'inputA', + }, + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + '_t2["field2"] = _t2["field1"] = (((ctx).foo));'); + }); + + it('should handle a single property bound to multiple fields, where one of them is coerced', + () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + field1: 'inputA', + field2: 'inputA', + }, + coercedInputFields: ['field1'], + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + 'var _t3: typeof Dir.ngAcceptInputType_field1 = (null!); ' + + '_t2["field2"] = _t3 = (((ctx).foo));'); + }); + + it('should handle a single property bound to multiple fields, where one of them is undeclared', + () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + field1: 'inputA', + field2: 'inputA', + }, + undeclaredInputFields: ['field1'], + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + '_t2["field2"] = (((ctx).foo));'); + }); + + it('should use coercion types if declared', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + coercedInputFields: ['fieldA'], + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + 'var _t3: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t3 = (((ctx).foo));'); + }); + + it('should use coercion types if declared, even when backing field is not declared', () => { + const TEMPLATE = `
`; + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + fieldA: 'inputA', + }, + coercedInputFields: ['fieldA'], + undeclaredInputFields: ['fieldA'], + }]; + expect(tcb(TEMPLATE, DIRECTIVES)) + .toContain( + 'var _t2: Dir = (null!); ' + + 'var _t3: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t3 = (((ctx).foo));'); }); it('should handle $any casts', () => { @@ -379,14 +639,14 @@ describe('type check blocks', () => { it('should include null and undefined when enabled', () => { const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('Dir.ngTypeCtor({ "dirInput": (((ctx).a)) })'); + expect(block).toContain('_t2["dirInput"] = (((ctx).a));'); expect(block).toContain('((ctx).b);'); }); it('should use the non-null assertion operator when disabled', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, strictNullInputBindings: false}; const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); - expect(block).toContain('Dir.ngTypeCtor({ "dirInput": (((ctx).a)!) })'); + expect(block).toContain('_t2["dirInput"] = (((ctx).a)!);'); expect(block).toContain('((ctx).b)!;'); }); }); @@ -395,7 +655,7 @@ describe('type check blocks', () => { it('should check types of bindings when enabled', () => { const TEMPLATE = `
`; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('Dir.ngTypeCtor({ "dirInput": (((ctx).a)) })'); + expect(block).toContain('_t2["dirInput"] = (((ctx).a));'); expect(block).toContain('((ctx).b);'); }); @@ -404,7 +664,7 @@ describe('type check blocks', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfInputBindings: false}; const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); - expect(block).toContain('Dir.ngTypeCtor({ "dirInput": ((((ctx).a) as any)) })'); + expect(block).toContain('_t2["dirInput"] = ((((ctx).a) as any));'); expect(block).toContain('(((ctx).b) as any);'); }); @@ -413,8 +673,7 @@ describe('type check blocks', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfInputBindings: false}; const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); - expect(block).toContain( - 'Dir.ngTypeCtor({ "dirInput": ((((((ctx).a)) === (((ctx).b))) as any)) })'); + expect(block).toContain('_t2["dirInput"] = ((((((ctx).a)) === (((ctx).b))) as any));'); }); }); @@ -534,17 +793,17 @@ describe('type check blocks', () => { it('should assign string value to the input when enabled', () => { const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('"disabled": ("")'); - expect(block).toContain('"cols": ("3")'); - expect(block).toContain('"rows": (2)'); + expect(block).toContain('_t2["disabled"] = ("");'); + expect(block).toContain('_t2["cols"] = ("3");'); + expect(block).toContain('_t2["rows"] = (2);'); }); it('should use any for attributes but still check bound attributes when disabled', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfAttributes: false}; const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); - expect(block).toContain('"disabled": (null as any)'); - expect(block).toContain('"cols": (null as any)'); - expect(block).toContain('"rows": (2)'); + expect(block).not.toContain('"disabled"'); + expect(block).not.toContain('"cols"'); + expect(block).toContain('_t2["rows"] = (2);'); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts index 432a1b8dd0..0a075efbf3 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts @@ -86,6 +86,7 @@ runInEachFileSystem(os => { selector: '[dir]', file: dirFile, type: 'directive', + isGeneric: true, }; const {program, templateTypeChecker, programStrategy} = setup([ { @@ -104,7 +105,7 @@ runInEachFileSystem(os => { // A non-exported interface used as a type bound for a generic directive causes // an inline type constructor to be required. interface NotExported {} - export class TestDir {}`, + export abstract class TestDir {}`, templates: {}, }, ]); @@ -161,7 +162,7 @@ runInEachFileSystem(os => { const {program, templateTypeChecker} = setup( [{ fileName, - source: `class Cmp {} // not exported, so requires inline`, + source: `abstract class Cmp {} // not exported, so requires inline`, templates: {'Cmp': '
'} }], {inlining: false}); @@ -188,6 +189,7 @@ runInEachFileSystem(os => { selector: '[dir]', file: dirFile, type: 'directive', + isGeneric: true, }] }, { @@ -196,7 +198,7 @@ runInEachFileSystem(os => { // A non-exported interface used as a type bound for a generic directive causes // an inline type constructor to be required. interface NotExported {} - export class TestDir {}`, + export abstract class TestDir {}`, templates: {}, } ], diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 7c9ec4dbf4..5646850c22 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -136,11 +136,42 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toEqual(`Type 'string' is not assignable to type 'number'.`); + expect(diags[0].messageText).toEqual(`Type '"2"' is not assignable to type 'number'.`); // The reported error code should be in the TS error space, not a -99 "NG" code. expect(diags[0].code).toBeGreaterThan(0); }); + it('should produce diagnostics when mapping to multiple fields and bound types are incorrect', + () => { + env.tsconfig( + {fullTemplateTypeCheck: true, strictInputTypes: true, strictAttributeTypes: true}); + env.write('test.ts', ` + import {Component, Directive, NgModule, Input} from '@angular/core'; + + @Component({ + selector: 'test', + template: '
', + }) + class TestCmp {} + + @Directive({selector: '[dir]'}) + class TestDir { + @Input('foo') foo1: number; + @Input('foo') foo2: number; + } + + @NgModule({ + declarations: [TestCmp, TestDir], + }) + class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toEqual(`Type '"2"' is not assignable to type 'number'.`); + expect(diags[1].messageText).toEqual(`Type '"2"' is not assignable to type 'number'.`); + }); + it('should support inputs and outputs with names that are not JavaScript identifiers', () => { env.tsconfig( {fullTemplateTypeCheck: true, strictInputTypes: true, strictOutputEventTypes: true}); @@ -173,7 +204,7 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText).toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[0].messageText).toEqual(`Type '2' is not assignable to type 'string'.`); expect(diags[1].messageText) .toEqual(`Argument of type 'string' is not assignable to parameter of type 'number'.`); }); @@ -349,7 +380,7 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText).toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[0].messageText).toEqual(`Type '1' is not assignable to type 'string'.`); expect(diags[1].messageText) .toEqual(`Property 'invalid' does not exist on type 'TestCmp'.`); }); @@ -359,7 +390,7 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText).toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[0].messageText).toEqual(`Type '1' is not assignable to type 'string'.`); expect(diags[1].messageText) .toEqual(`Property 'invalid' does not exist on type 'TestCmp'.`); }); @@ -685,8 +716,8 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText).toEqual(`Type 'string' is not assignable to type 'boolean'.`); - expect(diags[1].messageText).toEqual(`Type 'string' is not assignable to type 'number'.`); + expect(diags[0].messageText).toEqual(`Type '""' is not assignable to type 'boolean'.`); + expect(diags[1].messageText).toEqual(`Type '"3"' is not assignable to type 'number'.`); }); it('should produce an error for text attributes when overall strictness is enabled', () => { @@ -694,8 +725,8 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText).toEqual(`Type 'string' is not assignable to type 'boolean'.`); - expect(diags[1].messageText).toEqual(`Type 'string' is not assignable to type 'number'.`); + expect(diags[0].messageText).toEqual(`Type '""' is not assignable to type 'boolean'.`); + expect(diags[1].messageText).toEqual(`Type '"3"' is not assignable to type 'number'.`); }); it('should not produce an error for text attributes when not enabled', () => { @@ -1212,9 +1243,9 @@ export declare class AnimationEvent { expect(diags.length).toBe(3); expect(diags[0].messageText).toBe(`Type 'true' is not assignable to type 'number'.`); expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); - expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); + expect(diags[1].messageText).toBe(`Type '3' is not assignable to type 'string'.`); expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); - expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); + expect(diags[2].messageText).toBe(`Type '4' is not assignable to type 'boolean'.`); expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); }); @@ -1269,9 +1300,9 @@ export declare class AnimationEvent { expect(diags.length).toBe(3); expect(diags[0].messageText).toBe(`Type 'true' is not assignable to type 'number'.`); expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); - expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); + expect(diags[1].messageText).toBe(`Type '3' is not assignable to type 'string'.`); expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); - expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); + expect(diags[2].messageText).toBe(`Type '4' is not assignable to type 'boolean'.`); expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); }); @@ -1476,7 +1507,7 @@ export declare class AnimationEvent { it('should give an error if the binding expression type is not accepted by the coercion function', () => { env.write('test.ts', ` - import {Component, NgModule} from '@angular/core'; + import {Component, NgModule, Input, Directive} from '@angular/core'; import {MatInputModule} from '@angular/material'; @Component({ @@ -1533,6 +1564,199 @@ export declare class AnimationEvent { }); }); + describe('restricted inputs', () => { + const directiveDeclaration = ` + @Directive({selector: '[dir]'}) + export class TestDir { + @Input() + protected protectedField!: string; + @Input() + private privateField!: string; + @Input() + readonly readonlyField!: string; + } + `; + + describe('with strict inputs', () => { + beforeEach(() => { + env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true}); + }); + + it('should not produce diagnostics for correct inputs which assign to readonly, private, or protected fields', + () => { + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + value = "value"; + } + + ${directiveDeclaration} + + @NgModule({ + declarations: [FooCmp, TestDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not produce diagnostics for correct inputs which assign to readonly, private, or protected fields inherited from a base class', + () => { + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + value = "value"; + } + + ${directiveDeclaration} + + @Directive({selector: '[child-dir]'}) + export class ChildDir extends TestDir { + } + + @NgModule({ + declarations: [FooCmp, ChildDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should produce diagnostics when assigning incorrect type to readonly, private, or protected fields', + () => { + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + value = 1; + } + + ${directiveDeclaration} + + @NgModule({ + declarations: [FooCmp, TestDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + expect(diags[0].messageText) + .toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[1].messageText) + .toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[2].messageText) + .toEqual(`Type 'number' is not assignable to type 'string'.`); + }); + }); + }); + + it('should not produce diagnostics for undeclared inputs', () => { + env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true}); + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + value = "value"; + } + + @Directive({ + selector: '[dir]', + inputs: ['undeclared'], + }) + export class TestDir { + } + + @NgModule({ + declarations: [FooCmp, TestDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should produce diagnostics for invalid expressions when assigned into an undeclared input', + () => { + env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true}); + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + } + + @Directive({ + selector: '[dir]', + inputs: ['undeclared'], + }) + export class TestDir { + } + + @NgModule({ + declarations: [FooCmp, TestDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe(`Property 'value' does not exist on type 'FooCmp'.`); + }); + + it('should not produce diagnostics for undeclared inputs inherited from a base class', () => { + env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true}); + env.write('test.ts', ` + import {Component, NgModule, Input, Directive} from '@angular/core'; + + @Component({ + selector: 'blah', + template: '
', + }) + export class FooCmp { + value = "value"; + } + + @Directive({ + inputs: ['undeclaredBase'], + }) + export class BaseDir { + } + + @Directive({selector: '[dir]'}) + export class TestDir extends BaseDir { + } + + @NgModule({ + declarations: [FooCmp, TestDir], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + describe('legacy schema checking with the DOM schema', () => { beforeEach(() => { env.tsconfig({ivyTemplateTypeCheck: true, fullTemplateTypeCheck: false});