Skip to content

Commit 9bbb01c

Browse files
crisbetothePunderWoman
crisbeto
authored and
thePunderWoman
committed
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
1 parent 7cff2b7 commit 9bbb01c

3 files changed

Lines changed: 85 additions & 94 deletions

File tree

‎packages/compiler-cli/src/ngtsc/validation/src/rules/unused_standalone_imports_rule.ts‎

Lines changed: 34 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import ts from 'typescript';
1010

11-
import {ErrorCode, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
11+
import {ErrorCode, makeDiagnostic} from '../../../diagnostics';
1212
import type {ImportedSymbolsTracker, Reference} from '../../../imports';
1313
import type {ClassDeclaration} from '../../../reflection';
1414
import type {
@@ -37,7 +37,7 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
3737
);
3838
}
3939

40-
checkNode(node: ts.Node): ts.Diagnostic | null {
40+
checkNode(node: ts.Node): ts.Diagnostic | ts.Diagnostic[] | null {
4141
if (!ts.isClassDeclaration(node)) {
4242
return null;
4343
}
@@ -72,38 +72,36 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
7272
return null;
7373
}
7474

75+
const propertyAssignment = closestNode(metadata.rawImports, ts.isPropertyAssignment);
7576
const category =
7677
this.typeCheckingConfig.unusedStandaloneImports === 'error'
7778
? ts.DiagnosticCategory.Error
7879
: ts.DiagnosticCategory.Warning;
7980

80-
if (unused.length === metadata.imports.length) {
81+
if (unused.length === metadata.imports.length && propertyAssignment !== null) {
8182
return makeDiagnostic(
8283
ErrorCode.UNUSED_STANDALONE_IMPORTS,
83-
this.getDiagnosticNode(metadata.rawImports),
84+
propertyAssignment.name,
8485
'All imports are unused',
8586
undefined,
8687
category,
8788
);
8889
}
8990

90-
return makeDiagnostic(
91-
ErrorCode.UNUSED_STANDALONE_IMPORTS,
92-
this.getDiagnosticNode(metadata.rawImports),
93-
'Imports array contains unused imports',
94-
unused.map((ref) => {
95-
return makeRelatedInformation(
96-
// Intentionally don't pass a message to `makeRelatedInformation` to make the diagnostic
97-
// less noisy. The node will already be highlighted so the user can see which node is
98-
// unused. Note that in the case where an origin can't be resolved, we fall back to
99-
// the original node's identifier so the user can still see the name. This can happen
100-
// when the unused is coming from an imports array within the same file.
101-
ref.getOriginForDiagnostics(metadata.rawImports!, ref.node.name),
102-
'',
103-
);
104-
}),
105-
category,
106-
);
91+
return unused.map((ref) => {
92+
const diagnosticNode =
93+
ref.getIdentityInExpression(metadata.rawImports!) ||
94+
ref.getIdentityIn(node.getSourceFile()) ||
95+
metadata.rawImports!;
96+
97+
return makeDiagnostic(
98+
ErrorCode.UNUSED_STANDALONE_IMPORTS,
99+
diagnosticNode,
100+
`${ref.node.name.text} is not used within the template of ${metadata.name}`,
101+
undefined,
102+
category,
103+
);
104+
});
107105
}
108106

109107
private getUnusedSymbols(
@@ -181,21 +179,22 @@ export class UnusedStandaloneImportsRule implements SourceFileValidatorRule {
181179
// symbol like an array of shared common components.
182180
return true;
183181
}
182+
}
184183

185-
/** Gets the node on which to report the diagnostic. */
186-
private getDiagnosticNode(importsExpression: ts.Expression): ts.Node {
187-
let current = importsExpression.parent;
188-
189-
while (current) {
190-
// Highlight the `imports:` part of the node instead of the entire node, because
191-
// imports arrays can be long which makes the diagnostic harder to scan visually.
192-
if (ts.isPropertyAssignment(current)) {
193-
return current.name;
194-
} else {
195-
current = current.parent;
196-
}
184+
/** Gets the closest parent node of a certain type. */
185+
function closestNode<T extends ts.Node>(
186+
start: ts.Node,
187+
predicate: (node: ts.Node) => node is T,
188+
): T | null {
189+
let current = start.parent;
190+
191+
while (current) {
192+
if (predicate(current)) {
193+
return current;
194+
} else {
195+
current = current.parent;
197196
}
198-
199-
return importsExpression;
200197
}
198+
199+
return null;
201200
}

‎packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7709,9 +7709,7 @@ suppress
77097709

77107710
const diags = env.driveDiagnostics();
77117711
expect(diags.length).toBe(1);
7712-
expect(diags[0].messageText).toBe('Imports array contains unused imports');
7713-
expect(diags[0].relatedInformation?.length).toBe(1);
7714-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
7712+
expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp');
77157713
});
77167714

77177715
it('should report when a pipe is not used within a template', () => {
@@ -7766,9 +7764,7 @@ suppress
77667764

77677765
const diags = env.driveDiagnostics();
77687766
expect(diags.length).toBe(1);
7769-
expect(diags[0].messageText).toBe('Imports array contains unused imports');
7770-
expect(diags[0].relatedInformation?.length).toBe(1);
7771-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedPipe');
7767+
expect(diags[0].messageText).toBe('UnusedPipe is not used within the template of MyComp');
77727768
});
77737769

77747770
it('should not report imports only used inside @defer blocks', () => {
@@ -7835,7 +7831,6 @@ suppress
78357831
const diags = env.driveDiagnostics();
78367832
expect(diags.length).toBe(1);
78377833
expect(diags[0].messageText).toBe('All imports are unused');
7838-
expect(diags[0].relatedInformation).toBeFalsy();
78397834
});
78407835

78417836
it('should not report unused imports coming from modules', () => {
@@ -7953,11 +7948,9 @@ suppress
79537948
);
79547949

79557950
const diags = env.driveDiagnostics();
7956-
expect(diags.length).toBe(1);
7957-
expect(diags[0].messageText).toBe('Imports array contains unused imports');
7958-
expect(diags[0].relatedInformation?.length).toBe(2);
7959-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('NgFor');
7960-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('PercentPipe');
7951+
expect(diags.length).toBe(2);
7952+
expect(diags[0].messageText).toBe('NgFor is not used within the template of MyComp');
7953+
expect(diags[1].messageText).toBe('PercentPipe is not used within the template of MyComp');
79617954
});
79627955

