From 5656f35b6a1120599fb62291b16161f6f9b818b2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 31 Jan 2018 11:20:41 -0800 Subject: [PATCH 1/4] Introduce an organizeImports command In phase 1, it coalesces imports from the same module and sorts the results, but does not remove unused imports. Some trivia is lost during coalescing, but none should be duplicated. --- Jakefile.js | 1 + src/harness/harnessLanguageService.ts | 3 + src/harness/tsconfig.json | 1 + src/harness/unittests/organizeImports.ts | 426 ++++++++++++++++++ src/harness/unittests/session.ts | 2 + src/server/client.ts | 4 + src/server/protocol.ts | 25 + src/server/session.ts | 19 + src/services/services.ts | 287 ++++++++++++ src/services/types.ts | 3 + .../reference/api/tsserverlibrary.d.ts | 21 + tests/baselines/reference/api/typescript.d.ts | 2 + .../organizeImports/CoalesceTrivia.ts | 15 + .../reference/organizeImports/MoveToTop.ts | 18 + .../reference/organizeImports/Simple.ts | 20 + .../reference/organizeImports/SortTrivia.ts | 10 + 16 files changed, 857 insertions(+) create mode 100644 src/harness/unittests/organizeImports.ts create mode 100644 tests/baselines/reference/organizeImports/CoalesceTrivia.ts create mode 100644 tests/baselines/reference/organizeImports/MoveToTop.ts create mode 100644 tests/baselines/reference/organizeImports/Simple.ts create mode 100644 tests/baselines/reference/organizeImports/SortTrivia.ts diff --git a/Jakefile.js b/Jakefile.js index 9e8c51a306eea..d676926abac8d 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -141,6 +141,7 @@ var harnessSources = harnessCoreSources.concat([ "typingsInstaller.ts", "projectErrors.ts", "matchFiles.ts", + "organizeImports.ts", "initializeTSConfig.ts", "extractConstants.ts", "extractFunctions.ts", diff --git a/src/harness/harnessLanguageService.ts b/src/harness/harnessLanguageService.ts index 1c85acfb80e3f..d24f572d1d51d 100644 --- a/src/harness/harnessLanguageService.ts +++ b/src/harness/harnessLanguageService.ts @@ -522,6 +522,9 @@ namespace Harness.LanguageService { getApplicableRefactors(): ts.ApplicableRefactorInfo[] { throw new Error("Not supported on the shim."); } + organizeImports(_scope: ts.OrganizeImportsScope, _formatOptions: ts.FormatCodeSettings): ReadonlyArray { + throw new Error("Not supported on the shim."); + } getEmitOutput(fileName: string): ts.EmitOutput { return unwrapJSONCallResult(this.shim.getEmitOutput(fileName)); } diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index 25642ab517923..cd6dbc5e0bb85 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -117,6 +117,7 @@ "./unittests/tsserverProjectSystem.ts", "./unittests/tscWatchMode.ts", "./unittests/matchFiles.ts", + "./unittests/organizeImports.ts", "./unittests/initializeTSConfig.ts", "./unittests/compileOnSave.ts", "./unittests/typingsInstaller.ts", diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts new file mode 100644 index 0000000000000..5cac601d08e91 --- /dev/null +++ b/src/harness/unittests/organizeImports.ts @@ -0,0 +1,426 @@ +/// +/// + + +namespace ts { + describe("Organize imports", () => { + describe("Sort imports", () => { + it("No imports", () => { + assert.isEmpty(sortImports([])); + }); + + it("One import", () => { + const unsortedImports = parseImports(`import "lib";`); + const actualSortedImports = sortImports(unsortedImports); + const expectedSortedImports = unsortedImports; + assertListEqual(expectedSortedImports, actualSortedImports); + }); + + it("Stable - import kind", () => { + assertUnaffectedBySort( + `import "lib";`, + `import * as x from "lib";`, + `import x from "lib";`, + `import {x} from "lib";`); + }); + + it("Stable - default property alias", () => { + assertUnaffectedBySort( + `import x from "lib";`, + `import y from "lib";`); + }); + + it("Stable - module alias", () => { + assertUnaffectedBySort( + `import * as x from "lib";`, + `import * as y from "lib";`); + }); + + it("Stable - symbol", () => { + assertUnaffectedBySort( + `import {x} from "lib";`, + `import {y} from "lib";`); + }); + + it("Sort - non-relative vs non-relative", () => { + assertSortsBefore( + `import y from "lib1";`, + `import x from "lib2";`); + }); + + it("Sort - relative vs relative", () => { + assertSortsBefore( + `import y from "./lib1";`, + `import x from "./lib2";`); + }); + + it("Sort - invalid vs invalid", () => { + assertSortsBefore( + "import y from `${'lib1'}`;", + "import x from `${'lib2'}`;"); + }); + + it("Sort - relative vs non-relative", () => { + assertSortsBefore( + `import y from "lib";`, + `import x from "./lib";`); + }); + + it("Sort - non-relative vs invalid", () => { + assertSortsBefore( + `import y from "lib";`, + "import x from `${'lib'}`;"); + }); + + it("Sort - relative vs invalid", () => { + assertSortsBefore( + `import y from "./lib";`, + "import x from `${'lib'}`;"); + }); + + function assertUnaffectedBySort(...importStrings: string[]) { + const unsortedImports1 = parseImports(...importStrings); + assertListEqual(unsortedImports1, sortImports(unsortedImports1)); + + const unsortedImports2 = reverse(unsortedImports1); + assertListEqual(unsortedImports2, sortImports(unsortedImports2)); + } + + function assertSortsBefore(importString1: string, importString2: string) { + const imports = parseImports(importString1, importString2); + assertListEqual(imports, sortImports(imports)); + assertListEqual(imports, sortImports(reverse(imports))); + } + }); + + describe("Coalesce imports", () => { + it("No imports", () => { + assert.isEmpty(coalesceImports([])); + }); + + it("Sort specifiers", () => { + const sortedImports = parseImports(`import { default as m, a as n, b, y, z as o } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports(`import { a as n, b, default as m, y, z as o } from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine side-effect-only imports", () => { + const sortedImports = parseImports( + `import "lib";`, + `import "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports(`import "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine namespace imports", () => { + const sortedImports = parseImports( + `import * as x from "lib";`, + `import * as y from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine default imports", () => { + const sortedImports = parseImports( + `import x from "lib";`, + `import y from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports(`import { default as x, default as y } from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine property imports", () => { + const sortedImports = parseImports( + `import { x } from "lib";`, + `import { y as z } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports(`import { x, y as z } from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine side-effect-only import with namespace import", () => { + const sortedImports = parseImports( + `import "lib";`, + `import * as x from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine side-effect-only import with default import", () => { + const sortedImports = parseImports( + `import "lib";`, + `import x from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine side-effect-only import with property import", () => { + const sortedImports = parseImports( + `import "lib";`, + `import { x } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine namespace import with default import", () => { + const sortedImports = parseImports( + `import * as x from "lib";`, + `import y from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports( + `import y, * as x from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine namespace import with property import", () => { + const sortedImports = parseImports( + `import * as x from "lib";`, + `import { y } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine default import with property import", () => { + const sortedImports = parseImports( + `import x from "lib";`, + `import { y } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports( + `import x, { y } from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine many imports", () => { + const sortedImports = parseImports( + `import "lib";`, + `import * as y from "lib";`, + `import w from "lib";`, + `import { b } from "lib";`, + `import "lib";`, + `import * as x from "lib";`, + `import z from "lib";`, + `import { a } from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports( + `import "lib";`, + `import * as x from "lib";`, + `import * as y from "lib";`, + `import { a, b, default as w, default as z } from "lib";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + it("Combine imports from different modules", () => { + const sortedImports = parseImports( + `import { d } from "lib1";`, + `import { b } from "lib1";`, + `import { c } from "lib2";`, + `import { a } from "lib2";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = parseImports( + `import { b, d } from "lib1";`, + `import { a, c } from "lib2";`); + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + + // This is descriptive, rather than normative + it("Combine two namespace imports with one default import", () => { + const sortedImports = parseImports( + `import * as x from "lib";`, + `import * as y from "lib";`, + `import z from "lib";`); + const actualCoalescedImports = coalesceImports(sortedImports); + const expectedCoalescedImports = sortedImports; + assertListEqual(expectedCoalescedImports, actualCoalescedImports); + }); + }); + + describe("Baselines", () => { + + const libFile = { + path: "/lib.ts", + content: ` +export function F1(); +export default function F2(); +`, + }; + + testOrganizeImports("Simple", + { + path: "/test.ts", + content: ` +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; + +NS.F1(); +D(); +F1(); +F2(); +`, + }, + libFile); + + testOrganizeImports("MoveToTop", + { + path: "/test.ts", + content: ` +import { F1, F2 } from "lib"; +F1(); +F2(); +import * as NS from "lib"; +NS.F1(); +import D from "lib"; +D(); +`, + }, + libFile); + + testOrganizeImports("CoalesceTrivia", + { + path: "/test.ts", + content: ` +/*A*/import /*B*/ { /*C*/ F2 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/;/*H*/ //I +/*J*/import /*K*/ { /*L*/ F1 /*M*/ } /*N*/ from /*O*/ "lib" /*P*/;/*Q*/ //R + +F1(); +F2(); +`, + }, + libFile); + + testOrganizeImports("SortTrivia", + { + path: "/test.ts", + content: ` +/*A*/import /*B*/ "lib2" /*C*/;/*D*/ //E +/*F*/import /*G*/ "lib1" /*H*/;/*I*/ //J +`, + }, + { path: "/lib1.ts", content: "" }, + { path: "/lib2.ts", content: "" }); + + function testOrganizeImports(testName: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) { + it(testName, () => runBaseline(`organizeImports/${testName}.ts`, testFile, ...otherFiles)); + } + + function runBaseline(baselinePath: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) { + const { path: testPath, content: testContent } = testFile; + const languageService = makeLanguageService(testFile, ...otherFiles); + const changes = languageService.organizeImports({ type: "file", fileName: testPath }, testFormatOptions); + assert.equal(1, changes.length); + assert.equal(testPath, changes[0].fileName); + + Harness.Baseline.runBaseline(baselinePath, () => { + const data: string[] = []; + data.push(`// ==ORIGINAL==`); + data.push(testContent); + + data.push(`// ==ORGANIZED==`); + const newText = textChanges.applyChanges(testContent, changes[0].textChanges); + data.push(newText); + + return data.join(newLineCharacter); + }); + } + + function makeLanguageService(...files: TestFSWithWatch.FileOrFolder[]) { + const host = projectSystem.createServerHost(files); + const projectService = projectSystem.createProjectService(host, { useSingleInferredProject: true }); + files.forEach(f => projectService.openClientFile(f.path)); + return projectService.inferredProjects[0].getLanguageService(); + } + }); + + function parseImports(...importStrings: string[]): ReadonlyArray { + const sourceFile = createSourceFile("a.ts", importStrings.join("\n"), ScriptTarget.ES2015, /*setParentNodes*/ true, ScriptKind.TS); + const imports = filter(sourceFile.statements, isImportDeclaration); + assert.equal(importStrings.length, imports.length); + return imports; + } + + function assertEqual(node1?: Node, node2?: Node) { + if (node1 === undefined) { + assert.isUndefined(node2); + return; + } + else if (node2 === undefined) { + assert.isUndefined(node1); // Guaranteed to fail + return; + } + + assert.equal(node1.kind, node2.kind); + + switch(node1.kind) { + case SyntaxKind.ImportDeclaration: + const decl1 = node1 as ImportDeclaration; + const decl2 = node2 as ImportDeclaration; + assertEqual(decl1.importClause, decl2.importClause); + assertEqual(decl1.moduleSpecifier, decl2.moduleSpecifier); + break; + case SyntaxKind.ImportClause: + const clause1 = node1 as ImportClause; + const clause2 = node2 as ImportClause; + assertEqual(clause1.name, clause2.name); + assertEqual(clause1.namedBindings, clause2.namedBindings); + case SyntaxKind.NamespaceImport: + const nsi1 = node1 as NamespaceImport; + const nsi2 = node2 as NamespaceImport; + assertEqual(nsi1.name, nsi2.name); + break; + case SyntaxKind.NamedImports: + const ni1 = node1 as NamedImports; + const ni2 = node2 as NamedImports; + assertListEqual(ni1.elements, ni2.elements); + break; + case SyntaxKind.ImportSpecifier: + const is1 = node1 as ImportSpecifier; + const is2 = node2 as ImportSpecifier; + assertEqual(is1.name, is2.name); + assertEqual(is1.propertyName, is2.propertyName); + break; + case SyntaxKind.Identifier: + const id1 = node1 as Identifier; + const id2 = node2 as Identifier; + assert.equal(id1.text, id2.text); + break; + case SyntaxKind.StringLiteral: + case SyntaxKind.NoSubstitutionTemplateLiteral: + const sl1 = node1 as LiteralLikeNode; + const sl2 = node2 as LiteralLikeNode; + assert.equal(sl1.text, sl2.text); + break; + default: + assert.equal(node1.getText(), node2.getText()); + break; + } + } + + function assertListEqual(list1: ReadonlyArray, list2: ReadonlyArray) { + if (list1 === undefined || list2 === undefined) { + assert.isUndefined(list1); + assert.isUndefined(list2); + return; + } + + assert.equal(list1.length, list2.length); + for (let i = 0; i < list1.length; i++) { + assertEqual(list1[i], list2[i]); + } + } + + function reverse(list: ReadonlyArray) { + const result = []; + for (let i = list.length - 1; i >= 0; i--) { + result.push(list[i]); + } + return result; + } + }); +} \ No newline at end of file diff --git a/src/harness/unittests/session.ts b/src/harness/unittests/session.ts index 7d1e5b0816c87..765fc29ee49b7 100644 --- a/src/harness/unittests/session.ts +++ b/src/harness/unittests/session.ts @@ -262,6 +262,8 @@ namespace ts.server { CommandNames.GetApplicableRefactors, CommandNames.GetEditsForRefactor, CommandNames.GetEditsForRefactorFull, + CommandNames.OrganizeImports, + CommandNames.OrganizeImportsFull, ]; it("should not throw when commands are executed with invalid arguments", () => { diff --git a/src/server/client.ts b/src/server/client.ts index 8203475b06dbb..cee65c0e5a485 100644 --- a/src/server/client.ts +++ b/src/server/client.ts @@ -629,6 +629,10 @@ namespace ts.server { }; } + organizeImports(_scope: OrganizeImportsScope, _formatOptions: FormatCodeSettings): ReadonlyArray { + return notImplemented(); + } + private convertCodeEditsToTextChanges(edits: protocol.FileCodeEdits[]): FileTextChanges[] { return edits.map(edit => { const fileName = edit.fileName; diff --git a/src/server/protocol.ts b/src/server/protocol.ts index 45c08034e6e75..fbff4501133fe 100644 --- a/src/server/protocol.ts +++ b/src/server/protocol.ts @@ -113,6 +113,10 @@ namespace ts.server.protocol { /* @internal */ GetEditsForRefactorFull = "getEditsForRefactor-full", + OrganizeImports = "organizeImports", + /* @internal */ + OrganizeImportsFull = "organizeImports-full", + // NOTE: If updating this, be sure to also update `allCommandNames` in `harness/unittests/session.ts`. } @@ -547,6 +551,27 @@ namespace ts.server.protocol { renameFilename?: string; } + /** + * Organize imports by: + * 1) Removing unused imports + * 2) Coalescing imports from the same module + * 3) Sorting imports + */ + export interface OrganizeImportsRequest extends Request { + command: CommandTypes.OrganizeImports; + arguments: OrganizeImportsRequestArgs; + } + + export type OrganizeImportsScope = GetCombinedCodeFixScope; + + export interface OrganizeImportsRequestArgs { + scope: OrganizeImportsScope; + } + + export interface OrganizeImportsResponse extends Response { + edits: ReadonlyArray; + } + /** * Request for the available codefixes at a specific position. */ diff --git a/src/server/session.ts b/src/server/session.ts index 356ea0d254ea8..bff19d7bc232f 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -1597,6 +1597,19 @@ namespace ts.server { } } + private organizeImports({ scope }: protocol.OrganizeImportsRequestArgs, simplifiedResult: boolean): ReadonlyArray | ReadonlyArray { + Debug.assert(scope.type === "file"); + const { file, project } = this.getFileAndProject(scope.args); + const formatOptions = this.projectService.getFormatCodeOptions(file); + const changes = project.getLanguageService().organizeImports({ type: "file", fileName: file }, formatOptions); + if (simplifiedResult) { + return this.mapTextChangesToCodeEdits(project, changes); + } + else { + return changes; + } + } + private getCodeFixes(args: protocol.CodeFixRequestArgs, simplifiedResult: boolean): ReadonlyArray | ReadonlyArray { if (args.errorCodes.length === 0) { return undefined; @@ -2041,6 +2054,12 @@ namespace ts.server { }, [CommandNames.GetEditsForRefactorFull]: (request: protocol.GetEditsForRefactorRequest) => { return this.requiredResponse(this.getEditsForRefactor(request.arguments, /*simplifiedResult*/ false)); + }, + [CommandNames.OrganizeImports]: (request: protocol.OrganizeImportsRequest) => { + return this.requiredResponse(this.organizeImports(request.arguments, /*simplifiedResult*/ true)); + }, + [CommandNames.OrganizeImportsFull]: (request: protocol.OrganizeImportsRequest) => { + return this.requiredResponse(this.organizeImports(request.arguments, /*simplifiedResult*/ false)); } }); diff --git a/src/services/services.ts b/src/services/services.ts index 6df52dd804e84..22692f818e951 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1848,6 +1848,58 @@ namespace ts { return codefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext }); } + function organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray { + synchronizeHostData(); + Debug.assert(scope.type === "file"); + const sourceFile = getValidSourceFile(scope.fileName); + const formatContext = formatting.getFormatContext(formatOptions); + + // All of the (old) ImportDeclarations in the file, in syntactic order. + const oldImportDecls: ImportDeclaration[] = []; + + forEachChild(sourceFile, node => { + cancellationToken.throwIfCancellationRequested(); + if (isImportDeclaration(node)) { + oldImportDecls.push(node); + } + // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) + }); + + if (oldImportDecls.length === 0) { + return []; + } + + const usedImportDecls = removeUnusedImports(oldImportDecls); + const sortedImportDecls = sortImports(usedImportDecls); + const coalescedImportDecls = coalesceImports(sortedImportDecls); + + // All of the (new) ImportDeclarations in the file, in sorted order. + const newImportDecls = coalescedImportDecls; + + const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext }); + + // NB: Stopping before i === 0 + for (let i = oldImportDecls.length - 1; i > 0; i--) { + changeTracker.deleteNode(sourceFile, oldImportDecls[i]); + } + + if (newImportDecls.length === 0) { + changeTracker.deleteNode(sourceFile, oldImportDecls[0]); + } + else { + // Delete the surrounding trivia because it will have been retained in newImportDecls. + const replaceOptions = { + useNonAdjustedStartPosition: false, + useNonAdjustedEndPosition: false, + suffix: getNewLineOrDefaultFromHost(host, formatOptions), + }; + changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions); + } + + const changes = changeTracker.getChanges(); + return changes; + } + function applyCodeActionCommand(action: CodeActionCommand): Promise; function applyCodeActionCommand(action: CodeActionCommand[]): Promise; function applyCodeActionCommand(action: CodeActionCommand | CodeActionCommand[]): Promise; @@ -2143,6 +2195,7 @@ namespace ts { getCodeFixesAtPosition, getCombinedCodeFix, applyCodeActionCommand, + organizeImports, getEmitOutput, getNonBoundSourceFile, getSourceFile, @@ -2268,4 +2321,238 @@ namespace ts { } objectAllocator = getServicesObjectAllocator(); + + function removeUnusedImports(oldImports: ReadonlyArray) { + return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020) + } + + /* @internal */ // Internal for testing + export function sortImports(oldImports: ReadonlyArray) { + if (oldImports.length < 2) { + return oldImports; + } + + // NB: declaration order determines sort order + const enum ModuleNameKind { + NonRelative, + Relative, + Invalid, + } + + const importRecords = oldImports.map(createImportRecord); + + const sortedRecords = stableSort(importRecords, (import1, import2) => { + const { name: name1, kind: kind1 } = import1; + const { name: name2, kind: kind2 } = import2; + + if (kind1 !== kind2) { + return kind1 < kind2 + ? Comparison.LessThan + : Comparison.GreaterThan; + } + + // Note that we're using simple equality, retaining case-sensitivity. + if (name1 !== name2) { + return name1 < name2 + ? Comparison.LessThan + : Comparison.GreaterThan; + } + + return Comparison.EqualTo; + }); + + return sortedRecords.map(r => r.importDeclaration); + + function createImportRecord(importDeclaration: ImportDeclaration) { + const specifier = importDeclaration.moduleSpecifier; + const name = getExternalModuleName(specifier); + if (name) { + const isRelative = isExternalModuleNameRelative(name); + return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative } + } + + return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid }; + } + } + + function getExternalModuleName(specifier: Expression) { + return isStringLiteral(specifier) || isNoSubstitutionTemplateLiteral(specifier) + ? specifier.text + : undefined; + } + + /** + * @param sortedImports a non-empty list of ImportDeclarations, sorted by module name. + */ + function groupSortedImports(sortedImports: ReadonlyArray): ReadonlyArray> { + Debug.assert(length(sortedImports) > 0); + + const groups: ImportDeclaration[][] = []; + + let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier); + let group: ImportDeclaration[] = []; + + for (const importDeclaration of sortedImports) { + const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier); + if (moduleName && moduleName === groupName) { + group.push(importDeclaration); + } + else if (group.length) { + groups.push(group); + + groupName = moduleName; + group = [importDeclaration]; + } + } + + if (group.length) { + groups.push(group); + } + + return groups; + } + + /* @internal */ // Internal for testing + /** + * @param sortedImports a list of ImportDeclarations, sorted by module name. + */ + export function coalesceImports(sortedImports: ReadonlyArray) { + if (sortedImports.length === 0) { + return sortedImports; + } + + const coalescedImports: ImportDeclaration[] = []; + + const groupedImports = groupSortedImports(sortedImports); + for (const importGroup of groupedImports) { + + let seenImportWithoutClause = false; + + const defaultImports: Identifier[] = []; + const namespaceImports: NamespaceImport[] = []; + const namedImports: NamedImports[] = []; + + for (const importDeclaration of importGroup) { + if (importDeclaration.importClause === undefined) { + // Only the first such import is interesting - the others are redundant. + // Note: Unfortunately, we will lose trivia that was on this node. + if (!seenImportWithoutClause) { + coalescedImports.push(importDeclaration); + } + + seenImportWithoutClause = true; + continue; + } + + const { name, namedBindings } = importDeclaration.importClause; + + if (name) { + defaultImports.push(name); + } + + if (namedBindings) { + if (isNamespaceImport(namedBindings)) { + namespaceImports.push(namedBindings); + } + else { + namedImports.push(namedBindings); + } + } + } + + // Normally, we don't combine default and namespace imports, but it would be silly to + // 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; + coalescedImports.push( + updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0])); + + continue; + } + + // For convenience, we cheat and do a little sorting during coalescing. + // Seems reasonable since we're restructuring so much anyway. + const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name)); + + for (const namespaceImport of sortedNamespaceImports) { + // Drop the name, if any + coalescedImports.push( + updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport)); + } + + if (defaultImports.length === 0 && namedImports.length === 0) { + continue; + } + + let newDefaultImport: Identifier = undefined; + const newImportSpecifiers: ImportSpecifier[] = []; + if (defaultImports.length === 1) { + newDefaultImport = defaultImports[0]; + } + else { + for (const defaultImport of defaultImports) { + newImportSpecifiers.push( + createImportSpecifier(createIdentifier("default"), defaultImport)); + } + } + + for (const namedImport of namedImports) { + for (const specifier of namedImport.elements) { + newImportSpecifiers.push(specifier); + } + } + + const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => { + const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name); + return nameComparison != Comparison.EqualTo + ? nameComparison + : compareIdentifiers(s1.name, s2.name); + }); + + const importClause = defaultImports.length > 0 + ? defaultImports[0].parent as ImportClause + : namedImports[0].parent; + + const newNamedImports = sortedImportSpecifiers.length === 0 + ? undefined + : namedImports.length === 0 + ? createNamedImports(sortedImportSpecifiers) + : updateNamedImports(namedImports[0], sortedImportSpecifiers); + + coalescedImports.push( + updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports)); + } + + return coalescedImports; + + // `undefined` is the min value. + function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) { + return s1 === undefined + ? s2 === undefined + ? Comparison.EqualTo + : Comparison.LessThan + : s2 === undefined + ? Comparison.GreaterThan + : s1.text < s2.text + ? Comparison.LessThan + : s1.text > s2.text + ? Comparison.GreaterThan + : Comparison.EqualTo; + } + + function updateImportDeclarationAndClause( + importClause: ImportClause, + name: Identifier | undefined, + namedBindings: NamedImportBindings | undefined) { + + const importDeclaration = importClause.parent; + return updateImportDeclaration( + importDeclaration, + importDeclaration.decorators, + importDeclaration.modifiers, + updateImportClause(importClause, name, namedBindings), + importDeclaration.moduleSpecifier); + } + } } diff --git a/src/services/types.ts b/src/services/types.ts index 594484e1ea733..51710c88f4f73 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -308,6 +308,7 @@ namespace ts { applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise; getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange): ApplicableRefactorInfo[]; getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo | undefined; + organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray; getEmitOutput(fileName: string, emitOnlyDtsFiles?: boolean): EmitOutput; @@ -326,6 +327,8 @@ namespace ts { export interface CombinedCodeFixScope { type: "file"; fileName: string; } + export type OrganizeImportsScope = CombinedCodeFixScope; + export interface GetCompletionsAtPositionOptions { includeExternalModuleExports: boolean; includeInsertTextCompletions: boolean; diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 370d841a7f19f..1a7edaa44c8ef 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -4135,6 +4135,7 @@ declare namespace ts { applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise; getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange): ApplicableRefactorInfo[]; getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo | undefined; + organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray; getEmitOutput(fileName: string, emitOnlyDtsFiles?: boolean): EmitOutput; getProgram(): Program; dispose(): void; @@ -4143,6 +4144,7 @@ declare namespace ts { type: "file"; fileName: string; } + type OrganizeImportsScope = CombinedCodeFixScope; interface GetCompletionsAtPositionOptions { includeExternalModuleExports: boolean; includeInsertTextCompletions: boolean; @@ -5078,6 +5080,7 @@ declare namespace ts.server.protocol { GetSupportedCodeFixes = "getSupportedCodeFixes", GetApplicableRefactors = "getApplicableRefactors", GetEditsForRefactor = "getEditsForRefactor", + OrganizeImports = "organizeImports", } /** * A TypeScript Server message @@ -5429,6 +5432,23 @@ declare namespace ts.server.protocol { renameLocation?: Location; renameFilename?: string; } + /** + * Organize imports by: + * 1) Removing unused imports + * 2) Coalescing imports from the same module + * 3) Sorting imports + */ + interface OrganizeImportsRequest extends Request { + command: CommandTypes.OrganizeImports; + arguments: OrganizeImportsRequestArgs; + } + type OrganizeImportsScope = GetCombinedCodeFixScope; + interface OrganizeImportsRequestArgs { + scope: OrganizeImportsScope; + } + interface OrganizeImportsResponse extends Response { + edits: ReadonlyArray; + } /** * Request for the available codefixes at a specific position. */ @@ -7282,6 +7302,7 @@ declare namespace ts.server { private extractPositionAndRange(args, scriptInfo); private getApplicableRefactors(args); private getEditsForRefactor(args, simplifiedResult); + private organizeImports({scope}, simplifiedResult); private getCodeFixes(args, simplifiedResult); private getCombinedCodeFix({scope, fixId}, simplifiedResult); private applyCodeActionCommand(args); diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index baed457b3b055..8f9c095090dcf 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -4387,6 +4387,7 @@ declare namespace ts { applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise; getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange): ApplicableRefactorInfo[]; getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo | undefined; + organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray; getEmitOutput(fileName: string, emitOnlyDtsFiles?: boolean): EmitOutput; getProgram(): Program; dispose(): void; @@ -4395,6 +4396,7 @@ declare namespace ts { type: "file"; fileName: string; } + type OrganizeImportsScope = CombinedCodeFixScope; interface GetCompletionsAtPositionOptions { includeExternalModuleExports: boolean; includeInsertTextCompletions: boolean; diff --git a/tests/baselines/reference/organizeImports/CoalesceTrivia.ts b/tests/baselines/reference/organizeImports/CoalesceTrivia.ts new file mode 100644 index 0000000000000..972df9e18ebb6 --- /dev/null +++ b/tests/baselines/reference/organizeImports/CoalesceTrivia.ts @@ -0,0 +1,15 @@ +// ==ORIGINAL== + +/*A*/import /*B*/ { /*C*/ F2 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/;/*H*/ //I +/*J*/import /*K*/ { /*L*/ F1 /*M*/ } /*N*/ from /*O*/ "lib" /*P*/;/*Q*/ //R + +F1(); +F2(); + +// ==ORGANIZED== + +/*A*/ import { /*L*/ F1 /*M*/, /*C*/ F2 /*D*/ } /*E*/ from "lib" /*G*/; /*H*/ //I + + +F1(); +F2(); diff --git a/tests/baselines/reference/organizeImports/MoveToTop.ts b/tests/baselines/reference/organizeImports/MoveToTop.ts new file mode 100644 index 0000000000000..c0e57b930ab9f --- /dev/null +++ b/tests/baselines/reference/organizeImports/MoveToTop.ts @@ -0,0 +1,18 @@ +// ==ORIGINAL== + +import { F1, F2 } from "lib"; +F1(); +F2(); +import * as NS from "lib"; +NS.F1(); +import D from "lib"; +D(); + +// ==ORGANIZED== + +import * as NS from "lib"; +import D, { F1, F2 } from "lib"; +F1(); +F2(); +NS.F1(); +D(); diff --git a/tests/baselines/reference/organizeImports/Simple.ts b/tests/baselines/reference/organizeImports/Simple.ts new file mode 100644 index 0000000000000..3f36ae633a6cf --- /dev/null +++ b/tests/baselines/reference/organizeImports/Simple.ts @@ -0,0 +1,20 @@ +// ==ORIGINAL== + +import { F1, F2 } from "lib"; +import * as NS from "lib"; +import D from "lib"; + +NS.F1(); +D(); +F1(); +F2(); + +// ==ORGANIZED== + +import * as NS from "lib"; +import D, { F1, F2 } from "lib"; + +NS.F1(); +D(); +F1(); +F2(); diff --git a/tests/baselines/reference/organizeImports/SortTrivia.ts b/tests/baselines/reference/organizeImports/SortTrivia.ts new file mode 100644 index 0000000000000..e46c836b96656 --- /dev/null +++ b/tests/baselines/reference/organizeImports/SortTrivia.ts @@ -0,0 +1,10 @@ +// ==ORIGINAL== + +/*A*/import /*B*/ "lib2" /*C*/;/*D*/ //E +/*F*/import /*G*/ "lib1" /*H*/;/*I*/ //J + +// ==ORGANIZED== + +/*F*/ import "lib1" /*H*/; /*I*/ //J +/*A*/ import "lib2" /*C*/; /*D*/ //E + From 979b14689e1abb9f7ba6e314b163496bd0008ca3 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 12 Feb 2018 18:29:01 -0800 Subject: [PATCH 2/4] Fix lint errors --- src/harness/unittests/organizeImports.ts | 7 ++++++- src/services/services.ts | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index 5cac601d08e91..e78a88ca23288 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -56,7 +56,9 @@ namespace ts { it("Sort - invalid vs invalid", () => { assertSortsBefore( + // tslint:disable-next-line no-invalid-template-strings "import y from `${'lib1'}`;", + // tslint:disable-next-line no-invalid-template-strings "import x from `${'lib2'}`;"); }); @@ -69,12 +71,14 @@ namespace ts { it("Sort - non-relative vs invalid", () => { assertSortsBefore( `import y from "lib";`, + // tslint:disable-next-line no-invalid-template-strings "import x from `${'lib'}`;"); }); it("Sort - relative vs invalid", () => { assertSortsBefore( `import y from "./lib";`, + // tslint:disable-next-line no-invalid-template-strings "import x from `${'lib'}`;"); }); @@ -357,7 +361,7 @@ F2(); assert.equal(node1.kind, node2.kind); - switch(node1.kind) { + switch (node1.kind) { case SyntaxKind.ImportDeclaration: const decl1 = node1 as ImportDeclaration; const decl2 = node2 as ImportDeclaration; @@ -369,6 +373,7 @@ F2(); const clause2 = node2 as ImportClause; assertEqual(clause1.name, clause2.name); assertEqual(clause1.namedBindings, clause2.namedBindings); + break; case SyntaxKind.NamespaceImport: const nsi1 = node1 as NamespaceImport; const nsi2 = node2 as NamespaceImport; diff --git a/src/services/services.ts b/src/services/services.ts index 22692f818e951..3b984c2e15970 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2368,7 +2368,7 @@ namespace ts { const name = getExternalModuleName(specifier); if (name) { const isRelative = isExternalModuleNameRelative(name); - return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative } + return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative }; } return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid }; @@ -2505,7 +2505,7 @@ namespace ts { const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => { const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name); - return nameComparison != Comparison.EqualTo + return nameComparison !== Comparison.EqualTo ? nameComparison : compareIdentifiers(s1.name, s2.name); }); From f4141ac6bfd726becafb9eef00adb79afbc1f027 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 13 Feb 2018 14:52:15 -0800 Subject: [PATCH 3/4] Separate OrganizeImports into its own namespace and file --- src/harness/unittests/organizeImports.ts | 42 ++-- src/services/organizeImports.ts | 291 +++++++++++++++++++++++ src/services/services.ts | 280 +--------------------- 3 files changed, 314 insertions(+), 299 deletions(-) create mode 100644 src/services/organizeImports.ts diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index e78a88ca23288..eaa21bbb72f3a 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -6,12 +6,12 @@ namespace ts { describe("Organize imports", () => { describe("Sort imports", () => { it("No imports", () => { - assert.isEmpty(sortImports([])); + assert.isEmpty(OrganizeImports.sortImports([])); }); it("One import", () => { const unsortedImports = parseImports(`import "lib";`); - const actualSortedImports = sortImports(unsortedImports); + const actualSortedImports = OrganizeImports.sortImports(unsortedImports); const expectedSortedImports = unsortedImports; assertListEqual(expectedSortedImports, actualSortedImports); }); @@ -84,27 +84,27 @@ namespace ts { function assertUnaffectedBySort(...importStrings: string[]) { const unsortedImports1 = parseImports(...importStrings); - assertListEqual(unsortedImports1, sortImports(unsortedImports1)); + assertListEqual(unsortedImports1, OrganizeImports.sortImports(unsortedImports1)); const unsortedImports2 = reverse(unsortedImports1); - assertListEqual(unsortedImports2, sortImports(unsortedImports2)); + assertListEqual(unsortedImports2, OrganizeImports.sortImports(unsortedImports2)); } function assertSortsBefore(importString1: string, importString2: string) { const imports = parseImports(importString1, importString2); - assertListEqual(imports, sortImports(imports)); - assertListEqual(imports, sortImports(reverse(imports))); + assertListEqual(imports, OrganizeImports.sortImports(imports)); + assertListEqual(imports, OrganizeImports.sortImports(reverse(imports))); } }); describe("Coalesce imports", () => { it("No imports", () => { - assert.isEmpty(coalesceImports([])); + assert.isEmpty(OrganizeImports.coalesceImports([])); }); it("Sort specifiers", () => { const sortedImports = parseImports(`import { default as m, a as n, b, y, z as o } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports(`import { a as n, b, default as m, y, z as o } from "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -113,7 +113,7 @@ namespace ts { const sortedImports = parseImports( `import "lib";`, `import "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports(`import "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -122,7 +122,7 @@ namespace ts { const sortedImports = parseImports( `import * as x from "lib";`, `import * as y from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -131,7 +131,7 @@ namespace ts { const sortedImports = parseImports( `import x from "lib";`, `import y from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports(`import { default as x, default as y } from "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -140,7 +140,7 @@ namespace ts { const sortedImports = parseImports( `import { x } from "lib";`, `import { y as z } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports(`import { x, y as z } from "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -149,7 +149,7 @@ namespace ts { const sortedImports = parseImports( `import "lib";`, `import * as x from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -158,7 +158,7 @@ namespace ts { const sortedImports = parseImports( `import "lib";`, `import x from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -167,7 +167,7 @@ namespace ts { const sortedImports = parseImports( `import "lib";`, `import { x } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -176,7 +176,7 @@ namespace ts { const sortedImports = parseImports( `import * as x from "lib";`, `import y from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports( `import y, * as x from "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); @@ -186,7 +186,7 @@ namespace ts { const sortedImports = parseImports( `import * as x from "lib";`, `import { y } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); @@ -195,7 +195,7 @@ namespace ts { const sortedImports = parseImports( `import x from "lib";`, `import { y } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports( `import x, { y } from "lib";`); assertListEqual(expectedCoalescedImports, actualCoalescedImports); @@ -211,7 +211,7 @@ namespace ts { `import * as x from "lib";`, `import z from "lib";`, `import { a } from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports( `import "lib";`, `import * as x from "lib";`, @@ -226,7 +226,7 @@ namespace ts { `import { b } from "lib1";`, `import { c } from "lib2";`, `import { a } from "lib2";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = parseImports( `import { b, d } from "lib1";`, `import { a, c } from "lib2";`); @@ -239,7 +239,7 @@ namespace ts { `import * as x from "lib";`, `import * as y from "lib";`, `import z from "lib";`); - const actualCoalescedImports = coalesceImports(sortedImports); + const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports); const expectedCoalescedImports = sortedImports; assertListEqual(expectedCoalescedImports, actualCoalescedImports); }); diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts new file mode 100644 index 0000000000000..aaff3331941b4 --- /dev/null +++ b/src/services/organizeImports.ts @@ -0,0 +1,291 @@ +/* @internal */ +namespace ts.OrganizeImports { + export function organizeImports( + sourceFile: SourceFile, + formatContext: formatting.FormatContext, + host: LanguageServiceHost, + cancellationToken: CancellationToken) { + + // All of the (old) ImportDeclarations in the file, in syntactic order. + const oldImportDecls: ImportDeclaration[] = []; + + forEachChild(sourceFile, node => { + cancellationToken.throwIfCancellationRequested(); + if (isImportDeclaration(node)) { + oldImportDecls.push(node); + } + // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) + }); + + if (oldImportDecls.length === 0) { + return []; + } + + const usedImportDecls = removeUnusedImports(oldImportDecls); + cancellationToken.throwIfCancellationRequested(); + const sortedImportDecls = sortImports(usedImportDecls); + cancellationToken.throwIfCancellationRequested(); + const coalescedImportDecls = coalesceImports(sortedImportDecls); + cancellationToken.throwIfCancellationRequested(); + + // All of the (new) ImportDeclarations in the file, in sorted order. + const newImportDecls = coalescedImportDecls; + + const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext }); + + // NB: Stopping before i === 0 + for (let i = oldImportDecls.length - 1; i > 0; i--) { + changeTracker.deleteNode(sourceFile, oldImportDecls[i]); + } + + if (newImportDecls.length === 0) { + changeTracker.deleteNode(sourceFile, oldImportDecls[0]); + } + else { + // Delete the surrounding trivia because it will have been retained in newImportDecls. + const replaceOptions = { + useNonAdjustedStartPosition: false, + useNonAdjustedEndPosition: false, + suffix: getNewLineOrDefaultFromHost(host, formatContext.options), + }; + changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions); + } + + const changes = changeTracker.getChanges(); + return changes; + } + + function removeUnusedImports(oldImports: ReadonlyArray) { + return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020) + } + + /* @internal */ // Internal for testing + export function sortImports(oldImports: ReadonlyArray) { + if (oldImports.length < 2) { + return oldImports; + } + + // NB: declaration order determines sort order + const enum ModuleNameKind { + NonRelative, + Relative, + Invalid, + } + + const importRecords = oldImports.map(createImportRecord); + + const sortedRecords = stableSort(importRecords, (import1, import2) => { + const { name: name1, kind: kind1 } = import1; + const { name: name2, kind: kind2 } = import2; + + if (kind1 !== kind2) { + return kind1 < kind2 + ? Comparison.LessThan + : Comparison.GreaterThan; + } + + // Note that we're using simple equality, retaining case-sensitivity. + if (name1 !== name2) { + return name1 < name2 + ? Comparison.LessThan + : Comparison.GreaterThan; + } + + return Comparison.EqualTo; + }); + + return sortedRecords.map(r => r.importDeclaration); + + function createImportRecord(importDeclaration: ImportDeclaration) { + const specifier = importDeclaration.moduleSpecifier; + const name = getExternalModuleName(specifier); + if (name) { + const isRelative = isExternalModuleNameRelative(name); + return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative }; + } + + return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid }; + } + } + + function getExternalModuleName(specifier: Expression) { + return isStringLiteral(specifier) || isNoSubstitutionTemplateLiteral(specifier) + ? specifier.text + : undefined; + } + + /** + * @param sortedImports a non-empty list of ImportDeclarations, sorted by module name. + */ + function groupSortedImports(sortedImports: ReadonlyArray): ReadonlyArray> { + Debug.assert(length(sortedImports) > 0); + + const groups: ImportDeclaration[][] = []; + + let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier); + let group: ImportDeclaration[] = []; + + for (const importDeclaration of sortedImports) { + const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier); + if (moduleName && moduleName === groupName) { + group.push(importDeclaration); + } + else if (group.length) { + groups.push(group); + + groupName = moduleName; + group = [importDeclaration]; + } + } + + if (group.length) { + groups.push(group); + } + + return groups; + } + + /* @internal */ // Internal for testing + /** + * @param sortedImports a list of ImportDeclarations, sorted by module name. + */ + export function coalesceImports(sortedImports: ReadonlyArray) { + if (sortedImports.length === 0) { + return sortedImports; + } + + const coalescedImports: ImportDeclaration[] = []; + + const groupedImports = groupSortedImports(sortedImports); + for (const importGroup of groupedImports) { + + let seenImportWithoutClause = false; + + const defaultImports: Identifier[] = []; + const namespaceImports: NamespaceImport[] = []; + const namedImports: NamedImports[] = []; + + for (const importDeclaration of importGroup) { + if (importDeclaration.importClause === undefined) { + // Only the first such import is interesting - the others are redundant. + // Note: Unfortunately, we will lose trivia that was on this node. + if (!seenImportWithoutClause) { + coalescedImports.push(importDeclaration); + } + + seenImportWithoutClause = true; + continue; + } + + const { name, namedBindings } = importDeclaration.importClause; + + if (name) { + defaultImports.push(name); + } + + if (namedBindings) { + if (isNamespaceImport(namedBindings)) { + namespaceImports.push(namedBindings); + } + else { + namedImports.push(namedBindings); + } + } + } + + // Normally, we don't combine default and namespace imports, but it would be silly to + // 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; + coalescedImports.push( + updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0])); + + continue; + } + + // For convenience, we cheat and do a little sorting during coalescing. + // Seems reasonable since we're restructuring so much anyway. + const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name)); + + for (const namespaceImport of sortedNamespaceImports) { + // Drop the name, if any + coalescedImports.push( + updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport)); + } + + if (defaultImports.length === 0 && namedImports.length === 0) { + continue; + } + + let newDefaultImport: Identifier = undefined; + const newImportSpecifiers: ImportSpecifier[] = []; + if (defaultImports.length === 1) { + newDefaultImport = defaultImports[0]; + } + else { + for (const defaultImport of defaultImports) { + newImportSpecifiers.push( + createImportSpecifier(createIdentifier("default"), defaultImport)); + } + } + + for (const namedImport of namedImports) { + for (const specifier of namedImport.elements) { + newImportSpecifiers.push(specifier); + } + } + + const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => { + const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name); + return nameComparison !== Comparison.EqualTo + ? nameComparison + : compareIdentifiers(s1.name, s2.name); + }); + + const importClause = defaultImports.length > 0 + ? defaultImports[0].parent as ImportClause + : namedImports[0].parent; + + const newNamedImports = sortedImportSpecifiers.length === 0 + ? undefined + : namedImports.length === 0 + ? createNamedImports(sortedImportSpecifiers) + : updateNamedImports(namedImports[0], sortedImportSpecifiers); + + coalescedImports.push( + updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports)); + } + + return coalescedImports; + + // `undefined` is the min value. + function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) { + return s1 === undefined + ? s2 === undefined + ? Comparison.EqualTo + : Comparison.LessThan + : s2 === undefined + ? Comparison.GreaterThan + : s1.text < s2.text + ? Comparison.LessThan + : s1.text > s2.text + ? Comparison.GreaterThan + : Comparison.EqualTo; + } + + function updateImportDeclarationAndClause( + importClause: ImportClause, + name: Identifier | undefined, + namedBindings: NamedImportBindings | undefined) { + + const importDeclaration = importClause.parent; + return updateImportDeclaration( + importDeclaration, + importDeclaration.decorators, + importDeclaration.modifiers, + updateImportClause(importClause, name, namedBindings), + importDeclaration.moduleSpecifier); + } + } +} \ No newline at end of file diff --git a/src/services/services.ts b/src/services/services.ts index 3b984c2e15970..37cc192442f1c 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -14,6 +14,7 @@ /// /// /// +/// /// /// /// @@ -1854,50 +1855,7 @@ namespace ts { const sourceFile = getValidSourceFile(scope.fileName); const formatContext = formatting.getFormatContext(formatOptions); - // All of the (old) ImportDeclarations in the file, in syntactic order. - const oldImportDecls: ImportDeclaration[] = []; - - forEachChild(sourceFile, node => { - cancellationToken.throwIfCancellationRequested(); - if (isImportDeclaration(node)) { - oldImportDecls.push(node); - } - // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) - }); - - if (oldImportDecls.length === 0) { - return []; - } - - const usedImportDecls = removeUnusedImports(oldImportDecls); - const sortedImportDecls = sortImports(usedImportDecls); - const coalescedImportDecls = coalesceImports(sortedImportDecls); - - // All of the (new) ImportDeclarations in the file, in sorted order. - const newImportDecls = coalescedImportDecls; - - const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext }); - - // NB: Stopping before i === 0 - for (let i = oldImportDecls.length - 1; i > 0; i--) { - changeTracker.deleteNode(sourceFile, oldImportDecls[i]); - } - - if (newImportDecls.length === 0) { - changeTracker.deleteNode(sourceFile, oldImportDecls[0]); - } - else { - // Delete the surrounding trivia because it will have been retained in newImportDecls. - const replaceOptions = { - useNonAdjustedStartPosition: false, - useNonAdjustedEndPosition: false, - suffix: getNewLineOrDefaultFromHost(host, formatOptions), - }; - changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions); - } - - const changes = changeTracker.getChanges(); - return changes; + return OrganizeImports.organizeImports(sourceFile, formatContext, host, cancellationToken); } function applyCodeActionCommand(action: CodeActionCommand): Promise; @@ -2321,238 +2279,4 @@ namespace ts { } objectAllocator = getServicesObjectAllocator(); - - function removeUnusedImports(oldImports: ReadonlyArray) { - return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020) - } - - /* @internal */ // Internal for testing - export function sortImports(oldImports: ReadonlyArray) { - if (oldImports.length < 2) { - return oldImports; - } - - // NB: declaration order determines sort order - const enum ModuleNameKind { - NonRelative, - Relative, - Invalid, - } - - const importRecords = oldImports.map(createImportRecord); - - const sortedRecords = stableSort(importRecords, (import1, import2) => { - const { name: name1, kind: kind1 } = import1; - const { name: name2, kind: kind2 } = import2; - - if (kind1 !== kind2) { - return kind1 < kind2 - ? Comparison.LessThan - : Comparison.GreaterThan; - } - - // Note that we're using simple equality, retaining case-sensitivity. - if (name1 !== name2) { - return name1 < name2 - ? Comparison.LessThan - : Comparison.GreaterThan; - } - - return Comparison.EqualTo; - }); - - return sortedRecords.map(r => r.importDeclaration); - - function createImportRecord(importDeclaration: ImportDeclaration) { - const specifier = importDeclaration.moduleSpecifier; - const name = getExternalModuleName(specifier); - if (name) { - const isRelative = isExternalModuleNameRelative(name); - return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative }; - } - - return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid }; - } - } - - function getExternalModuleName(specifier: Expression) { - return isStringLiteral(specifier) || isNoSubstitutionTemplateLiteral(specifier) - ? specifier.text - : undefined; - } - - /** - * @param sortedImports a non-empty list of ImportDeclarations, sorted by module name. - */ - function groupSortedImports(sortedImports: ReadonlyArray): ReadonlyArray> { - Debug.assert(length(sortedImports) > 0); - - const groups: ImportDeclaration[][] = []; - - let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier); - let group: ImportDeclaration[] = []; - - for (const importDeclaration of sortedImports) { - const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier); - if (moduleName && moduleName === groupName) { - group.push(importDeclaration); - } - else if (group.length) { - groups.push(group); - - groupName = moduleName; - group = [importDeclaration]; - } - } - - if (group.length) { - groups.push(group); - } - - return groups; - } - - /* @internal */ // Internal for testing - /** - * @param sortedImports a list of ImportDeclarations, sorted by module name. - */ - export function coalesceImports(sortedImports: ReadonlyArray) { - if (sortedImports.length === 0) { - return sortedImports; - } - - const coalescedImports: ImportDeclaration[] = []; - - const groupedImports = groupSortedImports(sortedImports); - for (const importGroup of groupedImports) { - - let seenImportWithoutClause = false; - - const defaultImports: Identifier[] = []; - const namespaceImports: NamespaceImport[] = []; - const namedImports: NamedImports[] = []; - - for (const importDeclaration of importGroup) { - if (importDeclaration.importClause === undefined) { - // Only the first such import is interesting - the others are redundant. - // Note: Unfortunately, we will lose trivia that was on this node. - if (!seenImportWithoutClause) { - coalescedImports.push(importDeclaration); - } - - seenImportWithoutClause = true; - continue; - } - - const { name, namedBindings } = importDeclaration.importClause; - - if (name) { - defaultImports.push(name); - } - - if (namedBindings) { - if (isNamespaceImport(namedBindings)) { - namespaceImports.push(namedBindings); - } - else { - namedImports.push(namedBindings); - } - } - } - - // Normally, we don't combine default and namespace imports, but it would be silly to - // 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; - coalescedImports.push( - updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0])); - - continue; - } - - // For convenience, we cheat and do a little sorting during coalescing. - // Seems reasonable since we're restructuring so much anyway. - const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name)); - - for (const namespaceImport of sortedNamespaceImports) { - // Drop the name, if any - coalescedImports.push( - updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport)); - } - - if (defaultImports.length === 0 && namedImports.length === 0) { - continue; - } - - let newDefaultImport: Identifier = undefined; - const newImportSpecifiers: ImportSpecifier[] = []; - if (defaultImports.length === 1) { - newDefaultImport = defaultImports[0]; - } - else { - for (const defaultImport of defaultImports) { - newImportSpecifiers.push( - createImportSpecifier(createIdentifier("default"), defaultImport)); - } - } - - for (const namedImport of namedImports) { - for (const specifier of namedImport.elements) { - newImportSpecifiers.push(specifier); - } - } - - const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => { - const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name); - return nameComparison !== Comparison.EqualTo - ? nameComparison - : compareIdentifiers(s1.name, s2.name); - }); - - const importClause = defaultImports.length > 0 - ? defaultImports[0].parent as ImportClause - : namedImports[0].parent; - - const newNamedImports = sortedImportSpecifiers.length === 0 - ? undefined - : namedImports.length === 0 - ? createNamedImports(sortedImportSpecifiers) - : updateNamedImports(namedImports[0], sortedImportSpecifiers); - - coalescedImports.push( - updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports)); - } - - return coalescedImports; - - // `undefined` is the min value. - function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) { - return s1 === undefined - ? s2 === undefined - ? Comparison.EqualTo - : Comparison.LessThan - : s2 === undefined - ? Comparison.GreaterThan - : s1.text < s2.text - ? Comparison.LessThan - : s1.text > s2.text - ? Comparison.GreaterThan - : Comparison.EqualTo; - } - - function updateImportDeclarationAndClause( - importClause: ImportClause, - name: Identifier | undefined, - namedBindings: NamedImportBindings | undefined) { - - const importDeclaration = importClause.parent; - return updateImportDeclaration( - importDeclaration, - importDeclaration.decorators, - importDeclaration.modifiers, - updateImportClause(importClause, name, namedBindings), - importDeclaration.moduleSpecifier); - } - } } From 5c278cee17008877f3805f70a214e17ba3f37949 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 14 Feb 2018 13:57:09 -0800 Subject: [PATCH 4/4] Address PR feedback Eliminate cancellation token Add organizeImports.ts to tsconfig.json Simplify ts.OrganizeImports.organizeImports Simplify sortImports Semantic change: all invalid module specifiers are now considered to be equal. Simplify comparisons using || Pull out imports with invalid modules specifiers ...for separate processing. They are tacked on to the end of the organized imports in their original order. Bonus: downstream functions can now assume imports have valid module specifiers. Rename baseline folder with leading lowercase Simplify coalesceImports Remove some unnecessary null checks Simplify baseline generation --- src/compiler/core.ts | 5 + src/harness/unittests/organizeImports.ts | 55 +++-- src/services/organizeImports.ts | 209 +++++++----------- src/services/services.ts | 2 +- src/services/tsconfig.json | 1 + .../organizeImports/MoveToTop_Invalid.ts | 22 ++ 6 files changed, 130 insertions(+), 164 deletions(-) create mode 100644 tests/baselines/reference/organizeImports/MoveToTop_Invalid.ts diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 212fa86b3667d..698824fa9b0fb 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -1905,6 +1905,11 @@ namespace ts { Comparison.EqualTo; } + /** True is greater than false. */ + export function compareBooleans(a: boolean, b: boolean): Comparison { + return compareValues(a ? 1 : 0, b ? 1 : 0); + } + function compareMessageText(text1: string | DiagnosticMessageChain, text2: string | DiagnosticMessageChain): Comparison { while (text1 && text2) { // We still have both chains. diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index eaa21bbb72f3a..2accc84d43f9c 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -54,34 +54,12 @@ namespace ts { `import x from "./lib2";`); }); - it("Sort - invalid vs invalid", () => { - assertSortsBefore( - // tslint:disable-next-line no-invalid-template-strings - "import y from `${'lib1'}`;", - // tslint:disable-next-line no-invalid-template-strings - "import x from `${'lib2'}`;"); - }); - it("Sort - relative vs non-relative", () => { assertSortsBefore( `import y from "lib";`, `import x from "./lib";`); }); - it("Sort - non-relative vs invalid", () => { - assertSortsBefore( - `import y from "lib";`, - // tslint:disable-next-line no-invalid-template-strings - "import x from `${'lib'}`;"); - }); - - it("Sort - relative vs invalid", () => { - assertSortsBefore( - `import y from "./lib";`, - // tslint:disable-next-line no-invalid-template-strings - "import x from `${'lib'}`;"); - }); - function assertUnaffectedBySort(...importStrings: string[]) { const unsortedImports1 = parseImports(...importStrings); assertListEqual(unsortedImports1, OrganizeImports.sortImports(unsortedImports1)); @@ -286,6 +264,25 @@ D(); }, libFile); + // tslint:disable no-invalid-template-strings + testOrganizeImports("MoveToTop_Invalid", + { + path: "/test.ts", + content: ` +import { F1, F2 } from "lib"; +F1(); +F2(); +import * as NS from "lib"; +NS.F1(); +import b from ${"`${'lib'}`"}; +import a from ${"`${'lib'}`"}; +import D from "lib"; +D(); +`, + }, + libFile); + // tslint:enable no-invalid-template-strings + testOrganizeImports("CoalesceTrivia", { path: "/test.ts", @@ -322,15 +319,13 @@ F2(); assert.equal(testPath, changes[0].fileName); Harness.Baseline.runBaseline(baselinePath, () => { - const data: string[] = []; - data.push(`// ==ORIGINAL==`); - data.push(testContent); - - data.push(`// ==ORGANIZED==`); const newText = textChanges.applyChanges(testContent, changes[0].textChanges); - data.push(newText); - - return data.join(newLineCharacter); + return [ + "// ==ORIGINAL==", + testContent, + "// ==ORGANIZED==", + newText, + ].join(newLineCharacter); }); } diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index aaff3331941b4..d6b782a19aa6d 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -3,56 +3,44 @@ namespace ts.OrganizeImports { export function organizeImports( sourceFile: SourceFile, formatContext: formatting.FormatContext, - host: LanguageServiceHost, - cancellationToken: CancellationToken) { + host: LanguageServiceHost) { - // All of the (old) ImportDeclarations in the file, in syntactic order. - const oldImportDecls: ImportDeclaration[] = []; + // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) - forEachChild(sourceFile, node => { - cancellationToken.throwIfCancellationRequested(); - if (isImportDeclaration(node)) { - oldImportDecls.push(node); - } - // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) - }); + // All of the old ImportDeclarations in the file, in syntactic order. + const oldImportDecls = sourceFile.statements.filter(isImportDeclaration); if (oldImportDecls.length === 0) { return []; } - const usedImportDecls = removeUnusedImports(oldImportDecls); - cancellationToken.throwIfCancellationRequested(); - const sortedImportDecls = sortImports(usedImportDecls); - cancellationToken.throwIfCancellationRequested(); - const coalescedImportDecls = coalesceImports(sortedImportDecls); - cancellationToken.throwIfCancellationRequested(); + const oldValidImportDecls = oldImportDecls.filter(importDecl => getExternalModuleName(importDecl.moduleSpecifier)); + const oldInvalidImportDecls = oldImportDecls.filter(importDecl => !getExternalModuleName(importDecl.moduleSpecifier)); - // All of the (new) ImportDeclarations in the file, in sorted order. - const newImportDecls = coalescedImportDecls; + // All of the new ImportDeclarations in the file, in sorted order. + const newImportDecls = coalesceImports(sortImports(removeUnusedImports(oldValidImportDecls))).concat(oldInvalidImportDecls); const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext }); - // NB: Stopping before i === 0 - for (let i = oldImportDecls.length - 1; i > 0; i--) { - changeTracker.deleteNode(sourceFile, oldImportDecls[i]); - } - + // Delete or replace the first import. if (newImportDecls.length === 0) { changeTracker.deleteNode(sourceFile, oldImportDecls[0]); } else { - // Delete the surrounding trivia because it will have been retained in newImportDecls. - const replaceOptions = { + // Note: Delete the surrounding trivia because it will have been retained in newImportDecls. + changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, { useNonAdjustedStartPosition: false, useNonAdjustedEndPosition: false, suffix: getNewLineOrDefaultFromHost(host, formatContext.options), - }; - changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions); + }); } - const changes = changeTracker.getChanges(); - return changes; + // Delete any subsequent imports. + for (let i = 1; i < oldImportDecls.length; i++) { + changeTracker.deleteNode(sourceFile, oldImportDecls[i]); + } + + return changeTracker.getChanges(); } function removeUnusedImports(oldImports: ReadonlyArray) { @@ -61,51 +49,14 @@ namespace ts.OrganizeImports { /* @internal */ // Internal for testing export function sortImports(oldImports: ReadonlyArray) { - if (oldImports.length < 2) { - return oldImports; - } - - // NB: declaration order determines sort order - const enum ModuleNameKind { - NonRelative, - Relative, - Invalid, - } - - const importRecords = oldImports.map(createImportRecord); - - const sortedRecords = stableSort(importRecords, (import1, import2) => { - const { name: name1, kind: kind1 } = import1; - const { name: name2, kind: kind2 } = import2; - - if (kind1 !== kind2) { - return kind1 < kind2 - ? Comparison.LessThan - : Comparison.GreaterThan; - } - - // Note that we're using simple equality, retaining case-sensitivity. - if (name1 !== name2) { - return name1 < name2 - ? Comparison.LessThan - : Comparison.GreaterThan; - } - - return Comparison.EqualTo; + return stableSort(oldImports, (import1, import2) => { + const name1 = getExternalModuleName(import1.moduleSpecifier); + const name2 = getExternalModuleName(import2.moduleSpecifier); + Debug.assert(name1 !== undefined); + Debug.assert(name2 !== undefined); + return compareBooleans(isExternalModuleNameRelative(name1), isExternalModuleNameRelative(name2)) || + compareStringsCaseSensitive(name1, name2); }); - - return sortedRecords.map(r => r.importDeclaration); - - function createImportRecord(importDeclaration: ImportDeclaration) { - const specifier = importDeclaration.moduleSpecifier; - const name = getExternalModuleName(specifier); - if (name) { - const isRelative = isExternalModuleNameRelative(name); - return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative }; - } - - return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid }; - } } function getExternalModuleName(specifier: Expression) { @@ -123,11 +74,13 @@ namespace ts.OrganizeImports { const groups: ImportDeclaration[][] = []; let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier); + Debug.assert(groupName !== undefined); let group: ImportDeclaration[] = []; for (const importDeclaration of sortedImports) { const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier); - if (moduleName && moduleName === groupName) { + Debug.assert(moduleName !== undefined); + if (moduleName === groupName) { group.push(importDeclaration); } else if (group.length) { @@ -159,38 +112,10 @@ namespace ts.OrganizeImports { const groupedImports = groupSortedImports(sortedImports); for (const importGroup of groupedImports) { - let seenImportWithoutClause = false; + const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getImportParts(importGroup); - const defaultImports: Identifier[] = []; - const namespaceImports: NamespaceImport[] = []; - const namedImports: NamedImports[] = []; - - for (const importDeclaration of importGroup) { - if (importDeclaration.importClause === undefined) { - // Only the first such import is interesting - the others are redundant. - // Note: Unfortunately, we will lose trivia that was on this node. - if (!seenImportWithoutClause) { - coalescedImports.push(importDeclaration); - } - - seenImportWithoutClause = true; - continue; - } - - const { name, namedBindings } = importDeclaration.importClause; - - if (name) { - defaultImports.push(name); - } - - if (namedBindings) { - if (isNamespaceImport(namedBindings)) { - namespaceImports.push(namedBindings); - } - else { - namedImports.push(namedBindings); - } - } + if (importWithoutClause) { + coalescedImports.push(importWithoutClause); } // Normally, we don't combine default and namespace imports, but it would be silly to @@ -204,8 +129,6 @@ namespace ts.OrganizeImports { continue; } - // For convenience, we cheat and do a little sorting during coalescing. - // Seems reasonable since we're restructuring so much anyway. const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name)); for (const namespaceImport of sortedNamespaceImports) { @@ -218,7 +141,7 @@ namespace ts.OrganizeImports { continue; } - let newDefaultImport: Identifier = undefined; + let newDefaultImport: Identifier | undefined; const newImportSpecifiers: ImportSpecifier[] = []; if (defaultImports.length === 1) { newDefaultImport = defaultImports[0]; @@ -230,18 +153,11 @@ namespace ts.OrganizeImports { } } - for (const namedImport of namedImports) { - for (const specifier of namedImport.elements) { - newImportSpecifiers.push(specifier); - } - } + newImportSpecifiers.push(...flatMap(namedImports, n => n.elements)); - const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => { - const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name); - return nameComparison !== Comparison.EqualTo - ? nameComparison - : compareIdentifiers(s1.name, s2.name); - }); + 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 @@ -259,19 +175,46 @@ namespace ts.OrganizeImports { return coalescedImports; - // `undefined` is the min value. - function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) { - return s1 === undefined - ? s2 === undefined - ? Comparison.EqualTo - : Comparison.LessThan - : s2 === undefined - ? Comparison.GreaterThan - : s1.text < s2.text - ? Comparison.LessThan - : s1.text > s2.text - ? Comparison.GreaterThan - : Comparison.EqualTo; + function getImportParts(importGroup: ReadonlyArray) { + let importWithoutClause: ImportDeclaration | undefined; + const defaultImports: Identifier[] = []; + const namespaceImports: NamespaceImport[] = []; + const namedImports: NamedImports[] = []; + + for (const importDeclaration of importGroup) { + if (importDeclaration.importClause === undefined) { + // Only the first such import is interesting - the others are redundant. + // Note: Unfortunately, we will lose trivia that was on this node. + importWithoutClause = importWithoutClause || importDeclaration; + continue; + } + + const { name, namedBindings } = importDeclaration.importClause; + + if (name) { + defaultImports.push(name); + } + + if (namedBindings) { + if (isNamespaceImport(namedBindings)) { + namespaceImports.push(namedBindings); + } + else { + namedImports.push(namedBindings); + } + } + } + + return { + importWithoutClause, + defaultImports, + namespaceImports, + namedImports, + }; + } + + function compareIdentifiers(s1: Identifier, s2: Identifier) { + return compareStringsCaseSensitive(s1.text, s2.text); } function updateImportDeclarationAndClause( diff --git a/src/services/services.ts b/src/services/services.ts index 37cc192442f1c..b5d6f0ec2a655 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, cancellationToken); + return OrganizeImports.organizeImports(sourceFile, formatContext, host); } function applyCodeActionCommand(action: CodeActionCommand): Promise; diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index ef0d68b20412e..bbd88a1a00401 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -58,6 +58,7 @@ "jsTyping.ts", "navigateTo.ts", "navigationBar.ts", + "organizeImports.ts", "outliningElementsCollector.ts", "pathCompletions.ts", "patternMatcher.ts", diff --git a/tests/baselines/reference/organizeImports/MoveToTop_Invalid.ts b/tests/baselines/reference/organizeImports/MoveToTop_Invalid.ts new file mode 100644 index 0000000000000..e2372f680cfbe --- /dev/null +++ b/tests/baselines/reference/organizeImports/MoveToTop_Invalid.ts @@ -0,0 +1,22 @@ +// ==ORIGINAL== + +import { F1, F2 } from "lib"; +F1(); +F2(); +import * as NS from "lib"; +NS.F1(); +import b from `${'lib'}`; +import a from `${'lib'}`; +import D from "lib"; +D(); + +// ==ORGANIZED== + +import * as NS from "lib"; +import D, { F1, F2 } from "lib"; +import b from `${'lib'}`; +import a from `${'lib'}`; +F1(); +F2(); +NS.F1(); +D();