Skip to content

Commit 4f4f410

Browse files
cexbrayatdylhunn
cexbrayat
authored and
dylhunn
committed
fix(compiler-cli): dom property binding check in signal extended diagnostic (#54324)
The compiler now checks if a signal is properly called on dom property bindings. The ideal solution would be for the compiler to check if dom property bindings in general are properly typed, but this is currently not the case, and it is a bigger task to land this change. In the meantime, the signal diagnostic is augmented to catch cases like the following: ``` <div [id]="mySignal"></div> ``` PR Close #54324
1 parent c881c3e commit 4f4f410

File tree

2 files changed

+165
-5
lines changed

2 files changed

+165
-5
lines changed

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,15 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {AST, Interpolation, PropertyRead, TmplAstNode} from '@angular/compiler';
9+
import {
10+
AST,
11+
ASTWithSource,
12+
BindingType,
13+
Interpolation,
14+
PropertyRead,
15+
TmplAstBoundAttribute,
16+
TmplAstNode,
17+
} from '@angular/compiler';
1018
import ts from 'typescript';
1119

1220
import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
@@ -34,11 +42,28 @@ class InterpolatedSignalCheck extends TemplateCheckWithVisitor<ErrorCode.INTERPO
3442
component: ts.ClassDeclaration,
3543
node: TmplAstNode | AST,
3644
): NgTemplateDiagnostic<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>[] {
45+
// interpolations like `{{ mySignal }}`
3746
if (node instanceof Interpolation) {
3847
return node.expressions
3948
.filter((item): item is PropertyRead => item instanceof PropertyRead)
4049
.flatMap((item) => buildDiagnosticForSignal(ctx, item, component));
4150
}
51+
// bound properties like `[prop]="mySignal"`
52+
else if (node instanceof TmplAstBoundAttribute) {
53+
const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component);
54+
// we skip the check if the node is an input binding
55+
if (symbol !== null && symbol.kind === SymbolKind.Input) {
56+
return [];
57+
}
58+
// otherwise, we check if the node is a bound property like `[prop]="mySignal"`
59+
if (
60+
node.type === BindingType.Property &&
61+
node.value instanceof ASTWithSource &&
62+
node.value.ast instanceof PropertyRead
63+
) {
64+
return buildDiagnosticForSignal(ctx, node.value.ast, component);
65+
}
66+
}
4267
return [];
4368
}
4469
}

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

