Microsoft/TypeScript
1
0
Fork 0
mirror of https://github.com/microsoft/TypeScript.git synced 2025-11-18 17:21:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Added skipDestructiveCodeActions argument to organize imports server command (#43184)

Browse Source 
* Stopped removing unused imports in files with syntactic errors

* Added allowDestructiveCodeActions arg

* Updated .d.ts baselines

* Stop factoring syntax errors. Weird that no tests break...

* Have args extend scope so it is not a breaking change

* Update src/harness/harnessLanguageService.ts

Co-authored-by: Jesse Trinity <jetrinit@microsoft.com>

* Fixed API breaking change, and renamed to skip

* Always with the baselines

* One more .d.ts baseline to fix

* Remove blank line in src/harness/harnessLanguageService.ts

Co-authored-by: Jesse Trinity <jetrinit@microsoft.com>
This commit is contained in:
Josh Goldberg
2021-04-20 12:04:17 -04:00
committed by GitHub
parent f67ee44379
commit a910c8df13
14 changed files with 200 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/harness/client.ts
+1 -1
View File
@@ -710,7 +710,7 @@ namespace ts.server {
};
}
organizeImports(_scope: OrganizeImportsScope, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] {
organizeImports(_args: OrganizeImportsArgs, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] {
return notImplemented();
}
src/harness/harnessLanguageService.ts
+1 -1
View File
@@ -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[] {
src/server/protocol.ts
+1
View File
@@ -681,6 +681,7 @@ namespace ts.server.protocol {
export interface OrganizeImportsRequestArgs {
scope: OrganizeImportsScope;
skipDestructiveCodeActions?: boolean;
}
export interface OrganizeImportsResponse extends Response {
src/server/session.ts
+12 -4
View File
@@ -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);
}
src/services/organizeImports.ts
+8 -2
View File
@@ -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.
@@ -87,7 +88,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);
src/services/services.ts
+4 -4
View File
@@ -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[] {
src/services/types.ts
+4 -2
View File
@@ -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;
@@ -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 = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
src/testRunner/unittests/services/organizeImports.ts
+98 -5
View File
@@ -339,6 +339,7 @@ export const Other = 1;
});
testOrganizeImports("Renamed_used",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -349,6 +350,7 @@ EffOne();
libFile);
testOrganizeImports("Simple",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -365,6 +367,7 @@ F2();
libFile);
testOrganizeImports("Unused_Some",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -377,6 +380,70 @@ D();
},
libFile);
describe("skipDestructiveCodeActions=true", () => {
testOrganizeImports("Syntax_Error_Body_skipDestructiveCodeActions",
/*skipDestructiveCodeActions*/ true,
{
path: "/test.ts",
content: `
import { F1, F2 } from "lib";
import * as NS from "lib";
import D from "lib";
class class class;
D;
`,
},
libFile);
});
testOrganizeImports("Syntax_Error_Imports_skipDestructiveCodeActions",
/*skipDestructiveCodeActions*/ true,
{
path: "/test.ts",
content: `
import { F1, F2 class class class; } from "lib";
import * as NS from "lib";
class class class;
import D from "lib";
D;
`,
},
libFile);
describe("skipDestructiveCodeActions=false", () => {
testOrganizeImports("Syntax_Error_Body",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
import { F1, F2 } from "lib";
import * as NS from "lib";
import D from "lib";
class class class;
D;
`,
},
libFile);
testOrganizeImports("Syntax_Error_Imports",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
import { F1, F2 class class class; } from "lib";
import * as NS from "lib";
class class class;
import D from "lib";
D;
`,
},
libFile);
});
it("doesn't return any changes when the text would be identical", () => {
const testFile = {
path: "/a.ts",
@@ -388,6 +455,7 @@ D();
});
testOrganizeImports("Unused_All",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -411,6 +479,7 @@ import { } from "lib";
});
testOrganizeImports("Unused_false_positive_module_augmentation",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.d.ts",
content: `
@@ -426,6 +495,7 @@ declare module 'caseless' {
});
testOrganizeImports("Unused_preserve_imports_for_module_augmentation_in_non_declaration_file",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -467,6 +537,7 @@ export { x };
});
testOrganizeImports("MoveToTop",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -483,6 +554,7 @@ D();
/* eslint-disable no-template-curly-in-string */
testOrganizeImports("MoveToTop_Invalid",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -501,6 +573,7 @@ D();
/* eslint-enable no-template-curly-in-string */
testOrganizeImports("TypeOnly",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -513,6 +586,7 @@ export { A, B, X, Y, Z };`
});
testOrganizeImports("CoalesceMultipleModules",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -527,6 +601,7 @@ a + b + c + d;
{ path: "/lib2.ts", content: "export const a = 3, c = 4;" });
testOrganizeImports("CoalesceTrivia",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -540,6 +615,7 @@ F2();
libFile);
testOrganizeImports("SortTrivia",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -551,6 +627,7 @@ F2();
{ path: "/lib2.ts", content: "" });
testOrganizeImports("UnusedTrivia1",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -560,6 +637,7 @@ F2();
libFile);
testOrganizeImports("UnusedTrivia2",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -571,6 +649,7 @@ F1();
libFile);
testOrganizeImports("UnusedHeaderComment",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -581,6 +660,7 @@ import { F1 } from "lib";
libFile);
testOrganizeImports("SortHeaderComment",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -593,6 +673,7 @@ import "lib1";
{ path: "/lib2.ts", content: "" });
testOrganizeImports("SortComments",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -609,6 +690,7 @@ import "lib1";
{ path: "/lib3.ts", content: "" });
testOrganizeImports("AmbientModule",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -624,6 +706,7 @@ declare module "mod" {
libFile);
testOrganizeImports("TopLevelAndAmbientModule",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -646,6 +729,7 @@ D();
libFile);
testOrganizeImports("JsxFactoryUsedJsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.jsx",
content: `
@@ -657,6 +741,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxFactoryUsedJs",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.js",
content: `
@@ -668,6 +753,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxFactoryUsedTsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.tsx",
content: `
@@ -681,6 +767,7 @@ import { React, Other } from "react";
// TS files are not JSX contexts, so the parser does not treat
// `<div/>` as a JSX element.
testOrganizeImports("JsxFactoryUsedTs",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -692,6 +779,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxFactoryUnusedJsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.jsx",
content: `
@@ -703,6 +791,7 @@ import { React, Other } from "react";
// Note: Since the file extension does not end with "x", the jsx compiler option
// will not be enabled. The import should be retained regardless.
testOrganizeImports("JsxFactoryUnusedJs",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.js",
content: `
@@ -712,6 +801,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxFactoryUnusedTsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.tsx",
content: `
@@ -721,6 +811,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxFactoryUnusedTs",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.ts",
content: `
@@ -730,6 +821,7 @@ import { React, Other } from "react";
reactLibFile);
testOrganizeImports("JsxPragmaTsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.tsx",
content: `/** @jsx jsx */
@@ -758,6 +850,7 @@ export namespace React {
);
testOrganizeImports("JsxFragmentPragmaTsx",
/*skipDestructiveCodeActions*/ false,
{
path: "/test.tsx",
content: `/** @jsx h */
@@ -920,17 +1013,17 @@ export * from "lib";
});
function testOrganizeExports(testName: string, testFile: TestFSWithWatch.File, ...otherFiles: TestFSWithWatch.File[]) {
testOrganizeImports(`${testName}.exports`, testFile, ...otherFiles);
testOrganizeImports(`${testName}.exports`, /*skipDestructiveCodeActions*/ true, testFile, ...otherFiles);
}
function testOrganizeImports(testName: string, testFile: TestFSWithWatch.File, ...otherFiles: TestFSWithWatch.File[]) {
it(testName, () => runBaseline(`organizeImports/${testName}.ts`, testFile, ...otherFiles));
function testOrganizeImports(testName: string, skipDestructiveCodeActions: boolean, testFile: TestFSWithWatch.File, ...otherFiles: TestFSWithWatch.File[]) {
it(testName, () => runBaseline(`organizeImports/${testName}.ts`, skipDestructiveCodeActions, testFile, ...otherFiles));
}
function runBaseline(baselinePath: string, testFile: TestFSWithWatch.File, ...otherFiles: TestFSWithWatch.File[]) {
function runBaseline(baselinePath: string, skipDestructiveCodeActions: boolean, testFile: TestFSWithWatch.File, ...otherFiles: TestFSWithWatch.File[]) {
const { path: testPath, content: testContent } = testFile;
const languageService = makeLanguageService(testFile, ...otherFiles);
const changes = languageService.organizeImports({ type: "file", fileName: testPath }, testFormatSettings, emptyOptions);
const changes = languageService.organizeImports({ skipDestructiveCodeActions, type: "file", fileName: testPath }, testFormatSettings, emptyOptions);
assert.equal(changes.length, 1);
assert.equal(changes[0].fileName, testPath);
tests/baselines/reference/api/tsserverlibrary.d.ts
Vendored
+5 -2
View File
@@ -5649,7 +5649,7 @@ declare namespace ts {
applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise<ApplyCodeActionCommandResult | ApplyCodeActionCommandResult[]>;
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;
getProgram(): Program | undefined;
@@ -5666,7 +5666,9 @@ declare namespace ts {
type: "file";
fileName: string;
}
type OrganizeImportsScope = CombinedCodeFixScope;
interface OrganizeImportsArgs extends CombinedCodeFixScope {
skipDestructiveCodeActions?: boolean;
}
type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
interface GetCompletionsAtPositionOptions extends UserPreferences {
/**
@@ -7169,6 +7171,7 @@ declare namespace ts.server.protocol {
type OrganizeImportsScope = GetCombinedCodeFixScope;
interface OrganizeImportsRequestArgs {
scope: OrganizeImportsScope;
skipDestructiveCodeActions?: boolean;
}
interface OrganizeImportsResponse extends Response {
body: readonly FileCodeEdits[];
tests/baselines/reference/api/typescript.d.ts
Vendored
+4 -2
View File
@@ -5649,7 +5649,7 @@ declare namespace ts {
applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise<ApplyCodeActionCommandResult | ApplyCodeActionCommandResult[]>;
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;
getProgram(): Program | undefined;
@@ -5666,7 +5666,9 @@ declare namespace ts {
type: "file";
fileName: string;
}
type OrganizeImportsScope = CombinedCodeFixScope;
interface OrganizeImportsArgs extends CombinedCodeFixScope {
skipDestructiveCodeActions?: boolean;
}
type CompletionsTriggerCharacter = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " ";
interface GetCompletionsAtPositionOptions extends UserPreferences {
/**
tests/baselines/reference/organizeImports/Syntax_Error_Body.ts
+15
View File
@@ -0,0 +1,15 @@
// ==ORIGINAL==
import { F1, F2 } from "lib";
import * as NS from "lib";
import D from "lib";
class class class;
D;
// ==ORGANIZED==
import D from "lib";
class class class;
D;
tests/baselines/reference/organizeImports/Syntax_Error_Body_skipDestructiveCodeActions.ts
+16
View File
@@ -0,0 +1,16 @@
// ==ORIGINAL==
import { F1, F2 } from "lib";
import * as NS from "lib";
import D from "lib";
class class class;
D;
// ==ORGANIZED==
import * as NS from "lib";
import D, { F1, F2 } from "lib";
class class class;
D;
tests/baselines/reference/organizeImports/Syntax_Error_Imports.ts
+15
View File
@@ -0,0 +1,15 @@
// ==ORIGINAL==
import { F1, F2 class class class; } from "lib";
import * as NS from "lib";
class class class;
import D from "lib";
D;
// ==ORGANIZED==
import D from "lib";
class class class;
D;
tests/baselines/reference/organizeImports/Syntax_Error_Imports_skipDestructiveCodeActions.ts
+16
View File
@@ -0,0 +1,16 @@
// ==ORIGINAL==
import { F1, F2 class class class; } from "lib";
import * as NS from "lib";
class class class;
import D from "lib";
D;
// ==ORGANIZED==
import * as NS from "lib";
import D, { class, class, class, F1, F2 } from "lib";
class class class;
D;