Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crisbeto wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9031e8d
fix(compiler-cli): make the unused imports diagnostic easier to read
crisbeto Nov 1, 2024
b506933
refactor(language-service): update code fix for unused imports
crisbeto Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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,
);
}
Expand All @@ -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;
Expand All @@ -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;
}
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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;
}
}
28 changes: 8 additions & 20 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ runInEachFileSystem(() => {
export class TestCmp {}

@Component({
template: '',
template: '',
selector: 'target-cmp',
standalone: false,
})
Expand Down Expand Up @@ -432,7 +432,7 @@ runInEachFileSystem(() => {
export class TestCmp {}

@Component({
template: '',
template: '',
selector: 'target-cmp',
standalone: false,
})
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
32 changes: 24 additions & 8 deletions packages/language-service/src/codefixes/fix_unused_standalone_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,20 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = {
}

const node = findFirstMatchingNode(file, {
filter: (current): current is tss.ArrayLiteralExpression =>
current.getStart() === start &&
current.getWidth() === length &&
tss.isArrayLiteralExpression(current),
filter: (
current,
): current is tss.PropertyAssignment & {initializer: tss.ArrayLiteralExpression} =>
tss.isPropertyAssignment(current) &&
tss.isArrayLiteralExpression(current.initializer) &&
current.name.getStart() === start &&
current.name.getWidth() === length,
});

if (node === null) {
continue;
}

const importsArray = node.initializer;
let newText: string;

// If `relatedInformation` is empty, it means that all the imports are unused.
Expand All @@ -50,11 +54,23 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = {
// 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.map((info) => `${info.start}-${info.length}`),
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(
node,
node.elements.filter((el) => !excludeRanges.has(`${el.getStart()}-${el.getWidth()}`)),
importsArray,
importsArray.elements.filter(
(el) => !excludeRanges.has(`${el.getStart()}-${el.getWidth()}`),
),
);

newText = tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file);
Expand All @@ -64,7 +80,7 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = {
fileName: file.fileName,
textChanges: [
{
span: {start, length},
span: {start: importsArray.getStart(), length: importsArray.getWidth()},
newText,
},
],
Expand Down