From 106ba63650340cc828e302dc4eb46dbd4fb43a3b Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 22 Dec 2025 20:16:21 +0100 Subject: [PATCH] fix(compiler-cli): ensure component import diagnostics are reported within the `imports` expression PR #60455 improved error reporting for `@Component.imports` by scoping the diagnostic to an individual element within the `imports` array, but this may introduce hard to track diagnostics when it ends up being reported (far) away from the component itself. This can be even more problematic when the diagnostic would end up being reported in a declaration file, as happened in issue #65686; the declaration files of an imported library contained syntax that the static interpreter did not support, hence the `@Component.imports` was rejected with a diagnostic reported in the library's declaration file. This diagnostic isn't guaranteed to be reported (e.g. the CLI only gathers Angular-specific diagnostics for Angular-compiled files, which excludes declaration files). This commit changes the diagnostic location to ensure it is being reported within the `@Component.imports` expression, in most cases retaining the desirable effect of #60455 while avoiding out-of-band diagnostics. --- .../ngtsc/annotations/component/src/util.ts | 41 ++++++++---- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 63 ++++++++++++++++++- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts index 1a093b88f3b..086018392c1 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts @@ -99,10 +99,18 @@ export function validateAndFlattenComponentImports( for (let i = 0; i < imports.length; i++) { const ref = imports[i]; + let refExpr = expr; + if ( + ts.isArrayLiteralExpression(expr) && + expr.elements.length === imports.length && + !expr.elements.some(ts.isSpreadAssignment) + ) { + refExpr = expr.elements[i]; + } if (Array.isArray(ref)) { const {imports: childImports, diagnostics: childDiagnostics} = - validateAndFlattenComponentImports(ref, expr, isDeferred); + validateAndFlattenComponentImports(ref, refExpr, isDeferred); flattened.push(...childImports); diagnostics.push(...childDiagnostics); } else if (ref instanceof Reference) { @@ -138,20 +146,16 @@ export function validateAndFlattenComponentImports( let diagnosticNode: ts.Node; let diagnosticValue: ResolvedValue; - if (ref instanceof DynamicValue) { + // Reporting a diagnostic on the entire array can be noisy, especially if the user has a + // large array. Attempt to determine the most accurate position within the `imports` expression to report the + // diagnostic on. + if (ref instanceof DynamicValue && isWithinExpression(ref.node, expr)) { + // Use the dynamic value position itself if it occurs within the `imports` expression. diagnosticNode = ref.node; diagnosticValue = ref; - } else if ( - ts.isArrayLiteralExpression(expr) && - expr.elements.length === imports.length && - !expr.elements.some(ts.isSpreadAssignment) && - !imports.some(Array.isArray) - ) { - // Reporting a diagnostic on the entire array can be noisy, especially if the user has a - // large array. The most common case is that the array will be static so we can reliably - // trace back a `ResolvedValue` back to its node using its index. This allows us to report - // the exact node that caused the issue. - diagnosticNode = expr.elements[i]; + } else if (refExpr !== expr) { + // The reference comes from a specific element in `expr`, so use that element to report the diagnostic on. + diagnosticNode = refExpr; diagnosticValue = ref; } else { diagnosticNode = expr; @@ -167,6 +171,17 @@ export function validateAndFlattenComponentImports( return {imports: flattened, diagnostics}; } +function isWithinExpression(node: ts.Node, expr: ts.Expression): boolean { + let current: ts.Node | undefined = node; + while (current !== undefined) { + if (current === expr) { + return true; + } + current = current.parent; + } + return false; +} + /** * Inspects `value` to determine if it resembles a `ModuleWithProviders` value. This is an * approximation only suitable for error reporting as any resolved object with an `ngModule` diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index e8b4cc69ac2..84380680355 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2589,12 +2589,70 @@ runInEachFileSystem((os: string) => { export class HelloDir {} const someVar = {} as any; + const tuple = [() => {}] as const; @Component({ template: '
', imports: [ someVar, HelloDir, + 'invalid', + tuple, + ] + }) + export class TestCmp {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + { + const message = ts.flattenDiagnosticMessageText(diags[0].messageText, '\n'); + expect(getDiagnosticSourceCode(diags[0])).toBe('someVar'); + expect(message).toContain( + `'imports' must be an array of components, directives, pipes, or NgModules.`, + ); + expect(message).toContain(`Value is of type '{}'`); + } + { + const message = ts.flattenDiagnosticMessageText(diags[1].messageText, '\n'); + expect(getDiagnosticSourceCode(diags[1])).toBe(`'invalid'`); + expect(message).toContain( + `'imports' must be an array of components, directives, pipes, or NgModules.`, + ); + expect(message).toContain(`Value is of type 'string'`); + } + { + const message = ts.flattenDiagnosticMessageText(diags[2].messageText, '\n'); + expect(getDiagnosticSourceCode(diags[2])).toBe('tuple'); + expect(message).toContain( + `'imports' must be an array of components, directives, pipes, or NgModules.`, + ); + expect(message).toContain(`Value is of type '[(not statically analyzable)]'.`); + } + }); + + it('should report imports diagnostic for declaration file in original expression', () => { + env.write( + 'node_modules/external/index.d.ts', + ` + export declare const UNRESOLVED_ITEM: readonly [unresolved]; + `, + ); + env.write( + 'test.ts', + ` + import {Component, Directive} from '@angular/core'; + import {UNRESOLVED_ITEM as Unresolved} from 'external'; + + @Directive({selector: '[hello]'}) + export class HelloDir {} + + @Component({ + template: '
', + imports: [ + [Unresolved], + HelloDir, ] }) export class TestCmp {} @@ -2606,11 +2664,12 @@ runInEachFileSystem((os: string) => { ? ts.flattenDiagnosticMessageText(diags[0].messageText, '\n') : ''; expect(diags.length).toBe(1); - expect(getDiagnosticSourceCode(diags[0])).toBe('someVar'); + expect(diags[0].file!.fileName).toContain('test.ts'); + expect(getDiagnosticSourceCode(diags[0])).toBe('Unresolved'); expect(message).toContain( `'imports' must be an array of components, directives, pipes, or NgModules.`, ); - expect(message).toContain(`Value is of type '{}'`); + expect(message).toContain(`Value is of type '[(not statically analyzable)]'.`); }); describe('empty and missing selectors', () => {