From 70311ebca196c72aee4e25405393c267d1d1dd3f Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 11 Nov 2019 21:03:23 +0100 Subject: [PATCH] fix(ivy): handle non-standard input/output names in template type checking (#33741) The template type checker generates code to check directive inputs and outputs, whose name may contain characters that can not be used as identifier in TypeScript. Prior to this change, such names would be emitted into the generated code as is, resulting in invalid code and unexpected template type check errors. This commit fixes the bug by representing the potentially invalid names as string literal instead of raw identifier. Fixes #33590 PR Close #33741 --- .../ngtsc/typecheck/src/type_check_block.ts | 10 ++-- .../typecheck/test/type_check_block_spec.ts | 46 +++++++++---------- .../test/ngtsc/template_typecheck_spec.ts | 37 +++++++++++++++ 3 files changed, 66 insertions(+), 27 deletions(-) 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 1776122371..86be2c6bee 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 @@ -415,7 +415,7 @@ class TcbUnclaimedInputsOp extends TcbOp { if (binding.name !== 'style' && binding.name !== 'class') { // A direct binding to a property. const propertyName = ATTR_TO_PROP[binding.name] || binding.name; - const prop = ts.createPropertyAccess(elId, propertyName); + const prop = ts.createElementAccess(elId, ts.createStringLiteral(propertyName)); const stmt = ts.createBinary(prop, ts.SyntaxKind.EqualsToken, wrapForDiagnostics(expr)); addParseSpanInfo(stmt, binding.sourceSpan); this.scope.addStatement(ts.createExpressionStatement(stmt)); @@ -478,7 +478,7 @@ class TcbDirectiveOutputsOp extends TcbOp { // that has a `subscribe` method that properly carries the `T` into the handler function. const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Infer); - const outputField = ts.createPropertyAccess(dirId, field); + const outputField = ts.createElementAccess(dirId, ts.createStringLiteral(field)); const outputHelper = ts.createCall(this.tcb.env.declareOutputHelper(), undefined, [outputField]); const subscribeFn = ts.createPropertyAccess(outputHelper, 'subscribe'); @@ -1118,6 +1118,8 @@ function tcbCallTypeCtor( // Construct an array of `ts.PropertyAssignment`s for each of the directive's inputs. const members = inputs.map(input => { + const propertyName = ts.createStringLiteral(input.field); + if (input.type === 'binding') { // For bound inputs, the property is assigned the binding expression. let expr = input.expression; @@ -1131,13 +1133,13 @@ function tcbCallTypeCtor( expr = ts.createNonNullExpression(expr); } - const assignment = ts.createPropertyAssignment(input.field, wrapForDiagnostics(expr)); + const assignment = ts.createPropertyAssignment(propertyName, wrapForDiagnostics(expr)); 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); + return ts.createPropertyAssignment(propertyName, NULL_AS_ANY); } }); 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 f1a22fcffd..52f87c0fb2 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 @@ -43,7 +43,7 @@ describe('type check blocks', () => { selector: '[dir]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('inputA: ("value")'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": ("value")'); }); it('should handle empty bindings', () => { @@ -54,7 +54,7 @@ describe('type check blocks', () => { selector: '[dir-a]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('inputA: (undefined)'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); }); it('should handle bindings without value', () => { @@ -65,7 +65,7 @@ describe('type check blocks', () => { selector: '[dir-a]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('inputA: (undefined)'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('"inputA": (undefined)'); }); it('should handle implicit vars on ng-template', () => { @@ -96,7 +96,7 @@ describe('type check blocks', () => { }, }]; expect(tcb(TEMPLATE, DIRECTIVES)) - .toContain('var _t2 = Dir.ngTypeCtor({ fieldA: ((ctx).foo), fieldB: (null as any) });'); + .toContain('var _t2 = Dir.ngTypeCtor({ "fieldA": ((ctx).foo), "fieldB": (null as any) });'); }); it('should generate a forward element reference correctly', () => { @@ -147,7 +147,7 @@ describe('type check blocks', () => { }]; const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - 'var _t2 = Dir.ngTypeCtor({ color: (null as any), strong: (null as any), enabled: (null as any) });'); + 'var _t2 = Dir.ngTypeCtor({ "color": (null as any), "strong": (null as any), "enabled": (null as any) });'); expect(block).toContain('"blue"; false; true;'); }); @@ -162,7 +162,7 @@ describe('type check blocks', () => { exportAs: ['dir'], inputs: {input: 'input'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('var _t2 = Dir.ngTypeCtor({ input: (null!) });'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('var _t2 = Dir.ngTypeCtor({ "input": (null!) });'); }); it('should generate circular references between two directives correctly', () => { @@ -188,8 +188,8 @@ describe('type check blocks', () => { ]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t3 = DirB.ngTypeCtor({ inputA: (null!) }); ' + - 'var _t2 = DirA.ngTypeCtor({ inputA: (_t3) });'); + 'var _t3 = DirB.ngTypeCtor({ "inputA": (null!) }); ' + + 'var _t2 = DirA.ngTypeCtor({ "inputA": (_t3) });'); }); it('should handle $any casts', () => { @@ -203,7 +203,7 @@ describe('type check blocks', () => { const TEMPLATE = ``; const CONFIG = {...ALL_ENABLED_CONFIG, checkTypeOfDomBindings: true}; expect(tcb(TEMPLATE, /* declarations */ undefined, CONFIG)) - .toContain('_t1.htmlFor = ("test");'); + .toContain('_t1["htmlFor"] = ("test");'); }); }); @@ -253,7 +253,7 @@ describe('type check blocks', () => { const TEMPLATE = `
`; const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - '_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));'); + '_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));'); }); it('should emit a listener function with AnimationEvent for animation events', () => { @@ -339,14 +339,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('Dir.ngTypeCtor({ "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('Dir.ngTypeCtor({ "dirInput": ((ctx).a!) })'); expect(block).toContain('(ctx).b!;'); }); }); @@ -356,14 +356,14 @@ describe('type check blocks', () => { it('should check types of bindings when enabled', () => { const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('Dir.ngTypeCtor({ dirInput: ((ctx).a) })'); + expect(block).toContain('Dir.ngTypeCtor({ "dirInput": ((ctx).a) })'); expect(block).toContain('(ctx).b;'); }); it('should not check types of bindings when disabled', () => { 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('Dir.ngTypeCtor({ "dirInput": (((ctx).a as any)) })'); expect(block).toContain('((ctx).b as any);'); }); }); @@ -374,7 +374,7 @@ describe('type check blocks', () => { it('should check types of directive outputs when enabled', () => { const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - '_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));'); + '_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));'); expect(block).toContain( '_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));'); }); @@ -410,7 +410,7 @@ describe('type check blocks', () => { it('should check types of DOM events when enabled', () => { const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - '_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));'); + '_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));'); expect(block).toContain( '_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));'); }); @@ -420,7 +420,7 @@ describe('type check blocks', () => { // Note that directive outputs are still checked, that is controlled by // `checkTypeOfOutputEvents` expect(block).toContain( - '_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));'); + '_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));'); expect(block).toContain('($event: any): any => (ctx).foo($event);'); }); }); @@ -483,17 +483,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('"disabled": ("")'); + expect(block).toContain('"cols": ("3")'); + expect(block).toContain('"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).toContain('"disabled": (null as any)'); + expect(block).toContain('"cols": (null as any)'); + expect(block).toContain('"rows": (2)'); }); }); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 1f41873e0b..d102c204b4 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -123,6 +123,43 @@ export declare class AnimationEvent { expect(diags[0].messageText).toEqual(`Type 'string' 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}); + env.write('test.ts', ` + import {Component, Directive, NgModule, EventEmitter} from '@angular/core'; + + @Component({ + selector: 'test', + template: '
', + }) + class TestCmp { + handleEvent(event: number): void {} + } + + @Directive({ + selector: '[dir]', + inputs: ['some-input.xs'], + outputs: ['some-output'], + }) + class TestDir { + 'some-input.xs': string; + 'some-output': EventEmitter; + } + + @NgModule({ + declarations: [TestCmp, TestDir], + }) + class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toEqual(`Type 'number' is not assignable to type 'string'.`); + expect(diags[1].messageText) + .toEqual(`Argument of type 'string' is not assignable to parameter of type 'number'.`); + }); + it('should check event bindings', () => { env.tsconfig({fullTemplateTypeCheck: true, strictOutputEventTypes: true}); env.write('test.ts', `