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.

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

Added skipDestructiveCodeActions argument to organize imports server command #43184

Merged
merged 11 commits into from
Apr 20, 2021
Merged

Added skipDestructiveCodeActions argument to organize imports server command #43184

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1e8ea6b
Stopped removing unused imports in files with syntactic errors
Mar 10, 2021
3a61904
Added allowDestructiveCodeActions arg
Apr 12, 2021
27417f1
Updated .d.ts baselines
Apr 12, 2021
394eeb4
Stop factoring syntax errors. Weird that no tests break...
Apr 13, 2021
1637e1a
Have args extend scope so it is not a breaking change
Apr 13, 2021
49c61c2
Merge branch 'master'
Apr 13, 2021
4cd6220
Update src/harness/harnessLanguageService.ts
JoshuaKGoldberg Apr 15, 2021
dacfd7e
Fixed API breaking change, and renamed to skip
Apr 17, 2021
baca715
Always with the baselines
Apr 17, 2021
c38d291
One more .d.ts baseline to fix
Apr 17, 2021
54a544d
Remove blank line in src/harness/harnessLanguageService.ts
JoshuaKGoldberg Apr 17, 2021
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
2 changes: 1 addition & 1 deletion src/harness/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ namespace ts.server {
};
}

organizeImports(_scope: OrganizeImportsScope, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] {
organizeImports(_args: OrganizeImportsArgs, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] {
return notImplemented();
}

Expand Down
2 changes: 1 addition & 1 deletion src/harness/fourslashImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ namespace FourSlash {
}

public verifyOrganizeImports(newContent: string) {
const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file" }, this.formatCodeSettings, ts.emptyOptions);
const changes = this.languageService.organizeImports({ scope: { fileName: this.activeFile.fileName, type: "file" } }, this.formatCodeSettings, ts.emptyOptions);
this.applyChanges(changes);
this.verifyFileContent(this.activeFile.fileName, newContent);
}
Expand Down
2 changes: 1 addition & 1 deletion src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ namespace Harness.LanguageService {
getApplicableRefactors(): ts.ApplicableRefactorInfo[] {
throw new Error("Not supported on the shim.");
}
organizeImports(_scope: ts.OrganizeImportsScope, _formatOptions: ts.FormatCodeSettings): readonly ts.FileTextChanges[] {
organizeImports(_scope: ts.OrganizeImportsArgs, _formatOptions: ts.FormatCodeSettings): readonly ts.FileTextChanges[] {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
throw new Error("Not supported on the shim.");
}
getEditsForFileRename(): readonly ts.FileTextChanges[] {
Expand Down
1 change: 1 addition & 0 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,7 @@ namespace ts.server.protocol {
export type OrganizeImportsScope = GetCombinedCodeFixScope;

export interface OrganizeImportsRequestArgs {
allowDestructiveCodeActions?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

To confirm, we expect the editor to want to make this decision per-request and that's why it's not a UserPreference?

Copy link
Member

Choose a reason for hiding this comment

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

Also i would name this flag as skipDestructiveCodeActions so that you would need to check only for truthiness instead of defaults when not set..

Copy link
Contributor

@jessetrinity jessetrinity Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To confirm, we expect the editor to want to make this decision per-request and that's why it's not a UserPreference?

Yes, the editor would need to specify the option based on whether the command was sent as a part of compileOnSave or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dacfd7e renames to skipDestructiveCodeActions and defaults it to falsy.

scope: OrganizeImportsScope;
}

Expand Down
15 changes: 11 additions & 4 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2165,10 +2165,17 @@ namespace ts.server {
}
}

private organizeImports({ scope }: protocol.OrganizeImportsRequestArgs, simplifiedResult: boolean): readonly protocol.FileCodeEdits[] | readonly FileTextChanges[] {
Debug.assert(scope.type === "file");
const { file, project } = this.getFileAndProject(scope.args);
const changes = project.getLanguageService().organizeImports({ type: "file", fileName: file }, this.getFormatOptions(file), this.getPreferences(file));
private organizeImports(args: protocol.OrganizeImportsRequestArgs, simplifiedResult: boolean): readonly protocol.FileCodeEdits[] | readonly FileTextChanges[] {
Debug.assert(args.scope.type === "file");
const { file, project } = this.getFileAndProject(args.scope.args);
const changes = project.getLanguageService().organizeImports(
{
allowDestructiveCodeActions: args.allowDestructiveCodeActions,
scope: { type: "file", fileName: file },
},
this.getFormatOptions(file),
this.getPreferences(file)
);
if (simplifiedResult) {
return this.mapTextChangesToCodeEdits(changes);
}
Expand Down
10 changes: 8 additions & 2 deletions src/services/organizeImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ namespace ts.OrganizeImports {
host: LanguageServiceHost,
program: Program,
preferences: UserPreferences,
allowDestructiveCodeActions = true
) {

const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences });

const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => stableSort(
coalesceImports(removeUnusedImports(importGroup, sourceFile, program)),
coalesceImports(removeUnusedImports(importGroup, sourceFile, program, allowDestructiveCodeActions)),
(s1, s2) => compareImportsOrRequireStatements(s1, s2));

// All of the old ImportDeclarations in the file, in syntactic order.
Expand Down Expand Up @@ -83,7 +84,12 @@ namespace ts.OrganizeImports {
}
}

function removeUnusedImports(oldImports: readonly ImportDeclaration[], sourceFile: SourceFile, program: Program) {
function removeUnusedImports(oldImports: readonly ImportDeclaration[], sourceFile: SourceFile, program: Program, allowDestructiveCodeActions: boolean) {
// As a precaution, if there are *syntax* errors in the file, consider unused import detection to be destructive (GH #43051)
if (!allowDestructiveCodeActions && program.getSyntacticDiagnostics(sourceFile).length) {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
return oldImports;
}

const typeChecker = program.getTypeChecker();
const jsxNamespace = typeChecker.getJsxNamespace(sourceFile);
const jsxFragmentFactory = typeChecker.getJsxFragmentFactory(sourceFile);
Expand Down
8 changes: 4 additions & 4 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1973,13 +1973,13 @@ namespace ts {
return codefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext, preferences });
}

function organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
function organizeImports(args: OrganizeImportsArgs, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
synchronizeHostData();
Debug.assert(scope.type === "file");
const sourceFile = getValidSourceFile(scope.fileName);
Debug.assert(args.scope.type === "file");
const sourceFile = getValidSourceFile(args.scope.fileName);
const formatContext = formatting.getFormatContext(formatOptions, host);

return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences);
return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences, args.allowDestructiveCodeActions);
}

function getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
Expand Down
7 changes: 5 additions & 2 deletions src/services/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ namespace ts {

getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined, triggerReason?: RefactorTriggerReason, kind?: string): ApplicableRefactorInfo[];
getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string, preferences: UserPreferences | undefined): RefactorEditInfo | undefined;
organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings, preferences: UserPreferences | undefined): readonly FileTextChanges[];
organizeImports(args: OrganizeImportsArgs, formatOptions: FormatCodeSettings, preferences: UserPreferences | undefined): readonly FileTextChanges[];
getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences | undefined): readonly FileTextChanges[];

getEmitOutput(fileName: string, emitOnlyDtsFiles?: boolean, forceDtsEmit?: boolean): EmitOutput;
Expand All @@ -548,7 +548,10 @@ namespace ts {

export interface CombinedCodeFixScope { type: "file"; fileName: string; }

export type OrganizeImportsScope = CombinedCodeFixScope;
export interface OrganizeImportsArgs {
allowDestructiveCodeActions?: boolean;
scope: CombinedCodeFixScope;
}

export type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#";

Expand Down
Loading