refactor(compiler-cli): fix regression in two-way bindings to inputs …

…with different getter/setter types (#54252)

In a previous commit the TCB was changed to cast the assignment to an input in order to widen its type to allow `WritableSignal`. This ended up breaking existing inputs whose setter has a wider type than its getter. These changes switch to unwrapping the value on the binding side.

PR Close #54252

goldens/public-api/core/errors.md

@@ -123,10 +123,10 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     REQUIRED_INPUT_NO_VALUE = -950,
     // (undocumented)
-    REQUIRED_QUERY_NO_VALUE = -951,
-    // (undocumented)
     REQUIRED_MODEL_NO_VALUE = -952,
     // (undocumented)
+    REQUIRED_QUERY_NO_VALUE = -951,
+    // (undocumented)
     RUNTIME_DEPS_INVALID_IMPORTED_TYPE = 1000,
     // (undocumented)
     RUNTIME_DEPS_ORPHAN_COMPONENT = 1001,

packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts

@@ -132,6 +132,10 @@ export interface WritableSignal<T> extends Signal<T> {
   asReadonly(): Signal<T>;
 }
 
+export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
+  return null!;
+}
+
 export function signal<T>(initialValue: T): WritableSignal<T> {
   return null!;
 }

packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts

@@ -350,31 +350,15 @@ export class SymbolBuilder {
       return host !== null ? {kind: SymbolKind.DomBinding, host} : null;
     }
 
-    const isTwoWayBinding =
-        binding instanceof TmplAstBoundAttribute && binding.type === BindingType.TwoWay;
     const nodes = findAllMatchingNodes(
         this.typeCheckBlock, {withSpan: binding.sourceSpan, filter: isAssignment});
     const bindings: BindingSymbol[] = [];
     for (const node of nodes) {
-      let assignment: ts.PropertyAccessExpression|ts.ElementAccessExpression|null = null;
-
-      if (isAccessExpression(node.left)) {
-        // One-way bindings usually are in the form of `dir.input = expression`.
-        assignment = node.left;
-      } else if (
-          isTwoWayBinding && ts.isParenthesizedExpression(node.left) &&
-          ts.isAsExpression(node.left.expression) &&
-          isAccessExpression(node.left.expression.expression)) {
-        // The property side of two-way bindings is in the
-        // form of `(dir.input as someType) = expression`.
-        assignment = node.left.expression.expression;
-      }
-
-      if (assignment === null) {
+      if (!isAccessExpression(node.left)) {
         continue;
       }
 
-      const signalInputAssignment = unwrapSignalInputWriteTAccessor(assignment);
+      const signalInputAssignment = unwrapSignalInputWriteTAccessor(node.left);
       let symbolInfo: TsNodeSymbolInfo|null = null;
 
       // Signal inputs need special treatment because they are generated with an extra keyed
@@ -392,15 +376,15 @@ export class SymbolBuilder {
           tsType: typeSymbol.tsType,
         };
       } else {
-        symbolInfo = this.getSymbolOfTsNode(assignment);
+        symbolInfo = this.getSymbolOfTsNode(node.left);
       }
 
       if (symbolInfo === null || symbolInfo.tsSymbol === null) {
         continue;
       }
 
       const target = this.getDirectiveSymbolForAccessExpression(
-          signalInputAssignment?.fieldExpr ?? assignment, consumer);
+          signalInputAssignment?.fieldExpr ?? node.left, consumer);
       if (target === null) {
         continue;
       }

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

@@ -642,7 +642,7 @@ class TcbDirectiveCtorOp extends TcbOp {
           attr.attribute instanceof TmplAstTextAttribute) {
         continue;
       }
-      for (const {fieldName} of attr.inputs) {
+      for (const {fieldName, isTwoWayBinding} of attr.inputs) {
         // 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)) {
@@ -656,6 +656,7 @@ class TcbDirectiveCtorOp extends TcbOp {
           field: fieldName,
           expression,
           sourceSpan: attr.attribute.sourceSpan,
+          isTwoWayBinding,
         });
       }
     }
@@ -809,13 +810,15 @@ class TcbDirectiveInputsOp extends TcbOp {
           target = ts.factory.createElementAccessExpression(target, inputSignalBrandWriteSymbol);
         }
 
-        if (isTwoWayBinding) {
-          target = this.getTwoWayBindingTarget(target);
-        }
-
         if (attr.attribute.keySpan !== undefined) {
           addParseSpanInfo(target, attr.attribute.keySpan);
         }
