Add option to OrganizeImports for removal only (#50931)

andrewbranch · web-flow · commit 0ce72ef6c8b3 · 2022-09-29T16:30:02.000-07:00
* Remove unused imports

* Lint

* Update baselines

* Make mode paramter required

* Clean up
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -529,8 +529,8 @@ namespace FourSlash {
             }
         }
 
-        public verifyOrganizeImports(newContent: string) {
-            const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file" }, this.formatCodeSettings, ts.emptyOptions);
+        public verifyOrganizeImports(newContent: string, mode?: ts.OrganizeImportsMode) {
+            const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file", mode }, this.formatCodeSettings, ts.emptyOptions);
             this.applyChanges(changes);
             this.verifyFileContent(this.activeFile.fileName, newContent);
         }
diff --git a/src/harness/fourslashInterfaceImpl.ts b/src/harness/fourslashInterfaceImpl.ts
@@ -624,8 +624,8 @@ namespace FourSlashInterface {
             this.state.noMoveToNewFile();
         }
 
-        public organizeImports(newContent: string) {
-            this.state.verifyOrganizeImports(newContent);
+        public organizeImports(newContent: string, mode?: ts.OrganizeImportsMode): void {
+            this.state.verifyOrganizeImports(newContent, mode);
         }
     }
 
diff --git a/src/server/protocol.ts b/src/server/protocol.ts
@@ -681,9 +681,17 @@ namespace ts.server.protocol {
 
     export type OrganizeImportsScope = GetCombinedCodeFixScope;
 
+    export const enum OrganizeImportsMode {
+        All = "All",
+        SortAndCombine = "SortAndCombine",
+        RemoveUnused = "RemoveUnused",
+    }
+
     export interface OrganizeImportsRequestArgs {
         scope: OrganizeImportsScope;
+        /** @deprecated Use `mode` instead */
         skipDestructiveCodeActions?: boolean;
+        mode?: OrganizeImportsMode;
     }
 
     export interface OrganizeImportsResponse extends Response {
diff --git a/src/server/session.ts b/src/server/session.ts
@@ -2533,7 +2533,7 @@ namespace ts.server {
             const changes = project.getLanguageService().organizeImports(
                 {
                     fileName: file,
-                    skipDestructiveCodeActions: args.skipDestructiveCodeActions,
+                    mode: args.mode as OrganizeImportsMode | undefined ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : undefined),
                     type: "file",
                 },
                 this.getFormatOptions(file),
diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts
@@ -13,38 +13,51 @@ namespace ts.OrganizeImports {
         host: LanguageServiceHost,
         program: Program,
         preferences: UserPreferences,
-        skipDestructiveCodeActions?: boolean
+        mode: OrganizeImportsMode,
     ) {
         const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences });
-
-        const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => stableSort(
-            coalesceImports(removeUnusedImports(importGroup, sourceFile, program, skipDestructiveCodeActions)),
-            (s1, s2) => compareImportsOrRequireStatements(s1, s2));
+        const shouldSort = mode === OrganizeImportsMode.SortAndCombine || mode === OrganizeImportsMode.All;
+        const shouldCombine = shouldSort; // These are currently inseparable, but I draw a distinction for clarity and in case we add modes in the future.
+        const shouldRemove = mode === OrganizeImportsMode.RemoveUnused || mode === OrganizeImportsMode.All;
+        const maybeRemove = shouldRemove ? removeUnusedImports : identity;
+        const maybeCoalesce = shouldCombine ? coalesceImports : identity;
+        const processImportsOfSameModuleSpecifier = (importGroup: readonly ImportDeclaration[]) => {
+            const processedDeclarations = maybeCoalesce(maybeRemove(importGroup, sourceFile, program));
+            return shouldSort
+                ? stableSort(processedDeclarations, (s1, s2) => compareImportsOrRequireStatements(s1, s2))
+                : processedDeclarations;
+        };
 
         // All of the old ImportDeclarations in the file, in syntactic order.
         const topLevelImportGroupDecls = groupImportsByNewlineContiguous(sourceFile, sourceFile.statements.filter(isImportDeclaration));
-        topLevelImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, coalesceAndOrganizeImports));
+        topLevelImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, processImportsOfSameModuleSpecifier));
 
-        // All of the old ExportDeclarations in the file, in syntactic order.
-        const topLevelExportDecls = sourceFile.statements.filter(isExportDeclaration);
-        organizeImportsWorker(topLevelExportDecls, coalesceExports);
+        // Exports are always used
+        if (mode !== OrganizeImportsMode.RemoveUnused) {
+            // All of the old ExportDeclarations in the file, in syntactic order.
+            const topLevelExportDecls = sourceFile.statements.filter(isExportDeclaration);
+            organizeImportsWorker(topLevelExportDecls, coalesceExports);
+        }
 
         for (const ambientModule of sourceFile.statements.filter(isAmbientModule)) {
             if (!ambientModule.body) continue;
 
             const ambientModuleImportGroupDecls = groupImportsByNewlineContiguous(sourceFile, ambientModule.body.statements.filter(isImportDeclaration));
-            ambientModuleImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, coalesceAndOrganizeImports));
+            ambientModuleImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, processImportsOfSameModuleSpecifier));
 
