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 all 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 @@ -710,7 +710,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/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(_args: ts.OrganizeImportsArgs, _formatOptions: ts.FormatCodeSettings): readonly ts.FileTextChanges[] {
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 @@ -681,6 +681,7 @@ namespace ts.server.protocol {

export interface OrganizeImportsRequestArgs {
scope: OrganizeImportsScope;
skipDestructiveCodeActions?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have an actual convention, but it looks like we have a pattern of using positive names and, if necessary, treating undefined as true. e.g. UserPreferences.includeAutomaticOptionalChainCompletions or CompilerOptions.AllowUnreachableCode. I think @andrewbranch has also found this frustrating and might have suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don’t really have a preference here. This looks fine to me.

}

export interface OrganizeImportsResponse extends Response {
Expand Down
16 changes: 12 additions & 4 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2201,10 +2201,18 @@ 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(
{
fileName: file,
skipDestructiveCodeActions: args.skipDestructiveCodeActions,
type: "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,
skipDestructiveCodeActions?: boolean
) {

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

const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => stableSort(
coalesceImports(removeUnusedImports(importGroup, sourceFile, program)),
coalesceImports(removeUnusedImports(importGroup, sourceFile, program, skipDestructiveCodeActions)),
(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, skipDestructiveCodeActions: boolean | undefined) {
// As a precaution, consider unused import detection to be destructive (GH #43051)
if (skipDestructiveCodeActions) {
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 @@ -2017,13 +2017,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.type === "file");
const sourceFile = getValidSourceFile(args.fileName);
const formatContext = formatting.getFormatContext(formatOptions, host);

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

function getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
Expand Down
6 changes: 4 additions & 2 deletions src/services/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,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 @@ -552,7 +552,9 @@ namespace ts {

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

export type OrganizeImportsScope = CombinedCodeFixScope;
export interface OrganizeImportsArgs extends CombinedCodeFixScope {
skipDestructiveCodeActions?: boolean;
}

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

Expand Down
Loading