Skip to content

Commit 72a7194

Browse files
amcaseyamcasey
authored
Merge pull request #22007 from amcasey/UnusedImports
Implement ts.OrganizeImports.removeUnusedImports
2 parents 4f30970 + cc386d2 commit 72a7194

File tree

11 files changed

+274
-42
lines changed

11 files changed

+274
-42
lines changed

‎src/harness/unittests/organizeImports.ts

Lines changed: 91 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,17 @@ export default function F2();
175175
`,
176176
};
177177

178+
const reactLibFile = {
179+
path: "/react.ts",
180+
content: `
181+
export const React = {
182+
createElement: (_type, _props, _children) => {},
183+
};
184+
185+
export const Other = 1;
186+
`,
187+
};
188+
178189
// Don't bother to actually emit a baseline for this.
179190
it("NoImports", () => {
180191
const testFile = {
@@ -198,6 +209,30 @@ NS.F1();
198209
D();
199210
F1();
200211
F2();
212+
`,
213+
},
214+
libFile);
215+
216+
testOrganizeImports("Unused_Some",
217+
{
218+
path: "/test.ts",
219+
content: `
220+
import { F1, F2 } from "lib";
221+
import * as NS from "lib";
222+
import D from "lib";
223+
224+
D();
225+
`,
226+
},
227+
libFile);
228+
229+
testOrganizeImports("Unused_All",
230+
{
231+
path: "/test.ts",
232+
content: `
233+
import { F1, F2 } from "lib";
234+
import * as NS from "lib";
235+
import D from "lib";
201236
`,
202237
},
203238
libFile);
@@ -217,7 +252,7 @@ D();
217252
},
218253
libFile);
219254

220-
// tslint:disable no-invalid-template-strings
255+
// tslint:disable no-invalid-template-strings
221256
testOrganizeImports("MoveToTop_Invalid",
222257
{
223258
path: "/test.ts",
@@ -234,7 +269,7 @@ D();
234269
`,
235270
},
236271
libFile);
237-
// tslint:enable no-invalid-template-strings
272+
// tslint:enable no-invalid-template-strings
238273

239274
testOrganizeImports("CoalesceMultipleModules",
240275
{
@@ -244,10 +279,11 @@ import { d } from "lib1";
244279
import { b } from "lib1";
245280
import { c } from "lib2";
246281
import { a } from "lib2";
282+
a + b + c + d;
247283
`,
248284
},
249-
{ path: "/lib1.ts", content: "" },
250-
{ path: "/lib2.ts", content: "" });
285+
{ path: "/lib1.ts", content: "export const b = 1, d = 2;" },
286+
{ path: "/lib2.ts", content: "export const a = 3, c = 4;" });
251287

