diff --git a/src/harness/client.ts b/src/harness/client.ts index 537ee3f36a0b3..cd46f97a09d33 100644 --- a/src/harness/client.ts +++ b/src/harness/client.ts @@ -710,7 +710,7 @@ namespace ts.server { }; } - organizeImports(_scope: OrganizeImportsScope, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] { + organizeImports(_args: OrganizeImportsArgs, _formatOptions: FormatCodeSettings): readonly FileTextChanges[] { return notImplemented(); } diff --git a/src/harness/harnessLanguageService.ts b/src/harness/harnessLanguageService.ts index 17026bd5001b0..956d8c5ea9825 100644 --- a/src/harness/harnessLanguageService.ts +++ b/src/harness/harnessLanguageService.ts @@ -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[] { diff --git a/src/server/protocol.ts b/src/server/protocol.ts index 4608601f75ab0..5855fa0238d18 100644 --- a/src/server/protocol.ts +++ b/src/server/protocol.ts @@ -681,6 +681,7 @@ namespace ts.server.protocol { export interface OrganizeImportsRequestArgs { scope: OrganizeImportsScope; + skipDestructiveCodeActions?: boolean; } export interface OrganizeImportsResponse extends Response { diff --git a/src/server/session.ts b/src/server/session.ts index c31002b94a626..ce293e7f022da 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -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); } diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index c67b9df12e419..372dc45c53404 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -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. @@ -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); diff --git a/src/services/services.ts b/src/services/services.ts index 160c97e81e1e3..09af46f346ebd 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -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[] { diff --git a/src/services/types.ts b/src/services/types.ts index 0d50cab5e624c..fa7bcf31ac0b8 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -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 = "." | '"' | "'" | "`" | "/" | "@" | "<" | "#" | " "; diff --git a/src/testRunner/unittests/services/organizeImports.ts b/src/testRunner/unittests/services/organizeImports.ts index 13f3f126c9e46..496f2d401d38b 100644 --- a/src/testRunner/unittests/services/organizeImports.ts +++ b/src/testRunner/unittests/services/organizeImports.ts @@ -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 // `
` 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); diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index c0fd76b909302..3f7c5ae9783da 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -5649,7 +5649,7 @@ declare namespace ts { applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise