refactor(core): correctly distinguish getter functions from writable signals (angular#54252)

crisbeto · thePunderWoman · commit e921e108e183 · 2024-02-07T16:36:15.000Z
Fixes that `ɵunwrapWritableSignal` inferring getter functions as not matching the interface of `WritableSignal` instead of preserving them. PR Close angular#54252
diff --git a/goldens/public-api/core/index.md b/goldens/public-api/core/index.md
@@ -1766,6 +1766,8 @@ export abstract class ViewRef extends ChangeDetectorRef {
 
 // @public
 export interface WritableSignal<T> extends Signal<T> {
+    // (undocumented)
+    [WRITABLE_SIGNAL]: T;
     asReadonly(): Signal<T>;
     set(value: T): void;
     update(updateFn: (value: T) => T): void;
diff --git a/packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts b/packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts
@@ -126,13 +126,21 @@ export type Signal<T> = (() => T)&{
   [SIGNAL]: unknown;
 };
 
+/**
+ * Symbol used to tell distinguish `WritableSignal` from other non-writable signals and functions.
+ */
+const WRITABLE_SIGNAL = /* @__PURE__ */ Symbol('WRITABLE_SIGNAL');
+
 export interface WritableSignal<T> extends Signal<T> {
+  [WRITABLE_SIGNAL]: T;
   set(value: T): void;
   update(updateFn: (value: T) => T): void;
   asReadonly(): Signal<T>;
 }
 
-export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
+// Note: needs to be kept in sync with the copies in `render3/reactivity/signal.ts` and
+// `ngtsc/typecheck/testing/index.ts` to ensure consistent tests.
+export function ɵunwrapWritableSignal<T>(value: T|{[WRITABLE_SIGNAL]: T}): T {
   return null!;
 }
 
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -118,7 +118,12 @@ export function angularCoreDts(): TestFile {
     export declare function signal<T>(initialValue: T): WritableSignal<T>;
     export declare function computed<T>(computation: () => T): Signal<T>;
 
+    const WRITABLE_SIGNAL = /* @__PURE__ */ Symbol('WRITABLE_SIGNAL');
+
     export interface WritableSignal<T> extends Signal<T> {
+      [WRITABLE_SIGNAL]: T;
+      set(value: T): void;
+      update(updateFn: (value: T) => T): void;
       asReadonly(): Signal<T>;
     }
 
@@ -177,7 +182,9 @@ export function angularCoreDts(): TestFile {
 
     export type Signal<T> = (() => T);
 
-    export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
+    // Note: needs to be kept in sync with the copies in render3/reactivity/signal.ts and
+    // fake_core/index.ts to ensure consistent tests.
+    export function ɵunwrapWritableSignal<T>(value: T|{[WRITABLE_SIGNAL]: T}): T {
       return null!;
     }
 
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -479,6 +479,36 @@ export declare class AnimationEvent {
          expect(diags).toEqual([]);
        });
 
+    it('should type check a two-way binding to a function value', () => {
+      env.tsconfig({strictTemplates: true});
+      env.write('test.ts', `
+        import {Component, Directive, Input, Output, EventEmitter} from '@angular/core';
+
+        type TestFn = (val: number | null | undefined) => string;
+
+        @Directive({selector: '[dir]', standalone: true})
+        export class Dir {
+          @Input() val!: TestFn;
+          @Output() valChange = new EventEmitter<TestFn>();
+        }
+
+        @Component({
+          template: '<input dir [(val)]="invalidType">',
+          standalone: true,
+          imports: [Dir],
+        })
+        export class FooCmp {
+          invalidType = (val: string) => 0;
+        }
+      `);
+
+      const diags = env.driveDiagnostics();
+      expect(diags.length).toBe(1);
+      expect(diags[0].messageText).toEqual(jasmine.objectContaining({
+        messageText: `Type '(val: string) => number' is not assignable to type 'TestFn'.`,
+      }));
+    });
+
     describe('strictInputTypes', () => {
       beforeEach(() => {
         env.write('test.ts', `
diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts
@@ -408,6 +408,5 @@ export class Identifiers {
   // type-checking
   static InputSignalBrandWriteType = {name: 'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE', moduleName: CORE};
   static UnwrapDirectiveSignalInputs = {name: 'ɵUnwrapDirectiveSignalInputs', moduleName: CORE};
-  static WritableSignal = {name: 'WritableSignal', moduleName: CORE};
   static unwrapWritableSignal = {name: 'ɵunwrapWritableSignal', moduleName: CORE};
 }
diff --git a/packages/core/src/render3/reactivity/signal.ts b/packages/core/src/render3/reactivity/signal.ts
@@ -10,10 +10,15 @@ import {createSignal, SIGNAL, SignalGetter, SignalNode, signalSetFn, signalUpdat
 
 import {isSignal, Signal, ValueEqualityFn} from './api';
 
+/** Symbol used distinguish `WritableSignal` from other non-writable signals and functions. */
+const WRITABLE_SIGNAL = /* @__PURE__ */ Symbol('WRITABLE_SIGNAL');
+
 /**
  * A `Signal` with a value that can be mutated via a setter interface.
  */
 export interface WritableSignal<T> extends Signal<T> {
+  [WRITABLE_SIGNAL]: T;
+
   /**
    * Directly set the signal to a new value, and notify any dependents.
    */
@@ -37,7 +42,11 @@ export interface WritableSignal<T> extends Signal<T> {
  * Utility function used during template type checking to extract the value from a `WritableSignal`.
  * @codeGenApi
  */
-export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
+export function ɵunwrapWritableSignal<T>(value: T|{[WRITABLE_SIGNAL]: T}): T {
+  // Note: needs to be kept in sync with the copies in `fake_core/index.ts` and
+  // `ngtsc/typecheck/testing/index.ts` to ensure consistent tests.
+  // Note: the function uses `WRITABLE_SIGNAL` as a brand instead of `WritableSignal<T>`,
+  // because the latter incorrectly unwraps non-signal getter functions.
   return null!;
 }
 
diff --git a/packages/core/test/render3/jit_environment_spec.ts b/packages/core/test/render3/jit_environment_spec.ts
@@ -34,7 +34,6 @@ const AOT_ONLY = new Set<string>([
   // used in type-checking.
   'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE',
   'ɵUnwrapDirectiveSignalInputs',
-  'WritableSignal',
   'ɵunwrapWritableSignal',
 ]);
 

Original file line number	Diff line number	Diff line change
`@@ -408,6 +408,5 @@ export class Identifiers {`
`408`	`408`	`// type-checking`
`409`	`409`	`static InputSignalBrandWriteType = {name: 'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE', moduleName: CORE};`
`410`	`410`	`static UnwrapDirectiveSignalInputs = {name: 'ɵUnwrapDirectiveSignalInputs', moduleName: CORE};`
`411`		`- static WritableSignal = {name: 'WritableSignal', moduleName: CORE};`
`412`	`411`	`static unwrapWritableSignal = {name: 'ɵunwrapWritableSignal', moduleName: CORE};`
`413`	`412`	`}`