+
+        // Two-way bindings accept `T | WritableSignal<T>` so we have to unwrap the value.
+        if (isTwoWayBinding) {
+          assignment = unwrapWritableSignal(assignment, this.tcb);
+        }
+
         // Finally the assignment is extended by assigning it into the target expression.
         assignment =
             ts.factory.createBinaryExpression(target, ts.SyntaxKind.EqualsToken, assignment);
@@ -850,35 +853,6 @@ class TcbDirectiveInputsOp extends TcbOp {
           this.tcb.id, this.node, this.dir.name, this.dir.isComponent, missing);
     }
   }
-
-  private getTwoWayBindingTarget(target: ts.LeftHandSideExpression): ts.LeftHandSideExpression {
-    // Two-way bindings to inputs allow both the input's defined type and a `WritableSignal`
-    // of that type. For example `[(value)]="val"` where `@Input() value: number | string`
-    // allows `val` to be `number | string | WritableSignal<number | string>`. We generate the
-    // following expressions to expand the type:
-    // ```
-    // var captureType = dir.value;
-    // (dir.value as typeof captureType | WritableSignal<typeof captureType>) = expression;
-    // ```
-    // Some notes:
-    // - We wrap the assignment, insted of for example declaring a variable and assigning to it,
-    //   because keeping the assignment makes it easier to do lookups in the language service.
-    // - The `captureType` variable can be inline, but for signal input expressions it can be
-    //   long so we use it make the code a bit neater. It also saves us some code that would
-    //   have to convert property/element access expressions into type query nodes.
-    const captureType = this.tcb.allocateId();
-    const captureTypeVar = tsCreateVariable(captureType, target);
-    markIgnoreDiagnostics(captureTypeVar);
-    this.scope.addStatement(captureTypeVar);
-
-    const typeQuery = ts.factory.createTypeQueryNode(captureType);
-    const writableSignalRef = this.tcb.env.referenceExternalType(
-        R3Identifiers.WritableSignal.moduleName, R3Identifiers.WritableSignal.name,
-        [new ExpressionType(new TypeofExpr(new WrappedNodeExpr(captureType)))]);
-
-    return ts.factory.createParenthesizedExpression(ts.factory.createAsExpression(
-        target, ts.factory.createUnionTypeNode([typeQuery, writableSignalRef])));
-  }
 }
 
 /**
@@ -2523,7 +2497,12 @@ function tcbCallTypeCtor(
 
     if (input.type === 'binding') {
       // For bound inputs, the property is assigned the binding expression.
-      const expr = widenBinding(input.expression, tcb);
+      let expr = widenBinding(input.expression, tcb);
+
+      if (input.isTwoWayBinding) {
+        expr = unwrapWritableSignal(expr, tcb);
+      }
+
       const assignment =
           ts.factory.createPropertyAssignment(propertyName, wrapForDiagnostics(expr));
       addParseSpanInfo(assignment, input.sourceSpan);
@@ -2623,6 +2602,15 @@ function widenBinding(expr: ts.Expression, tcb: Context): ts.Expression {
   }
 }
 
+/**
+ * Wraps an expression in an `unwrapSignal` call which extracts the signal's value.
+ */
+function unwrapWritableSignal(expression: ts.Expression, tcb: Context): ts.CallExpression {
+  const unwrapRef = tcb.env.referenceExternalSymbol(
+      R3Identifiers.unwrapWritableSignal.moduleName, R3Identifiers.unwrapWritableSignal.name);
+  return ts.factory.createCallExpression(unwrapRef, undefined, [expression]);
+}
+
 /**
  * An input binding that corresponds with a field of a directive.
  */
