From 086cb2c40e2dcbdf831657d7ed3b3e98acf515ae Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 11 Nov 2024 10:27:54 +0100 Subject: [PATCH] fix(compiler-cli): report individual diagnostics for unused imports (#58589) Initially the unused imports check was implemented so that it reports one diagnostic per component with the individual unused imports being highlighted through the `relatedInformation`. This works fine when reporting errors to the command line, but vscode appears to only show `relatedInformation` when the user hovers over a diagnostic which is a sub-par experience. These changes switch to reporting a diagnostic for each unused import instead. PR Close #58589 --- .../rules/unused_standalone_imports_rule.ts | 75 ++++++++-------- .../test/ngtsc/template_typecheck_spec.ts | 25 ++---- .../fix_unused_standalone_imports.ts | 89 ++++++++++--------- 3 files changed, 90 insertions(+), 99 deletions(-) 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 index 0c18768feae..c1df67e667b 100644 --- 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 @@ -8,7 +8,7 @@ import ts from 'typescript'; -import {ErrorCode, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics'; +import {ErrorCode, makeDiagnostic} from '../../../diagnostics'; import type {ImportedSymbolsTracker, Reference} from '../../../imports'; import type {ClassDeclaration} from '../../../reflection'; import type { @@ -37,7 +37,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule { ); } - checkNode(node: ts.Node): ts.Diagnostic | null { + checkNode(node: ts.Node): ts.Diagnostic | ts.Diagnostic[] | null { if (!ts.isClassDeclaration(node)) { return null; } @@ -72,38 +72,36 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule { return null; } + const propertyAssignment = closestNode(metadata.rawImports, ts.isPropertyAssignment); const category = this.typeCheckingConfig.unusedStandaloneImports === 'error' ? ts.DiagnosticCategory.Error : ts.DiagnosticCategory.Warning; - if (unused.length === metadata.imports.length) { + if (unused.length === metadata.imports.length && propertyAssignment !== null) { return makeDiagnostic( ErrorCode.UNUSED_STANDALONE_IMPORTS, - this.getDiagnosticNode(metadata.rawImports), + propertyAssignment.name, 'All imports are unused', undefined, category, ); } - return makeDiagnostic( - ErrorCode.UNUSED_STANDALONE_IMPORTS, - this.getDiagnosticNode(metadata.rawImports), - 'Imports array contains unused imports', - 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, - ); + return unused.map((ref) => { + const diagnosticNode = + ref.getIdentityInExpression(metadata.rawImports!) || + ref.getIdentityIn(node.getSourceFile()) || + metadata.rawImports!; + + return makeDiagnostic( + ErrorCode.UNUSED_STANDALONE_IMPORTS, + diagnosticNode, + `${ref.node.name.text} is not used within the template of ${metadata.name}`, + undefined, + category, + ); + }); } private getUnusedSymbols( @@ -181,21 +179,22 @@ 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; - } +} + +/** Gets the closest parent node of a certain type. */ +function closestNode( + start: ts.Node, + predicate: (node: ts.Node) => node is T, +): T | null { + let current = start.parent; + + while (current) { + if (predicate(current)) { + return current; + } else { + current = current.parent; + } + } + + return null; } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index e2dab56f9d5..9df55c00793 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -7709,9 +7709,7 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('Imports array contains unused imports'); - expect(diags[0].relatedInformation?.length).toBe(1); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir'); + expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp'); }); it('should report when a pipe is not used within a template', () => { @@ -7766,9 +7764,7 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('Imports array contains unused imports'); - expect(diags[0].relatedInformation?.length).toBe(1); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedPipe'); + expect(diags[0].messageText).toBe('UnusedPipe is not used within the template of MyComp'); }); it('should not report imports only used inside @defer blocks', () => { @@ -7835,7 +7831,6 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); expect(diags[0].messageText).toBe('All imports are unused'); - expect(diags[0].relatedInformation).toBeFalsy(); }); it('should not report unused imports coming from modules', () => { @@ -7953,11 +7948,9 @@ suppress ); const diags = env.driveDiagnostics(); - expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('Imports array contains unused imports'); - expect(diags[0].relatedInformation?.length).toBe(2); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('NgFor'); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('PercentPipe'); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toBe('NgFor is not used within the template of MyComp'); + expect(diags[1].messageText).toBe('PercentPipe is not used within the template of MyComp'); }); it('should report unused imports coming from a nested array from the same file', () => { @@ -8017,9 +8010,7 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('Imports array contains unused imports'); - expect(diags[0].relatedInformation?.length).toBe(1); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir'); + expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp'); }); it('should report unused imports coming from an array used as the `imports` initializer', () => { @@ -8068,9 +8059,7 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('Imports array contains unused imports'); - expect(diags[0].relatedInformation?.length).toBe(1); - expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir'); + expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp'); }); it('should not report unused imports coming from an array through a spread expression from a different file', () => { diff --git a/packages/language-service/src/codefixes/fix_unused_standalone_imports.ts b/packages/language-service/src/codefixes/fix_unused_standalone_imports.ts index b464b947a97..148a5a5bb28 100644 --- a/packages/language-service/src/codefixes/fix_unused_standalone_imports.ts +++ b/packages/language-service/src/codefixes/fix_unused_standalone_imports.ts @@ -20,68 +20,71 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = { getCodeActions: () => [], fixIds: [FixIdForCodeFixesAll.FIX_UNUSED_STANDALONE_IMPORTS], getAllCodeActions: ({diagnostics}) => { + const arrayUpdates = new Map>(); + const arraysToClear = new Set(); const changes: tss.FileTextChanges[] = []; for (const diag of diagnostics) { - const {start, length, file, relatedInformation} = diag; + const {start, length, file} = diag; if (file === undefined || start === undefined || length == undefined) { continue; } const node = findFirstMatchingNode(file, { - filter: ( - current, - ): current is tss.PropertyAssignment & {initializer: tss.ArrayLiteralExpression} => - tss.isPropertyAssignment(current) && - tss.isArrayLiteralExpression(current.initializer) && - current.name.getStart() === start && - current.name.getWidth() === length, + filter: (n): n is tss.Expression => n.getStart() === start && n.getWidth() === length, }); + const parent = node?.parent || null; - if (node === null) { + if (node === null || parent === null) { continue; } - const importsArray = node.initializer; - let newText: string; - - // If `relatedInformation` is empty, it means that all the imports are unused. - // Replace the array with an empty array. - if (relatedInformation === undefined || relatedInformation.length === 0) { - newText = '[]'; - } else { - // Otherwise each `relatedInformation` entry points to an unused import that should be - // filtered out. We make a set of ranges corresponding to nodes which will be deleted and - // remove all nodes that belong to the set. - const excludeRanges = new Set( - relatedInformation.reduce((ranges, info) => { - // If the compiler can't resolve the unused import to an identifier within the array, - // it falls back to reporting the identifier of the class declaration instead. In theory - // that class could have the same offsets as the diagnostic location. It's a slim chance - // that would happen, but we filter out reports from other files just in case. - if (info.file === file) { - ranges.push(`${info.start}-${info.length}`); - } - return ranges; - }, [] as string[]), - ); - - const newArray = tss.factory.updateArrayLiteralExpression( - importsArray, - importsArray.elements.filter( - (el) => !excludeRanges.has(`${el.getStart()}-${el.getWidth()}`), - ), - ); - - newText = tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file); + // If the diagnostic is reported on the name of the `imports` array initializer, it means + // that all imports are unused so we can clear the entire array. Otherwise if it's reported + // on a single element, we only have to remove that element. + if ( + tss.isPropertyAssignment(parent) && + parent.name === node && + tss.isArrayLiteralExpression(parent.initializer) + ) { + arraysToClear.add(parent.initializer); + } else if (tss.isArrayLiteralExpression(parent)) { + if (!arrayUpdates.has(parent)) { + arrayUpdates.set(parent, new Set()); + } + arrayUpdates.get(parent)!.add(node); } + } + + for (const array of arraysToClear) { + changes.push({ + fileName: array.getSourceFile().fileName, + textChanges: [ + { + span: {start: array.getStart(), length: array.getWidth()}, + newText: '[]', + }, + ], + }); + } + + for (const [array, toRemove] of arrayUpdates) { + if (arraysToClear.has(array)) { + continue; + } + + const file = array.getSourceFile(); + const newArray = tss.factory.updateArrayLiteralExpression( + array, + array.elements.filter((el) => !toRemove.has(el)), + ); changes.push({ fileName: file.fileName, textChanges: [ { - span: {start: importsArray.getStart(), length: importsArray.getWidth()}, - newText, + span: {start: array.getStart(), length: array.getWidth()}, + newText: tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file), }, ], });