-            const ambientModuleExportDecls = ambientModule.body.statements.filter(isExportDeclaration);
-            organizeImportsWorker(ambientModuleExportDecls, coalesceExports);
+            // Exports are always used
+            if (mode !== OrganizeImportsMode.RemoveUnused) {
+                const ambientModuleExportDecls = ambientModule.body.statements.filter(isExportDeclaration);
+                organizeImportsWorker(ambientModuleExportDecls, coalesceExports);
+            }
         }
 
         return changeTracker.getChanges();
 
         function organizeImportsWorker<T extends ImportDeclaration | ExportDeclaration>(
             oldImportDecls: readonly T[],
-            coalesce: (group: readonly T[]) => readonly T[]) {
-
+            coalesce: (group: readonly T[]) => readonly T[],
+        ) {
             if (length(oldImportDecls) === 0) {
                 return;
             }
@@ -56,8 +69,12 @@ namespace ts.OrganizeImports {
             // but the consequences of being wrong are very minor.
             suppressLeadingTrivia(oldImportDecls[0]);
 
-            const oldImportGroups = group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!);
-            const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier));
+            const oldImportGroups = shouldCombine
+                ? group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!)
+                : [oldImportDecls];
+            const sortedImportGroups = shouldSort
+                ? stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier))
+                : oldImportGroups;
             const newImportDecls = flatMap(sortedImportGroups, importGroup =>
                 getExternalModuleName(importGroup[0].moduleSpecifier!)
                     ? coalesce(importGroup)
@@ -129,12 +146,7 @@ namespace ts.OrganizeImports {
         return false;
     }
 
-    function removeUnusedImports(oldImports: readonly ImportDeclaration[], sourceFile: SourceFile, program: Program, skipDestructiveCodeActions: boolean | undefined) {
-        // As a precaution, consider unused import detection to be destructive (GH #43051)
-        if (skipDestructiveCodeActions) {
-            return oldImports;
-        }
-
+    function removeUnusedImports(oldImports: readonly ImportDeclaration[], sourceFile: SourceFile, program: Program) {
         const typeChecker = program.getTypeChecker();
         const compilerOptions = program.getCompilerOptions();
         const jsxNamespace = typeChecker.getJsxNamespace(sourceFile);
diff --git a/src/services/services.ts b/src/services/services.ts
@@ -2076,7 +2076,8 @@ namespace ts {
             const sourceFile = getValidSourceFile(args.fileName);
             const formatContext = formatting.getFormatContext(formatOptions, host);
 
-            return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, args.skipDestructiveCodeActions);
+            const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : OrganizeImportsMode.All);
+            return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, mode);
         }
 
         function getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
