From feb94d09e714b345a41d77b72db61afc0f2d68d3 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Fri, 20 Dec 2024 02:33:35 +0000 Subject: [PATCH] fix(lsp): rewrite imports for 'Move to a new file' action (#27427) --- cli/lsp/analysis.rs | 16 +++-- cli/lsp/documents.rs | 7 ++ cli/lsp/language_server.rs | 44 +++++-------- tests/integration/lsp_tests.rs | 113 +++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 33 deletions(-) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 9f9cf14864..4c6a20927b 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -603,18 +603,24 @@ fn try_reverse_map_package_json_exports( /// For a set of tsc changes, can them for any that contain something that looks /// like an import and rewrite the import specifier to include the extension pub fn fix_ts_import_changes( - referrer: &ModuleSpecifier, - resolution_mode: ResolutionMode, changes: &[tsc::FileTextChanges], language_server: &language_server::Inner, ) -> Result, AnyError> { - let import_mapper = language_server.get_ts_response_import_mapper(referrer); let mut r = Vec::new(); for change in changes { + let Ok(referrer) = ModuleSpecifier::parse(&change.file_name) else { + continue; + }; + let referrer_doc = language_server.get_asset_or_document(&referrer).ok(); + let resolution_mode = referrer_doc + .as_ref() + .map(|d| d.resolution_mode()) + .unwrap_or(ResolutionMode::Import); + let import_mapper = + language_server.get_ts_response_import_mapper(&referrer); let mut text_changes = Vec::new(); for text_change in &change.text_changes { let lines = text_change.new_text.split('\n'); - let new_lines: Vec = lines .map(|line| { // This assumes that there's only one import per line. @@ -622,7 +628,7 @@ pub fn fix_ts_import_changes( let specifier = captures.iter().skip(1).find_map(|s| s).unwrap().as_str(); if let Some(new_specifier) = import_mapper - .check_unresolved_specifier(specifier, referrer, resolution_mode) + .check_unresolved_specifier(specifier, &referrer, resolution_mode) { line.replace(specifier, &new_specifier) } else { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index bdb64c9da3..d15cfe5a6c 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -251,6 +251,13 @@ impl AssetOrDocument { pub fn document_lsp_version(&self) -> Option { self.document().and_then(|d| d.maybe_lsp_version()) } + + pub fn resolution_mode(&self) -> ResolutionMode { + match self { + AssetOrDocument::Asset(_) => ResolutionMode::Import, + AssetOrDocument::Document(d) => d.resolution_mode(), + } + } } type ModuleResult = Result; diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index bd461833c2..a7a0a59743 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1855,20 +1855,12 @@ impl Inner { } let changes = if code_action_data.fix_id == "fixMissingImport" { - fix_ts_import_changes( - &code_action_data.specifier, - maybe_asset_or_doc - .as_ref() - .and_then(|d| d.document()) - .map(|d| d.resolution_mode()) - .unwrap_or(ResolutionMode::Import), - &combined_code_actions.changes, - self, - ) - .map_err(|err| { - error!("Unable to remap changes: {:#}", err); - LspError::internal_error() - })? + fix_ts_import_changes(&combined_code_actions.changes, self).map_err( + |err| { + error!("Unable to remap changes: {:#}", err); + LspError::internal_error() + }, + )? } else { combined_code_actions.changes }; @@ -1912,20 +1904,16 @@ impl Inner { asset_or_doc.scope().cloned(), ) .await?; - if kind_suffix == ".rewrite.function.returnType" { - refactor_edit_info.edits = fix_ts_import_changes( - &action_data.specifier, - asset_or_doc - .document() - .map(|d| d.resolution_mode()) - .unwrap_or(ResolutionMode::Import), - &refactor_edit_info.edits, - self, - ) - .map_err(|err| { - error!("Unable to remap changes: {:#}", err); - LspError::internal_error() - })? + if kind_suffix == ".rewrite.function.returnType" + || kind_suffix == ".move.newFile" + { + refactor_edit_info.edits = + fix_ts_import_changes(&refactor_edit_info.edits, self).map_err( + |err| { + error!("Unable to remap changes: {:#}", err); + LspError::internal_error() + }, + )? } code_action.edit = refactor_edit_info.to_workspace_edit(self)?; code_action diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 56c060d958..825cef6247 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -6066,6 +6066,119 @@ fn lsp_jsr_code_action_missing_declaration() { ); } +#[test] +fn lsp_jsr_code_action_move_to_new_file() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + let file = source_file( + temp_dir.path().join("file.ts"), + r#" + import { someFunction } from "jsr:@denotest/types-file"; + export const someValue = someFunction(); + "#, + ); + let mut client = context.new_lsp_command().build(); + client.initialize_default(); + client.write_request( + "workspace/executeCommand", + json!({ + "command": "deno.cache", + "arguments": [[], file.url()], + }), + ); + client.did_open_file(&file); + let list = client + .write_request_with_res_as::>( + "textDocument/codeAction", + json!({ + "textDocument": { "uri": file.url() }, + "range": { + "start": { "line": 2, "character": 19 }, + "end": { "line": 2, "character": 28 }, + }, + "context": { "diagnostics": [] }, + }), + ) + .unwrap(); + let action = list + .iter() + .find_map(|c| match c { + lsp::CodeActionOrCommand::CodeAction(a) + if &a.title == "Move to a new file" => + { + Some(a) + } + _ => None, + }) + .unwrap(); + let res = client.write_request("codeAction/resolve", json!(action)); + assert_eq!( + res, + json!({ + "title": "Move to a new file", + "kind": "refactor.move.newFile", + "edit": { + "documentChanges": [ + { + "textDocument": { "uri": file.url(), "version": 1 }, + "edits": [ + { + "range": { + "start": { "line": 1, "character": 6 }, + "end": { "line": 2, "character": 0 }, + }, + "newText": "", + }, + { + "range": { + "start": { "line": 2, "character": 0 }, + "end": { "line": 3, "character": 4 }, + }, + "newText": "", + }, + ], + }, + { + "kind": "create", + "uri": file.url().join("someValue.ts").unwrap(), + "options": { + "ignoreIfExists": true, + }, + }, + { + "textDocument": { + "uri": file.url().join("someValue.ts").unwrap(), + "version": null, + }, + "edits": [ + { + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "import { someFunction } from \"jsr:@denotest/types-file\";\n\nexport const someValue = someFunction();\n", + }, + ], + }, + ], + }, + "isPreferred": false, + "data": { + "specifier": file.url(), + "range": { + "start": { "line": 2, "character": 19 }, + "end": { "line": 2, "character": 28 }, + }, + "refactorName": "Move to a new file", + "actionName": "Move to a new file", + }, + }), + ); +} + #[test] fn lsp_code_actions_deno_cache_npm() { let context = TestContextBuilder::new().use_temp_cwd().build();