crisbeto · Nov 29, 2024 · Dec 2, 2024 · Dec 5, 2024 · crisbeto · Dec 5, 2024
diff --git a/packages/compiler-cli/src/ngtsc/core/api/src/options.ts b/packages/compiler-cli/src/ngtsc/core/api/src/options.ts
@@ -120,6 +120,12 @@ export interface InternalOptions {
    * @internal
    */
   _enableHmr?: boolean;
+
+  // TODO(crisbeto): this is a temporary flag that will be removed in v20.
+  /**
+   * Whether to check the event side of two-way bindings.
+   */
+  _checkTwoWayBoundEvents?: boolean;
 }
 
 /**

diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts
@@ -1027,6 +1027,7 @@ export class NgCompiler {
     const strictTemplates = !!this.options.strictTemplates;
 
     const useInlineTypeConstructors = this.programDriver.supportsInlineOperations;
+    const checkTwoWayBoundEvents = this.options['_checkTwoWayBoundEvents'] ?? false;
 
     // Check whether the loaded version of `@angular/core` in the `ts.Program` supports unwrapping
     // writable signals for type-checking. If this check fails to find a suitable .d.ts file, fall
@@ -1080,6 +1081,7 @@ export class NgCompiler {
         unusedStandaloneImports:
           this.options.extendedDiagnostics?.defaultCategory || DiagnosticCategoryLabel.Warning,
         allowSignalsInTwoWayBindings,
+        checkTwoWayBoundEvents,
       };
     } else {
       typeCheckingConfig = {
@@ -1114,6 +1116,7 @@ export class NgCompiler {
         unusedStandaloneImports:
           this.options.extendedDiagnostics?.defaultCategory || DiagnosticCategoryLabel.Warning,
         allowSignalsInTwoWayBindings,
+        checkTwoWayBoundEvents,
       };
     }
 

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
@@ -357,6 +357,11 @@ export interface TypeCheckingConfig {
    * Whether to descend into the bodies of control flow blocks (`@if`, `@switch` and `@for`).
    */
   checkControlFlowBodies: boolean;
