Skip to content

Commit 84ade02

Browse files
crisbetodylhunn
crisbeto
authored and
dylhunn
committed
refactor(compiler-cli): changes to get signal diagnostic working internally (#54585)
The diagnostic for signals that haven't been invoked wasn't working internally, because the path to `@angular/core` is different. These changes resolve the issue and do some general cleanup. PR Close #54585
1 parent dbe673b commit 84ade02

File tree

2 files changed

+66
-25
lines changed

2 files changed

+66
-25
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,15 @@ import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics
1313
import {NgTemplateDiagnostic, SymbolKind} from '../../../api';
1414
import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
1515

16+
/** Names of known signal functions. */
17+
const SIGNAL_FNS = new Set([
18+
'WritableSignal',
19+
'Signal',
20+
'InputSignal',
21+
'InputSignalWithTransform',
22+
'ModelSignal',
23+
]);
24+
1625
/**
1726
* Ensures Signals are invoked when used in template interpolations.
1827
*/
@@ -26,23 +35,22 @@ class InterpolatedSignalCheck extends
2635
node: TmplAstNode|AST): NgTemplateDiagnostic<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>[] {
2736
if (node instanceof Interpolation) {
2837
return node.expressions.filter((item): item is PropertyRead => item instanceof PropertyRead)
29-
.flatMap((item) => {
30-
if (item instanceof PropertyRead) {
31-
return buildDiagnosticForSignal(ctx, item, component);
32-
}
33-
return [];
34-
});
38+
.flatMap(item => buildDiagnosticForSignal(ctx, item, component));
3539
}
3640
return [];
3741
}
3842
}
3943

4044
function isSignal(symbol: ts.Symbol|undefined): boolean {
41-
return (symbol?.escapedName === 'WritableSignal' || symbol?.escapedName === 'Signal' ||
42-
symbol?.escapedName === 'InputSignal' ||
43-
symbol?.escapedName === 'InputSignalWithTransform' ||
44-
symbol?.escapedName === 'ModelSignal') &&
45-
(symbol as any).parent.escapedName.includes('@angular/core');
45+
const declarations = symbol?.getDeclarations();
46+
47+
return declarations !== undefined && declarations.some(decl => {
48+
const fileName = decl.getSourceFile().fileName;
49+
50+
return (ts.isInterfaceDeclaration(decl) || ts.isTypeAliasDeclaration(decl)) &&
51+
SIGNAL_FNS.has(decl.name.text) &&
52+
(fileName.includes('@angular/core') || fileName.includes('angular2/rc/packages/core'));
53+
});
4654
}
4755

