From f578889ca2dbb22f2e050d4c4af6aa2d597bf70d Mon Sep 17 00:00:00 2001 From: cexbrayat Date: Wed, 7 Feb 2024 22:23:26 +0100 Subject: [PATCH] fix(compiler-cli): catch function instance properties in interpolated signal diagnostic (#54325) Currently, the following template compiles without error, even if the signal is not properly called: ```
{{ mySignal.name }}
``` 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 --- .../interpolated_signal_not_invoked/index.ts | 38 ++++++- .../interpolated_signal_not_invoked_spec.ts | 102 +++++++++++++++++- 2 files changed, 134 insertions(+), 6 deletions(-) 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 index 2fe718715c1..9641972c35b 100644 --- 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, node: PropertyRead, component: ts.ClassDeclaration): Array> { + // 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 index 09e3812b6ab..a1be83e5a7f 100644 --- 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': `
`, }, source: ` - import {signal, Signal, computed} from '@angular/core'; + import {signal} from '@angular/core'; export class TestCmp { mySignal = signal(0); @@ -399,7 +399,7 @@ runInEachFileSystem(() => { 'TestCmp': `
`, }, source: ` - import {signal, Signal, computed} from '@angular/core'; + import {signal} from '@angular/core'; export class TestCmp { mySignal = signal(0); @@ -424,7 +424,7 @@ runInEachFileSystem(() => { 'TestCmp': `
`, }, source: ` - import {signal, Signal, computed} from '@angular/core'; + import {signal} from '@angular/core'; export class TestCmp { mySignal = signal(0); @@ -453,7 +453,7 @@ runInEachFileSystem(() => { 'TestCmp': `
`, }, source: ` - import {signal, Signal, computed} from '@angular/core'; + import {signal} from '@angular/core'; export class TestCmp { mySignal = signal(0); @@ -479,7 +479,7 @@ runInEachFileSystem(() => { 'TestCmp': `
`, }, source: ` - import {signal, Signal, computed} from '@angular/core'; + import {signal} from '@angular/core'; export class TestCmp { myObject = {myObject2: {myNestedSignal: signal(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': `
{{myObject.mySignal.${functionInstanceProperty}}}
`, + }, + 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': `
{{mySignal().${functionInstanceProperty}}}
`, + }, + 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': `
{{myObject.${functionInstanceProperty}}}
`, + }, + 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); + }); + }); });