Lines changed: 139 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ runInEachFileSystem(() => {
6868
6969
export class TestCmp {
7070
mySignal1 = signal<number>(0);
71-
mySignal2:Signal<number>;
71+
mySignal2: Signal<number>;
7272
}`,
7373
},
7474
]);
@@ -128,7 +128,7 @@ runInEachFileSystem(() => {
128128
'TestCmp': `<div>{{ mySignal2 }}</div>`,
129129
},
130130
source: `
131-
import {signal, Signal, computed} from '@angular/core';
131+
import {signal, computed} from '@angular/core';
132132
133133
export class TestCmp {
134134
mySignal1 = signal<number>(0);
@@ -275,6 +275,82 @@ runInEachFileSystem(() => {
275275
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myRequiredModel`);
276276
});
277277

278+
it('should not produce a warning when a signal is not invoked in a banana in box binding', () => {
279+
const fileName = absoluteFrom('/main.ts');
280+
const {program, templateTypeChecker} = setup([
281+
{
282+
fileName,
283+
templates: {
284+
'TestCmp': `<div [(value)]="signal">{{ myRequiredModel }}</div>`,
285+
},
286+
source: `
287+
import {signal} from '@angular/core';
288+
289+
export class TestCmp {
290+
mySignal = signal(0);
291+
}`,
292+
},
293+
]);
294+
const sf = getSourceFileOrError(program, fileName);
295+
const component = getClass(sf, 'TestCmp');
296+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
297+
templateTypeChecker,
298+
program.getTypeChecker(),
299+
[interpolatedSignalFactory],
300+
{} /* options */,
301+
);
302+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
303+
expect(diags.length).toBe(0);
304+
});
305+
306+
it('should not produce a warning when a signal is not invoked in an input binding as they are skipped', () => {
307+
const fileName = absoluteFrom('/main.ts');
308+
const {program, templateTypeChecker} = setup([
309+
{
310+
fileName,
311+
templates: {
312+
'TestCmp': `<div dir [myInput]="mySignal"></div>`,
313+
},
314+
source: `
315+
import {signal, input} from '@angular/core';
316+
317+
export class TestDir {
318+
myInput = input.required();
319+
}
320+
export class TestCmp {
321+
mySignal = signal(0);
322+
}`,
323+
declarations: [
324+
{
325+
type: 'directive',
326+
name: 'TestDir',
327+
selector: '[dir]',
328+
inputs: {
329+
myInput: {
330+
isSignal: true,
331+
bindingPropertyName: 'myInput',
332+
classPropertyName: 'myInput',
333+
required: true,
334+
transform: null,
335+
},
336+
},
337+
},
338+
],
339+
},
340+
]);
341+
const sf = getSourceFileOrError(program, fileName);
342+
const component = getClass(sf, 'TestCmp');
343+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
344+
templateTypeChecker,
345+
program.getTypeChecker(),
346+
[interpolatedSignalFactory],
347+
{},
348+
/* options */
349+
);
350+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
351+
expect(diags.length).toBe(0);
352+
});
353+
278354
it('should produce a warning when a signal in a nested property read is not invoked', () => {
279355
const fileName = absoluteFrom('/main.ts');
280356
const {program, templateTypeChecker} = setup([
@@ -284,7 +360,7 @@ runInEachFileSystem(() => {
284360
'TestCmp': `<div>{{ obj.nested.prop.signal }}</div>`,
285361
},
286362
source: `
287-
import {signal, Signal} from '@angular/core';
363+
import {signal} from '@angular/core';
288364
289365
export class TestCmp {
290366
obj = {
@@ -350,7 +426,7 @@ runInEachFileSystem(() => {
350426
'TestCmp': `<div>{{ mySignal2() }}</div>`,
351427
},
352428
source: `
353-
import {signal, Signal, computed} from '@angular/core';
429+
import {signal, computed} from '@angular/core';
354430
355431
export class TestCmp {
356432
mySignal1 = signal<number>(0);
@@ -549,6 +625,65 @@ runInEachFileSystem(() => {
549625
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myNestedSignal`);
550626
});
551627

628+
it("should produce a warning when signal isn't invoked on dom property binding", () => {
629+
const fileName = absoluteFrom('/main.ts');
630+
const {program, templateTypeChecker} = setup([
631+
{
632+
fileName,
633+
templates: {
634+
'TestCmp': `<div [id]="mySignal"></div>`,
635+
},
636+
source: `
637+
import {signal} from '@angular/core';
638+
639+
export class TestCmp {
640+
mySignal = signal<number>(0);
641+
}`,
642+
},
643+
]);
644+
const sf = getSourceFileOrError(program, fileName);
645+
const component = getClass(sf, 'TestCmp');
646+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
647+
templateTypeChecker,
648+
program.getTypeChecker(),
649+
[interpolatedSignalFactory],
650+
{} /* options */,
651+
);
652+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
653+
expect(diags.length).toBe(1);
654+
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
655+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED));
656+
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`mySignal`);
657+
});
658+
659+
it('should not produce a warning when signal is invoked on dom property binding', () => {
660+
const fileName = absoluteFrom('/main.ts');
661+
const {program, templateTypeChecker} = setup([
662+
{
663+
fileName,
664+
templates: {
665+
'TestCmp': `<div [id]="mySignal()"></div>`,
666+
},
667+
source: `
668+
import {signal} from '@angular/core';
669+
670+
export class TestCmp {
671+
mySignal = signal<number>(0);
672+
}`,
673+
},
674+
]);
675+
const sf = getSourceFileOrError(program, fileName);
676+
const component = getClass(sf, 'TestCmp');
677+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
678+
templateTypeChecker,
679+
program.getTypeChecker(),
680+
[interpolatedSignalFactory],
681+
{} /* options */,
682+
);
683+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
684+
expect(diags.length).toBe(0);
685+
});
686+
552687
it('should not produce a warning with other Signal type', () => {
553688
const fileName = absoluteFrom('/main.ts');
554689
const {program, templateTypeChecker} = setup([

0 commit comments

Comments
 (0)