4856
function buildDiagnosticForSignal(
@@ -55,7 +63,6 @@ function buildDiagnosticForSignal(
5563
(isSignal(symbol.tsType.symbol) || isSignal(symbol.tsType.aliasSymbol))) {
5664
const templateMapping =
5765
ctx.templateTypeChecker.getTemplateMappingAtTcbLocation(symbol.tcbLocation)!;
58-
5966
const errorString = `${node.name} is a function and should be invoked: ${node.name}()`;
6067
const diagnostic = ctx.makeTemplateDiagnostic(templateMapping.span, errorString);
6168
return [diagnostic];

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/interpolated_signal_not_invoked/interpolated_signal_not_invoked_spec.ts‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {factory as interpolatedSignalFactory} from '../../../checks/interpolated
1717
import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
1818

1919
runInEachFileSystem(() => {
20-
describe('Interpolated Signal ', () => {
20+
describe('Interpolated Signal', () => {
2121
it('binds the error code to its extended template diagnostic name', () => {
2222
expect(interpolatedSignalFactory.code).toBe(ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED);
2323
expect(interpolatedSignalFactory.name)
@@ -51,7 +51,7 @@ runInEachFileSystem(() => {
5151
});
5252
});
5353

54-
it('should produce a warning when a signal isn\'t invoked', () => {
54+
it('should produce a warning when a signal is not invoked', () => {
5555
const fileName = absoluteFrom('/main.ts');
5656
const {program, templateTypeChecker} = setup([
5757
{
@@ -81,7 +81,7 @@ runInEachFileSystem(() => {
8181
expect(getSourceCodeForDiagnostic(diags[1])).toBe(`mySignal2`);
8282
});
8383

84-
it('should produce a warning when a readonly signal isn\'t invoked', () => {
84+
it('should produce a warning when a readonly signal is not invoked', () => {
8585
const fileName = absoluteFrom('/main.ts');
8686
const {program, templateTypeChecker} = setup([
8787
{
@@ -109,7 +109,7 @@ runInEachFileSystem(() => {
109109
expect(getSourceCodeForDiagnostic(diags[0])).toBe('count');
110110
});
111111

112-
it('should produce a warning when a computed signal isn\'t invoked', () => {
112+
it('should produce a warning when a computed signal is not invoked', () => {
113113
const fileName = absoluteFrom('/main.ts');
114114
const {program, templateTypeChecker} = setup([
115115
{
@@ -138,7 +138,7 @@ runInEachFileSystem(() => {
138138
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`mySignal2`);
139139
});
140140

141-
it('should produce a warning when an input signal isn\'t invoked', () => {
141+
it('should produce a warning when an input signal is not invoked', () => {
142142
const fileName = absoluteFrom('/main.ts');
143143
const {program, templateTypeChecker} = setup([
144144
{
@@ -166,7 +166,7 @@ runInEachFileSystem(() => {
166166
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myInput`);
167167
});
168168

169-
it('should produce a warning when a required input signal isn\'t invoked', () => {
169+
it('should produce a warning when a required input signal is not invoked', () => {
170170
const fileName = absoluteFrom('/main.ts');
171171
const {program, templateTypeChecker} = setup([
172172
{
@@ -194,7 +194,7 @@ runInEachFileSystem(() => {
194194
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myRequiredInput`);
195195
});
196196

197-
it('should produce a warning when a model signal isn\'t invoked', () => {
197+
it('should produce a warning when a model signal is not invoked', () => {
198198
const fileName = absoluteFrom('/main.ts');
199199
const {program, templateTypeChecker} = setup([
200200
{
@@ -213,7 +213,7 @@ runInEachFileSystem(() => {
213213
const sf = getSourceFileOrError(program, fileName);
214214
const component = getClass(sf, 'TestCmp');
215215
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
216-
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
216+
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
217217
);
218218
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
219219
expect(diags.length).toBe(1);
@@ -222,7 +222,7 @@ runInEachFileSystem(() => {
222222
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myModel`);
223223
});
224224

225-
it('should produce a warning when a required model signal isn\'t invoked', () => {
225+
it('should produce a warning when a required model signal is not invoked', () => {
226226
const fileName = absoluteFrom('/main.ts');
227227
const {program, templateTypeChecker} = setup([
228228
{
@@ -241,7 +241,7 @@ runInEachFileSystem(() => {
241241
const sf = getSourceFileOrError(program, fileName);
242242
const component = getClass(sf, 'TestCmp');
243243
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
244-
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
244+
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
245245
);
246246
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
247247
expect(diags.length).toBe(1);
@@ -250,6 +250,40 @@ runInEachFileSystem(() => {
250250
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myRequiredModel`);
251251
});
252252

253+
it('should produce a warning when a signal in a nested property read is not invoked', () => {
254+
const fileName = absoluteFrom('/main.ts');
255+
const {program, templateTypeChecker} = setup([
256+
{
257+
fileName,
258+
templates: {
259+
'TestCmp': `<div>{{ obj.nested.prop.signal }}</div>`,
260+
},
261+
source: `
262+
import {signal, Signal} from '@angular/core';
263+
264+
export class TestCmp {
265+
obj = {
266+
nested: {
267+
prop: {
268+
signal: signal<number>(0)
269+
}
270+
}
271+
}
272+
}`,
273+
},
274+
]);
275+
const sf = getSourceFileOrError(program, fileName);
276+
const component = getClass(sf, 'TestCmp');
277+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
278+
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
279+
);
280+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
281+
expect(diags.length).toBe(1);
282+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
283+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED));
284+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`signal`);
285+
});
286+
253287
it('should not produce a warning when model signals are invoked', () => {
254288
const fileName = absoluteFrom('/main.ts');
255289
const {program, templateTypeChecker} = setup([
@@ -270,7 +304,7 @@ runInEachFileSystem(() => {
270304
const sf = getSourceFileOrError(program, fileName);
271305
const component = getClass(sf, 'TestCmp');
272306
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
273-
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
307+
templateTypeChecker, program.getTypeChecker(), [interpolatedSignalFactory], {} /* options */
274308
);
275309
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
276310
expect(diags.length).toBe(0);
@@ -328,7 +362,7 @@ runInEachFileSystem(() => {
328362
expect(diags.length).toBe(0);
329363
});
330364

331-
it('should produce a warning when signal isn\'t invoked on interpolated binding', () => {
365+
it('should produce a warning when signal is not invoked on interpolated binding', () => {
332366
const fileName = absoluteFrom('/main.ts');
333367
const {program, templateTypeChecker} = setup([
334368
{
@@ -436,7 +470,7 @@ runInEachFileSystem(() => {
436470
expect(diags.length).toBe(0);
437471
});
438472

439-
it('should produce a warning when nested signal isn\'t invoked on interpolated binding', () => {
473+
it('should produce a warning when nested signal is not invoked on interpolated binding', () => {
440474
const fileName = absoluteFrom('/main.ts');
441475
const {program, templateTypeChecker} = setup([
442476
{

0 commit comments

Comments
 (0)