252288
testOrganizeImports("CoalesceTrivia",
253289
{
@@ -273,6 +309,56 @@ F2();
273309
{ path: "/lib1.ts", content: "" },
274310
{ path: "/lib2.ts", content: "" });
275311

312+
testOrganizeImports("UnusedTrivia1",
313+
{
314+
path: "/test.ts",
315+
content: `
316+
/*A*/import /*B*/ { /*C*/ F1 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/;/*H*/ //I
317+
`,
318+
},
319+
libFile);
320+
321+
testOrganizeImports("UnusedTrivia2",
322+
{
323+
path: "/test.ts",
324+
content: `
325+
/*A*/import /*B*/ { /*C*/ F1 /*D*/, /*E*/ F2 /*F*/ } /*G*/ from /*H*/ "lib" /*I*/;/*J*/ //K
326+
327+
F1();
328+
`,
329+
},
330+
libFile);
331+
332+
testOrganizeImports("JsxFactoryUsed",
333+
{
334+
path: "/test.tsx",
335+
content: `
336+
import { React, Other } from "react";
337+
338+
<div/>;
339+
`,
340+
},
341+
reactLibFile);
342+
343+
// This is descriptive, rather than normative
344+
testOrganizeImports("JsxFactoryUnusedTsx",
345+
{
346+
path: "/test.tsx",
347+
content: `
348+
import { React, Other } from "react";
349+
`,
350+
},
351+
reactLibFile);
352+
353+
testOrganizeImports("JsxFactoryUnusedTs",
354+
{
355+
path: "/test.ts",
356+
content: `
357+
import { React, Other } from "react";
358+
`,
359+
},
360+
reactLibFile);
361+
276362
function testOrganizeImports(testName: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) {
277363
it(testName, () => runBaseline(`organizeImports/${testName}.ts`, testFile, ...otherFiles));
278364
}
@@ -298,6 +384,7 @@ F2();
298384
function makeLanguageService(...files: TestFSWithWatch.FileOrFolder[]) {
299385
const host = projectSystem.createServerHost(files);
300386
const projectService = projectSystem.createProjectService(host, { useSingleInferredProject: true });
387+
projectService.setCompilerOptionsForInferredProjects({ jsx: files.some(f => f.path.endsWith("x")) ? JsxEmit.React : JsxEmit.None });
301388
files.forEach(f => projectService.openClientFile(f.path));
302389
return projectService.inferredProjects[0].getLanguageService();
303390
}

‎src/services/organizeImports.ts

Lines changed: 117 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,17 @@
11
/* @internal */
22
namespace ts.OrganizeImports {
3+
4+
/**
5+
* Organize imports by:
6+
* 1) Removing unused imports
7+
* 2) Coalescing imports from the same module
8+
* 3) Sorting imports
9+
*/
310
export function organizeImports(
411
sourceFile: SourceFile,
512
formatContext: formatting.FormatContext,
6-
host: LanguageServiceHost) {
13+
host: LanguageServiceHost,
14+
program: Program) {
715

816
// TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule)
917

@@ -21,7 +29,7 @@ namespace ts.OrganizeImports {
2129

2230
const newImportDecls = flatMap(sortedImportGroups, importGroup =>
2331
getExternalModuleName(importGroup[0].moduleSpecifier)
24-
? coalesceImports(removeUnusedImports(importGroup))
32+
? coalesceImports(removeUnusedImports(importGroup, sourceFile, program))
2533
: importGroup);
2634

2735
const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext });
@@ -47,8 +55,73 @@ namespace ts.OrganizeImports {
4755
return changeTracker.getChanges();
4856
}
4957

50-
function removeUnusedImports(oldImports: ReadonlyArray<ImportDeclaration>) {
51-
return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020)
58+
function removeUnusedImports(oldImports: ReadonlyArray<ImportDeclaration>, sourceFile: SourceFile, program: Program) {
59+
const typeChecker = program.getTypeChecker();
60+
const jsxNamespace = typeChecker.getJsxNamespace();
61+
const jsxContext = sourceFile.languageVariant === LanguageVariant.JSX && program.getCompilerOptions().jsx;
62+
63+
const usedImports: ImportDeclaration[] = [];
64+
65+
for (const importDecl of oldImports) {
66+
const {importClause} = importDecl;
67+
68+
if (!importClause) {
69+
// Imports without import clauses are assumed to be included for their side effects and are not removed.
70+
usedImports.push(importDecl);
71+
continue;
72+
}
73+
74+
let { name, namedBindings } = importClause;
75+
76+
// Default import
77+
if (name && !isDeclarationUsed(name)) {
78+
name = undefined;
79+
}
80+
81+
if (namedBindings) {
82+
if (isNamespaceImport(namedBindings)) {
83+
// Namespace import
84+
if (!isDeclarationUsed(namedBindings.name)) {
85+
namedBindings = undefined;
86+
}
87+
}
88+
else {
89+
// List of named imports
90+
const newElements = namedBindings.elements.filter(e => isDeclarationUsed(e.propertyName || e.name));
91+
if (newElements.length < namedBindings.elements.length) {
92+
namedBindings = newElements.length
93+
? updateNamedImports(namedBindings, newElements)
94+
: undefined;
95+
}
96+
}
97+
}
98+
99+
if (name || namedBindings) {
100+
usedImports.push(updateImportDeclarationAndClause(importDecl, name, namedBindings));
101+
}
102+
}
103+
104+
return usedImports;
105+
106+
function isDeclarationUsed(identifier: Identifier) {
107+
const symbol = typeChecker.getSymbolAtLocation(identifier);
108+
109+
// Be lenient with invalid code.
110+
if (symbol === undefined) {
111+
return true;
112+
}
113+
114+
// The JSX factory symbol is always used.
115+
if (jsxContext && symbol.name === jsxNamespace) {
116+
return true;
117+
}
118+
119+
const entries = FindAllReferences.getReferenceEntriesForNode(identifier.pos, identifier, program, [sourceFile], {
120+
isCancellationRequested: () => false,
121+
throwIfCancellationRequested: () => { /*noop*/ },
122+
}).filter(e => e.type === "node" && e.node.getSourceFile() === sourceFile);
123+
return entries.length > 1;
124+
}
52125
}
53126

54127
function getExternalModuleName(specifier: Expression) {
@@ -66,7 +139,7 @@ namespace ts.OrganizeImports {
66139
return importGroup;
67140
}
68141

69-
const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getImportParts(importGroup);
142+
const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getCategorizedImports(importGroup);
70143

71144
const coalescedImports: ImportDeclaration[] = [];
72145

@@ -78,19 +151,20 @@ namespace ts.OrganizeImports {
78151
// produce two import declarations in this special case.
79152
if (defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
80153
// Add the namespace import to the existing default ImportDeclaration.
81-
const defaultImportClause = defaultImports[0].parent as ImportClause;
154+
const defaultImport = defaultImports[0];
82155
coalescedImports.push(
83-
updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0]));
156+
updateImportDeclarationAndClause(defaultImport, defaultImport.importClause.name, namespaceImports[0].importClause.namedBindings));
84157

85158
return coalescedImports;
86159
}
87160

88-
const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name));
161+
const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) =>
162+
compareIdentifiers((i1.importClause.namedBindings as NamespaceImport).name, (i2.importClause.namedBindings as NamespaceImport).name));
89163

