mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
fix(compiler-cli): do not flag callable objects with zero parameters in uninvoked track function check
In `@for` blocks, tracking callable objects by reference (e.g. signal forms `FieldTree`, signals, or custom callable objects) is a valid pattern when tracking by object identity. Previously, `UninvokedTrackFunctionCheck` (NG8115) flagged any property read whose type has call signatures, regardless of whether the target was an actual track function expecting arguments or a method reference. This commit updates `UninvokedTrackFunctionCheck` to only emit a diagnostic when the target expression has call signatures that declare parameters (functions/methods expecting arguments like `(item)` or `(index, item)`) or is a method declaration. Callable objects without parameters accessed as properties are now recognized as tracked values and not flagged as uninvoked track functions. Fixes #70207
This commit is contained in:
committed by
Kristiyan Kostadinov
parent
8227e5cf6d
commit
f0a271c7bd
+42
-10
@@ -59,18 +59,27 @@ class UninvokedTrackFunctionCheck extends TemplateCheckWithVisitor<ErrorCode.UNI
|
||||
|
||||
if (symbol !== null && symbol.kind === SymbolKind.Expression) {
|
||||
const type = ctx.templateTypeChecker.getTypeOfSymbol(symbol);
|
||||
if (type && type.getCallSignatures()?.length > 0) {
|
||||
const fullExpressionText = generateStringFromExpression(
|
||||
node.trackBy.ast,
|
||||
node.trackBy.source || '',
|
||||
);
|
||||
if (type) {
|
||||
const callSignatures = type.getCallSignatures();
|
||||
if (callSignatures.length > 0) {
|
||||
const hasParameters = callSignatures.some((sig) => sig.parameters.length > 0);
|
||||
const tsSymbol = ctx.templateTypeChecker.getTsSymbolOfSymbol(symbol);
|
||||
const isMethod = isMethodSymbol(tsSymbol, callSignatures);
|
||||
|
||||
const errorString = formatExtendedError(
|
||||
ErrorCode.UNINVOKED_TRACK_FUNCTION,
|
||||
`The track function in the @for block should be invoked: ${fullExpressionText}(/* arguments */)`,
|
||||
);
|
||||
if (hasParameters || isMethod) {
|
||||
const fullExpressionText = generateStringFromExpression(
|
||||
node.trackBy.ast,
|
||||
node.trackBy.source || '',
|
||||
);
|
||||
|
||||
return [ctx.makeTemplateDiagnostic(node.sourceSpan, errorString)];
|
||||
const errorString = formatExtendedError(
|
||||
ErrorCode.UNINVOKED_TRACK_FUNCTION,
|
||||
`The track function in the @for block should be invoked: ${fullExpressionText}(/* arguments */)`,
|
||||
);
|
||||
|
||||
return [ctx.makeTemplateDiagnostic(node.sourceSpan, errorString)];
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -78,6 +87,29 @@ class UninvokedTrackFunctionCheck extends TemplateCheckWithVisitor<ErrorCode.UNI
|
||||
}
|
||||
}
|
||||
|
||||
function isMethodSymbol(
|
||||
tsSymbol: ts.Symbol | null,
|
||||
callSignatures: readonly ts.Signature[],
|
||||
): boolean {
|
||||
if (tsSymbol !== null) {
|
||||
if ((tsSymbol.flags & ts.SymbolFlags.Method) !== 0) {
|
||||
return true;
|
||||
}
|
||||
const declarations = tsSymbol.getDeclarations();
|
||||
if (declarations !== undefined) {
|
||||
if (declarations.some((decl) => ts.isMethodDeclaration(decl) || ts.isMethodSignature(decl))) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return callSignatures.some(
|
||||
(sig) =>
|
||||
sig.declaration !== undefined &&
|
||||
(ts.isMethodDeclaration(sig.declaration) || ts.isMethodSignature(sig.declaration)),
|
||||
);
|
||||
}
|
||||
|
||||
function generateStringFromExpression(expression: AST, source: string): string {
|
||||
return source.substring(expression.span.start, expression.span.end);
|
||||
}
|
||||
|
||||
+1
@@ -23,5 +23,6 @@ jasmine_test(
|
||||
data = [
|
||||
":test_lib",
|
||||
"//packages/core:npm_package",
|
||||
"//packages/forms:npm_package",
|
||||
],
|
||||
)
|
||||
|
||||
+149
-9
@@ -8,6 +8,7 @@
|
||||
|
||||
import ts from 'typescript';
|
||||
|
||||
import {formatExtendedError} from '@angular/compiler-cli/src/ngtsc/typecheck/extended/api';
|
||||
import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
|
||||
import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
|
||||
import {runInEachFileSystem} from '../../../../../file_system/testing';
|
||||
@@ -15,7 +16,6 @@ import {getSourceCodeForDiagnostic} from '../../../../../testing';
|
||||
import {getClass, setup} from '../../../../testing';
|
||||
import {factory as uninvokedTrackFunctionCheckFactory} from '../../../checks/uninvoked_track_function';
|
||||
import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
|
||||
import {formatExtendedError} from '@angular/compiler-cli/src/ngtsc/typecheck/extended/api';
|
||||
|
||||
runInEachFileSystem(() => {
|
||||
describe('UninvokedTrackFunctionCheck', () => {
|
||||
@@ -75,23 +75,163 @@ runInEachFileSystem(() => {
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should not produce a warning when track is a FieldTree property', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (row of rows; track row.field) {}
|
||||
`,
|
||||
`rows!: {field: FieldTree<string>}[];`,
|
||||
`import type {FieldTree} from '@angular/forms/signals';`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should not produce a warning when track is a ReadonlyFieldTree property', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (row of rows; track row.field) {}
|
||||
`,
|
||||
`rows!: {field: ReadonlyFieldTree<string>}[];`,
|
||||
`import type {ReadonlyFieldTree} from '@angular/forms/signals';`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should not produce a warning when track is a Field property', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (row of rows; track row.field) {}
|
||||
`,
|
||||
`rows!: {field: Field<string>}[];`,
|
||||
`import type {Field} from '@angular/forms/signals';`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should not produce a warning when track is a nested FieldTree property', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (row of rows; track row.field.subField) {}
|
||||
`,
|
||||
`rows!: {field: FieldTree<{subField: string}>}[];`,
|
||||
`import type {FieldTree} from '@angular/forms/signals';`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should produce a warning when track is a regular function on an object', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (row of rows; track row.trackFn) {}
|
||||
`,
|
||||
`rows!: {trackFn: (item: any) => string}[];`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(1);
|
||||
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
|
||||
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNINVOKED_TRACK_FUNCTION));
|
||||
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`@for (row of rows; track row.trackFn) {}`);
|
||||
expect(diags[0].messageText).toBe(generateDiagnosticText('row.trackFn'));
|
||||
});
|
||||
|
||||
it('should not produce a warning when track is a simple callable object with no parameters', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (item of callableItems; track item) {}
|
||||
@for (row of rows; track row.item) {}
|
||||
`,
|
||||
`
|
||||
callableItems!: (((() => string) & {id: number})[]);
|
||||
rows!: {item: (() => string) & {id: number}}[];
|
||||
`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
|
||||
it('should produce a warning when track is a method on an item', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (item of itemsWithMethod; track item.getId) {}
|
||||
`,
|
||||
`itemsWithMethod!: {getId(): string}[];`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(1);
|
||||
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
|
||||
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNINVOKED_TRACK_FUNCTION));
|
||||
expect(getSourceCodeForDiagnostic(diags[0])).toBe(
|
||||
`@for (item of itemsWithMethod; track item.getId) {}`,
|
||||
);
|
||||
expect(diags[0].messageText).toBe(generateDiagnosticText('item.getId'));
|
||||
});
|
||||
|
||||
it('should produce a warning when track is an arrow function property on component', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (item of items; track trackFn) {}
|
||||
`,
|
||||
`trackFn = (item: any) => item.name;`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(1);
|
||||
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
|
||||
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNINVOKED_TRACK_FUNCTION));
|
||||
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`@for (item of items; track trackFn) {}`);
|
||||
expect(diags[0].messageText).toBe(generateDiagnosticText('trackFn'));
|
||||
});
|
||||
|
||||
it('should not produce a warning with signal forms field tracking patterns', () => {
|
||||
const diags = diagnoseTestComponent(
|
||||
`
|
||||
@for (email of emailsForm.emails; track email) {}
|
||||
@for (row of rows(); track row.field) {}
|
||||
`,
|
||||
`
|
||||
readonly model = signal({
|
||||
emails: ['john.doe@mail.com', 'max.musterman@mail.com'],
|
||||
});
|
||||
readonly emailsForm = form(this.model);
|
||||
|
||||
readonly rows = computed(() =>
|
||||
this.model().emails.map((_, index) => ({
|
||||
index,
|
||||
field: this.emailsForm.emails[index],
|
||||
}))
|
||||
);
|
||||
`,
|
||||
`import {computed, signal} from '@angular/core';\nimport {form} from '@angular/forms/signals';`,
|
||||
);
|
||||
|
||||
expect(diags.length).toBe(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
function diagnoseTestComponent(template: string, classField: string) {
|
||||
function diagnoseTestComponent(template: string, classField: string, imports: string = '') {
|
||||
const fileName = absoluteFrom('/main.ts');
|
||||
const {program, templateTypeChecker} = setup([
|
||||
{
|
||||
fileName,
|
||||
templates: {'TestCmp': template},
|
||||
source: `
|
||||
const {program, templateTypeChecker} = setup(
|
||||
[
|
||||
{
|
||||
fileName,
|
||||
templates: {'TestCmp': template},
|
||||
source: `
|
||||
${imports}
|
||||
export class TestCmp {
|
||||
items = [{name: 'a'}, {name: 'b'}];
|
||||
signalItems = [{name: signal('a')}, {name: signal('b')}];
|
||||
${classField}
|
||||
}`,
|
||||
},
|
||||
]);
|
||||
},
|
||||
],
|
||||
{},
|
||||
{forms: true},
|
||||
);
|
||||
const sf = getSourceFileOrError(program, fileName);
|
||||
const component = getClass(sf, 'TestCmp');
|
||||
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
|
||||
|
||||
@@ -182,6 +182,21 @@ export function angularCoreDtsFiles(): TestFile[] {
|
||||
})));
|
||||
}
|
||||
|
||||
let _angularFormsDts: TestFile[] | null = null;
|
||||
export function angularFormsDtsFiles(): TestFile[] {
|
||||
if (_angularFormsDts !== null) {
|
||||
return _angularFormsDts;
|
||||
}
|
||||
|
||||
const directory = resolveFromRunfiles('_main/packages/forms/npm_package');
|
||||
const dtsFiles = globSync('**/*.d.ts', {cwd: directory});
|
||||
|
||||
return (_angularFormsDts = ['package.json', ...dtsFiles].map((fileName) => ({
|
||||
name: absoluteFrom(`/node_modules/@angular/forms/${fileName}`),
|
||||
contents: readFileSync(path.join(directory, fileName), 'utf8'),
|
||||
})));
|
||||
}
|
||||
|
||||
export function angularAnimationsDts(): TestFile {
|
||||
return {
|
||||
name: absoluteFrom('/node_modules/@angular/animations/index.d.ts'),
|
||||
@@ -535,12 +550,20 @@ export function setup(
|
||||
parseOptions?: ParseTemplateOptions;
|
||||
referenceEmitter?: ReferenceEmitter;
|
||||
} = {},
|
||||
load: {
|
||||
forms?: boolean;
|
||||
} = {},
|
||||
): {
|
||||
templateTypeChecker: TemplateTypeChecker;
|
||||
program: ts.Program;
|
||||
programStrategy: TsCreateProgramDriver;
|
||||
} {
|
||||
const files = [typescriptLibDts(), ...angularCoreDtsFiles(), angularAnimationsDts()];
|
||||
const files = [
|
||||
typescriptLibDts(),
|
||||
...angularCoreDtsFiles(),
|
||||
angularAnimationsDts(),
|
||||
...(load.forms ? angularFormsDtsFiles() : []),
|
||||
];
|
||||
const fakeMetadataRegistry = new Map();
|
||||
const shims = new Map<AbsoluteFsPath, AbsoluteFsPath>();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user