From 50bf17aca03727fae6389d90df4ff8f6b68d2f82 Mon Sep 17 00:00:00 2001 From: JoostK Date: Wed, 9 Oct 2019 22:31:04 +0200 Subject: [PATCH] fix(ivy): do not always accept `undefined` for directive inputs (#33066) Prior to this change, the template type checker would always allow a value of type `undefined` to be passed into a directive's inputs, even if the input's type did not allow for it. This was due to how the type constructor for a directive was generated, where a `Partial` mapped type was used to allow for inputs to be unset. This essentially introduces the `undefined` type as acceptable type for all inputs. This commit removes the `Partial` type from the type constructor, which means that we can no longer omit any properties that were unset. Instead, any properties that are not set will still be included in the type constructor call, having their value assigned to `any`. Before: ```typescript class NgForOf { static ngTypeCtor(init: Partial, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf; } NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']}); ``` After: ```typescript class NgForOf { static ngTypeCtor(init: Pick, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf; } NgForOf.ngTypeCtor(init: { ngForOf: ['foo', 'bar'], ngForTrackBy: null as any, ngForTemplate: null as any, }); ``` This change only affects generated type check code, the generated runtime code is not affected. Fixes #32690 Resolves FW-1606 PR Close #33066 --- .../src/ngtsc/typecheck/src/context.ts | 6 +- .../src/ngtsc/typecheck/src/expression.ts | 2 +- .../ngtsc/typecheck/src/type_check_block.ts | 72 ++++++++++++------- .../ngtsc/typecheck/src/type_constructor.ts | 52 +++++++------- .../ngtsc/typecheck/test/diagnostics_spec.ts | 2 +- .../typecheck/test/type_check_block_spec.ts | 15 ++++ .../test/ngtsc/template_typecheck_spec.ts | 6 +- 7 files changed, 93 insertions(+), 62 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 3aa4e3dd4b..318d8006f9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -123,7 +123,7 @@ export class TypeCheckContext { const ops = this.opMap.get(sf) !; // Push a `TypeCtorOp` into the operation queue for the source file. - ops.push(new TypeCtorOp(ref, ctorMeta, this.config)); + ops.push(new TypeCtorOp(ref, ctorMeta)); } /** @@ -287,7 +287,7 @@ class TcbOp implements Op { class TypeCtorOp implements Op { constructor( readonly ref: Reference>, - readonly meta: TypeCtorMetadata, private config: TypeCheckingConfig) {} + readonly meta: TypeCtorMetadata) {} /** * Type constructor operations are inserted immediately before the end of the directive class. @@ -296,7 +296,7 @@ class TypeCtorOp implements Op { execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer): string { - const tcb = generateInlineTypeCtor(this.ref.node, this.meta, this.config); + const tcb = generateInlineTypeCtor(this.ref.node, this.meta); return printer.printNode(ts.EmitHint.Unspecified, tcb, sf); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index 7839a77dda..1f98c37cd2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {TypeCheckingConfig} from './api'; import {AbsoluteSpan, addParseSpanInfo, wrapForDiagnostics} from './diagnostics'; -const NULL_AS_ANY = +export const NULL_AS_ANY = ts.createAsExpression(ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); const UNDEFINED = ts.createIdentifier('undefined'); 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 6e8c71909b..58def5ef89 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 @@ -16,7 +16,7 @@ import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; import {addParseSpanInfo, addSourceId, toAbsoluteSpan, wrapForDiagnostics} from './diagnostics'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; -import {astToTypescript} from './expression'; +import {NULL_AS_ANY, astToTypescript} from './expression'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; @@ -290,11 +290,11 @@ class TcbDirectiveOp extends TcbOp { execute(): ts.Identifier { const id = this.tcb.allocateId(); // Process the directive and construct expressions for each of its bindings. - const bindings = tcbGetInputBindingExpressions(this.node, this.dir, this.tcb, this.scope); + const inputs = tcbGetDirectiveInputs(this.node, this.dir, this.tcb, this.scope); // Call the type constructor of the directive to infer a type, and assign the directive // instance. - const typeCtor = tcbCallTypeCtor(this.dir, this.tcb, bindings); + const typeCtor = tcbCallTypeCtor(this.dir, this.tcb, inputs); addParseSpanInfo(typeCtor, this.node.sourceSpan); this.scope.addStatement(tsCreateVariable(id, typeCtor)); return id; @@ -774,18 +774,25 @@ function tcbExpression( * the directive instance from any bound inputs. */ function tcbCallTypeCtor( - dir: TypeCheckableDirectiveMeta, tcb: Context, bindings: TcbBinding[]): ts.Expression { + dir: TypeCheckableDirectiveMeta, tcb: Context, inputs: TcbDirectiveInput[]): ts.Expression { const typeCtor = tcb.env.typeCtorFor(dir); - // Construct an array of `ts.PropertyAssignment`s for each input of the directive that has a - // matching binding. - const members = bindings.map(({field, expression, sourceSpan}) => { - if (!tcb.env.config.checkTypeOfInputBindings) { - expression = tsCastToAny(expression); + // Construct an array of `ts.PropertyAssignment`s for each of the directive's inputs. + const members = inputs.map(input => { + if (input.type === 'binding') { + // For bound inputs, the property is assigned the binding expression. + let expression = input.expression; + if (!tcb.env.config.checkTypeOfInputBindings) { + expression = tsCastToAny(expression); + } + const assignment = ts.createPropertyAssignment(input.field, wrapForDiagnostics(expression)); + addParseSpanInfo(assignment, input.sourceSpan); + return assignment; + } else { + // A type constructor is required to be called with all input properties, so any unset + // inputs are simply assigned a value of type `any` to ignore them. + return ts.createPropertyAssignment(input.field, NULL_AS_ANY); } - const assignment = ts.createPropertyAssignment(field, wrapForDiagnostics(expression)); - addParseSpanInfo(assignment, sourceSpan); - return assignment; }); // Call the `ngTypeCtor` method on the directive class, with an object literal argument created @@ -796,17 +803,18 @@ function tcbCallTypeCtor( /* argumentsArray */[ts.createObjectLiteral(members)]); } -interface TcbBinding { +type TcbDirectiveInput = { + type: 'binding'; field: string; expression: ts.Expression; sourceSpan: ParseSourceSpan; +} | +{ + type: 'unset'; field: string; - property: string; - expression: ts.Expression; - sourceSpan: ParseSourceSpan; -} +}; -function tcbGetInputBindingExpressions( +function tcbGetDirectiveInputs( el: TmplAstElement | TmplAstTemplate, dir: TypeCheckableDirectiveMeta, tcb: Context, - scope: Scope): TcbBinding[] { - const bindings: TcbBinding[] = []; + scope: Scope): TcbDirectiveInput[] { + const directiveInputs: TcbDirectiveInput[] = []; // `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. @@ -817,11 +825,21 @@ function tcbGetInputBindingExpressions( propMatch.set(inputs[key] as string, key); }); + // To determine which of directive's inputs are unset, we keep track of the set of field names + // that have not been seen yet. A field is removed from this set once a binding to it is found. + const unsetFields = new Set(propMatch.values()); + el.inputs.forEach(processAttribute); if (el instanceof TmplAstTemplate) { el.templateAttrs.forEach(processAttribute); } - return bindings; + + // Add unset directive inputs for each of the remaining unset fields. + for (const field of unsetFields) { + directiveInputs.push({type: 'unset', field}); + } + + return directiveInputs; /** * Add a binding expression to the map for each input/template attribute of the directive that has @@ -829,12 +847,16 @@ function tcbGetInputBindingExpressions( */ function processAttribute(attr: TmplAstBoundAttribute | TmplAstTextAttribute): void { if (attr instanceof TmplAstBoundAttribute && propMatch.has(attr.name)) { + const field = propMatch.get(attr.name) !; + + // Remove the field from the set of unseen fields, now that it's been assigned to. + unsetFields.delete(field); + // Produce an expression representing the value of the binding. const expr = tcbExpression(attr.value, tcb, scope, attr.valueSpan || attr.sourceSpan); - // Call the callback. - bindings.push({ - property: attr.name, - field: propMatch.get(attr.name) !, + directiveInputs.push({ + type: 'binding', + field: field, expression: expr, sourceSpan: attr.sourceSpan, }); 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 91f541ce90..5607044447 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts @@ -24,7 +24,7 @@ export function generateTypeCtorDeclarationFn( node.typeParameters !== undefined ? generateGenericArgs(node.typeParameters) : undefined; const rawType: ts.TypeNode = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs); - const initParam = constructTypeCtorParameter(node, meta, rawType, config.checkQueries); + const initParam = constructTypeCtorParameter(node, meta, rawType); const typeParameters = typeParametersWithDefaultTypes(node.typeParameters); @@ -67,12 +67,19 @@ export function generateTypeCtorDeclarationFn( * * An inline type constructor for NgFor looks like: * - * static ngTypeCtor(init: Partial, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): + * static ngTypeCtor(init: Pick, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): * NgForOf; * * A typical constructor would be: * - * NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']}); // Infers a type of NgForOf. + * NgForOf.ngTypeCtor(init: { + * ngForOf: ['foo', 'bar'], + * ngForTrackBy: null as any, + * ngForTemplate: null as any, + * }); // Infers a type of NgForOf. + * + * Any inputs declared on the type for which no property binding is present are assigned a value of + * type `any`, to avoid producing any type errors for unset inputs. * * Inline type constructors are used when the type being created has bounded generic types which * make writing a declared type constructor (via `generateTypeCtorDeclarationFn`) difficult or @@ -84,8 +91,7 @@ export function generateTypeCtorDeclarationFn( * @returns a `ts.MethodDeclaration` for the type constructor. */ export function generateInlineTypeCtor( - node: ClassDeclaration, meta: TypeCtorMetadata, - config: TypeCheckingConfig): ts.MethodDeclaration { + node: ClassDeclaration, meta: TypeCtorMetadata): ts.MethodDeclaration { // Build rawType, a `ts.TypeNode` of the class with its generic parameters passed through from // the definition without any type bounds. For example, if the class is // `FooDirective`, its rawType would be `FooDirective`. @@ -93,7 +99,7 @@ export function generateInlineTypeCtor( node.typeParameters !== undefined ? generateGenericArgs(node.typeParameters) : undefined; const rawType: ts.TypeNode = ts.createTypeReferenceNode(node.name, rawTypeArgs); - const initParam = constructTypeCtorParameter(node, meta, rawType, config.checkQueries); + const initParam = constructTypeCtorParameter(node, meta, rawType); // If this constructor is being generated into a .ts file, then it needs a fake body. The body // is set to a return of `null!`. If the type constructor is being generated into a .d.ts file, @@ -119,27 +125,20 @@ export function generateInlineTypeCtor( } function constructTypeCtorParameter( - node: ClassDeclaration, meta: TypeCtorMetadata, rawType: ts.TypeNode, - includeQueries: boolean): ts.ParameterDeclaration { + node: ClassDeclaration, meta: TypeCtorMetadata, + rawType: ts.TypeNode): ts.ParameterDeclaration { // initType is the type of 'init', the single argument to the type constructor method. - // If the Directive has any inputs, outputs, or queries, its initType will be: + // If the Directive has any inputs, its initType will be: // - // Partial> + // Pick // // Pick here is used to select only those fields from which the generic type parameters of the - // directive will be inferred. Partial is used because inputs are optional, so there may not be - // bindings for each field. + // directive will be inferred. // - // In the special case there are no inputs/outputs/etc, initType is set to {}. + // In the special case there are no inputs, initType is set to {}. let initType: ts.TypeNode; - const keys: string[] = [ - ...meta.fields.inputs, - ...meta.fields.outputs, - ]; - if (includeQueries) { - keys.push(...meta.fields.queries); - } + const keys: string[] = meta.fields.inputs; if (keys.length === 0) { // Special case - no inputs, outputs, or other fields which could influence the result type. initType = ts.createTypeLiteralNode([]); @@ -149,10 +148,7 @@ function constructTypeCtorParameter( keys.map(key => ts.createLiteralTypeNode(ts.createStringLiteral(key)))); // Construct the Pick. - const pickType = ts.createTypeReferenceNode('Pick', [rawType, keyTypeUnion]); - - // Construct the Partial. - initType = ts.createTypeReferenceNode('Partial', [pickType]); + initType = ts.createTypeReferenceNode('Pick', [rawType, keyTypeUnion]); } // Create the 'init' parameter itself. @@ -187,20 +183,20 @@ export function requiresInlineTypeCtor(node: ClassDeclaration(o: Partial, 'ngForOf'>>): NgFor; + * declare function ctor(o: Pick, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgFor; * ``` * * An invocation looks like: * * ``` - * var _t1 = ctor({ngForOf: [1, 2]}); + * var _t1 = ctor({ngForOf: [1, 2], ngForTrackBy: null as any, ngForTemplate: null as any}); * ``` * * This correctly infers the type `NgFor` for `_t1`, since `T` is inferred from the * assignment of type `number[]` to `ngForOf`'s type `T[]`. However, if `any` is passed instead: * * ``` - * var _t2 = ctor({ngForOf: [1, 2] as any}); + * var _t2 = ctor({ngForOf: [1, 2] as any, ngForTrackBy: null as any, ngForTemplate: null as any}); * ``` * * then inference for `T` fails (it cannot be inferred from `T[] = any`). In this case, `T` takes @@ -210,7 +206,7 @@ export function requiresInlineTypeCtor(node: ClassDeclaration(o: Partial, 'ngForOf'>>): NgFor; + * declare function ctor(o: Pick, 'ngForOf'>): NgFor; * * var _t3 = ctor({ngForOf: [1, 2] as any}); * ``` 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 255b6623f3..cc3ae3584e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -33,7 +33,7 @@ runInEachFileSystem(() => { }]); expect(messages).toEqual( - [`synthetic.html(1, 10): Type 'string' is not assignable to type 'number | undefined'.`]); + [`synthetic.html(1, 10): Type 'string' is not assignable to type 'number'.`]); }); it('infers type of template variables', () => { 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 64dcd2b6cc..ee0e013a71 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 @@ -67,6 +67,21 @@ 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) });'); + }); + it('should generate a forward element reference correctly', () => { const TEMPLATE = ` {{ i.value }} diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index d045a1b86f..410655dc49 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -336,12 +336,10 @@ export declare class CommonModule { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - expect(diags[0].messageText) - .toBe(`Type 'number' is not assignable to type 'string | undefined'.`); + expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); expect(diags[0].start).toEqual(386); expect(diags[0].length).toEqual(14); - expect(diags[1].messageText) - .toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`); + expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); expect(diags[1].start).toEqual(401); expect(diags[1].length).toEqual(15); });