Skip to content

Commit

Permalink
fix(compiler-cli): catch function instance properties in interpolated…
Browse files Browse the repository at this point in the history 
… signal diagnostic (#54325)

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
  • Loading branch information
@cexbrayat @dylhunn
cexbrayat authored and dylhunn committed Feb 27, 2024
1 parent 6842909 commit f578889
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 6 deletions.
38 changes: 37 additions & 1 deletion ...compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts
View file
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 =
Expand All @@ -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 [];
}

Expand Down
102 changes: 97 additions & 5 deletions ...ended/test/checks/interpolated_signal_not_invoked/interpolated_signal_not_invoked_spec.ts
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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)}};
Expand Down Expand Up @@ -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);
});
});
});

0 comments on commit f578889

Please sign in to comment.