fix(compiler-cli): make the unused imports diagnostic easier to read (#58468)

crisbeto · thePunderWoman · commit 22cd6869ef45 · 2024-11-01T14:33:56.000Z
The unused imports diagnostic reports once on the entire initializer and then again once per unused imports. This ends up being a bit hard to follow, because in a lot of cases the code snippet looks identical. These changes switch to highlighting the `imports:` part of the property declaration and only highlighting the unused imports without a message. PR Close #58468
diff --git a/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts b/packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts
@@ -9,14 +9,15 @@
 import ts from 'typescript';
 
 import {ErrorCode, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
-import {ImportedSymbolsTracker, Reference} from '../../../imports';
-import {
+import type {ImportedSymbolsTracker, Reference} from '../../../imports';
+import type {ClassDeclaration} from '../../../reflection';
+import type {
   TemplateTypeChecker,
   TypeCheckableDirectiveMeta,
   TypeCheckingConfig,
 } from '../../../typecheck/api';
 
-import {SourceFileValidatorRule} from './api';
+import type {SourceFileValidatorRule} from './api';
 
 /**
  * Rule that flags unused symbols inside of the `imports` array of a component.
@@ -79,7 +80,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
     if (unused.length === metadata.imports.length) {
       return makeDiagnostic(
         ErrorCode.UNUSED_STANDALONE_IMPORTS,
-        metadata.rawImports,
+        this.getDiagnosticNode(metadata.rawImports),
         'All imports are unused',
         undefined,
         category,
@@ -88,14 +89,19 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
 
     return makeDiagnostic(
       ErrorCode.UNUSED_STANDALONE_IMPORTS,
-      metadata.rawImports,
+      this.getDiagnosticNode(metadata.rawImports),
       'Imports array contains unused imports',
-      unused.map(([ref, type, name]) =>
-        makeRelatedInformation(
-          ref.getOriginForDiagnostics(metadata.rawImports!),
-          `${type} "${name}" is not used within the template`,
-        ),
-      ),
+      unused.map((ref) => {
+        return makeRelatedInformation(
+          // Intentionally don't pass a message to `makeRelatedInformation` to make the diagnostic
+          // less noisy. The node will already be highlighted so the user can see which node is
+          // unused. Note that in the case where an origin can't be resolved, we fall back to
+          // the original node's identifier so the user can still see the name. This can happen
+          // when the unused is coming from an imports array within the same file.
+          ref.getOriginForDiagnostics(metadata.rawImports!, ref.node.name),
+          '',
+        );
+      }),
       category,
     );
   }
@@ -111,7 +117,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
       return null;
     }
 
-    let unused: [ref: Reference, type: string, name: string][] | null = null;
+    let unused: Reference<ClassDeclaration>[] | null = null;
 
     for (const current of imports) {
       const currentNode = current.node as ts.ClassDeclaration;
@@ -124,7 +130,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
           !this.isPotentialSharedReference(current, rawImports)
         ) {
           unused ??= [];
-          unused.push([current, dirMeta.isComponent ? 'Component' : 'Directive', dirMeta.name]);
+          unused.push(current);
         }
         continue;
       }
@@ -138,7 +144,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
         !this.isPotentialSharedReference(current, rawImports)
       ) {
         unused ??= [];
-        unused.push([current, 'Pipe', pipeMeta.ref.node.name.text]);
+        unused.push(current);
       }
     }
 
@@ -175,4 +181,21 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
     // symbol like an array of shared common components.
     return true;
   }
+
+  /** Gets the node on which to report the diagnostic. */
+  private getDiagnosticNode(importsExpression: ts.Expression): ts.Node {
+    let current = importsExpression.parent;
+
+    while (current) {
+      // Highlight the `imports:` part of the node instead of the entire node, because
+      // imports arrays can be long which makes the diagnostic harder to scan visually.
+      if (ts.isPropertyAssignment(current)) {
+        return current.name;
+      } else {
+        current = current.parent;
+      }
+    }
+
+    return importsExpression;
+  }
 }
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -397,7 +397,7 @@ runInEachFileSystem(() => {
         export class TestCmp {}
 
         @Component({
-          template: '', 
+          template: '',
           selector: 'target-cmp',
           standalone: false,
         })
@@ -432,7 +432,7 @@ runInEachFileSystem(() => {
         export class TestCmp {}
 
         @Component({
-          template: '', 
+          template: '',
           selector: 'target-cmp',
           standalone: false,
         })
@@ -7711,9 +7711,7 @@ suppress
         expect(diags.length).toBe(1);
         expect(diags[0].messageText).toBe('Imports array contains unused imports');
         expect(diags[0].relatedInformation?.length).toBe(1);
-        expect(diags[0].relatedInformation![0].messageText).toBe(
-          'Directive "UnusedDir" is not used within the template',
-        );
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
       });
 
       it('should report when a pipe is not used within a template', () => {
@@ -7770,9 +7768,7 @@ suppress
         expect(diags.length).toBe(1);
         expect(diags[0].messageText).toBe('Imports array contains unused imports');
         expect(diags[0].relatedInformation?.length).toBe(1);
-        expect(diags[0].relatedInformation?.[0].messageText).toBe(
-          'Pipe "UnusedPipe" is not used within the template',
-        );
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedPipe');
       });
 
       it('should not report imports only used inside @defer blocks', () => {
@@ -7960,12 +7956,8 @@ suppress
         expect(diags.length).toBe(1);
         expect(diags[0].messageText).toBe('Imports array contains unused imports');
         expect(diags[0].relatedInformation?.length).toBe(2);
-        expect(diags[0].relatedInformation![0].messageText).toBe(
-          'Directive "NgFor" is not used within the template',
-        );
-        expect(diags[0].relatedInformation![1].messageText).toBe(
-          'Pipe "PercentPipe" is not used within the template',
-        );
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('NgFor');
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('PercentPipe');
       });
 
       it('should report unused imports coming from a nested array from the same file', () => {
@@ -8027,9 +8019,7 @@ suppress
         expect(diags.length).toBe(1);
         expect(diags[0].messageText).toBe('Imports array contains unused imports');
         expect(diags[0].relatedInformation?.length).toBe(1);
-        expect(diags[0].relatedInformation![0].messageText).toBe(
-          'Directive "UnusedDir" is not used within the template',
-        );
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
       });
 
       it('should report unused imports coming from an array used as the `imports` initializer', () => {
@@ -8080,9 +8070,7 @@ suppress
         expect(diags.length).toBe(1);
         expect(diags[0].messageText).toBe('Imports array contains unused imports');
         expect(diags[0].relatedInformation?.length).toBe(1);
-        expect(diags[0].relatedInformation![0].messageText).toBe(
-          'Directive "UnusedDir" is not used within the template',
-        );
+        expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
       });
 
       it('should not report unused imports coming from an array through a spread expression from a different file', () => {
