diff --git a/goldens/public-api/compiler-cli/error_code.api.md b/goldens/public-api/compiler-cli/error_code.api.md index 5f4109efd87..b12e3875c54 100644 --- a/goldens/public-api/compiler-cli/error_code.api.md +++ b/goldens/public-api/compiler-cli/error_code.api.md @@ -50,6 +50,7 @@ export enum ErrorCode { // (undocumented) DUPLICATE_DECORATED_PROPERTIES = 1012, DUPLICATE_VARIABLE_DECLARATION = 8006, + FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION = 8118, HOST_BINDING_PARSE_ERROR = 5001, HOST_DIRECTIVE_COMPONENT = 2015, HOST_DIRECTIVE_CONFLICTING_ALIAS = 2018, diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 283848fa401..d0a9b07fd42 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -611,6 +611,22 @@ export enum ErrorCode { */ UNINVOKED_FUNCTION_IN_TEXT_INTERPOLATION = 8117, + /** + * A required initializer is being invoked in a forbidden context such as a property initializer + * or a constructor. + * + * For example: + * ```ts + * class MyComponent { + * myInput = input.required(); + * somValue = this.myInput(); // Error + * + * constructor() { + * this.myInput(); // Error + * } + */ + FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION = 8118, + /** * 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. diff --git a/packages/compiler-cli/src/ngtsc/validation/src/rules/forbidden_required_initializer_invocation_rule.ts b/packages/compiler-cli/src/ngtsc/validation/src/rules/forbidden_required_initializer_invocation_rule.ts new file mode 100644 index 00000000000..be2b21ded97 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/validation/src/rules/forbidden_required_initializer_invocation_rule.ts @@ -0,0 +1,129 @@ +/*! + * @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.dev/license + */ + +import ts from 'typescript'; + +import { + InitializerApiFunction, + INPUT_INITIALIZER_FN, + MODEL_INITIALIZER_FN, + QUERY_INITIALIZER_FNS, + tryParseInitializerApi, +} from '../../../annotations'; +import {ErrorCode, makeDiagnostic} from '../../../diagnostics'; +import {ImportedSymbolsTracker} from '../../../imports'; +import {ReflectionHost} from '../../../reflection'; + +import {SourceFileValidatorRule} from './api'; + +/** APIs whose usages should be checked by the rule. */ +const APIS_TO_CHECK: InitializerApiFunction[] = [ + INPUT_INITIALIZER_FN, + MODEL_INITIALIZER_FN, + ...QUERY_INITIALIZER_FNS, +]; + +/** + * Rule that flags forbidden invocations of required initializers in property initializers and constructors. + */ +export class ForbiddenRequiredInitializersInvocationRule implements SourceFileValidatorRule { + constructor( + private reflector: ReflectionHost, + private importedSymbolsTracker: ImportedSymbolsTracker, + ) {} + + shouldCheck(sourceFile: ts.SourceFile): boolean { + // Skip the traversal if there are no imports of the initializer APIs. + return APIS_TO_CHECK.some(({functionName, owningModule}) => { + return ( + this.importedSymbolsTracker.hasNamedImport(sourceFile, functionName, owningModule) || + this.importedSymbolsTracker.hasNamespaceImport(sourceFile, owningModule) + ); + }); + } + + checkNode(node: ts.Node): ts.Diagnostic[] | null { + if (!ts.isClassDeclaration(node)) return null; + + const requiredInitializerDeclarations = node.members.filter( + (m) => ts.isPropertyDeclaration(m) && this.isPropDeclarationARequiredInitializer(m), + ); + + const diagnostics: ts.Diagnostic[] = []; + + // Handling of the usages in props initializations + for (let decl of node.members) { + if (!ts.isPropertyDeclaration(decl)) continue; + + const initiallizerExpr = decl.initializer; + if (!initiallizerExpr) continue; + + checkForbiddenInvocation(initiallizerExpr); + } + + function checkForbiddenInvocation(node: ts.Node): boolean | undefined { + if (ts.isArrowFunction(node) || ts.isFunctionExpression(node)) return; + + if ( + ts.isPropertyAccessExpression(node) && + node.expression.kind === ts.SyntaxKind.ThisKeyword && + // With the following we make sure we only flag invoked required initializers + ts.isCallExpression(node.parent) && + node.parent.expression === node + ) { + const requiredProp = requiredInitializerDeclarations.find( + (prop) => prop.name.getText() === node.name.getText(), + ); + if (requiredProp) { + const initializerFn = ( + requiredProp.initializer.expression as ts.PropertyAccessExpression + ).expression.getText(); + diagnostics.push( + makeDiagnostic( + ErrorCode.FORBIDDEN_REQUIRED_INITIALIZER_INVOCATION, + node, + `\`${node.name.getText()}\` is a required \`${initializerFn}\` and does not have a value in this context.`, + ), + ); + } + } + + return node.forEachChild(checkForbiddenInvocation); + } + + const ctor = getConstructorFromClass(node); + if (ctor) { + checkForbiddenInvocation(ctor); + } + + return diagnostics; + } + private isPropDeclarationARequiredInitializer( + node: ts.PropertyDeclaration, + ): node is ts.PropertyDeclaration & {initializer: ts.CallExpression} { + if (!node.initializer) return false; + + const identifiedInitializer = tryParseInitializerApi( + APIS_TO_CHECK, + node.initializer, + this.reflector, + this.importedSymbolsTracker, + ); + + if (identifiedInitializer === null || !identifiedInitializer.isRequired) return false; + + return true; + } +} + +function getConstructorFromClass(node: ts.ClassDeclaration): ts.ConstructorDeclaration | undefined { + // We also check for a constructor body to avoid picking up parent constructors. + return node.members.find( + (m): m is ts.ConstructorDeclaration => ts.isConstructorDeclaration(m) && m.body !== undefined, + ); +} diff --git a/packages/compiler-cli/src/ngtsc/validation/src/source_file_validator.ts b/packages/compiler-cli/src/ngtsc/validation/src/source_file_validator.ts index b41421bc202..9c6a994a741 100644 --- a/packages/compiler-cli/src/ngtsc/validation/src/source_file_validator.ts +++ b/packages/compiler-cli/src/ngtsc/validation/src/source_file_validator.ts @@ -15,6 +15,7 @@ import {SourceFileValidatorRule} from './rules/api'; import {InitializerApiUsageRule} from './rules/initializer_api_usage_rule'; import {UnusedStandaloneImportsRule} from './rules/unused_standalone_imports_rule'; import {TemplateTypeChecker, TypeCheckingConfig} from '../../typecheck/api'; +import {ForbiddenRequiredInitializersInvocationRule} from './rules/forbidden_required_initializer_invocation_rule'; /** * Validates that TypeScript files match a specific set of rules set by the Angular compiler. @@ -37,6 +38,10 @@ export class SourceFileValidator { importedSymbolsTracker, ), ); + + this.rules.push( + new ForbiddenRequiredInitializersInvocationRule(reflector, importedSymbolsTracker), + ); } /** diff --git a/packages/compiler-cli/test/ngtsc/authoring_diagnostics_spec.ts b/packages/compiler-cli/test/ngtsc/authoring_diagnostics_spec.ts index e4dd21226f4..b5b4f57a5e0 100644 --- a/packages/compiler-cli/test/ngtsc/authoring_diagnostics_spec.ts +++ b/packages/compiler-cli/test/ngtsc/authoring_diagnostics_spec.ts @@ -274,5 +274,197 @@ runInEachFileSystem(() => { 'Unsupported call to the input function. This function can only be used as the initializer of a property on a @Component or @Directive class.', ); }); + + describe('required initializer functions validation', () => { + it('should report required input being used as property initialized', () => { + env.write( + 'test.ts', + ` + import {input, Component} from '@angular/core'; + + function foobar(x:any) {} + + @Component({template: \`\`}) + export class Test { + reqInp = input.required(); + reqInp2 = input.required(); + smthg = this.reqInp(); + smthg2 = foobar(this.reqInp2()); + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toContain('`reqInp` is a required `input`'); + expect(diags[1].messageText).toContain('`reqInp2` is a required `input`'); + }); + + it('should report required input being invoked inside a constructor', () => { + env.write( + 'test.ts', + ` + import {input, Component} from '@angular/core'; + + function foobar(x:any) {} + + @Component({template: \`\`}) + export class Test { + reqInp = input.required(); + reqInp2 = input.required(); + + constructor() { + const smthg = this.reqInp(); + const smthg2 = foobar(this.reqInp2()); + } + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toContain('`reqInp` is a required `input`'); + expect(diags[1].messageText).toContain('`reqInp2` is a required `input`'); + }); + + it('should report required model being used as property initialized', () => { + env.write( + 'test.ts', + ` + import {model, Component} from '@angular/core'; + + @Component({template: \`\`}) + export class Test { + reqModel = model.required(); + smthg = this.reqModel(); + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('`reqModel` is a required `model`'); + }); + + it('should report required model being invoked in a constructor despite being set before hand', () => { + // While this code might be valid at runtime, we still prefer to flag any required model reads. + env.write( + 'test.ts', + ` + import {model, Component} from '@angular/core'; + + @Component({template: \`\`}) + export class Test { + reqModel = model.required(); + constructor() { + this.reqModel.set('foobar'); + const smthg = this.reqModel(); + } + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('`reqModel` is a required `model`'); + }); + + it('should report required viewChild being used as property initialized', () => { + env.write( + 'test.ts', + ` + import {viewChild, Component} from '@angular/core'; + + @Component({template: \`\`}) + export class Test { + reqChild = viewChild.required('someRef'); + smthg = this.reqChild(); + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('`reqChild` is a required `viewChild`'); + }); + + it('should not report required input being invoked inside an effect or a computed', () => { + env.write( + 'test.ts', + ` + import {input, Component, effect, computed} from '@angular/core'; + + function foobar(x:any) {} + + @Component({template: \`\`}) + export class Test { + reqInp = input.required(); + reqInp2 = input.required(); + _ = effect(() => { this.reqInp(); foobar(this.reqInp2()); }); + __ = effect(function() { this.reqInp(); foobar(this.reqInp2()); }); + comp = computed(() => this.reqInp()); + + constructor() { + effect(() => { this.reqInp(); foobar(this.reqInp2()); }); + } + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not report non-required input being invoked in contructrors or in property initialization', () => { + env.write( + 'test.ts', + ` + import {input, Component, effect, computed} from '@angular/core'; + + function foobar(x:any) {} + + @Component({template: \`\`}) + export class Test { + reqInp = input(''); + reqInp2 = input(true); + smthg = this.reqInp(); + smthg2 = foobar(this.reqInp2()); + + constructor() { + const smthg = this.reqInp(); + const smthg2 = foobar(this.reqInp2()); + } + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not report non-invocations of required inputs', () => { + env.write( + 'test.ts', + ` + import {input, Component, effect, computed} from '@angular/core'; + import {toObservable} from '@angular/core/rxjs-interop'; + + + @Component({template: \`\`}) + export class Test { + reqInp = input.required(); + obs$ = toObservable(this.reqInp); + + constructor() { + const obs$ = toObservable(this.reqInp); + } + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + }); }); }); diff --git a/packages/core/test/acceptance/authoring/model_inputs_spec.ts b/packages/core/test/acceptance/authoring/model_inputs_spec.ts index 5c9b1d7f05d..c1dff7a1c56 100644 --- a/packages/core/test/acceptance/authoring/model_inputs_spec.ts +++ b/packages/core/test/acceptance/authoring/model_inputs_spec.ts @@ -359,29 +359,6 @@ describe('model inputs', () => { expect(emittedValues).toEqual([2]); }); - it('should throw if a required model input is accessed too early', () => { - @Directive({selector: '[dir]'}) - class Dir { - value = model.required(); - - constructor() { - this.value(); - } - } - - @Component({ - template: '
', - imports: [Dir], - }) - class App { - value = 1; - } - - expect(() => TestBed.createComponent(App)).toThrowError( - /Model is required but no value is available yet/, - ); - }); - it('should throw if a required model input is updated too early', () => { @Directive({selector: '[dir]'}) class Dir { diff --git a/packages/core/test/acceptance/authoring/signal_inputs_spec.ts b/packages/core/test/acceptance/authoring/signal_inputs_spec.ts index 4289674be9f..8559214a2b4 100644 --- a/packages/core/test/acceptance/authoring/signal_inputs_spec.ts +++ b/packages/core/test/acceptance/authoring/signal_inputs_spec.ts @@ -180,32 +180,6 @@ describe('signal inputs', () => { expect(transformRunCount).toBe(1); }); - it('should throw error if a required input is accessed too early', () => { - @Component({ - selector: 'input-comp', - template: 'input:{{input()}}', - }) - class InputComp { - input = input.required({debugName: 'input'}); - - constructor() { - this.input(); - } - } - - @Component({ - template: ``, - imports: [InputComp], - }) - class TestCmp { - value = 1; - } - - expect(() => TestBed.createComponent(TestCmp)).toThrowError( - /Input "input" is required but no value is available yet/, - ); - }); - it('should be possible to bind to an inherited input', () => { @Directive() class BaseDir { diff --git a/packages/core/test/acceptance/authoring/signal_queries_spec.ts b/packages/core/test/acceptance/authoring/signal_queries_spec.ts index 078be291f19..e228ceb4533 100644 --- a/packages/core/test/acceptance/authoring/signal_queries_spec.ts +++ b/packages/core/test/acceptance/authoring/signal_queries_spec.ts @@ -83,24 +83,6 @@ describe('queries as signals', () => { expect(fixture.componentInstance.foundEl()).toBeTrue(); }); - it('should throw if required query is read in the constructor', () => { - @Component({ - template: `
`, - }) - class AppComponent { - divEl = viewChild.required>('el'); - - constructor() { - this.divEl(); - } - } - - // non-required query results are undefined before we run creation mode on the view queries - expect(() => { - TestBed.createComponent(AppComponent); - }).toThrowError(/NG0951: Child query result is required but no value is available/); - }); - it('should query for multiple elements in a template', () => { @Component({ template: ` diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts index a94fda3cc09..5d6b3c48e40 100644 --- a/packages/core/test/acceptance/component_spec.ts +++ b/packages/core/test/acceptance/component_spec.ts @@ -21,10 +21,12 @@ import { Injector, input, Input, + model, NgModule, OnDestroy, reflectComponentType, Renderer2, + viewChild, ViewChild, ViewContainerRef, ViewEncapsulation, @@ -922,4 +924,74 @@ describe('component', () => { forbidOrphanRendering: true, }); }); + + describe('required initiliazers', () => { + // The following tests are specifically not in the authoring subdirectory to ensure that AOT doesn't check (and throws) forbidden required reads. + it('should throw error if a required input is accessed too early', () => { + @Component({ + selector: 'input-comp', + template: 'input:{{input()}}', + }) + class InputComp { + input = input.required({debugName: 'input'}); + + constructor() { + this.input(); + } + } + + @Component({ + template: ``, + imports: [InputComp], + }) + class TestCmp { + value = 1; + } + + expect(() => TestBed.createComponent(TestCmp)).toThrowError( + /Input "input" is required but no value is available yet/, + ); + }); + + it('should throw if a required model input is accessed too early', () => { + @Directive({selector: '[dir]'}) + class Dir { + value = model.required(); + + constructor() { + this.value(); + } + } + + @Component({ + template: '
', + imports: [Dir], + }) + class App { + value = 1; + } + + expect(() => TestBed.createComponent(App)).toThrowError( + /Model is required but no value is available yet/, + ); + }); + + it('should throw if required query is read in the constructor', () => { + @Component({ + template: `
`, + }) + class AppComponent { + divEl = viewChild.required>('el'); + + constructor() { + this.divEl(); + } + } + + // non-required query results are undefined before we run creation mode on the view queries + expect(() => { + TestBed.createComponent(AppComponent); + }).toThrowError(/NG0951: Child query result is required but no value is available/); + }); + }); });