Skip to content

Add option to OrganizeImports for removal only #50931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 29, 2022
Merged

Add option to OrganizeImports for removal only #50931

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b99e35f
Remove unused imports
andrewbranch Sep 23, 2022
742ddfa
Lint
andrewbranch Sep 23, 2022
ae0dfb6
Update baselines
andrewbranch Sep 23, 2022
0d06478
Make mode paramter required
andrewbranch Sep 27, 2022
6b35ddc
Clean up
andrewbranch Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashInterfaceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,9 +681,17 @@ namespace ts.server.protocol {

export type OrganizeImportsScope = GetCombinedCodeFixScope;

export const enum OrganizeImportsMode {
All = "All",
SortAndCombine = "SortAndCombine",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the PR focuses on the RemoveUnused... I'm more excited about SortAndCombine :P I see that skipDestructiveCodeActions was a thing (didn't know about it), gonna give it a try in my projects for now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to be clear, SortAndCombine ought to be identical to setting skipDestructiveCodeActions which is already set in a VS Code code action called sortImports. (It’s not very discoverable because I learned recently that these code actions lack autocomplete support in the VS Code settings.json files.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye, I got the fact that this one isn't a new option - but I had no idea that it was already a thing. So I'm glad that I've discovered that old option through this PR :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how to actually use this old option in VS Code. I've found this in the source code:
https://github.com/microsoft/vscode/blob/3a8b1fe03ebbcf57fb9c50b161db91229e2fe04a/extensions/typescript-language-features/src/languageFeatures/organizeImports.ts#L50

I've tried a lot of combinations with _typescript.*, typescript.*, source.* and *.organizeImports.sortOnly: true but none of those work for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  "editor.codeActionsOnSave": {
    "source.sortImports": true
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Organize Imports is a VS Code standard command, and so automatically appears in the command palette when registered. These variations are registered so can be triggered by ID in configurations like codeActionsOnSave, but I think we would need to add them to the contributes of the extension package.json to show up in the command palette. @mjbvz is that correct?

RemoveUnused = "RemoveUnused",
}

export interface OrganizeImportsRequestArgs {
scope: OrganizeImportsScope;
/** @deprecated Use `mode` instead */
skipDestructiveCodeActions?: boolean;
mode?: OrganizeImportsMode;
}

export interface OrganizeImportsResponse extends Response {
Expand Down
2 changes: 1 addition & 1 deletion src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
56 changes: 35 additions & 21 deletions src/services/organizeImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T extends ImportDeclaration | ExportDeclaration>(
oldImportDecls: readonly T[],
coalesce: (group: readonly T[]) => readonly T[]) {
coalesce: (group: readonly T[]) => readonly T[],
mode: OrganizeImportsMode,
) {

if (length(oldImportDecls) === 0) {
return;
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] {
Expand Down
8 changes: 8 additions & 0 deletions src/services/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +585 to +587
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think skipDestructiveCodeActions was vaguely named due to aspirations of being used in more than one place, but it didn’t end up that way. FWIW, VS never picked this argument up, so it’s likely only VS Code that uses it.

}

export type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
Expand Down
14 changes: 14 additions & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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[];
Expand Down
7 changes: 7 additions & 0 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 7 additions & 1 deletion tests/cases/fourslash/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ declare module ts {
Message
}

enum OrganizeImportsMode {
All = "All",
SortAndCombine = "SortAndCombine",
RemoveUnused = "RemoveUnused",
}

interface DiagnosticMessage {
key: string;
category: DiagnosticCategory;
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 16 additions & 0 deletions tests/cases/fourslash/organizeImports_removeOnly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);