90164
for (const namespaceImport of sortedNamespaceImports) {
91165
// Drop the name, if any
92166
coalescedImports.push(
93-
updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport));
167+
updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause.namedBindings));
94168
}
95169

96170
if (defaultImports.length === 0 && namedImports.length === 0) {
@@ -100,41 +174,48 @@ namespace ts.OrganizeImports {
100174
let newDefaultImport: Identifier | undefined;
101175
const newImportSpecifiers: ImportSpecifier[] = [];
102176
if (defaultImports.length === 1) {
103-
newDefaultImport = defaultImports[0];
177+
newDefaultImport = defaultImports[0].importClause.name;
104178
}
105179
else {
106180
for (const defaultImport of defaultImports) {
107181
newImportSpecifiers.push(
108-
createImportSpecifier(createIdentifier("default"), defaultImport));
182+
createImportSpecifier(createIdentifier("default"), defaultImport.importClause.name));
109183
}
110184
}
111185

112-
newImportSpecifiers.push(...flatMap(namedImports, n => n.elements));
186+
newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause.namedBindings as NamedImports).elements));
113187

114188
const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) =>
115189
compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
116190
compareIdentifiers(s1.name, s2.name));
117191

118-
const importClause = defaultImports.length > 0
119-
? defaultImports[0].parent as ImportClause
120-
: namedImports[0].parent;
192+
const importDecl = defaultImports.length > 0
193+
? defaultImports[0]
194+
: namedImports[0];
121195

122196
const newNamedImports = sortedImportSpecifiers.length === 0
123197
? undefined
124198
: namedImports.length === 0
125199
? createNamedImports(sortedImportSpecifiers)
126-
: updateNamedImports(namedImports[0], sortedImportSpecifiers);
200+
: updateNamedImports(namedImports[0].importClause.namedBindings as NamedImports, sortedImportSpecifiers);
127201

128202
coalescedImports.push(
129-
updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports));
203+
updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports));
130204

131205
return coalescedImports;
132206

133-
function getImportParts(importGroup: ReadonlyArray<ImportDeclaration>) {
207+
/*
208+
* Returns entire import declarations because they may already have been rewritten and
209+
* may lack parent pointers. The desired parts can easily be recovered based on the
210+
* categorization.
211+
*
212+
* NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`.
213+
*/
214+
function getCategorizedImports(importGroup: ReadonlyArray<ImportDeclaration>) {
134215
let importWithoutClause: ImportDeclaration | undefined;
135-
const defaultImports: Identifier[] = [];
136-
const namespaceImports: NamespaceImport[] = [];
137-
const namedImports: NamedImports[] = [];
216+
const defaultImports: ImportDeclaration[] = [];
217+
const namespaceImports: ImportDeclaration[] = [];
218+
const namedImports: ImportDeclaration[] = [];
138219

139220
for (const importDeclaration of importGroup) {
140221
if (importDeclaration.importClause === undefined) {
@@ -147,15 +228,15 @@ namespace ts.OrganizeImports {
147228
const { name, namedBindings } = importDeclaration.importClause;
148229

149230
if (name) {
150-
defaultImports.push(name);
231+
defaultImports.push(importDeclaration);
151232
}
152233

153234
if (namedBindings) {
154235
if (isNamespaceImport(namedBindings)) {
155-
namespaceImports.push(namedBindings);
236+
namespaceImports.push(importDeclaration);
156237
}
157238
else {
158-
namedImports.push(namedBindings);
239+
namedImports.push(importDeclaration);
159240
}
160241
}
161242
}
@@ -171,20 +252,19 @@ namespace ts.OrganizeImports {
171252
function compareIdentifiers(s1: Identifier, s2: Identifier) {
172253
return compareStringsCaseSensitive(s1.text, s2.text);
173254
}
255+
}
174256

175-
function updateImportDeclarationAndClause(
176-
importClause: ImportClause,
177-
name: Identifier | undefined,
178-
namedBindings: NamedImportBindings | undefined) {
179-
180-
const importDeclaration = importClause.parent;
181-
return updateImportDeclaration(
182-
importDeclaration,
183-
importDeclaration.decorators,
184-
importDeclaration.modifiers,
185-
updateImportClause(importClause, name, namedBindings),
186-
importDeclaration.moduleSpecifier);
187-
}
257+
function updateImportDeclarationAndClause(
258+
importDeclaration: ImportDeclaration,
259+
name: Identifier | undefined,
260+
namedBindings: NamedImportBindings | undefined) {
261+
262+
return updateImportDeclaration(
263+
importDeclaration,
264+
importDeclaration.decorators,
265+
importDeclaration.modifiers,
266+
updateImportClause(importDeclaration.importClause, name, namedBindings),
267+
importDeclaration.moduleSpecifier);
188268
}
189269

190270
/* internal */ // Exported for testing

0 commit comments

Comments
 (0)