Skip to content

Commit c5c20e9

Browse files
crisbetoalxhub
crisbeto
authored and
alxhub
committed
fix(compiler-cli): check event side of two-way bindings (#59002)
In the past two-way bindings used to be interpreted as `foo = $event` at the parser level. In #54065 it was changed to preserve the actual expression, because it was problematic for supporting two-way binding to signals. This unintentionally ended up causing the TCB to two-way bindings to look something like `someOutput.subscribe($event => expr);` which does nothing. It largely hasn't been a problem, because the input side of two-way bindings was still being checked, except for the case where the input side of the two-way binding has a wider type than the output side. These changes re-add type checking for the output side by generating the following TCB instead: ``` someOutput.subscribe($event => { var _t1 = unwrapSignalValue(this.someField); _t1 = $event; }); ``` PR Close #59002
1 parent 68c5e02 commit c5c20e9

File tree

10 files changed

+159
-44
lines changed

10 files changed

+159
-44
lines changed

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3092,6 +3092,30 @@ function tcbCreateEventHandler(
30923092
eventType: EventParamType | ts.TypeNode,
30933093
): ts.Expression {
30943094
const handler = tcbEventHandlerExpression(event.handler, tcb, scope);
3095+
const statements: ts.Statement[] = [];
3096+
3097+
if (event.type === ParsedEventType.TwoWay) {
3098+
// If we're dealing with a two-way event, we create a variable initialized to the unwrapped
3099+
// signal value of the expression and then we assign `$event` to it. Note that in most cases
3100+
// this will already be covered by the corresponding input binding, however it allows us to
3101+
// handle the case where the input has a wider type than the output (see #58971).
3102+
const target = tcb.allocateId();
3103+
const assignment = ts.factory.createBinaryExpression(
3104+
target,
3105+
ts.SyntaxKind.EqualsToken,
3106+
ts.factory.createIdentifier(EVENT_PARAMETER),
3107+
);
3108+
3109+
statements.push(
3110+
tsCreateVariable(
3111+
target,
3112+
tcb.env.config.allowSignalsInTwoWayBindings ? unwrapWritableSignal(handler, tcb) : handler,
3113+
),
3114+
ts.factory.createExpressionStatement(assignment),
3115+
);
3116+
} else {
3117+
statements.push(ts.factory.createExpressionStatement(handler));
3118+
}
30953119

30963120
let eventParamType: ts.TypeNode | undefined;
30973121
if (eventType === EventParamType.Infer) {
@@ -3106,10 +3130,10 @@ function tcbCreateEventHandler(
31063130
// repeated within the handler function for their narrowing to be in effect within the handler.
31073131
const guards = scope.guards();
31083132

3109-
let body: ts.Statement = ts.factory.createExpressionStatement(handler);
3133+
let body = ts.factory.createBlock(statements);
31103134
if (guards !== null) {
31113135
// Wrap the body in an `if` statement containing all guards that have to be applied.
3112-
body = ts.factory.createIfStatement(guards, body);
3136+
body = ts.factory.createBlock([ts.factory.createIfStatement(guards, body)]);
31133137
}
31143138

31153139
const eventParam = ts.factory.createParameterDeclaration(
@@ -3128,7 +3152,7 @@ function tcbCreateEventHandler(
31283152
/* parameters */ [eventParam],
31293153
/* type */ ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword),
31303154
/* equalsGreaterThanToken */ undefined,
3131-
/* body */ ts.factory.createBlock([body]),
3155+
/* body */ body,
31323156
);
31333157
}
31343158

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

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,10 @@ runInEachFileSystem(() => {
381381
otherVal!: string;
382382
`,
383383
template: `<div dir [(gen)]="genVal" [(other)]="otherVal">`,
384-
expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
384+
expected: [
385+
`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
386+
`TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
387+
],
385388
},
386389
{
387390
id: 'generic inference and two-way binding to directive, mix of zone input and model',
@@ -405,7 +408,10 @@ runInEachFileSystem(() => {
405408
genVal!: boolean;
406409
otherVal!: string;
407410
`,
408-
expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
411+
expected: [
412+
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
413+
`TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
414+
],
409415
},
410416
{
411417
id: 'generic inference and two-way binding to directive (with `extends boolean`), all model inputs',
@@ -426,7 +432,10 @@ runInEachFileSystem(() => {
426432
genVal!: boolean;
427433
otherVal!: string;
428434
`,
429-
expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
435+
expected: [
436+
`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
437+
`TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
438+
],
430439
},
431440
{
432441
id: 'generic inference and two-way binding to directive (with `extends boolean`), mix of zone inputs and model',
@@ -450,7 +459,10 @@ runInEachFileSystem(() => {
450459
genVal!: boolean;
451460
otherVal!: string;
452461
`,
453-
expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
462+
expected: [
463+
`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
464+
`TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
465+
],
454466
},
455467
{
456468
id: 'generic multi-inference and two-way bindings to directive, all model inputs',
@@ -583,7 +595,10 @@ runInEachFileSystem(() => {
583595
outputs: {valueChange: {type: 'ModelSignal<string>'}},
584596
template: `<div dir [(value)]="bla">`,
585597
component: `bla = true;`,
586-
expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
598+
expected: [
599+
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
600+
`TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
601+
],
587602
},
588603
{
589604
id: 'two-way binding to primitive, valid',
@@ -652,7 +667,10 @@ runInEachFileSystem(() => {
652667
outputs: {valueChange: {type: 'ModelSignal<boolean>'}},
653668
template: `<div dir [(value)]="val">`,
654669
component: `val!: WritableSignal<string>;`,
655-
expected: [`TestComponent.html(1, 12): Type 'string' is not assignable to type 'boolean'.`],
670+
expected: [
671+
`TestComponent.html(1, 12): Type 'string' is not assignable to type 'boolean'.`,
672+
`TestComponent.html(1, 10): Type 'boolean' is not assignable to type 'string'.`,
673+
],
656674
},
657675
{
658676
id: 'non-writable signal binding',
@@ -662,6 +680,7 @@ runInEachFileSystem(() => {
662680
component: `val!: InputSignal<boolean>;`,
663681
expected: [
664682
`TestComponent.html(1, 10): Type 'InputSignal<boolean>' is not assignable to type 'boolean'.`,
683+
`TestComponent.html(1, 10): Type 'boolean' is not assignable to type 'InputSignal<boolean>'.`,
665684
],
666685
},
667686
{
@@ -682,6 +701,9 @@ runInEachFileSystem(() => {
682701
jasmine.stringContaining(
683702
`TestComponent.html(1, 12): Type '(v: string) => number' is not assignable to type '(v: number) => number`,
684703
),
704+
jasmine.stringContaining(
705+
`TestComponent.html(1, 10): Type '(v: number) => number' is not assignable to type '(v: string) => number`,
706+
),
685707
],
686708
},
687709
];

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ runInEachFileSystem(() => {
3131
outputs: {'valueChange': {type: 'OutputEmitterRef<string>'}},
3232
template: `<div dir [(value)]="bla">`,
3333
component: `bla = true;`,
34-
expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
34+
expected: [
35+
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
36+
`TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
37+
],
3538
},
3639
{
3740
id: 'two way data binding, valid',

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,8 @@ describe('type check blocks', () => {
717717
const block = tcb(TEMPLATE, DIRECTIVES);
718718
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
719719
expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
720+
expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
721+
expect(block).toContain('_t2 = $event;');
720722
});
721723

722724
it('should handle a two-way binding to an input/output pair of a generic directive', () => {
@@ -739,6 +741,8 @@ describe('type check blocks', () => {
739741
'var _t1 = _ctor1({ "input": (i1.ɵunwrapWritableSignal(((this).value))) });',
740742
);
741743
expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
744+
expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
745+
expect(block).toContain('_t2 = $event;');
742746
});
743747

744748
it('should handle a two-way binding to a model()', () => {
@@ -765,6 +769,8 @@ describe('type check blocks', () => {
765769
expect(block).toContain(
766770
'_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] = i1.ɵunwrapWritableSignal((((this).value)));',
767771
);
772+
expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
773+
expect(block).toContain('_t2 = $event;');
768774
});
769775

770776
it('should handle a two-way binding to an input with a transform', () => {
@@ -806,6 +812,8 @@ describe('type check blocks', () => {
806812
const block = tcb(TEMPLATE, DIRECTIVES);
807813
expect(block).toContain('var _t1 = null! as boolean | string;');
808814
expect(block).toContain('_t1 = i1.ɵunwrapWritableSignal((((this).value)));');
815+
expect(block).toContain('var _t3 = i1.ɵunwrapWritableSignal(((this).value));');
816+
expect(block).toContain('_t3 = $event;');
809817
});
810818

811819
describe('experimental DOM checking via lib.dom.d.ts', () => {
@@ -1733,15 +1741,15 @@ describe('type check blocks', () => {
17331741

17341742
const result = tcb(TEMPLATE);
17351743

1736-
expect(result).toContain(`if ((((this).expr)) === (0)) (this).zero();`);
1744+
expect(result).toContain(`if ((((this).expr)) === (0)) { (this).zero(); }`);
17371745
expect(result).toContain(
1738-
`if (!((((this).expr)) === (0)) && (((this).expr)) === (1)) (this).one();`,
1746+
`if (!((((this).expr)) === (0)) && (((this).expr)) === (1)) { (this).one(); }`,
17391747
);
17401748
expect(result).toContain(
1741-
`if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && (((this).expr)) === (2)) (this).two();`,
1749+
`if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && (((this).expr)) === (2)) { (this).two(); }`,
17421750
);
17431751
expect(result).toContain(
1744-
`if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && !((((this).expr)) === (2))) (this).otherwise();`,
1752+
`if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && !((((this).expr)) === (2))) { (this).otherwise(); }`,
17451753
);
17461754
});
17471755

@@ -1830,9 +1838,11 @@ describe('type check blocks', () => {
18301838

18311839
const result = tcb(TEMPLATE);
18321840

1833-
expect(result).toContain(`if (((this).expr) === 1) (this).one();`);
1834-
expect(result).toContain(`if (((this).expr) === 2) (this).two();`);
1835-
expect(result).toContain(`if (((this).expr) !== 1 && ((this).expr) !== 2) (this).default();`);
1841+
expect(result).toContain(`if (((this).expr) === 1) { (this).one(); }`);
1842+
expect(result).toContain(`if (((this).expr) === 2) { (this).two(); }`);
1843+
expect(result).toContain(
1844+
`if (((this).expr) !== 1 && ((this).expr) !== 2) { (this).default(); }`,
1845+
);
18361846
});
18371847

18381848
it('should generate a switch block inside a template', () => {

‎packages/compiler-cli/test/ngtsc/authoring_inputs_spec.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,9 @@ runInEachFileSystem(() => {
256256
);
257257

258258
const diags = env.driveDiagnostics();
259-
expect(diags.length).toBe(1);
259+
expect(diags.length).toBe(2);
260260
expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`);
261+
expect(diags[1].messageText).toBe(`Type 'string' is not assignable to type 'number'.`);
261262
});
262263

263264
describe('type checking', () => {

‎packages/compiler-cli/test/ngtsc/authoring_models_spec.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,9 @@ runInEachFileSystem(() => {
298298
);
299299

300300
const diags = env.driveDiagnostics();
301-
expect(diags.length).toBe(1);
301+
expect(diags.length).toBe(2);
302302
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
303+
expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`);
303304
});
304305

305306
it('should check a signal value bound to a model input via a two-way binding', () => {
@@ -328,8 +329,9 @@ runInEachFileSystem(() => {
328329
);
329330

330331
const diags = env.driveDiagnostics();
331-
expect(diags.length).toBe(1);
332+
expect(diags.length).toBe(2);
332333
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
334+
expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`);
333335
});
334336

335337
it('should check two-way binding of a signal to a decorator-based input/output pair', () => {
@@ -359,8 +361,9 @@ runInEachFileSystem(() => {
359361
);
360362

361363
const diags = env.driveDiagnostics();
362-
expect(diags.length).toBe(1);
364+
expect(diags.length).toBe(2);
363365
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
366+
expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`);
364367
});
365368

366369
it('should not allow a non-writable signal to be assigned to a model', () => {
@@ -389,10 +392,13 @@ runInEachFileSystem(() => {
389392
);
390393

391394
const diags = env.driveDiagnostics();
392-
expect(diags.length).toBe(1);
395+
expect(diags.length).toBe(2);
393396
expect(diags[0].messageText).toBe(
394397
`Type 'InputSignal<number>' is not assignable to type 'number'.`,
395398
);
399+
expect(diags[1].messageText).toBe(
400+
`Type 'number' is not assignable to type 'InputSignal<number>'.`,
401+
);
396402
});
397403

398404
it('should allow a model signal to be bound to another model signal', () => {
@@ -513,12 +519,17 @@ runInEachFileSystem(() => {
513519
);
514520

515521
const diags = env.driveDiagnostics();
516-
expect(diags.length).toBe(1);
522+
expect(diags.length).toBe(2);
517523
expect(diags[0].messageText).toEqual(
518524
jasmine.objectContaining({
519525
messageText: `Type '{ id: number; }' is not assignable to type '{ id: string; }'.`,
520526
}),
521527
);
528+
expect(diags[1].messageText).toEqual(
529+
jasmine.objectContaining({
530+
messageText: `Type '{ id: string; }' is not assignable to type '{ id: number; }'.`,
531+
}),
532+
);
522533
});
523534

524535
it('should check generic two-way model binding with a signal value', () => {
@@ -547,12 +558,17 @@ runInEachFileSystem(() => {
547558
);
548559

549560
const diags = env.driveDiagnostics();
550-
expect(diags.length).toBe(1);
561+
expect(diags.length).toBe(2);
551562
expect(diags[0].messageText).toEqual(
552563
jasmine.objectContaining({
553564
messageText: `Type '{ id: number; }' is not assignable to type '{ id: string; }'.`,
554565
}),
555566
);
567+
expect(diags[1].messageText).toEqual(
568+
jasmine.objectContaining({
569+
messageText: `Type '{ id: string; }' is not assignable to type '{ id: number; }'.`,
570+
}),
571+
);
556572
});
557573

558574
it('should report unwrapped signals assigned to a model in a one-way binding', () => {

‎packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -568,12 +568,17 @@ runInEachFileSystem(() => {
568568
);
569569

570570
const diags = env.driveDiagnostics();
571-
expect(diags.length).toBe(1);
571+
expect(diags.length).toBe(2);
572572
expect(diags[0].messageText).toEqual(
573573
jasmine.objectContaining({
574574
messageText: `Type '{ id: number; }' is not assignable to type '{ id: string; }'.`,
575575
}),
576576
);
577+
expect(diags[1].messageText).toEqual(
578+
jasmine.objectContaining({
579+
messageText: `Type '{ id: string; }' is not assignable to type '{ id: number; }'.`,
580+
}),
581+
);
577582
});
578583

579584
it('should use the setter type when assigning using a two-way binding to an input with different getter and setter types', () => {
@@ -603,7 +608,7 @@ runInEachFileSystem(() => {
603608
imports: [Dir],
604609
})
605610
export class FooCmp {
606-
nullableType = null;
611+
nullableType: string | null = null;
607612
}
608613
`,
609614
);
@@ -639,12 +644,45 @@ runInEachFileSystem(() => {
639644
);
640645

641646
const diags = env.driveDiagnostics();
642-
expect(diags.length).toBe(1);
647+
expect(diags.length).toBe(2);
643648
expect(diags[0].messageText).toEqual(
644649
jasmine.objectContaining({
645650
messageText: `Type '(val: string) => number' is not assignable to type 'TestFn'.`,
646651
}),
647652
);
653+
expect(diags[1].messageText).toEqual(
654+
jasmine.objectContaining({
655+
messageText: `Type 'TestFn' is not assignable to type '(val: string) => number'.`,
656+
}),
657+
);
658+
});
659+
660+
it('should type check a two-way binding to input/output pair where the input has a wider type than the output', () => {
661+
env.tsconfig({strictTemplates: true});
662+
env.write(
663+
'test.ts',
664+
`
665+
import {Component, Directive, Input, Output, EventEmitter} from '@angular/core';
666+
667+
@Directive({selector: '[dir]'})
668+
export class Dir {
669+
@Input() value: string | number;
670+
@Output() valueChange = new EventEmitter<number>();
671+
}
672+
673+
@Component({
674+
template: '<div dir [(value)]="value"></div>',
675+
imports: [Dir],
676+
})
677+
export class App {
678+
value = 'hello';
679+
}
680+
`,
681+
);
682+
683+
const diags = env.driveDiagnostics();
684+
expect(diags.length).toBe(1);
685+
expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`);
648686
});
649687

650688
it('should check the fallback content of ng-content', () => {

0 commit comments

Comments
 (0)