Skip to content

Commit 6dff287

Browse files
JeanMechethePunderWoman
JeanMeche
authored and
thePunderWoman
committed
refactor(compiler-cli): Add a diagnostic to detect forbiden invocations of required initializers (#63614)
The diagnostic will raise an error when required initializers (input, model, queries) are invoked the context of property initializers and contructors. Docs will be provided in a follow-up fixes #63602 PR Close #63614
1 parent 803dc8e commit 6dff287

File tree

9 files changed

+415
-67
lines changed

9 files changed

+415
-67
lines changed

‎goldens/public-api/compiler-cli/error_code.api.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ export enum ErrorCode {
5050
// (undocumented)
5151
DUPLICATE_DECORATED_PROPERTIES = 1012,
5252
DUPLICATE_VARIABLE_DECLARATION = 8006,
53+
FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION = 8118,
5354
HOST_BINDING_PARSE_ERROR = 5001,
5455
HOST_DIRECTIVE_COMPONENT = 2015,
5556
HOST_DIRECTIVE_CONFLICTING_ALIAS = 2018,

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,22 @@ export enum ErrorCode {
611611
*/
612612
UNINVOKED_FUNCTION_IN_TEXT_INTERPOLATION = 8117,
613613

614+
/**
615+
* A required initializer is being invoked in a forbidden context such as a property initializer
616+
* or a constructor.
617+
*
618+
* For example:
619+
* ```ts
620+
* class MyComponent {
621+
* myInput = input.required();
622+
* somValue = this.myInput(); // Error
623+
*
624+
* constructor() {
625+
* this.myInput(); // Error
626+
* }
627+
*/
628+
FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION = 8118,
629+
614630
/**
615631
* The template type-checking engine would need to generate an inline type check block for a
616632
* component, but the current type-checking environment doesn't support it.

‎packages/compiler-cli/src/ngtsc/validation/src/rules/forbidden_required_initializer_invocation_rule.ts‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/*!
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import ts from 'typescript';
10+
11+
import {
12+
InitializerApiFunction,
13+
INPUT_INITIALIZER_FN,
14+
MODEL_INITIALIZER_FN,
15+
QUERY_INITIALIZER_FNS,
16+
tryParseInitializerApi,
17+
} from '../../../annotations';
18+
import {ErrorCode, makeDiagnostic} from '../../../diagnostics';
19+
import {ImportedSymbolsTracker} from '../../../imports';
20+
import {ReflectionHost} from '../../../reflection';
21+
22+
import {SourceFileValidatorRule} from './api';
23+
24+
/** APIs whose usages should be checked by the rule. */
25+
const APIS_TO_CHECK: InitializerApiFunction[] = [
26+
INPUT_INITIALIZER_FN,
27+
MODEL_INITIALIZER_FN,
28+
...QUERY_INITIALIZER_FNS,
29+
];
30+
31+
/**
32+
* Rule that flags forbidden invocations of required initializers in property initializers and constructors.
33+
*/
34+
export class ForbiddenRequiredInitializersInvocationRule implements SourceFileValidatorRule {
35+
constructor(
36+
private reflector: ReflectionHost,
37+
private importedSymbolsTracker: ImportedSymbolsTracker,
38+
) {}
39+
40+
shouldCheck(sourceFile: ts.SourceFile): boolean {
41+
// Skip the traversal if there are no imports of the initializer APIs.
42+
return APIS_TO_CHECK.some(({functionName, owningModule}) => {
43+
return (
44+
this.importedSymbolsTracker.hasNamedImport(sourceFile, functionName, owningModule) ||
45+
this.importedSymbolsTracker.hasNamespaceImport(sourceFile, owningModule)
46+
);
47+
});
48+
}
49+
50+
checkNode(node: ts.Node): ts.Diagnostic[] | null {
51+
if (!ts.isClassDeclaration(node)) return null;
52+
53+
const requiredInitializerDeclarations = node.members.filter(
54+
(m) => ts.isPropertyDeclaration(m) && this.isPropDeclarationARequiredInitializer(m),
55+
);
56+
57+
const diagnostics: ts.Diagnostic[] = [];
58+
59+
// Handling of the usages in props initializations
60+
for (let decl of node.members) {
61+
if (!ts.isPropertyDeclaration(decl)) continue;
62+
63+
const initiallizerExpr = decl.initializer;
64+
if (!initiallizerExpr) continue;
65+
66+
checkForbiddenInvocation(initiallizerExpr);
67+
}
68+
69+
function checkForbiddenInvocation(node: ts.Node): boolean | undefined {
70+
if (ts.isArrowFunction(node) || ts.isFunctionExpression(node)) return;
71+
72+
if (
73+
ts.isPropertyAccessExpression(node) &&
74+
node.expression.kind === ts.SyntaxKind.ThisKeyword &&
75+
// With the following we make sure we only flag invoked required initializers
76+
ts.isCallExpression(node.parent) &&
77+
node.parent.expression === node
78+
) {
79+
const requiredProp = requiredInitializerDeclarations.find(
80+
(prop) => prop.name.getText() === node.name.getText(),
81+
);
82+
if (requiredProp) {
83+
const initializerFn = (
84+
requiredProp.initializer.expression as ts.PropertyAccessExpression
85+
).expression.getText();
86+
diagnostics.push(
87+
makeDiagnostic(
88+
ErrorCode.FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION,
89+
node,
90+
`\`${node.name.getText()}\` is a required \`${initializerFn}\` and does not have a value in this context.`,
91+
),
92+
);
93+
}
94+
}
95+
96+
return node.forEachChild(checkForbiddenInvocation);
97+
}
98+
99+
const ctor = getConstructorFromClass(node);
100+
if (ctor) {
101+
checkForbiddenInvocation(ctor);
102+
}
103+
104+
return diagnostics;
105+
}
106+
private isPropDeclarationARequiredInitializer(
107+
node: ts.PropertyDeclaration,
108+
): node is ts.PropertyDeclaration & {initializer: ts.CallExpression} {
109+
if (!node.initializer) return false;
110+
111+
const identifiedInitializer = tryParseInitializerApi(
112+
APIS_TO_CHECK,
113+
node.initializer,
114+
this.reflector,
115+
this.importedSymbolsTracker,
116+
);
117+
118+
if (identifiedInitializer === null || !identifiedInitializer.isRequired) return false;
119+
120+
return true;
121+
}
122+
}
123+
124+
function getConstructorFromClass(node: ts.ClassDeclaration): ts.ConstructorDeclaration | undefined {
125+
// We also check for a constructor body to avoid picking up parent constructors.
126+
return node.members.find(
127+
(m): m is ts.ConstructorDeclaration => ts.isConstructorDeclaration(m) && m.body !== undefined,
128+
);
129+
}

‎packages/compiler-cli/src/ngtsc/validation/src/source_file_validator.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {SourceFileValidatorRule} from './rules/api';
1515
import {InitializerApiUsageRule} from './rules/initializer_api_usage_rule';
1616
import {UnusedStandaloneImportsRule} from './rules/unused_standalone_imports_rule';
1717
import {TemplateTypeChecker, TypeCheckingConfig} from '../../typecheck/api';
18+
import {ForbiddenRequiredInitializersInvocationRule} from './rules/forbidden_required_initializer_invocation_rule';
1819

1920
/**
2021
* Validates that TypeScript files match a specific set of rules set by the Angular compiler.
@@ -37,6 +38,10 @@ export class SourceFileValidator {
3738
importedSymbolsTracker,
3839
),
3940
);
41+
42+
this.rules.push(
43+
new ForbiddenRequiredInitializersInvocationRule(reflector, importedSymbolsTracker),
44+
);
4045
}
4146

4247
/**

‎packages/compiler-cli/test/ngtsc/authoring_diagnostics_spec.ts‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,5 +274,197 @@ runInEachFileSystem(() => {
274274
'Unsupported call to the input function. This function can only be used as the initializer of a property on a @Component or @Directive class.',
275275
);
276276
});
277+
278+
describe('required initializer functions validation', () => {
279+
it('should report required input being used as property initialized', () => {
280+
env.write(
281+
'test.ts',
282+
`
283+
import {input, Component} from '@angular/core';
284+
285+
function foobar(x:any) {}
286+
287+
@Component({template: \`\`})
288+
export class Test {
289+
reqInp = input.required<string>();
290+
reqInp2 = input.required<boolean>();
291+
smthg = this.reqInp();
292+
smthg2 = foobar(this.reqInp2());
293+
}
294+
`,
295+
);
296+
297+
const diags = env.driveDiagnostics();
298+
expect(diags.length).toBe(2);
299+
expect(diags[0].messageText).toContain('`reqInp` is a required `input`');
300+
expect(diags[1].messageText).toContain('`reqInp2` is a required `input`');
301+
});
302+
303+
it('should report required input being invoked inside a constructor', () => {
304+
env.write(
305+
'test.ts',
306+
`
307+
import {input, Component} from '@angular/core';
308+
309+
function foobar(x:any) {}
310+
311+
@Component({template: \`\`})
312+
export class Test {
313+
reqInp = input.required<string>();
314+
reqInp2 = input.required<boolean>();
315+
316+
constructor() {
317+
const smthg = this.reqInp();
318+
const smthg2 = foobar(this.reqInp2());
319+
}
320+
}
321+
`,
322+
);
323+
324+
const diags = env.driveDiagnostics();
325+
expect(diags.length).toBe(2);
326+
expect(diags[0].messageText).toContain('`reqInp` is a required `input`');
327+
expect(diags[1].messageText).toContain('`reqInp2` is a required `input`');
328+
});
329+
330+
it('should report required model being used as property initialized', () => {
331+
env.write(
332+
'test.ts',
333+
`
334+
import {model, Component} from '@angular/core';
335+
336+
@Component({template: \`\`})
337+
export class Test {
338+
reqModel = model.required<string>();
339+
smthg = this.reqModel();
340+
}
341+
`,
342+
);
343+
344+
const diags = env.driveDiagnostics();
345+
expect(diags.length).toBe(1);
346+
expect(diags[0].messageText).toContain('`reqModel` is a required `model`');
347+
});
348+
349+
it('should report required model being invoked in a constructor despite being set before hand', () => {
350+
// While this code might be valid at runtime, we still prefer to flag any required model reads.
351+
env.write(
352+
'test.ts',
353+
`
354+
import {model, Component} from '@angular/core';
355+
356+
@Component({template: \`\`})
357+
export class Test {
358+
reqModel = model.required<string>();
359+
constructor() {
360+
this.reqModel.set('foobar');
361+
const smthg = this.reqModel();
362+
}
363+
}
364+
`,
365+
);
366+
367+
const diags = env.driveDiagnostics();
368+
expect(diags.length).toBe(1);
369+
expect(diags[0].messageText).toContain('`reqModel` is a required `model`');
370+
});
371+
372+
it('should report required viewChild being used as property initialized', () => {
373+
env.write(
374+
'test.ts',
375+
`
376+
import {viewChild, Component} from '@angular/core';
377+
378+
@Component({template: \`\`})
379+
export class Test {
380+
reqChild = viewChild.required('someRef');
381+
smthg = this.reqChild();
382+
}
383+
`,
384+
);
385+
386+
const diags = env.driveDiagnostics();
387+
expect(diags.length).toBe(1);
388+
expect(diags[0].messageText).toContain('`reqChild` is a required `viewChild`');
389+
});
390+
391+
it('should not report required input being invoked inside an effect or a computed', () => {
392+
env.write(
393+
'test.ts',
394+
`
395+
import {input, Component, effect, computed} from '@angular/core';
396+
397+
function foobar(x:any) {}
398+
399+
@Component({template: \`\`})
400+
export class Test {
401+
reqInp = input.required<string>();
402+
reqInp2 = input.required<boolean>();
403+
_ = effect(() => { this.reqInp(); foobar(this.reqInp2()); });
404+
__ = effect(function() { this.reqInp(); foobar(this.reqInp2()); });
405+
comp = computed(() => this.reqInp());
406+
407+
constructor() {
408+
effect(() => { this.reqInp(); foobar(this.reqInp2()); });
409+
}
410+
}
411+
`,
412+
);
413+
414+
const diags = env.driveDiagnostics();
415+
expect(diags.length).toBe(0);
416+
});
417+
418+
it('should not report non-required input being invoked in contructrors or in property initialization', () => {
419+
env.write(
420+
'test.ts',
421+
`
422+
import {input, Component, effect, computed} from '@angular/core';
423+
424+
function foobar(x:any) {}
425+
426+
@Component({template: \`\`})
427+
export class Test {
428+
reqInp = input<string>('');
429+
reqInp2 = input<boolean>(true);
430+
smthg = this.reqInp();
431+
smthg2 = foobar(this.reqInp2());
432+
433+
constructor() {
434+
const smthg = this.reqInp();
435+
const smthg2 = foobar(this.reqInp2());
436+
}
437+
}
438+
`,
439+
);
440+
441+
const diags = env.driveDiagnostics();
442+
expect(diags.length).toBe(0);
443+
});
444+
445+
it('should not report non-invocations of required inputs', () => {
446+
env.write(
447+
'test.ts',
448+
`
449+
import {input, Component, effect, computed} from '@angular/core';
450+
import {toObservable} from '@angular/core/rxjs-interop';
451+
452+
453+
@Component({template: \`\`})
454+
export class Test {
455+
reqInp = input.required<string>();
456+
obs$ = toObservable(this.reqInp);
457+
458+
constructor() {
459+
const obs$ = toObservable(this.reqInp);
460+
}
461+
}
462+
`,
463+
);
464+
465+
const diags = env.driveDiagnostics();
466+
expect(diags.length).toBe(0);
467+
});
468+
});
277469
});
278470
});

0 commit comments

Comments
 (0)