Skip to content
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.

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

Add option to OrganizeImports for removal only #50931

Merged
merged 5 commits into from Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
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
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
Expand Up @@ -681,9 +681,17 @@ namespace ts.server.protocol {

export type OrganizeImportsScope = GetCombinedCodeFixScope;

export const enum OrganizeImportsMode {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
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;
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
}

export interface OrganizeImportsResponse extends Response {
Expand Down
2 changes: 1 addition & 1 deletion src/server/session.ts
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
Expand Up @@ -13,37 +13,50 @@ namespace ts.OrganizeImports {
host: LanguageServiceHost,
program: Program,
preferences: UserPreferences,
skipDestructiveCodeActions?: boolean
mode = OrganizeImportsMode.All,
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
) {
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
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
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",
}
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved

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
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
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
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
@@ -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);