diff --git a/src/services/types.ts b/src/services/types.ts
@@ -575,8 +575,16 @@ namespace ts {
 
     export interface CombinedCodeFixScope { type: "file"; fileName: string; }
 
+    export const enum OrganizeImportsMode {
+        All = "All",
+        SortAndCombine = "SortAndCombine",
+        RemoveUnused = "RemoveUnused",
+    }
+
     export interface OrganizeImportsArgs extends CombinedCodeFixScope {
+        /** @deprecated Use `mode` instead */
         skipDestructiveCodeActions?: boolean;
+        mode?: OrganizeImportsMode;
     }
 
     export type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts
@@ -6032,8 +6032,15 @@ declare namespace ts {
         type: "file";
         fileName: string;
     }
+    enum OrganizeImportsMode {
+        All = "All",
+        SortAndCombine = "SortAndCombine",
+        RemoveUnused = "RemoveUnused"
+    }
     interface OrganizeImportsArgs extends CombinedCodeFixScope {
+        /** @deprecated Use `mode` instead */
         skipDestructiveCodeActions?: boolean;
+        mode?: OrganizeImportsMode;
     }
     type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
     enum CompletionTriggerKind {
@@ -7616,9 +7623,16 @@ declare namespace ts.server.protocol {
         arguments: OrganizeImportsRequestArgs;
     }
     type OrganizeImportsScope = GetCombinedCodeFixScope;
+    enum OrganizeImportsMode {
+        All = "All",
+        SortAndCombine = "SortAndCombine",
+        RemoveUnused = "RemoveUnused"
+    }
     interface OrganizeImportsRequestArgs {
         scope: OrganizeImportsScope;
+        /** @deprecated Use `mode` instead */
         skipDestructiveCodeActions?: boolean;
+        mode?: OrganizeImportsMode;
     }
     interface OrganizeImportsResponse extends Response {
         body: readonly FileCodeEdits[];
diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -6032,8 +6032,15 @@ declare namespace ts {
         type: "file";
         fileName: string;
     }
+    enum OrganizeImportsMode {
+        All = "All",
+        SortAndCombine = "SortAndCombine",
+        RemoveUnused = "RemoveUnused"
+    }
     interface OrganizeImportsArgs extends CombinedCodeFixScope {
+        /** @deprecated Use `mode` instead */
         skipDestructiveCodeActions?: boolean;
+        mode?: OrganizeImportsMode;
     }
     type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
     enum CompletionTriggerKind {
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -91,6 +91,12 @@ declare module ts {
         Message
     }
 
+    enum OrganizeImportsMode {
+      All = "All",
+      SortAndCombine = "SortAndCombine",
+      RemoveUnused = "RemoveUnused",
+  }
+
     interface DiagnosticMessage {
         key: string;
         category: DiagnosticCategory;
@@ -442,7 +448,7 @@ declare namespace FourSlashInterface {
 
         generateTypes(...options: GenerateTypesOptions[]): void;
 
-        organizeImports(newContent: string): void;
+        organizeImports(newContent: string, mode?: ts.OrganizeImportsMode): void;
 
         toggleLineComment(newFileContent: string): void;
         toggleMultilineComment(newFileContent: string): void;
diff --git a/tests/cases/fourslash/organizeImports_removeOnly.ts b/tests/cases/fourslash/organizeImports_removeOnly.ts
@@ -0,0 +1,16 @@
+/// <reference path="fourslash.ts" />
+
+//// import { c, b, a } from "foo";
+//// import d, { e } from "bar";
+//// import * as f from "baz";
+//// import { g } from "foo";
+////
+//// export { g, e, b, c };
+
+verify.organizeImports(
+`import { c, b } from "foo";
+import { e } from "bar";
+import { g } from "foo";
+
+export { g, e, b, c };`,
+  ts.OrganizeImportsMode.RemoveUnused);

Original file line number	Diff line number	Diff line change
`@@ -529,8 +529,8 @@ namespace FourSlash {`
`529`	`529`	`}`
`530`	`530`	`}`
`531`	`531`
`532`		`- public verifyOrganizeImports(newContent: string) {`
`533`		`- const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file" }, this.formatCodeSettings, ts.emptyOptions);`
	`532`	`+ public verifyOrganizeImports(newContent: string, mode?: ts.OrganizeImportsMode) {`
	`533`	`+ const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file", mode }, this.formatCodeSettings, ts.emptyOptions);`
`534`	`534`	`this.applyChanges(changes);`
`535`	`535`	`this.verifyFileContent(this.activeFile.fileName, newContent);`
`536`	`536`	`}`
Original file line number	Diff line number	Diff line change
`@@ -624,8 +624,8 @@ namespace FourSlashInterface {`
`624`	`624`	`this.state.noMoveToNewFile();`
`625`	`625`	`}`
`626`	`626`
`627`		`- public organizeImports(newContent: string) {`
`628`		`- this.state.verifyOrganizeImports(newContent);`
	`627`	`+ public organizeImports(newContent: string, mode?: ts.OrganizeImportsMode): void {`
	`628`	`+ this.state.verifyOrganizeImports(newContent, mode);`
`629`	`629`	`}`
`630`	`630`	`}`
`631`	`631`
Original file line number	Diff line number	Diff line change
`@@ -2533,7 +2533,7 @@ namespace ts.server {`
`2533`	`2533`	`const changes = project.getLanguageService().organizeImports(`
`2534`	`2534`	`{`
`2535`	`2535`	`fileName: file,`
`2536`		`- skipDestructiveCodeActions: args.skipDestructiveCodeActions,`
	`2536`	`+ mode: args.mode as OrganizeImportsMode \| undefined ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : undefined),`
`2537`	`2537`	`type: "file",`
`2538`	`2538`	`},`
`2539`	`2539`	`this.getFormatOptions(file),`
Original file line number	Diff line number	Diff line change
`@@ -2076,7 +2076,8 @@ namespace ts {`
`2076`	`2076`	`const sourceFile = getValidSourceFile(args.fileName);`
`2077`	`2077`	`const formatContext = formatting.getFormatContext(formatOptions, host);`
`2078`	`2078`
`2079`		`- return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, args.skipDestructiveCodeActions);`
	`2079`	`+ const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : OrganizeImportsMode.All);`
	`2080`	`+ return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, mode);`
`2080`	`2081`	`}`
`2081`	`2082`
`2082`	`2083`	`function getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {`