Skip to content

fix(compiler-cli): detect uninvoked signals in bound expressions using ternary #68079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cexbrayat wants to merge 1 commit into
base: main
Choose a base branch
from
+162 −11
Open

fix(compiler-cli): detect uninvoked signals in bound expressions using ternary #68079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions ...compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import {
AST,
ASTWithSource,
BindingType,
Conditional,
Interpolation,
NonNullAssert,
ParenthesizedExpression,
PrefixNot,
PropertyRead,
TmplAstBoundAttribute,
Expand Down Expand Up @@ -120,7 +123,11 @@ function checkBoundAttribute(
}

// otherwise, we check if the node is
const nodeAst = isPropertyReadNodeAst(node);
if (node.value instanceof ASTWithSource === false) {
return [];
}
const propertyReads = getPropertyReads(node.value.ast);

if (
// a bound property like `[prop]="mySignal"`
(node.type === BindingType.Property ||
Expand All @@ -134,25 +141,45 @@ function checkBoundAttribute(
node.type === BindingType.Animation ||
// or an animation binding like `[@myAnimation]="mySignal"`
node.type === BindingType.LegacyAnimation) &&
nodeAst
propertyReads.length > 0
) {
return buildDiagnosticForSignal(ctx, nodeAst, component);
return propertyReads.flatMap((nodeAst) => buildDiagnosticForSignal(ctx, nodeAst, component));
}

return [];
}

function isPropertyReadNodeAst(node: TmplAstBoundAttribute): PropertyRead | undefined {
if (node.value instanceof ASTWithSource === false) {
return undefined;
function getPropertyReads(ast: AST): PropertyRead[] {
// Handle unary negation, such as `!mySignal`.
if (ast instanceof PrefixNot) {
return ast.expression instanceof PropertyRead ? [ast.expression] : [];
}
if (node.value.ast instanceof PrefixNot && node.value.ast.expression instanceof PropertyRead) {
return node.value.ast.expression;

// Handle direct reads, such as `mySignal`.
if (ast instanceof PropertyRead) {
return [ast];
}
if (node.value.ast instanceof PropertyRead) {
return node.value.ast;

// Handle ternary expressions, such as `flag ? mySignal : otherSignal`.
if (ast instanceof Conditional) {
return [
...getPropertyReads(ast.condition),
...getPropertyReads(ast.trueExp),
...getPropertyReads(ast.falseExp),
];
}
return undefined;

// Handle parenthesized expressions, such as `(mySignal)`.
if (ast instanceof ParenthesizedExpression) {
return getPropertyReads(ast.expression);
}

// Handle non-null assertions, such as `mySignal!`.
if (ast instanceof NonNullAssert) {
return getPropertyReads(ast.expression);
}

return [];
}

function isFunctionInstanceProperty(name: string): boolean {
Expand Down
124 changes: 124 additions & 0 deletions ...ended/test/checks/interpolated_signal_not_invoked/interpolated_signal_not_invoked_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,130 @@ runInEachFileSystem(() => {
});
});

it('should produce a warning when a signal is not invoked in a conditional style binding branch', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [style.width]="width() ? 1 : width"></div>`,
Copy link
Copy Markdown
Member

@JeanMeche JeanMeche Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be picked up by the typechecker ?

Copy link
Copy Markdown
Member Author

@cexbrayat cexbrayat Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think the type-checker can catch this, because checkTypeOfDomBindings is currently always false (even with strictTemplates)

Copy link
Copy Markdown
Member

@JeanMeche JeanMeche Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but is it a reason to fix it here ?
This diagnostic should only address cases where the type system cannot see the problem.

Copy link
Copy Markdown
Member Author

@cexbrayat cexbrayat Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is, as we don't have a compiler that checks dom bindings (and we'll maybe never have one?). Even if it was, the error message would not be as precise as the one coming from the diagnostic.

This is a real issue we encountered in a migrated app, so I thought it would be worth it to spare the pain to other developers.

},
source: `
import {signal} from '@angular/core';

export class TestCmp {
width = signal<number>(0);
}`,
},
]);
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(`width`);
});

it('should produce a warning when a signal is not invoked in a parenthesized style binding expression', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [style.width]="(width)"></div>`,
},
source: `
import {signal} from '@angular/core';

export class TestCmp {
width = signal<number>(0);
}`,
},
]);
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(`width`);
});

it('should produce a warning when a signal is not invoked in a non-null asserted style binding expression', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [style.width]="width!"></div>`,
},
source: `
import {signal} from '@angular/core';

export class TestCmp {
width = signal<number>(0);
}`,
},
]);
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(`width`);
});

it('should produce a warning when a signal is not invoked in a prefixed-not property binding expression', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<button [disabled]="!isEnabled"></button>`,
Copy link
Copy Markdown
Member

@JeanMeche JeanMeche Apr 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems a good case for the diagnostic to cover !

},
source: `
import {signal} from '@angular/core';

export class TestCmp {
isEnabled = signal<boolean>(true);
}`,
},
]);
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(`isEnabled`);
});

it('should not produce a warning with other Signal type', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
Expand Down
Loading