fix(compiler-cli): report more accurate diagnostic for invalid import (#60455)

crisbeto · pkozlowski-opensource · commit aa8ea7a5b227 · 2025-03-19T19:06:38.000+01:00
Currently when an incorrect value is in the `imports` array, we highlight the entire array which can be very noisy for large arrays. This comes up semi-regularly (at least for me) when an import is missing. These changes add some logic that reports a more accurate diagnostic location for the most common case where the `imports` array is static. Non-static arrays will fall back to the current behavior. PR Close #60455
diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts
@@ -11,11 +11,16 @@ import {
   isResolvedModuleWithProviders,
   ResolvedModuleWithProviders,
 } from '@angular/compiler-cli/src/ngtsc/annotations/ng_module';
-import {ErrorCode, makeDiagnostic} from '@angular/compiler-cli/src/ngtsc/diagnostics';
+import {
+  ErrorCode,
+  FatalDiagnosticError,
+  makeDiagnostic,
+} from '@angular/compiler-cli/src/ngtsc/diagnostics';
 import ts from 'typescript';
 
 import {Reference} from '../../../imports';
 import {
+  DynamicValue,
   ForeignFunctionResolver,
   ResolvedValue,
   ResolvedValueMap,
@@ -96,7 +101,9 @@ export function validateAndFlattenComponentImports(
   }
   const diagnostics: ts.Diagnostic[] = [];
 
-  for (const ref of imports) {
+  for (let i = 0; i < imports.length; i++) {
+    const ref = imports[i];
+
     if (Array.isArray(ref)) {
       const {imports: childImports, diagnostics: childDiagnostics} =
         validateAndFlattenComponentImports(ref, expr, isDeferred);
@@ -132,7 +139,32 @@ export function validateAndFlattenComponentImports(
         ),
       );
     } else {
-      diagnostics.push(createValueHasWrongTypeError(expr, imports, errorMessage).toDiagnostic());
+      let diagnosticNode: ts.Node;
+      let diagnosticValue: ResolvedValue;
+
+      if (ref instanceof DynamicValue) {
+        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];
+        diagnosticValue = ref;
+      } else {
+        diagnosticNode = expr;
+        diagnosticValue = imports;
+      }
+
+      diagnostics.push(
+        createValueHasWrongTypeError(diagnosticNode, diagnosticValue, errorMessage).toDiagnostic(),
+      );
     }
   }
 
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -2282,6 +2282,43 @@ runInEachFileSystem((os: string) => {
       );
     });
 
+    it('should report diagnostic on the exact element in the `imports` array', () => {
+      // Note: the scenario here is slightly contrived, but we want to hit the code
+      // path where TS doesn't report a type error before Angular which appears to be
+      // common with the language service.
+      env.write(
+        'test.ts',
+        `
+          import {Component, Directive} from '@angular/core';
+
+          @Directive({selector: '[hello]'})
+          export class HelloDir {}
+
+          const someVar = {} as any;
+
+          @Component({
+            template: '<div hello></div>',
+            imports: [
+              someVar,
+              HelloDir,
+            ]
+          })
+          export class TestCmp {}
+        `,
+      );
+
+      const diags = env.driveDiagnostics();
+      const message = diags.length
+        ? ts.flattenDiagnosticMessageText(diags[0].messageText, '\n')
+        : '';
+      expect(diags.length).toBe(1);
+      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 '{}'`);
+    });
+
     describe('empty and missing selectors', () => {
       it('should use default selector for Components when no selector present', () => {
         env.write(
