fix(compiler-cli): catch function instance properties in interpolated signal diagnostic (#54325)

cexbrayat · dylhunn · commit f578889ca2db · 2024-02-26T18:30:05.000-08:00
Currently, the following template compiles without error, even if the signal is not properly called: ``` <div>{{ mySignal.name }}</div> ``` This is because `name` is one of the instance properties of Function (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function#instance_properties). The interpolated signal diagnostic is now extended to catch such issues. PR Close #54325
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts
@@ -22,6 +22,15 @@ const SIGNAL_FNS = new Set([
   'ModelSignal',
 ]);
 
+/** Names of known signal instance properties. */
+const SIGNAL_INSTANCE_PROPERTIES = new Set(['set', 'update', 'asReadonly']);
+
+/**
+ * Names of known function instance properties.
+ * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function#instance_properties
+ */
+const FUNCTION_INSTANCE_PROPERTIES = new Set(['name', 'length', 'prototype']);
+
 /**
  * Ensures Signals are invoked when used in template interpolations.
  */
@@ -53,12 +62,20 @@ function isSignal(symbol: ts.Symbol|undefined): boolean {
   });
 }
 
+function isFunctionInstanceProperty(name: string): boolean {
+  return FUNCTION_INSTANCE_PROPERTIES.has(name);
+}
+
+function isSignalInstanceProperty(name: string): boolean {
+  return SIGNAL_INSTANCE_PROPERTIES.has(name);
+}
+
 function buildDiagnosticForSignal(
     ctx: TemplateContext<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>, node: PropertyRead,
     component: ts.ClassDeclaration):
     Array<NgTemplateDiagnostic<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>> {
+  // check for `{{ mySignal }}`
   const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component);
-
   if (symbol?.kind === SymbolKind.Expression &&
       (isSignal(symbol.tsType.symbol) || isSignal(symbol.tsType.aliasSymbol))) {
     const templateMapping =
@@ -68,6 +85,25 @@ function buildDiagnosticForSignal(
     return [diagnostic];
   }
 
+  // check for `{{ mySignal.name }}` or `{{ mySignal.length }}` or `{{ mySignal.prototype }}`
+  // as these are the names of instance properties of Function, the compiler does _not_ throw an
+  // error.
+  // We also check for `{{ mySignal.set }}` or `{{ mySignal.update }}` or
+  // `{{ mySignal.asReadonly }}` as these are the names of instance properties of Signal
+  const symbolOfReceiver = ctx.templateTypeChecker.getSymbolOfNode(node.receiver, component);
+  if ((isFunctionInstanceProperty(node.name) || isSignalInstanceProperty(node.name)) &&
+      symbolOfReceiver?.kind === SymbolKind.Expression &&
+      (isSignal(symbolOfReceiver.tsType.symbol) || isSignal(symbolOfReceiver.tsType.aliasSymbol))) {
+    const templateMapping =
+        ctx.templateTypeChecker.getTemplateMappingAtTcbLocation(symbolOfReceiver.tcbLocation)!;
+
+    const errorString =
+        `${(node.receiver as PropertyRead).name} is a function and should be invoked: ${
+            (node.receiver as PropertyRead).name}()`;
+    const diagnostic = ctx.makeTemplateDiagnostic(templateMapping.span, errorString);
+    return [diagnostic];
+  }
+
   return [];
 }
 
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/interpolated_signal_not_invoked/interpolated_signal_not_invoked_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/interpolated_signal_not_invoked/interpolated_signal_not_invoked_spec.ts
@@ -371,7 +371,7 @@ runInEachFileSystem(() => {
           'TestCmp': `<div id="{{mySignal}}"></div>`,
         },
         source: `
-          import {signal, Signal, computed} from '@angular/core';
+          import {signal} from '@angular/core';
 
           export class TestCmp {
             mySignal = signal<number>(0);
@@ -399,7 +399,7 @@ runInEachFileSystem(() => {
           'TestCmp': `<div id="{{mySignal()}}"></div>`,
         },
         source: `
-          import {signal, Signal, computed} from '@angular/core';
+          import {signal} from '@angular/core';
 
           export class TestCmp {
             mySignal = signal<number>(0);
@@ -424,7 +424,7 @@ runInEachFileSystem(() => {
           'TestCmp': `<div attr.id="my-{{mySignal}}-item"></div>`,
         },
         source: `
-          import {signal, Signal, computed} from '@angular/core';
+          import {signal} from '@angular/core';
 
           export class TestCmp {
             mySignal = signal<number>(0);
@@ -453,7 +453,7 @@ runInEachFileSystem(() => {
              'TestCmp': `<div attr.id="my-{{mySignal()}}-item"></div>`,
            },
            source: `
-          import {signal, Signal, computed} from '@angular/core';
+          import {signal} from '@angular/core';
 
           export class TestCmp {
             mySignal = signal<number>(0);
@@ -479,7 +479,7 @@ runInEachFileSystem(() => {
           'TestCmp': `<div id="{{myObject.myObject2.myNestedSignal}}"></div>`,
         },
         source: `
-          import {signal, Signal, computed} from '@angular/core';
+          import {signal} from '@angular/core';
 
           export class TestCmp {
             myObject = {myObject2: {myNestedSignal: signal<number>(0)}};
@@ -553,4 +553,96 @@ runInEachFileSystem(() => {
     const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
     expect(diags.length).toBe(0);
   });
+
+  ['name', 'length', 'prototype', 'set', 'update', 'asReadonly'].forEach(
+      functionInstanceProperty => {
+        it(`should produce a warning when a property named '${
+               functionInstanceProperty}' of a not invoked signal is used in interpolation`,
+           () => {
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([
+               {
+                 fileName,
+                 templates: {
+                   'TestCmp': `<div>{{myObject.mySignal.${functionInstanceProperty}}}</div>`,
+                 },
+                 source: `
+          import {signal} from '@angular/core';
+
+          export class TestCmp {
+            myObject = { mySignal: signal<{ ${functionInstanceProperty}: string }>({ ${
+                     functionInstanceProperty}: 'foo' }) };
+          }`,
+               },
+             ]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {}
+                 /* options */
+             );
+             const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+             expect(diags.length).toBe(1);
+             expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
+             expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED));
+             expect(getSourceCodeForDiagnostic(diags[0])).toBe(`mySignal`);
+           });
+
+        it(`should not produce a warning when a property named ${
+               functionInstanceProperty} of an invoked signal is used in interpolation`,
+           () => {
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([
+               {
+                 fileName,
+                 templates: {
+                   'TestCmp': `<div>{{mySignal().${functionInstanceProperty}}}</div>`,
+                 },
+                 source: `
+            import {signal} from '@angular/core';
+
+            export class TestCmp {
+              mySignal = signal<{ ${functionInstanceProperty}: string }>({ ${
+                     functionInstanceProperty}: 'foo' });
+            }`,
+               },
+             ]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {}
+                 /* options */
+             );
+             const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+             expect(diags.length).toBe(0);
+           });
+
+        it(`should not produce a warning when a property named ${
+               functionInstanceProperty} of an object is used in interpolation`,
+           () => {
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([
+               {
+                 fileName,
+                 templates: {
+                   'TestCmp': `<div>{{myObject.${functionInstanceProperty}}}</div>`,
+                 },
+                 source: `
+            import {signal} from '@angular/core';
+
+            export class TestCmp {
+              myObject = { ${functionInstanceProperty}: 'foo' };
+            }`,
+               },
+             ]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {}
+                 /* options */
+             );
+             const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+             expect(diags.length).toBe(0);
+           });
+      });
 });
