From 0ce72ef6c8b39cd2d07e5b0eb3a0c144a7783ad2 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Thu, 29 Sep 2022 16:30:02 -0700 Subject: [PATCH] Add option to OrganizeImports for removal only (#50931) * Remove unused imports * Lint * Update baselines * Make mode paramter required * Clean up --- src/harness/fourslashImpl.ts | 4 +- src/harness/fourslashInterfaceImpl.ts | 4 +- src/server/protocol.ts | 8 +++ src/server/session.ts | 2 +- src/services/organizeImports.ts | 56 +++++++++++-------- src/services/services.ts | 3 +- src/services/types.ts | 8 +++ .../reference/api/tsserverlibrary.d.ts | 14 +++++ tests/baselines/reference/api/typescript.d.ts | 7 +++ tests/cases/fourslash/fourslash.ts | 8 ++- .../fourslash/organizeImports_removeOnly.ts | 16 ++++++ 11 files changed, 101 insertions(+), 29 deletions(-) create mode 100644 tests/cases/fourslash/organizeImports_removeOnly.ts diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 72ceb827adf95..2370c23f9efbe 100644 --- 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 index 52acb7d347eec..12fe11fb1a81e 100644 --- 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 index e78d2262b5bca..372898c4aff73 100644 --- 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 index 7f7940ae8b3de..302884cf0801c 100644 --- 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 index d1cd6c517f8c9..2928d604acdf3 100644 --- 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( 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 index 975e36b1dd40c..618a274766957 100644 --- 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 index 915b7b71c8394..3b63a68a5d30b 100644 --- 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 index 575116ceed587..43681f1bd3fcf 100644 --- 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 index 0e6b2559af02e..ba0f15b86dd85 100644 --- 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 index 76ca5d33314d7..7d5bd0d6c5015 100644 --- 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 new file mode 100644 index 0000000000000..20f7fc77fa31f --- /dev/null +++ b/tests/cases/fourslash/organizeImports_removeOnly.ts @@ -0,0 +1,16 @@ +/// + +//// 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);