+
+  /**
+   * Whether the event side of a two-way binding should be type checked.
+   */
+  checkTwoWayBoundEvents: boolean;
 }
 
 export type TemplateSourceMapping =

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
@@ -1407,7 +1407,7 @@ export class TcbDirectiveOutputsOp extends TcbOp {
 
       if (this.tcb.env.config.checkTypeOfOutputEvents && output.name.endsWith('Change')) {
         const inputName = output.name.slice(0, -6);
-        isSplitTwoWayBinding(inputName, output, this.node.inputs, this.tcb);
+        checkSplitTwoWayBinding(inputName, output, this.node.inputs, this.tcb);
       }
       // TODO(alxhub): consider supporting multiple fields with the same property name for outputs.
       const field = outputs.getByBindingPropertyName(output.name)![0].classPropertyName;
@@ -1481,7 +1481,7 @@ class TcbUnclaimedOutputsOp extends TcbOp {
 
       if (this.tcb.env.config.checkTypeOfOutputEvents && output.name.endsWith('Change')) {
         const inputName = output.name.slice(0, -6);
-        if (isSplitTwoWayBinding(inputName, output, this.element.inputs, this.tcb)) {
+        if (checkSplitTwoWayBinding(inputName, output, this.element.inputs, this.tcb)) {
           // Skip this event handler as the error was already handled.
           continue;
         }
@@ -3092,6 +3092,31 @@ function tcbCreateEventHandler(
   eventType: EventParamType | ts.TypeNode,
 ): ts.Expression {
   const handler = tcbEventHandlerExpression(event.handler, tcb, scope);
+  const statements: ts.Statement[] = [];
+
+  // TODO(crisbeto): remove the `checkTwoWayBoundEvents` check in v20.
+  if (event.type === ParsedEventType.TwoWay && tcb.env.config.checkTwoWayBoundEvents) {
+    // If we're dealing with a two-way event, we create a variable initialized to the unwrapped
+    // signal value of the expression and then we assign `$event` to it. Note that in most cases
+    // this will already be covered by the corresponding input binding, however it allows us to
+    // handle the case where the input has a wider type than the output (see #58971).
+    const target = tcb.allocateId();
+    const assignment = ts.factory.createBinaryExpression(
+      target,
+      ts.SyntaxKind.EqualsToken,
+      ts.factory.createIdentifier(EVENT_PARAMETER),
+    );
+
+    statements.push(
+      tsCreateVariable(
+        target,
+        tcb.env.config.allowSignalsInTwoWayBindings ? unwrapWritableSignal(handler, tcb) : handler,
+      ),
+      ts.factory.createExpressionStatement(assignment),
+    );
+  } else {
+    statements.push(ts.factory.createExpressionStatement(handler));
+  }
 
   let eventParamType: ts.TypeNode | undefined;
   if (eventType === EventParamType.Infer) {
@@ -3106,10 +3131,10 @@ function tcbCreateEventHandler(
   // repeated within the handler function for their narrowing to be in effect within the handler.
   const guards = scope.guards();
 
-  let body: ts.Statement = ts.factory.createExpressionStatement(handler);
+  let body = ts.factory.createBlock(statements);
   if (guards !== null) {
     // Wrap the body in an `if` statement containing all guards that have to be applied.
-    body = ts.factory.createIfStatement(guards, body);
+    body = ts.factory.createBlock([ts.factory.createIfStatement(guards, body)]);
   }
 
   const eventParam = ts.factory.createParameterDeclaration(
@@ -3128,7 +3153,7 @@ function tcbCreateEventHandler(
     /* parameters */ [eventParam],
     /* type */ ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword),
     /* equalsGreaterThanToken */ undefined,
-    /* body */ ts.factory.createBlock([body]),
+    /* body */ body,
   );
 }
 
@@ -3142,7 +3167,7 @@ function tcbEventHandlerExpression(ast: AST, tcb: Context, scope: Scope): ts.Exp
   return translator.translate(ast);
 }
 
-function isSplitTwoWayBinding(
+function checkSplitTwoWayBinding(
   inputName: string,
   output: TmplAstBoundEvent,
   inputs: TmplAstBoundAttribute[],

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/model_signal_diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/model_signal_diagnostics_spec.ts
@@ -381,7 +381,10 @@ runInEachFileSystem(() => {
           otherVal!: string;
         `,
         template: `<div dir [(gen)]="genVal" [(other)]="otherVal">`,
-        expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
+        expected: [
+          `TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
+          `TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
+        ],
       },
       {
         id: 'generic inference and two-way binding to directive, mix of zone input and model',
@@ -405,7 +408,10 @@ runInEachFileSystem(() => {
           genVal!: boolean;
           otherVal!: string;
         `,
-        expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
+        expected: [
+          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
+          `TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
+        ],
       },
       {
         id: 'generic inference and two-way binding to directive (with `extends boolean`), all model inputs',
@@ -426,7 +432,10 @@ runInEachFileSystem(() => {
           genVal!: boolean;
           otherVal!: string;
         `,
-        expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
+        expected: [
+          `TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
+          `TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
+        ],
       },
       {
         id: 'generic inference and two-way binding to directive (with `extends boolean`), mix of zone inputs and model',
@@ -450,7 +459,10 @@ runInEachFileSystem(() => {
           genVal!: boolean;
           otherVal!: string;
         `,
-        expected: [`TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`],
+        expected: [
+          `TestComponent.html(1, 29): Type 'string' is not assignable to type 'boolean'.`,
+          `TestComponent.html(1, 27): Type 'boolean' is not assignable to type 'string'.`,
+        ],
       },
       {
         id: 'generic multi-inference and two-way bindings to directive, all model inputs',
@@ -583,7 +595,10 @@ runInEachFileSystem(() => {
         outputs: {valueChange: {type: 'ModelSignal<string>'}},
         template: `<div dir [(value)]="bla">`,
         component: `bla = true;`,
-        expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
+        expected: [
+          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
+          `TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
+        ],
       },
       {
         id: 'two-way binding to primitive, valid',
@@ -652,7 +667,10 @@ runInEachFileSystem(() => {
         outputs: {valueChange: {type: 'ModelSignal<boolean>'}},
         template: `<div dir [(value)]="val">`,
         component: `val!: WritableSignal<string>;`,
-        expected: [`TestComponent.html(1, 12): Type 'string' is not assignable to type 'boolean'.`],
+        expected: [
+          `TestComponent.html(1, 12): Type 'string' is not assignable to type 'boolean'.`,
+          `TestComponent.html(1, 10): Type 'boolean' is not assignable to type 'string'.`,
+        ],
       },
       {
         id: 'non-writable signal binding',
@@ -662,6 +680,7 @@ runInEachFileSystem(() => {
         component: `val!: InputSignal<boolean>;`,
         expected: [
           `TestComponent.html(1, 10): Type 'InputSignal<boolean>' is not assignable to type 'boolean'.`,
+          `TestComponent.html(1, 10): Type 'boolean' is not assignable to type 'InputSignal<boolean>'.`,
         ],
       },
       {
@@ -682,6 +701,9 @@ runInEachFileSystem(() => {
           jasmine.stringContaining(
             `TestComponent.html(1, 12): Type '(v: string) => number' is not assignable to type '(v: number) => number`,
           ),
+          jasmine.stringContaining(
+            `TestComponent.html(1, 10): Type '(v: number) => number' is not assignable to type '(v: string) => number`,
+          ),
         ],
       },
     ];

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts
@@ -31,7 +31,10 @@ runInEachFileSystem(() => {
         outputs: {'valueChange': {type: 'OutputEmitterRef<string>'}},
         template: `<div dir [(value)]="bla">`,
         component: `bla = true;`,
-        expected: [`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`],
+        expected: [
+          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
+          `TestComponent.html(1, 10): Type 'string' is not assignable to type 'boolean'.`,
+        ],
       },
       {
         id: 'two way data binding, valid',

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
@@ -717,6 +717,8 @@ describe('type check blocks', () => {
     const block = tcb(TEMPLATE, DIRECTIVES);
     expect(block).toContain('var _t1 = null! as i0.TwoWay;');
     expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
+    expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
+    expect(block).toContain('_t2 = $event;');
   });
 
   it('should handle a two-way binding to an input/output pair of a generic directive', () => {
@@ -739,6 +741,8 @@ describe('type check blocks', () => {
       'var _t1 = _ctor1({ "input": (i1.ɵunwrapWritableSignal(((this).value))) });',
     );
     expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
+    expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
+    expect(block).toContain('_t2 = $event;');
   });
 
   it('should handle a two-way binding to a model()', () => {
@@ -765,6 +769,8 @@ describe('type check blocks', () => {
     expect(block).toContain(
       '_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] = i1.ɵunwrapWritableSignal((((this).value)));',
     );
+    expect(block).toContain('var _t2 = i1.ɵunwrapWritableSignal(((this).value));');
+    expect(block).toContain('_t2 = $event;');
   });
 
   it('should handle a two-way binding to an input with a transform', () => {
@@ -806,6 +812,8 @@ describe('type check blocks', () => {
     const block = tcb(TEMPLATE, DIRECTIVES);
     expect(block).toContain('var _t1 = null! as boolean | string;');
     expect(block).toContain('_t1 = i1.ɵunwrapWritableSignal((((this).value)));');
+    expect(block).toContain('var _t3 = i1.ɵunwrapWritableSignal(((this).value));');
+    expect(block).toContain('_t3 = $event;');
   });
 
   describe('experimental DOM checking via lib.dom.d.ts', () => {
@@ -973,6 +981,7 @@ describe('type check blocks', () => {
       controlFlowPreventingContentProjection: 'warning',
       unusedStandaloneImports: 'warning',
       allowSignalsInTwoWayBindings: true,
+      checkTwoWayBoundEvents: true,
     };
 
     describe('config.applyTemplateContextGuards', () => {
@@ -1733,15 +1742,15 @@ describe('type check blocks', () => {
 
       const result = tcb(TEMPLATE);
 
-      expect(result).toContain(`if ((((this).expr)) === (0)) (this).zero();`);
+      expect(result).toContain(`if ((((this).expr)) === (0)) { (this).zero(); }`);
       expect(result).toContain(
-        `if (!((((this).expr)) === (0)) && (((this).expr)) === (1)) (this).one();`,
+        `if (!((((this).expr)) === (0)) && (((this).expr)) === (1)) { (this).one(); }`,
       );
       expect(result).toContain(
-        `if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && (((this).expr)) === (2)) (this).two();`,
+        `if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && (((this).expr)) === (2)) { (this).two(); }`,
       );
       expect(result).toContain(
-        `if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && !((((this).expr)) === (2))) (this).otherwise();`,
+        `if (!((((this).expr)) === (0)) && !((((this).expr)) === (1)) && !((((this).expr)) === (2))) { (this).otherwise(); }`,
       );
     });
 
@@ -1830,9 +1839,11 @@ describe('type check blocks', () => {
 
       const result = tcb(TEMPLATE);
 
-      expect(result).toContain(`if (((this).expr) === 1) (this).one();`);
-      expect(result).toContain(`if (((this).expr) === 2) (this).two();`);
-      expect(result).toContain(`if (((this).expr) !== 1 && ((this).expr) !== 2) (this).default();`);
+      expect(result).toContain(`if (((this).expr) === 1) { (this).one(); }`);
+      expect(result).toContain(`if (((this).expr) === 2) { (this).two(); }`);
+      expect(result).toContain(
+        `if (((this).expr) !== 1 && ((this).expr) !== 2) { (this).default(); }`,
+      );
     });
 
     it('should generate a switch block inside a template', () => {

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -284,6 +284,7 @@ export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
   controlFlowPreventingContentProjection: 'warning',
   unusedStandaloneImports: 'warning',
   allowSignalsInTwoWayBindings: true,
+  checkTwoWayBoundEvents: true,
 };
 
 // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
@@ -423,6 +424,7 @@ export function tcb(
     useInlineTypeConstructors: true,
     suggestionsForSuboptimalTypeInference: false,
     allowSignalsInTwoWayBindings: true,
+    checkTwoWayBoundEvents: true,
     ...config,
   };
   options = options || {

diff --git a/packages/compiler-cli/test/ngtsc/authoring_inputs_spec.ts b/packages/compiler-cli/test/ngtsc/authoring_inputs_spec.ts
@@ -21,7 +21,7 @@ runInEachFileSystem(() => {
 
     beforeEach(() => {
       env = NgtscTestEnvironment.setup(testFiles);
-      env.tsconfig({strictTemplates: true});
+      env.tsconfig({strictTemplates: true, _checkTwoWayBoundEvents: true});
     });
 
     it('should handle a basic, primitive valued input', () => {
@@ -256,8 +256,9 @@ runInEachFileSystem(() => {
       );
 
       const diags = env.driveDiagnostics();
-      expect(diags.length).toBe(1);
+      expect(diags.length).toBe(2);
       expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`);
+      expect(diags[1].messageText).toBe(`Type 'string' is not assignable to type 'number'.`);
     });
 
     describe('type checking', () => {