From fee1df34ce7cea5d76832284d542efa198e7509f Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 14 Feb 2018 13:52:48 -0800 Subject: [PATCH] Implement ts.OrganizeImports.removeUnusedImports TODO: Still need to add support for organizing imports in ambient modules --- src/harness/unittests/organizeImports.ts | 95 ++++++++++- src/services/organizeImports.ts | 152 +++++++++++++----- src/services/services.ts | 2 +- .../CoalesceMultipleModules.ts | 2 + .../organizeImports/JsxFactoryUnusedTs.ts | 6 + .../organizeImports/JsxFactoryUnusedTsx.ts | 7 + .../organizeImports/JsxFactoryUsed.ts | 11 ++ .../organizeImports/UnusedTrivia1.ts | 6 + .../organizeImports/UnusedTrivia2.ts | 12 ++ .../reference/organizeImports/Unused_All.ts | 8 + .../reference/organizeImports/Unused_Some.ts | 13 ++ 11 files changed, 273 insertions(+), 41 deletions(-) create mode 100644 tests/baselines/reference/organizeImports/JsxFactoryUnusedTs.ts create mode 100644 tests/baselines/reference/organizeImports/JsxFactoryUnusedTsx.ts create mode 100644 tests/baselines/reference/organizeImports/JsxFactoryUsed.ts create mode 100644 tests/baselines/reference/organizeImports/UnusedTrivia1.ts create mode 100644 tests/baselines/reference/organizeImports/UnusedTrivia2.ts create mode 100644 tests/baselines/reference/organizeImports/Unused_All.ts create mode 100644 tests/baselines/reference/organizeImports/Unused_Some.ts diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index 8d7b0904eb7..0b9781462a4 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -175,6 +175,17 @@ export default function F2(); `, }; + const reactLibFile = { + path: "/react.ts", + content: ` +export const React = { +createElement: (_type, _props, _children) => {}, +}; + +export const Other = 1; +`, + }; + // Don't bother to actually emit a baseline for this. it("NoImports", () => { const testFile = { @@ -198,6 +209,30 @@ NS.F1(); D(); F1(); F2(); +`, + }, + libFile); + + testOrganizeImports("Unused_Some", + { + path: "/test.ts", + content: ` +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; + +D(); +`, + }, + libFile); + + testOrganizeImports("Unused_All", + { + path: "/test.ts", + content: ` +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; `, }, libFile); @@ -217,7 +252,7 @@ D(); }, libFile); - // tslint:disable no-invalid-template-strings + // tslint:disable no-invalid-template-strings testOrganizeImports("MoveToTop_Invalid", { path: "/test.ts", @@ -234,7 +269,7 @@ D(); `, }, libFile); - // tslint:enable no-invalid-template-strings + // tslint:enable no-invalid-template-strings testOrganizeImports("CoalesceMultipleModules", { @@ -244,10 +279,11 @@ import { d } from "lib1"; import { b } from "lib1"; import { c } from "lib2"; import { a } from "lib2"; +a + b + c + d; `, }, - { path: "/lib1.ts", content: "" }, - { path: "/lib2.ts", content: "" }); + { path: "/lib1.ts", content: "export const b = 1, d = 2;" }, + { path: "/lib2.ts", content: "export const a = 3, c = 4;" }); testOrganizeImports("CoalesceTrivia", { @@ -273,6 +309,56 @@ F2(); { path: "/lib1.ts", content: "" }, { path: "/lib2.ts", content: "" }); + testOrganizeImports("UnusedTrivia1", + { + path: "/test.ts", + content: ` +/*A*/import /*B*/ { /*C*/ F1 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/;/*H*/ //I +`, + }, + libFile); + + testOrganizeImports("UnusedTrivia2", + { + path: "/test.ts", + content: ` +/*A*/import /*B*/ { /*C*/ F1 /*D*/, /*E*/ F2 /*F*/ } /*G*/ from /*H*/ "lib" /*I*/;/*J*/ //K + +F1(); +`, + }, + libFile); + + testOrganizeImports("JsxFactoryUsed", + { + path: "/test.tsx", + content: ` +import { React, Other } from "react"; + +
; +`, + }, + reactLibFile); + + // This is descriptive, rather than normative + testOrganizeImports("JsxFactoryUnusedTsx", + { + path: "/test.tsx", + content: ` +import { React, Other } from "react"; +`, + }, + reactLibFile); + + testOrganizeImports("JsxFactoryUnusedTs", + { + path: "/test.ts", + content: ` +import { React, Other } from "react"; +`, + }, + reactLibFile); + function testOrganizeImports(testName: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) { it(testName, () => runBaseline(`organizeImports/${testName}.ts`, testFile, ...otherFiles)); } @@ -298,6 +384,7 @@ F2(); function makeLanguageService(...files: TestFSWithWatch.FileOrFolder[]) { const host = projectSystem.createServerHost(files); const projectService = projectSystem.createProjectService(host, { useSingleInferredProject: true }); + projectService.setCompilerOptionsForInferredProjects({ jsx: files.some(f => f.path.endsWith("x")) ? JsxEmit.React : JsxEmit.None }); files.forEach(f => projectService.openClientFile(f.path)); return projectService.inferredProjects[0].getLanguageService(); } diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index e75db1aa59d..4a03864207e 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -1,9 +1,17 @@ /* @internal */ namespace ts.OrganizeImports { + + /** + * Organize imports by: + * 1) Removing unused imports + * 2) Coalescing imports from the same module + * 3) Sorting imports + */ export function organizeImports( sourceFile: SourceFile, formatContext: formatting.FormatContext, - host: LanguageServiceHost) { + host: LanguageServiceHost, + program: Program) { // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) @@ -21,7 +29,7 @@ namespace ts.OrganizeImports { const newImportDecls = flatMap(sortedImportGroups, importGroup => getExternalModuleName(importGroup[0].moduleSpecifier) - ? coalesceImports(removeUnusedImports(importGroup)) + ? coalesceImports(removeUnusedImports(importGroup, sourceFile, program)) : importGroup); const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext }); @@ -47,8 +55,73 @@ namespace ts.OrganizeImports { return changeTracker.getChanges(); } - function removeUnusedImports(oldImports: ReadonlyArray) { - return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020) + function removeUnusedImports(oldImports: ReadonlyArray, sourceFile: SourceFile, program: Program) { + const typeChecker = program.getTypeChecker(); + const jsxNamespace = typeChecker.getJsxNamespace(); + const jsxContext = sourceFile.languageVariant === LanguageVariant.JSX && program.getCompilerOptions().jsx; + + const usedImports: ImportDeclaration[] = []; + + for (const importDecl of oldImports) { + const {importClause} = importDecl; + + if (!importClause) { + // Imports without import clauses are assumed to be included for their side effects and are not removed. + usedImports.push(importDecl); + continue; + } + + let { name, namedBindings } = importClause; + + // Default import + if (name && !isDeclarationUsed(name)) { + name = undefined; + } + + if (namedBindings) { + if (isNamespaceImport(namedBindings)) { + // Namespace import + if (!isDeclarationUsed(namedBindings.name)) { + namedBindings = undefined; + } + } + else { + // List of named imports + const newElements = namedBindings.elements.filter(e => isDeclarationUsed(e.propertyName || e.name)); + if (newElements.length < namedBindings.elements.length) { + namedBindings = newElements.length + ? updateNamedImports(namedBindings, newElements) + : undefined; + } + } + } + + if (name || namedBindings) { + usedImports.push(updateImportDeclarationAndClause(importDecl, name, namedBindings)); + } + } + + return usedImports; + + function isDeclarationUsed(identifier: Identifier) { + const symbol = typeChecker.getSymbolAtLocation(identifier); + + // Be lenient with invalid code. + if (symbol === undefined) { + return true; + } + + // The JSX factory symbol is always used. + if (jsxContext && symbol.name === jsxNamespace) { + return true; + } + + const entries = FindAllReferences.getReferenceEntriesForNode(identifier.pos, identifier, program, [sourceFile], { + isCancellationRequested: () => false, + throwIfCancellationRequested: () => { /*noop*/ }, + }); + return entries.length > 1; + } } function getExternalModuleName(specifier: Expression) { @@ -66,7 +139,7 @@ namespace ts.OrganizeImports { return importGroup; } - const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getImportParts(importGroup); + const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getCategorizedImports(importGroup); const coalescedImports: ImportDeclaration[] = []; @@ -78,19 +151,20 @@ namespace ts.OrganizeImports { // produce two import declarations in this special case. if (defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) { // Add the namespace import to the existing default ImportDeclaration. - const defaultImportClause = defaultImports[0].parent as ImportClause; + const defaultImport = defaultImports[0]; coalescedImports.push( - updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0])); + updateImportDeclarationAndClause(defaultImport, defaultImport.importClause.name, namespaceImports[0].importClause.namedBindings)); return coalescedImports; } - const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name)); + const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) => + compareIdentifiers((i1.importClause.namedBindings as NamespaceImport).name, (i2.importClause.namedBindings as NamespaceImport).name)); for (const namespaceImport of sortedNamespaceImports) { // Drop the name, if any coalescedImports.push( - updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport)); + updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause.namedBindings)); } if (defaultImports.length === 0 && namedImports.length === 0) { @@ -100,41 +174,48 @@ namespace ts.OrganizeImports { let newDefaultImport: Identifier | undefined; const newImportSpecifiers: ImportSpecifier[] = []; if (defaultImports.length === 1) { - newDefaultImport = defaultImports[0]; + newDefaultImport = defaultImports[0].importClause.name; } else { for (const defaultImport of defaultImports) { newImportSpecifiers.push( - createImportSpecifier(createIdentifier("default"), defaultImport)); + createImportSpecifier(createIdentifier("default"), defaultImport.importClause.name)); } } - newImportSpecifiers.push(...flatMap(namedImports, n => n.elements)); + newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause.namedBindings as NamedImports).elements)); const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) || compareIdentifiers(s1.name, s2.name)); - const importClause = defaultImports.length > 0 - ? defaultImports[0].parent as ImportClause - : namedImports[0].parent; + const importDecl = defaultImports.length > 0 + ? defaultImports[0] + : namedImports[0]; const newNamedImports = sortedImportSpecifiers.length === 0 ? undefined : namedImports.length === 0 ? createNamedImports(sortedImportSpecifiers) - : updateNamedImports(namedImports[0], sortedImportSpecifiers); + : updateNamedImports(namedImports[0].importClause.namedBindings as NamedImports, sortedImportSpecifiers); coalescedImports.push( - updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports)); + updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports)); return coalescedImports; - function getImportParts(importGroup: ReadonlyArray) { + /* + * Returns entire import declarations because they may already have been rewritten and + * may lack parent pointers. The desired parts can easily be recovered based on the + * categorization. + * + * NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`. + */ + function getCategorizedImports(importGroup: ReadonlyArray) { let importWithoutClause: ImportDeclaration | undefined; - const defaultImports: Identifier[] = []; - const namespaceImports: NamespaceImport[] = []; - const namedImports: NamedImports[] = []; + const defaultImports: ImportDeclaration[] = []; + const namespaceImports: ImportDeclaration[] = []; + const namedImports: ImportDeclaration[] = []; for (const importDeclaration of importGroup) { if (importDeclaration.importClause === undefined) { @@ -147,15 +228,15 @@ namespace ts.OrganizeImports { const { name, namedBindings } = importDeclaration.importClause; if (name) { - defaultImports.push(name); + defaultImports.push(importDeclaration); } if (namedBindings) { if (isNamespaceImport(namedBindings)) { - namespaceImports.push(namedBindings); + namespaceImports.push(importDeclaration); } else { - namedImports.push(namedBindings); + namedImports.push(importDeclaration); } } } @@ -171,20 +252,19 @@ namespace ts.OrganizeImports { function compareIdentifiers(s1: Identifier, s2: Identifier) { return compareStringsCaseSensitive(s1.text, s2.text); } + } - function updateImportDeclarationAndClause( - importClause: ImportClause, - name: Identifier | undefined, - namedBindings: NamedImportBindings | undefined) { + function updateImportDeclarationAndClause( + importDeclaration: ImportDeclaration, + name: Identifier | undefined, + namedBindings: NamedImportBindings | undefined) { - const importDeclaration = importClause.parent; - return updateImportDeclaration( - importDeclaration, - importDeclaration.decorators, - importDeclaration.modifiers, - updateImportClause(importClause, name, namedBindings), - importDeclaration.moduleSpecifier); - } + return updateImportDeclaration( + importDeclaration, + importDeclaration.decorators, + importDeclaration.modifiers, + updateImportClause(importDeclaration.importClause, name, namedBindings), + importDeclaration.moduleSpecifier); } /* internal */ // Exported for testing diff --git a/src/services/services.ts b/src/services/services.ts index b5d6f0ec2a6..1aecef09e90 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1855,7 +1855,7 @@ namespace ts { const sourceFile = getValidSourceFile(scope.fileName); const formatContext = formatting.getFormatContext(formatOptions); - return OrganizeImports.organizeImports(sourceFile, formatContext, host); + return OrganizeImports.organizeImports(sourceFile, formatContext, host, program); } function applyCodeActionCommand(action: CodeActionCommand): Promise; diff --git a/tests/baselines/reference/organizeImports/CoalesceMultipleModules.ts b/tests/baselines/reference/organizeImports/CoalesceMultipleModules.ts index 6278722f2a9..c321f0f8fc5 100644 --- a/tests/baselines/reference/organizeImports/CoalesceMultipleModules.ts +++ b/tests/baselines/reference/organizeImports/CoalesceMultipleModules.ts @@ -4,8 +4,10 @@ import { d } from "lib1"; import { b } from "lib1"; import { c } from "lib2"; import { a } from "lib2"; +a + b + c + d; // ==ORGANIZED== import { b, d } from "lib1"; import { a, c } from "lib2"; +a + b + c + d; diff --git a/tests/baselines/reference/organizeImports/JsxFactoryUnusedTs.ts b/tests/baselines/reference/organizeImports/JsxFactoryUnusedTs.ts new file mode 100644 index 00000000000..60afb95a192 --- /dev/null +++ b/tests/baselines/reference/organizeImports/JsxFactoryUnusedTs.ts @@ -0,0 +1,6 @@ +// ==ORIGINAL== + +import { React, Other } from "react"; + +// ==ORGANIZED== + diff --git a/tests/baselines/reference/organizeImports/JsxFactoryUnusedTsx.ts b/tests/baselines/reference/organizeImports/JsxFactoryUnusedTsx.ts new file mode 100644 index 00000000000..6a97e7f660a --- /dev/null +++ b/tests/baselines/reference/organizeImports/JsxFactoryUnusedTsx.ts @@ -0,0 +1,7 @@ +// ==ORIGINAL== + +import { React, Other } from "react"; + +// ==ORGANIZED== + +import { React } from "react"; diff --git a/tests/baselines/reference/organizeImports/JsxFactoryUsed.ts b/tests/baselines/reference/organizeImports/JsxFactoryUsed.ts new file mode 100644 index 00000000000..430a5b12cd4 --- /dev/null +++ b/tests/baselines/reference/organizeImports/JsxFactoryUsed.ts @@ -0,0 +1,11 @@ +// ==ORIGINAL== + +import { React, Other } from "react"; + +
; + +// ==ORGANIZED== + +import { React } from "react"; + +
; diff --git a/tests/baselines/reference/organizeImports/UnusedTrivia1.ts b/tests/baselines/reference/organizeImports/UnusedTrivia1.ts new file mode 100644 index 00000000000..7c25f40e6c2 --- /dev/null +++ b/tests/baselines/reference/organizeImports/UnusedTrivia1.ts @@ -0,0 +1,6 @@ +// ==ORIGINAL== + +/*A*/import /*B*/ { /*C*/ F1 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/;/*H*/ //I + +// ==ORGANIZED== + diff --git a/tests/baselines/reference/organizeImports/UnusedTrivia2.ts b/tests/baselines/reference/organizeImports/UnusedTrivia2.ts new file mode 100644 index 00000000000..f853015303e --- /dev/null +++ b/tests/baselines/reference/organizeImports/UnusedTrivia2.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +/*A*/import /*B*/ { /*C*/ F1 /*D*/, /*E*/ F2 /*F*/ } /*G*/ from /*H*/ "lib" /*I*/;/*J*/ //K + +F1(); + +// ==ORGANIZED== + +/*A*/ import { /*C*/ F1 /*D*/ } /*G*/ from "lib" /*I*/; /*J*/ //K + + +F1(); diff --git a/tests/baselines/reference/organizeImports/Unused_All.ts b/tests/baselines/reference/organizeImports/Unused_All.ts new file mode 100644 index 00000000000..bef217724b7 --- /dev/null +++ b/tests/baselines/reference/organizeImports/Unused_All.ts @@ -0,0 +1,8 @@ +// ==ORIGINAL== + +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; + +// ==ORGANIZED== + diff --git a/tests/baselines/reference/organizeImports/Unused_Some.ts b/tests/baselines/reference/organizeImports/Unused_Some.ts new file mode 100644 index 00000000000..2f0aaa6d499 --- /dev/null +++ b/tests/baselines/reference/organizeImports/Unused_Some.ts @@ -0,0 +1,13 @@ +// ==ORIGINAL== + +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; + +D(); + +// ==ORGANIZED== + +import D from "lib"; + +D();