From b99e35f161b3d861e049309ef254b70537e70cc3 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 23 Sep 2022 15:57:23 -0700 Subject: [PATCH 1/5] Remove unused imports --- 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 +++ tests/cases/fourslash/fourslash.ts | 8 ++- .../fourslash/organizeImports_removeOnly.ts | 16 ++++++ 9 files changed, 81 insertions(+), 28 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 f0d26443dd522..0c41b7c870809 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -2535,7 +2535,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..4cb0ec51c62bd 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -13,37 +13,50 @@ namespace ts.OrganizeImports { host: LanguageServiceHost, program: Program, preferences: UserPreferences, - skipDestructiveCodeActions?: boolean + mode = OrganizeImportsMode.All, ) { 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 maybeRemove = mode === OrganizeImportsMode.RemoveUnused || mode === OrganizeImportsMode.All ? removeUnusedImports : identity; + const maybeCoalesce = mode === OrganizeImportsMode.SortAndCombine || mode === OrganizeImportsMode.All ? coalesceImports : identity; + const shouldSort = mode === OrganizeImportsMode.SortAndCombine || mode === OrganizeImportsMode.All; + 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, mode)); - // 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, mode); + } 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, mode)); - 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, mode); + } } return changeTracker.getChanges(); function organizeImportsWorker( oldImportDecls: readonly T[], - coalesce: (group: readonly T[]) => readonly T[]) { + coalesce: (group: readonly T[]) => readonly T[], + mode: OrganizeImportsMode, + ) { if (length(oldImportDecls) === 0) { return; @@ -56,8 +69,14 @@ 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 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 oldImportGroups = shouldCombine + ? group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!) + : [oldImportDecls]; + const sortedImportGroups = mode + ? 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 +148,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 a2078b875a568..674f6b5e5c48c 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 : undefined) + 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 d22416abb0b50..6154e19b4b30f 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/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); From 742ddfa0dbfb489be86ddd94771d14ca14634865 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 23 Sep 2022 16:04:20 -0700 Subject: [PATCH 2/5] Lint --- src/services/services.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/services.ts b/src/services/services.ts index 674f6b5e5c48c..9b6ccb9b08b29 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2076,7 +2076,7 @@ namespace ts { const sourceFile = getValidSourceFile(args.fileName); const formatContext = formatting.getFormatContext(formatOptions, host); - const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : undefined) + const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : undefined); return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, mode); } From ae0dfb61aaf4d78b474746f0a449d7792f1143df Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 23 Sep 2022 16:31:41 -0700 Subject: [PATCH 3/5] Update baselines --- tests/baselines/reference/api/tsserverlibrary.d.ts | 14 ++++++++++++++ tests/baselines/reference/api/typescript.d.ts | 7 +++++++ 2 files changed, 21 insertions(+) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 50fbd1d4dea72..60172518f403e 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -6028,8 +6028,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 { @@ -7612,9 +7619,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 d323d77c229dd..ad12962128797 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -6028,8 +6028,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 { From 0d064781bb37603f9d5d5b8d71c169984bcc1567 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Tue, 27 Sep 2022 14:22:50 -0700 Subject: [PATCH 4/5] Make mode paramter required --- src/services/organizeImports.ts | 2 +- src/services/services.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index 4cb0ec51c62bd..93c0f0aa049d8 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -13,7 +13,7 @@ namespace ts.OrganizeImports { host: LanguageServiceHost, program: Program, preferences: UserPreferences, - mode = OrganizeImportsMode.All, + mode: OrganizeImportsMode, ) { const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences }); const maybeRemove = mode === OrganizeImportsMode.RemoveUnused || mode === OrganizeImportsMode.All ? removeUnusedImports : identity; diff --git a/src/services/services.ts b/src/services/services.ts index 9b6ccb9b08b29..441a5860e54bd 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2076,7 +2076,7 @@ namespace ts { const sourceFile = getValidSourceFile(args.fileName); const formatContext = formatting.getFormatContext(formatOptions, host); - const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : undefined); + const mode = args.mode ?? (args.skipDestructiveCodeActions ? OrganizeImportsMode.SortAndCombine : OrganizeImportsMode.All); return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, mode); } From 6b35ddc0346412f7c87a262a3bf585b11e95141a Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Thu, 29 Sep 2022 14:21:03 -0700 Subject: [PATCH 5/5] Clean up --- src/services/organizeImports.ts | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index 93c0f0aa049d8..2928d604acdf3 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -16,9 +16,11 @@ namespace ts.OrganizeImports { mode: OrganizeImportsMode, ) { const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences }); - const maybeRemove = mode === OrganizeImportsMode.RemoveUnused || mode === OrganizeImportsMode.All ? removeUnusedImports : identity; - const maybeCoalesce = mode === OrganizeImportsMode.SortAndCombine || mode === OrganizeImportsMode.All ? coalesceImports : identity; 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 @@ -28,25 +30,25 @@ namespace ts.OrganizeImports { // All of the old ImportDeclarations in the file, in syntactic order. const topLevelImportGroupDecls = groupImportsByNewlineContiguous(sourceFile, sourceFile.statements.filter(isImportDeclaration)); - topLevelImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, processImportsOfSameModuleSpecifier, mode)); + topLevelImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, processImportsOfSameModuleSpecifier)); // 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, mode); + 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, processImportsOfSameModuleSpecifier, mode)); + ambientModuleImportGroupDecls.forEach(importGroupDecl => organizeImportsWorker(importGroupDecl, processImportsOfSameModuleSpecifier)); // Exports are always used if (mode !== OrganizeImportsMode.RemoveUnused) { const ambientModuleExportDecls = ambientModule.body.statements.filter(isExportDeclaration); - organizeImportsWorker(ambientModuleExportDecls, coalesceExports, mode); + organizeImportsWorker(ambientModuleExportDecls, coalesceExports); } } @@ -55,9 +57,7 @@ namespace ts.OrganizeImports { function organizeImportsWorker( oldImportDecls: readonly T[], coalesce: (group: readonly T[]) => readonly T[], - mode: OrganizeImportsMode, ) { - if (length(oldImportDecls) === 0) { return; } @@ -69,12 +69,10 @@ namespace ts.OrganizeImports { // but the consequences of being wrong are very minor. suppressLeadingTrivia(oldImportDecls[0]); - 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 oldImportGroups = shouldCombine ? group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!) : [oldImportDecls]; - const sortedImportGroups = mode + const sortedImportGroups = shouldSort ? stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier)) : oldImportGroups; const newImportDecls = flatMap(sortedImportGroups, importGroup =>