Skip to content

Commit

Permalink
fix(compiler-cli): report individual diagnostics for unused imports (#…
Browse files Browse the repository at this point in the history
…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
crisbeto authored and thePunderWoman committed Nov 11, 2024
1 parent 5ac0538 commit 086cb2c
Showing 3 changed files with 85 additions and 94 deletions.
Original file line number Diff line number Diff line change
@@ -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;
}
/** Gets the closest parent node of a certain type. */
function closestNode<T extends ts.Node>(
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 importsExpression;
}

return null;
}
25 changes: 7 additions & 18 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Original file line number Diff line number Diff line change
@@ -20,68 +20,71 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = {
getCodeActions: () => [],
fixIds: [FixIdForCodeFixesAll.FIX_UNUSED_STANDALONE_IMPORTS],
getAllCodeActions: ({diagnostics}) => {
const arrayUpdates = new Map<tss.ArrayLiteralExpression, Set<tss.Expression>>();
const arraysToClear = new Set<tss.ArrayLiteralExpression>();
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[]),
);
// 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);
}
}

const newArray = tss.factory.updateArrayLiteralExpression(
importsArray,
importsArray.elements.filter(
(el) => !excludeRanges.has(`${el.getStart()}-${el.getWidth()}`),
),
);
for (const array of arraysToClear) {
changes.push({
fileName: array.getSourceFile().fileName,
textChanges: [
{
span: {start: array.getStart(), length: array.getWidth()},
newText: '[]',
},
],
});
}

newText = tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file);
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),
},
],
});

0 comments on commit 086cb2c

Please sign in to comment.