feat(compiler): Add extended diagnostic to warn when there are uncalled functions in event bindings (#56295)

The diagnostic will catch issues like:

```html
<button (click)="increment"></button>
<button (click)="increment; decrement"></button>
<button (click)="true ? increment : decrement"></button>
<button (click)="nested.nested1.nested2.increment"></button>
```

PR Close #56295
This commit is contained in:
Enea Jahollari 2024-06-06 01:57:09 +02:00 committed by Jessica Janiuk
parent a0fbd0e5e1
commit fd6cd0422d
13 changed files with 486 additions and 12 deletions

View file

@ -1388,6 +1388,11 @@ const REFERENCE_SUB_NAVIGATION_DATA: NavigationItem[] = [
path: 'extended-diagnostics/NG8109',
contentPath: 'reference/extended-diagnostics/NG8109',
},
{
label: 'NG8111: Functions must be invoked in event bindings',
path: 'extended-diagnostics/NG8111',
contentPath: 'reference/extended-diagnostics/NG8111',
},
],
},
{

View file

@ -0,0 +1,65 @@
# Functions should be invoked in event bindings.
This diagnostic detects uninvoked functions in event bindings.
<docs-code language="typescript">
import {Component, signal, Signal} from '@angular/core';
@Component({
template: `<button (click)="onClick">Click me</button>`,
})
class MyComponent {
onClick() {
console.log('clicked');
}
}
</docs-code>
## What's wrong with that?
Functions in event bindings should be invoked when the event is triggered.
If the function is not invoked, it will not execute when the event is triggered.
## What should I do instead?
Ensure to invoke the function when you use it in an event binding to execute the function when the event is triggered.
<docs-code language="typescript">
import {Component} from '@angular/core';
@Component({
template: `<button (click)="onClick()">Click me</button>`,
})
class MyComponent {
onClick() {
console.log('clicked');
}
}
</docs-code>
## Configuration requirements
[`strictTemplates`](tools/cli/template-typecheck#strict-mode) must be enabled for any extended diagnostic to emit.
`uninvokedFunctionInEventBinding` has no additional requirements beyond `strictTemplates`.
## What if I can't avoid this?
This diagnostic can be disabled by editing the project's `tsconfig.json` file:
<docs-code language="json">
{
"angularCompilerOptions": {
"extendedDiagnostics": {
"checks": {
"uninvokedFunctionInEventBinding": "suppress"
}
}
}
}
</docs-code>
See [extended diagnostic configuration](extended-diagnostics#configuration) for more info.

View file

@ -8,17 +8,18 @@ The Angular compiler includes "extended diagnostics" which identify many of thes
Currently, Angular supports the following extended diagnostics:
| Code | Name |
|:--- |:--- |
| `NG8101` | [`invalidBananaInBox`](extended-diagnostics/NG8101) |
| `NG8102` | [`nullishCoalescingNotNullable`](extended-diagnostics/NG8102) |
| `NG8103` | [`missingControlFlowDirective`](extended-diagnostics/NG8103) |
| `NG8104` | [`textAttributeNotBinding`](extended-diagnostics/NG8104) |
| `NG8105` | [`missingNgForOfLet`](extended-diagnostics/NG8105) |
| `NG8106` | [`suffixNotSupported`](extended-diagnostics/NG8106) |
| `NG8107` | [`optionalChainNotNullable`](extended-diagnostics/NG8107) |
| `NG8108` | [`skipHydrationNotStatic`](extended-diagnostics/NG8108) |
| `NG8109` | [`interpolatedSignalNotInvoked`](extended-diagnostics/NG8109) |
| Code | Name |
|:---------|:-----------------------------------------------------------------|
| `NG8101` | [`invalidBananaInBox`](extended-diagnostics/NG8101) |
| `NG8102` | [`nullishCoalescingNotNullable`](extended-diagnostics/NG8102) |
| `NG8103` | [`missingControlFlowDirective`](extended-diagnostics/NG8103) |
| `NG8104` | [`textAttributeNotBinding`](extended-diagnostics/NG8104) |
| `NG8105` | [`missingNgForOfLet`](extended-diagnostics/NG8105) |
| `NG8106` | [`suffixNotSupported`](extended-diagnostics/NG8106) |
| `NG8107` | [`optionalChainNotNullable`](extended-diagnostics/NG8107) |
| `NG8108` | [`skipHydrationNotStatic`](extended-diagnostics/NG8108) |
| `NG8109` | [`interpolatedSignalNotInvoked`](extended-diagnostics/NG8109) |
| `NG8111` | [`uninvokedFunctionInEventBinding`](extended-diagnostics/NG8111) |
## Configuration

View file

@ -103,6 +103,7 @@ export enum ErrorCode {
TEXT_ATTRIBUTE_NOT_BINDING = 8104,
UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007,
UNDECORATED_PROVIDER = 2005,
UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
UNSUPPORTED_INITIALIZER_API_USAGE = 8110,
// (undocumented)
VALUE_HAS_WRONG_TYPE = 1010,

View file

@ -25,7 +25,9 @@ export enum ExtendedTemplateDiagnosticName {
// (undocumented)
SUFFIX_NOT_SUPPORTED = "suffixNotSupported",
// (undocumented)
TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding"
TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding",
// (undocumented)
UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding"
}
// (No @packageDocumentation comment for this package)

View file

@ -482,6 +482,19 @@ export enum ErrorCode {
*/
UNSUPPORTED_INITIALIZER_API_USAGE = 8110,
/**
* A function in an event binding is not called.
*
* For example:
* ```
* <button (click)="myFunc"></button>
* ```
*
* This will not call `myFunc` when the button is clicked. Instead, it should be
* `<button (click)="myFunc()"></button>`.
*/
UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
/**
* The template type-checking engine would need to generate an inline type check block for a
* component, but the current type-checking environment doesn't support it.

View file

@ -21,6 +21,7 @@ export enum ExtendedTemplateDiagnosticName {
OPTIONAL_CHAIN_NOT_NULLABLE = 'optionalChainNotNullable',
MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective',
TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding',
UNINVOKED_FUNCTION_IN_EVENT_BINDING = 'uninvokedFunctionInEventBinding',
MISSING_NGFOROF_LET = 'missingNgForOfLet',
SUFFIX_NOT_SUPPORTED = 'suffixNotSupported',
SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic',

View file

@ -21,6 +21,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/skip_hydration_not_static",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/suffix_not_supported",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding",
"@npm//typescript",
],
)

View file

@ -0,0 +1,17 @@
load("//tools:defaults.bzl", "ts_library")
ts_library(
name = "uninvoked_function_in_event_binding",
srcs = ["index.ts"],
visibility = [
"//packages/compiler-cli/src/ngtsc:__subpackages__",
"//packages/compiler-cli/test/ngtsc:__pkg__",
],
deps = [
"//packages/compiler",
"//packages/compiler-cli/src/ngtsc/diagnostics",
"//packages/compiler-cli/src/ngtsc/typecheck/api",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
"@npm//typescript",
],
)

View file

@ -0,0 +1,114 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {
AST,
ASTWithSource,
Call,
Chain,
Conditional,
ParsedEventType,
PropertyRead,
SafeCall,
SafePropertyRead,
TmplAstBoundEvent,
TmplAstNode,
} from '@angular/compiler';
import ts from 'typescript';
import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
import {NgTemplateDiagnostic, SymbolKind} from '../../../api';
import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
/**
* Ensures that function in event bindings are called. For example, `<button (click)="myFunc"></button>`
* will not call `myFunc` when the button is clicked. Instead, it should be `<button (click)="myFunc()"></button>`.
* This is likely not the intent of the developer. Instead, the intent is likely to call `myFunc`.
*/
class UninvokedFunctionInEventBindingSpec extends TemplateCheckWithVisitor<ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING> {
override code = ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING as const;
override visitNode(
ctx: TemplateContext<ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING>,
component: ts.ClassDeclaration,
node: TmplAstNode | AST,
): NgTemplateDiagnostic<ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING>[] {
// If the node is not a bound event, skip it.
if (!(node instanceof TmplAstBoundEvent)) return [];
// If the node is not a regular or animation event, skip it.
if (node.type !== ParsedEventType.Regular && node.type !== ParsedEventType.Animation) return [];
if (!(node.handler instanceof ASTWithSource)) return [];
const sourceExpressionText = node.handler.source || '';
if (node.handler.ast instanceof Chain) {
// (click)="increment; decrement"
return node.handler.ast.expressions.flatMap((expression) =>
assertExpressionInvoked(expression, component, node, sourceExpressionText, ctx),
);
}
if (node.handler.ast instanceof Conditional) {
// (click)="true ? increment : decrement"
const {trueExp, falseExp} = node.handler.ast;
return [trueExp, falseExp].flatMap((expression) =>
assertExpressionInvoked(expression, component, node, sourceExpressionText, ctx),
);
}
// (click)="increment"
return assertExpressionInvoked(node.handler.ast, component, node, sourceExpressionText, ctx);
}
}
/**
* Asserts that the expression is invoked.
* If the expression is a property read, and it has a call signature, a diagnostic is generated.
*/
function assertExpressionInvoked(
expression: AST,
component: ts.ClassDeclaration,
node: TmplAstBoundEvent,
expressionText: string,
ctx: TemplateContext<ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING>,
): NgTemplateDiagnostic<ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING>[] {
if (expression instanceof Call || expression instanceof SafeCall) {
return []; // If the method is called, skip it.
}
if (!(expression instanceof PropertyRead) && !(expression instanceof SafePropertyRead)) {
return []; // If the expression is not a property read, skip it.
}
const symbol = ctx.templateTypeChecker.getSymbolOfNode(expression, component);
if (symbol !== null && symbol.kind === SymbolKind.Expression) {
if (symbol.tsType.getCallSignatures()?.length > 0) {
const fullExpressionText = generateStringFromExpression(expression, expressionText);
const errorString = `Function in event binding should be invoked: ${fullExpressionText}()`;
return [ctx.makeTemplateDiagnostic(node.sourceSpan, errorString)];
}
}
return [];
}
function generateStringFromExpression(expression: AST, source: string): string {
return source.substring(expression.span.start, expression.span.end);
}
export const factory: TemplateCheckFactory<
ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING,
ExtendedTemplateDiagnosticName.UNINVOKED_FUNCTION_IN_EVENT_BINDING
> = {
code: ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING,
name: ExtendedTemplateDiagnosticName.UNINVOKED_FUNCTION_IN_EVENT_BINDING,
create: () => new UninvokedFunctionInEventBindingSpec(),
};

View file

@ -17,6 +17,7 @@ import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_c
import {factory as optionalChainNotNullableFactory} from './checks/optional_chain_not_nullable';
import {factory as suffixNotSupportedFactory} from './checks/suffix_not_supported';
import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding';
import {factory as uninvokedFunctionInEventBindingFactory} from './checks/uninvoked_function_in_event_binding';
export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker';
@ -32,6 +33,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory<
missingNgForOfLetFactory,
suffixNotSupportedFactory,
interpolatedSignalNotInvoked,
uninvokedFunctionInEventBindingFactory,
];
export const SUPPORTED_DIAGNOSTIC_NAMES = new Set<string>([

View file

@ -0,0 +1,30 @@
load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
ts_library(
name = "test_lib",
testonly = True,
srcs = ["uninvoked_function_in_event_binding_spec.ts"],
deps = [
"//packages/compiler",
"//packages/compiler-cli/src/ngtsc/core:api",
"//packages/compiler-cli/src/ngtsc/diagnostics",
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/file_system/testing",
"//packages/compiler-cli/src/ngtsc/testing",
"//packages/compiler-cli/src/ngtsc/typecheck/extended",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding",
"//packages/compiler-cli/src/ngtsc/typecheck/testing",
"@npm//typescript",
],
)
jasmine_node_test(
name = "test",
bootstrap = ["//tools/testing:node_no_angular"],
data = [
"//packages/core:npm_package",
],
deps = [
":test_lib",
],
)

View file

@ -0,0 +1,222 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import ts from 'typescript';
import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
import {runInEachFileSystem} from '../../../../../file_system/testing';
import {getSourceCodeForDiagnostic} from '../../../../../testing';
import {getClass, setup} from '../../../../testing';
import {factory as uninvokedFunctionInEventBindingFactory} from '../../../checks/uninvoked_function_in_event_binding';
import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
runInEachFileSystem(() => {
describe('UninvokedFunctionInEventBindingFactoryCheck', () => {
it('binds the error code to its extended template diagnostic name', () => {
expect(uninvokedFunctionInEventBindingFactory.code).toBe(
ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING,
);
expect(uninvokedFunctionInEventBindingFactory.name).toBe(
ExtendedTemplateDiagnosticName.UNINVOKED_FUNCTION_IN_EVENT_BINDING,
);
});
it('should produce a diagnostic when a function in an event binding is not invoked', () => {
const diags = setupTestComponent(`<button (click)="increment"></button>`, `increment() { }`);
expect(diags.length).toBe(1);
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNINVOKED_FUNCTION_IN_EVENT_BINDING));
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`(click)="increment"`);
expect(diags[0].messageText).toBe(generateDiagnosticText('increment()'));
});
it('should produce a diagnostic when a nested function in an event binding is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="nested.nested1.nested2.increment"></button>`,
`nested = { nested1: { nested2: { increment() { } } } }`,
);
expect(diags.length).toBe(1);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(
`(click)="nested.nested1.nested2.increment"`,
);
expect(diags[0].messageText).toBe(
generateDiagnosticText('nested.nested1.nested2.increment()'),
);
});
it('should produce a diagnostic when a nested function that uses key read in an event binding is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="nested.nested1['nested2'].increment"></button>`,
`nested = { nested1: { nested2: { increment() { } } } }`,
);
expect(diags.length).toBe(1);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(
`(click)="nested.nested1['nested2'].increment"`,
);
expect(diags[0].messageText).toBe(
generateDiagnosticText(`nested.nested1['nested2'].increment()`),
);
});
it('should produce a diagnostic when a function in a chain is not invoked', () => {
const diags = setupTestComponent(
`
<button (click)="increment; decrement"></button>
<button (click)="increment; decrement()"></button>
<button (click)="increment(); decrement"></button>
`,
`increment() { } decrement() { }`,
);
expect(diags.length).toBe(4);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`(click)="increment; decrement"`);
expect(diags[0].messageText).toBe(generateDiagnosticText('increment()'));
expect(getSourceCodeForDiagnostic(diags[1])).toBe(`(click)="increment; decrement"`);
expect(diags[1].messageText).toBe(generateDiagnosticText('decrement()'));
expect(getSourceCodeForDiagnostic(diags[2])).toBe(`(click)="increment; decrement()"`);
expect(diags[2].messageText).toBe(generateDiagnosticText('increment()'));
expect(getSourceCodeForDiagnostic(diags[3])).toBe(`(click)="increment(); decrement"`);
expect(diags[3].messageText).toBe(generateDiagnosticText('decrement()'));
});
it('should produce a diagnostic when a function in a conditional is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="true ? increment : decrement"></button>`,
`increment() { } decrement() { }`,
);
expect(diags.length).toBe(2);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`(click)="true ? increment : decrement"`);
expect(diags[0].messageText).toBe(generateDiagnosticText('increment()'));
expect(getSourceCodeForDiagnostic(diags[1])).toBe(`(click)="true ? increment : decrement"`);
expect(diags[1].messageText).toBe(generateDiagnosticText('decrement()'));
});
it('should produce a diagnostic when a function in a conditional is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="true ? increment() : decrement"></button>`,
`increment() { } decrement() { }`,
);
expect(diags.length).toBe(1);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`(click)="true ? increment() : decrement"`);
expect(diags[0].messageText).toBe(generateDiagnosticText('decrement()'));
});
it('should produce a diagnostic when a nested function in a conditional is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="true ? counter.increment : nested['nested1'].nested2?.source().decrement"></button>`,
`
counter = { increment() { } }
nested = { nested1: { nested2?: { source() { return { decrement() } { } } } } }
`,
);
expect(diags.length).toBe(2);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(
`(click)="true ? counter.increment : nested['nested1'].nested2?.source().decrement"`,
);
expect(diags[0].messageText).toBe(generateDiagnosticText('counter.increment()'));
expect(getSourceCodeForDiagnostic(diags[1])).toBe(
`(click)="true ? counter.increment : nested['nested1'].nested2?.source().decrement"`,
);
expect(diags[1].messageText).toBe(
generateDiagnosticText(`nested['nested1'].nested2?.source().decrement()`),
);
});
it('should produce a diagnostic when a function in a function is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="nested.nested1.nested2.source().decrement"></button>`,
`nested = { nested1: { nested2: { source() { return { decrement() { } } } } } }`,
);
expect(diags.length).toBe(1);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(
`(click)="nested.nested1.nested2.source().decrement"`,
);
expect(diags[0].messageText).toBe(
generateDiagnosticText('nested.nested1.nested2.source().decrement()'),
);
});
it('should produce a diagnostic when a function that returns a function is not invoked', () => {
const diags = setupTestComponent(
`<button (click)="incrementAndLaterDecrement"></button>`,
`incrementAndLaterDecrement(): () => void { return () => {} }`,
);
expect(diags.length).toBe(1);
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`(click)="incrementAndLaterDecrement"`);
expect(diags[0].messageText).toBe(generateDiagnosticText('incrementAndLaterDecrement()'));
});
it('should not produce a diagnostic when an invoked function returns a function', () => {
const diags = setupTestComponent(
`<button (click)="incrementAndLaterDecrement()"></button>`,
`incrementAndLaterDecrement(): () => void { return () => {} }`,
);
expect(diags.length).toBe(0);
});
it('should not produce a warning when the function is not invoked in two-way-binding', () => {
const diags = setupTestComponent(
`<button [(event)]="increment"></button>`,
`increment() { }`,
);
expect(diags.length).toBe(0);
});
it('should not produce a warning when the function is invoked', () => {
const diags = setupTestComponent(
`
<button (click)="increment()"></button>
<button (click)="counter.increment()"></button>
<button (click)="increment?.()"></button>
`,
`
counter = { increment() { } }
increment() { }
`,
);
expect(diags.length).toBe(0);
});
});
});
function setupTestComponent(template: string, classField: string) {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {'TestCmp': template},
source: `export class TestCmp { ${classField} }`,
},
]);
const sf = getSourceFileOrError(program, fileName);
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker,
program.getTypeChecker(),
[uninvokedFunctionInEventBindingFactory],
{} /* options */,
);
return extendedTemplateChecker.getDiagnosticsForComponent(component);
}
function generateDiagnosticText(text: string): string {
return `Function in event binding should be invoked: ${text}`;
}