@@ -2643,6 +2631,11 @@ interface TcbDirectiveBoundInput {
    * The source span of the full attribute binding.
    */
   sourceSpan: ParseSourceSpan;
+
+  /**
+   * Whether the binding is part of a two-way binding.
+   */
+  isTwoWayBinding: boolean;
 }
 
 /**

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

@@ -37,7 +37,7 @@ runInEachFileSystem(() => {
         template: `<div dir [(value)]="bla">`,
         component: `bla = true;`,
         expected: [
-          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string | WritableSignal<string>'.`,
+          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
         ],
       },
       {

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

@@ -659,9 +659,23 @@ describe('type check blocks', () => {
     }];
     const block = tcb(TEMPLATE, DIRECTIVES);
     expect(block).toContain('var _t1 = null! as i0.TwoWay;');
-    expect(block).toContain('var _t2 = _t1.input;');
-    expect(block).toContain(
-        '(_t1.input as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
+    expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
+  });
+
+  it('should handle a two-way binding to an input/output pair of a generic directive', () => {
+    const TEMPLATE = `<div twoWay [(input)]="value"></div>`;
+    const DIRECTIVES: TestDeclaration[] = [{
+      type: 'directive',
+      name: 'TwoWay',
+      selector: '[twoWay]',
+      inputs: {input: 'input'},
+      outputs: {inputChange: 'inputChange'},
+      isGeneric: true,
+    }];
+    const block = tcb(TEMPLATE, DIRECTIVES);
+    expect(block).toContain('const _ctor1: <T extends string = any>(init: Pick<i0.TwoWay<T>, "input">) => i0.TwoWay<T> = null!');
+    expect(block).toContain('var _t1 = _ctor1({ "input": (i1.ɵunwrapWritableSignal(((this).value))) });');
+    expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
   });
 
   it('should handle a two-way binding to a model()', () => {
@@ -683,9 +697,7 @@ describe('type check blocks', () => {
     }];
     const block = tcb(TEMPLATE, DIRECTIVES);
     expect(block).toContain('var _t1 = null! as i0.TwoWay;');
-    expect(block).toContain('var _t2 = _t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE];');
-    expect(block).toContain(
-        '(_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
+    expect(block).toContain('_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] = i1.ɵunwrapWritableSignal((((this).value)));');
   });
 
   it('should handle a two-way binding to an input with a transform', () => {
@@ -715,9 +727,7 @@ describe('type check blocks', () => {
     }];
     const block = tcb(TEMPLATE, DIRECTIVES);
     expect(block).toContain('var _t1 = null! as boolean | string;');
-    expect(block).toContain('var _t2 = _t1;');
-    expect(block).toContain(
-        '(_t1 as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
+    expect(block).toContain('_t1 = i1.ɵunwrapWritableSignal((((this).value)));');
   });
 
   describe('experimental DOM checking via lib.dom.d.ts', () => {

packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts

@@ -177,6 +177,10 @@ export function angularCoreDts(): TestFile {
 
     export type Signal<T> = (() => T);
 
+    export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
+      return null!;
+    }
+
     export interface ModelOptions {
       alias?: string;
     }

packages/compiler-cli/test/ngtsc/authoring_spec.ts

@@ -222,8 +222,7 @@ runInEachFileSystem(() => {
 
       const diags = env.driveDiagnostics();
       expect(diags.length).toBe(1);
-      expect(diags[0].messageText)
-          .toBe(`Type 'number' is not assignable to type 'string | WritableSignal<string>'.`);
+      expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`);
     });
 
     describe('type checking', () => {
@@ -552,8 +551,7 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText)
-            .toBe(`Type 'boolean' is not assignable to type 'number | WritableSignal<number>'.`);
+        expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
       });
 
       it('should check a signal value bound to a model input via a two-way binding', () => {
@@ -580,10 +578,7 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText).toEqual(jasmine.objectContaining({
-          messageText:
-              `Type 'WritableSignal<boolean>' is not assignable to type 'number | WritableSignal<number>'.`
-        }));
+        expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
       });
 
       it('should check two-way binding of a signal to a decorator-based input/output pair', () => {
@@ -611,10 +606,7 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText).toEqual(jasmine.objectContaining({
-          messageText:
-              `Type 'WritableSignal<boolean>' is not assignable to type 'number | WritableSignal<number>'.`
-        }));
+        expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
       });
 
       it('should not allow a non-writable signal to be assigned to a model', () => {
@@ -641,10 +633,8 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText).toEqual(jasmine.objectContaining({
-          messageText:
-              `Type 'InputSignal<number>' is not assignable to type 'number | WritableSignal<number>'.`
-        }));
+        expect(diags[0].messageText)
+            .toBe(`Type 'InputSignal<number>' is not assignable to type 'number'.`);
       });
 
       it('should allow a model signal to be bound to another model signal', () => {
@@ -752,10 +742,8 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText).toEqual(jasmine.objectContaining({
-          messageText:
-              `Type '{ id: number; }' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
-        }));
+        expect(diags[0].messageText)
+            .toBe(`Type '{ id: number; }' is not assignable to type '{ id: string; }'.`);
       });
 
       it('should check generic two-way model binding with a signal value', () => {
@@ -782,10 +770,8 @@ runInEachFileSystem(() => {
 
         const diags = env.driveDiagnostics();
         expect(diags.length).toBe(1);
-        expect(diags[0].messageText).toEqual(jasmine.objectContaining({
-          messageText:
-              `Type 'WritableSignal<{ id: number; }>' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
-        }));
+        expect(diags[0].messageText)
+            .toEqual(`Type '{ id: number; }' is not assignable to type '{ id: string; }'.`);
       });
 
       it('should report unwrapped signals assigned to a model in a one-way binding', () => {