From 925ba8fbbf947c2c95a616b43e3f89e20cd69e93 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Sat, 29 May 2021 21:21:11 +1000 Subject: [PATCH] fix(#10765): lsp import fixes include extensions (#10778) Fixes #10765 --- cli/lsp/analysis.rs | 145 +++++++++++ cli/lsp/language_server.rs | 28 +- cli/lsp/tsc.rs | 10 +- cli/tests/integration_tests_lsp.rs | 51 ++++ cli/tests/lsp/code_action_params_imports.json | 54 ++++ .../code_action_resolve_params_imports.json | 26 ++ .../code_action_resolve_response_imports.json | 51 ++++ .../lsp/code_action_response_imports.json | 242 ++++++++++++++++++ 8 files changed, 594 insertions(+), 13 deletions(-) create mode 100644 cli/tests/lsp/code_action_params_imports.json create mode 100644 cli/tests/lsp/code_action_resolve_params_imports.json create mode 100644 cli/tests/lsp/code_action_resolve_response_imports.json create mode 100644 cli/tests/lsp/code_action_response_imports.json diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 9a1a2d5077..4ef4a6e22a 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -11,6 +11,7 @@ use crate::module_graph::parse_ts_reference; use crate::module_graph::TypeScriptReference; use crate::tools::lint::create_linter; +use deno_core::error::anyhow; use deno_core::error::custom_error; use deno_core::error::AnyError; use deno_core::serde::Deserialize; @@ -23,6 +24,7 @@ use deno_lint::rules; use lspower::lsp; use lspower::lsp::Position; use lspower::lsp::Range; +use regex::Regex; use std::cmp::Ordering; use std::collections::HashMap; use std::fmt; @@ -56,8 +58,12 @@ lazy_static::lazy_static! { .iter() .cloned() .collect(); + + static ref IMPORT_SPECIFIER_RE: Regex = Regex::new(r#"\sfrom\s+["']([^"']*)["']"#).unwrap(); } +const SUPPORTED_EXTENSIONS: &[&str] = &[".ts", ".tsx", ".js", ".jsx", ".mjs"]; + /// Category of self-generated diagnostic messages (those not coming from) /// TypeScript. #[derive(Debug, PartialEq, Eq)] @@ -417,6 +423,143 @@ fn code_as_string(code: &Option) -> String { } } +/// Iterate over the supported extensions, concatenating the extension on the +/// specifier, returning the first specifier that is resolve-able, otherwise +/// None if none match. +fn check_specifier( + specifier: &str, + referrer: &ModuleSpecifier, + snapshot: &language_server::StateSnapshot, + maybe_import_map: &Option, +) -> Option { + for ext in SUPPORTED_EXTENSIONS { + let specifier_with_ext = format!("{}{}", specifier, ext); + if let ResolvedDependency::Resolved(resolved_specifier) = + resolve_import(&specifier_with_ext, referrer, maybe_import_map) + { + if snapshot.documents.contains_key(&resolved_specifier) + || snapshot.sources.contains_key(&resolved_specifier) + { + return Some(specifier_with_ext); + } + } + } + + None +} + +/// 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(crate) fn fix_ts_import_changes( + referrer: &ModuleSpecifier, + changes: &[tsc::FileTextChanges], + language_server: &language_server::Inner, +) -> Result, AnyError> { + let mut r = Vec::new(); + let snapshot = language_server.snapshot()?; + for change in changes { + let mut text_changes = Vec::new(); + for text_change in &change.text_changes { + if let Some(captures) = + IMPORT_SPECIFIER_RE.captures(&text_change.new_text) + { + let specifier = captures + .get(1) + .ok_or_else(|| anyhow!("Missing capture."))? + .as_str(); + if let Some(new_specifier) = check_specifier( + specifier, + referrer, + &snapshot, + &language_server.maybe_import_map, + ) { + let new_text = + text_change.new_text.replace(specifier, &new_specifier); + text_changes.push(tsc::TextChange { + span: text_change.span.clone(), + new_text, + }); + } else { + text_changes.push(text_change.clone()); + } + } else { + text_changes.push(text_change.clone()); + } + } + r.push(tsc::FileTextChanges { + file_name: change.file_name.clone(), + text_changes, + is_new_file: change.is_new_file, + }); + } + Ok(r) +} + +/// Fix tsc import code actions so that the module specifier is correct for +/// resolution by Deno (includes the extension). +fn fix_ts_import_action( + referrer: &ModuleSpecifier, + action: &tsc::CodeFixAction, + language_server: &language_server::Inner, +) -> Result { + if action.fix_name == "import" { + let change = action + .changes + .get(0) + .ok_or_else(|| anyhow!("Unexpected action changes."))?; + let text_change = change + .text_changes + .get(0) + .ok_or_else(|| anyhow!("Missing text change."))?; + if let Some(captures) = IMPORT_SPECIFIER_RE.captures(&text_change.new_text) + { + let specifier = captures + .get(1) + .ok_or_else(|| anyhow!("Missing capture."))? + .as_str(); + let snapshot = language_server.snapshot()?; + if let Some(new_specifier) = check_specifier( + specifier, + referrer, + &snapshot, + &language_server.maybe_import_map, + ) { + let description = action.description.replace(specifier, &new_specifier); + let changes = action + .changes + .iter() + .map(|c| { + let text_changes = c + .text_changes + .iter() + .map(|tc| tsc::TextChange { + span: tc.span.clone(), + new_text: tc.new_text.replace(specifier, &new_specifier), + }) + .collect(); + tsc::FileTextChanges { + file_name: c.file_name.clone(), + text_changes, + is_new_file: c.is_new_file, + } + }) + .collect(); + + return Ok(tsc::CodeFixAction { + description, + changes, + commands: None, + fix_name: action.fix_name.clone(), + fix_id: None, + fix_all_description: None, + }); + } + } + } + + Ok(action.clone()) +} + /// Determines if two TypeScript diagnostic codes are effectively equivalent. fn is_equivalent_code( a: &Option, @@ -547,6 +690,7 @@ impl CodeActionCollection { /// Add a TypeScript code fix action to the code actions collection. pub(crate) async fn add_ts_fix_action( &mut self, + specifier: &ModuleSpecifier, action: &tsc::CodeFixAction, diagnostic: &lsp::Diagnostic, language_server: &mut language_server::Inner, @@ -564,6 +708,7 @@ impl CodeActionCollection { "The action returned from TypeScript is unsupported.", )); } + let action = fix_ts_import_action(specifier, action, language_server)?; let edit = ts_changes_to_edit(&action.changes, language_server).await?; let code_action = lsp::CodeAction { title: action.description.clone(), diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index d86ccefd52..ebcb6b9e36 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -29,6 +29,7 @@ use std::sync::Arc; use tokio::fs; use super::analysis; +use super::analysis::fix_ts_import_changes; use super::analysis::ts_changes_to_edit; use super::analysis::CodeActionCollection; use super::analysis::CodeActionData; @@ -964,7 +965,7 @@ impl Inner { }; for action in actions { code_actions - .add_ts_fix_action(&action, diagnostic, self) + .add_ts_fix_action(&specifier, &action, diagnostic, self) .await .map_err(|err| { error!("Unable to convert fix: {}", err); @@ -1009,7 +1010,7 @@ impl Inner { LspError::invalid_params("The CodeAction's data is invalid.") })?; let req = tsc::RequestMethod::GetCombinedCodeFix(( - code_action_data.specifier, + code_action_data.specifier.clone(), json!(code_action_data.fix_id.clone()), )); let combined_code_actions: tsc::CombinedCodeActions = self @@ -1024,14 +1025,25 @@ impl Inner { error!("Deno does not support code actions with commands."); Err(LspError::invalid_request()) } else { + let changes = if code_action_data.fix_id == "fixMissingImport" { + fix_ts_import_changes( + &code_action_data.specifier, + &combined_code_actions.changes, + self, + ) + .map_err(|err| { + error!("Unable to remap changes: {}", err); + LspError::internal_error() + })? + } else { + combined_code_actions.changes.clone() + }; let mut code_action = params.clone(); code_action.edit = - ts_changes_to_edit(&combined_code_actions.changes, self) - .await - .map_err(|err| { - error!("Unable to convert changes to edits: {}", err); - LspError::internal_error() - })?; + ts_changes_to_edit(&changes, self).await.map_err(|err| { + error!("Unable to convert changes to edits: {}", err); + LspError::internal_error() + })?; Ok(code_action) } } else { diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index c345b705d0..df39a3ba31 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -925,8 +925,8 @@ impl DocumentHighlights { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] #[serde(rename_all = "camelCase")] pub struct TextChange { - span: TextSpan, - new_text: String, + pub span: TextSpan, + pub new_text: String, } impl TextChange { @@ -944,10 +944,10 @@ impl TextChange { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] #[serde(rename_all = "camelCase")] pub struct FileTextChanges { - file_name: String, - text_changes: Vec, + pub file_name: String, + pub text_changes: Vec, #[serde(skip_serializing_if = "Option::is_none")] - is_new_file: Option, + pub is_new_file: Option, } impl FileTextChanges { diff --git a/cli/tests/integration_tests_lsp.rs b/cli/tests/integration_tests_lsp.rs index 8e04cbb95d..744b8d3872 100644 --- a/cli/tests/integration_tests_lsp.rs +++ b/cli/tests/integration_tests_lsp.rs @@ -1425,6 +1425,57 @@ fn lsp_code_actions_deno_cache() { shutdown(&mut client); } +#[test] +fn lsp_code_actions_imports() { + let mut client = init("initialize_params.json"); + did_open( + &mut client, + json!({ + "textDocument": { + "uri": "file:///a/file00.ts", + "languageId": "typescript", + "version": 1, + "text": "export const abc = \"abc\";\nexport const def = \"def\";\n" + } + }), + ); + did_open( + &mut client, + json!({ + "textDocument": { + "uri": "file:///a/file01.ts", + "languageId": "typescript", + "version": 1, + "text": "\nconsole.log(abc);\nconsole.log(def)\n" + } + }), + ); + + let (maybe_res, maybe_err) = client + .write_request( + "textDocument/codeAction", + load_fixture("code_action_params_imports.json"), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert_eq!( + maybe_res, + Some(load_fixture("code_action_response_imports.json")) + ); + let (maybe_res, maybe_err) = client + .write_request( + "codeAction/resolve", + load_fixture("code_action_resolve_params_imports.json"), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert_eq!( + maybe_res, + Some(load_fixture("code_action_resolve_response_imports.json")) + ); + shutdown(&mut client); +} + #[test] fn lsp_code_actions_deadlock() { let mut client = init("initialize_params.json"); diff --git a/cli/tests/lsp/code_action_params_imports.json b/cli/tests/lsp/code_action_params_imports.json new file mode 100644 index 0000000000..7a5824923a --- /dev/null +++ b/cli/tests/lsp/code_action_params_imports.json @@ -0,0 +1,54 @@ +{ + "textDocument": { + "uri": "file:///a/file01.ts" + }, + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "context": { + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + }, + { + "range": { + "start": { + "line": 2, + "character": 12 + }, + "end": { + "line": 2, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'def'." + } + ], + "only": [ + "quickfix" + ] + } +} diff --git a/cli/tests/lsp/code_action_resolve_params_imports.json b/cli/tests/lsp/code_action_resolve_params_imports.json new file mode 100644 index 0000000000..60178bbfea --- /dev/null +++ b/cli/tests/lsp/code_action_resolve_params_imports.json @@ -0,0 +1,26 @@ +{ + "title": "Add all missing imports", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "data": { + "specifier": "file:///a/file01.ts", + "fixId": "fixMissingImport" + } +} diff --git a/cli/tests/lsp/code_action_resolve_response_imports.json b/cli/tests/lsp/code_action_resolve_response_imports.json new file mode 100644 index 0000000000..6621c501f0 --- /dev/null +++ b/cli/tests/lsp/code_action_resolve_response_imports.json @@ -0,0 +1,51 @@ +{ + "title": "Add all missing imports", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///a/file01.ts", + "version": 1 + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 0 + } + }, + "newText": "import { abc,def } from \"./file00.ts\";\n" + } + ] + } + ] + }, + "data": { + "specifier": "file:///a/file01.ts", + "fixId": "fixMissingImport" + } +} diff --git a/cli/tests/lsp/code_action_response_imports.json b/cli/tests/lsp/code_action_response_imports.json new file mode 100644 index 0000000000..e4d926bdd1 --- /dev/null +++ b/cli/tests/lsp/code_action_response_imports.json @@ -0,0 +1,242 @@ +[ + { + "title": "Import 'abc' from module \"./file00.ts\"", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///a/file01.ts", + "version": 1 + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 0 + } + }, + "newText": "import { abc } from \"./file00.ts\";\n" + } + ] + } + ] + } + }, + { + "title": "Add all missing imports", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "data": { + "specifier": "file:///a/file01.ts", + "fixId": "fixMissingImport" + } + }, + { + "title": "Add missing function declaration 'abc'", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///a/file01.ts", + "version": 1 + }, + "edits": [ + { + "range": { + "start": { + "line": 3, + "character": 0 + }, + "end": { + "line": 3, + "character": 0 + } + }, + "newText": "\nfunction abc(abc: any) {\nthrow new Error(\"Function not implemented.\");\n}\n" + } + ] + } + ] + } + }, + { + "title": "Import 'def' from module \"./file00.ts\"", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 2, + "character": 12 + }, + "end": { + "line": 2, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'def'." + } + ], + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///a/file01.ts", + "version": 1 + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 0 + } + }, + "newText": "import { def } from \"./file00.ts\";\n" + } + ] + } + ] + } + }, + { + "title": "Add missing function declaration 'def'", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 2, + "character": 12 + }, + "end": { + "line": 2, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'def'." + } + ], + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///a/file01.ts", + "version": 1 + }, + "edits": [ + { + "range": { + "start": { + "line": 3, + "character": 0 + }, + "end": { + "line": 3, + "character": 0 + } + }, + "newText": "\nfunction def(def: any) {\nthrow new Error(\"Function not implemented.\");\n}\n" + } + ] + } + ] + } + }, + { + "title": "Add all missing function declarations", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 1, + "character": 12 + }, + "end": { + "line": 1, + "character": 15 + } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'abc'." + } + ], + "data": { + "specifier": "file:///a/file01.ts", + "fixId": "fixMissingFunctionDeclaration" + } + } +]