79637956
it('should report unused imports coming from a nested array from the same file', () => {
@@ -8017,9 +8010,7 @@ suppress
80178010

80188011
const diags = env.driveDiagnostics();
80198012
expect(diags.length).toBe(1);
8020-
expect(diags[0].messageText).toBe('Imports array contains unused imports');
8021-
expect(diags[0].relatedInformation?.length).toBe(1);
8022-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
8013+
expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp');
80238014
});
80248015

80258016
it('should report unused imports coming from an array used as the `imports` initializer', () => {
@@ -8068,9 +8059,7 @@ suppress
80688059

80698060
const diags = env.driveDiagnostics();
80708061
expect(diags.length).toBe(1);
8071-
expect(diags[0].messageText).toBe('Imports array contains unused imports');
8072-
expect(diags[0].relatedInformation?.length).toBe(1);
8073-
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('UnusedDir');
8062+
expect(diags[0].messageText).toBe('UnusedDir is not used within the template of MyComp');
80748063
});
80758064

80768065
it('should not report unused imports coming from an array through a spread expression from a different file', () => {

‎packages/language-service/src/codefixes/fix_unused_standalone_imports.ts‎

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -20,68 +20,71 @@ export const fixUnusedStandaloneImportsMeta: CodeActionMeta = {
2020
getCodeActions: () => [],
2121
fixIds: [FixIdForCodeFixesAll.FIX_UNUSED_STANDALONE_IMPORTS],
2222
getAllCodeActions: ({diagnostics}) => {
23+
const arrayUpdates = new Map<tss.ArrayLiteralExpression, Set<tss.Expression>>();
24+
const arraysToClear = new Set<tss.ArrayLiteralExpression>();
2325
const changes: tss.FileTextChanges[] = [];
2426

2527
for (const diag of diagnostics) {
26-
const {start, length, file, relatedInformation} = diag;
28+
const {start, length, file} = diag;
2729
if (file === undefined || start === undefined || length == undefined) {
2830
continue;
2931
}
3032

3133
const node = findFirstMatchingNode(file, {
32-
filter: (
33-
current,
34-
): current is tss.PropertyAssignment & {initializer: tss.ArrayLiteralExpression} =>
35-
tss.isPropertyAssignment(current) &&
36-
tss.isArrayLiteralExpression(current.initializer) &&
37-
current.name.getStart() === start &&
38-
current.name.getWidth() === length,
34+
filter: (n): n is tss.Expression => n.getStart() === start && n.getWidth() === length,
3935
});
36+
const parent = node?.parent || null;
4037

41-
if (node === null) {
38+
if (node === null || parent === null) {
4239
continue;
4340
}
4441

45-
const importsArray = node.initializer;
46-
let newText: string;
47-
48-
// If `relatedInformation` is empty, it means that all the imports are unused.
49-
// Replace the array with an empty array.
50-
if (relatedInformation === undefined || relatedInformation.length === 0) {
51-
newText = '[]';
52-
} else {
53-
// Otherwise each `relatedInformation` entry points to an unused import that should be
54-
// filtered out. We make a set of ranges corresponding to nodes which will be deleted and
55-
// remove all nodes that belong to the set.
56-
const excludeRanges = new Set(
57-
relatedInformation.reduce((ranges, info) => {
58-
// If the compiler can't resolve the unused import to an identifier within the array,
59-
// it falls back to reporting the identifier of the class declaration instead. In theory
60-
// that class could have the same offsets as the diagnostic location. It's a slim chance
61-
// that would happen, but we filter out reports from other files just in case.
62-
if (info.file === file) {
63-
ranges.push(`${info.start}-${info.length}`);
64-
}
65-
return ranges;
66-
}, [] as string[]),
67-
);
42+
// If the diagnostic is reported on the name of the `imports` array initializer, it means
43+
// that all imports are unused so we can clear the entire array. Otherwise if it's reported
44+
// on a single element, we only have to remove that element.
45+
if (
46+
tss.isPropertyAssignment(parent) &&
47+
parent.name === node &&
48+
tss.isArrayLiteralExpression(parent.initializer)
49+
) {
50+
arraysToClear.add(parent.initializer);
51+
} else if (tss.isArrayLiteralExpression(parent)) {
52+
if (!arrayUpdates.has(parent)) {
53+
arrayUpdates.set(parent, new Set());
54+
}
55+
arrayUpdates.get(parent)!.add(node);
56+
}
57+
}
6858

69-
const newArray = tss.factory.updateArrayLiteralExpression(
70-
importsArray,
71-
importsArray.elements.filter(
72-
(el) => !excludeRanges.has(`${el.getStart()}-${el.getWidth()}`),
73-
),
74-
);
59+
for (const array of arraysToClear) {
60+
changes.push({
61+
fileName: array.getSourceFile().fileName,
62+
textChanges: [
63+
{
64+
span: {start: array.getStart(), length: array.getWidth()},
65+
newText: '[]',
66+
},
67+
],
68+
});
69+
}
7570

76-
newText = tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file);
71+
for (const [array, toRemove] of arrayUpdates) {
72+
if (arraysToClear.has(array)) {
73+
continue;
7774
}
7875

76+
const file = array.getSourceFile();
77+
const newArray = tss.factory.updateArrayLiteralExpression(
78+
array,
79+
array.elements.filter((el) => !toRemove.has(el)),
80+
);
81+
7982
changes.push({
8083
fileName: file.fileName,
8184
textChanges: [
8285
{
83-
span: {start: importsArray.getStart(), length: importsArray.getWidth()},
84-
newText,
86+
span: {start: array.getStart(), length: array.getWidth()},
87+
newText: tss.createPrinter().printNode(tss.EmitHint.Unspecified, newArray, file),
8588
},
8689
],
8790
});

0 commit comments

Comments
 (0)