From feb744cebd37263026893c7e7c4852daa5df24d0 Mon Sep 17 00:00:00 2001 From: Hajime-san <41257923+Hajime-san@users.noreply.github.com> Date: Thu, 28 Mar 2024 00:58:18 +0900 Subject: [PATCH] fix(lsp): decoding percent-encoding(non-ASCII) file path correctly (#22582) --- cli/lsp/diagnostics.rs | 51 ++++++++++++++++++++++++++++++++-- cli/lsp/language_server.rs | 14 +++++++--- cli/lsp/tsc.rs | 45 ++++++++++++++++++++++++++++-- cli/util/path.rs | 29 +++++++++++++++++-- tests/integration/lsp_tests.rs | 42 ++++++++++++++++++++++------ 5 files changed, 162 insertions(+), 19 deletions(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index e3b922ba82..03d9627416 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -21,6 +21,7 @@ use crate::graph_util::enhanced_resolution_error_message; use crate::lsp::lsp_custom::DiagnosticBatchNotificationParams; use crate::resolver::SloppyImportsResolution; use crate::resolver::SloppyImportsResolver; +use crate::util::path::to_percent_decoded_str; use deno_ast::MediaType; use deno_core::anyhow::anyhow; @@ -1212,8 +1213,10 @@ impl DenoDiagnostic { specifier: &ModuleSpecifier, sloppy_resolution: SloppyImportsResolution, ) -> String { - let mut message = - format!("Unable to load a local module: {}\n", specifier); + let mut message = format!( + "Unable to load a local module: {}\n", + to_percent_decoded_str(specifier.as_ref()) + ); if let Some(additional_message) = sloppy_resolution.as_suggestion_message() { @@ -1971,6 +1974,50 @@ let c: number = "a"; ); } + #[tokio::test] + async fn unable_to_load_a_local_module() { + let temp_dir = TempDir::new(); + let (snapshot, _) = setup( + &temp_dir, + &[( + "file:///a.ts", + r#" + import { 東京 } from "./πŸ¦•.ts"; + "#, + 1, + LanguageId::TypeScript, + )], + None, + ) + .await; + let config = mock_config(); + let token = CancellationToken::new(); + let actual = generate_deno_diagnostics(&snapshot, &config, token); + assert_eq!(actual.len(), 1); + let record = actual.first().unwrap(); + assert_eq!( + json!(record.versioned.diagnostics), + json!([ + { + "range": { + "start": { + "line": 1, + "character": 27 + }, + "end": { + "line": 1, + "character": 35 + } + }, + "severity": 1, + "code": "no-local", + "source": "deno", + "message": "Unable to load a local module: file:///πŸ¦•.ts\nPlease check the file path.", + } + ]) + ); + } + #[test] fn test_specifier_text_for_redirected() { #[track_caller] diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index fafd9fe4ca..e7e48a04c2 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -122,6 +122,7 @@ use crate::tools::upgrade::upgrade_check_enabled; use crate::util::fs::remove_dir_all_if_exists; use crate::util::path::is_importable_ext; use crate::util::path::specifier_to_file_path; +use crate::util::path::to_percent_decoded_str; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; @@ -1738,16 +1739,21 @@ impl Inner { match resolution { Resolution::Ok(resolved) => { let specifier = &resolved.specifier; + let format = |scheme: &str, rest: &str| -> String { + format!("{}​{}", scheme, rest).replace('@', "​@") + }; match specifier.scheme() { "data" => "_(a data url)_".to_string(), "blob" => "_(a blob url)_".to_string(), + "file" => format( + &specifier[..url::Position::AfterScheme], + &to_percent_decoded_str(&specifier[url::Position::AfterScheme..]), + ), _ => { - let mut result = format!( - "{}​{}", + let mut result = format( &specifier[..url::Position::AfterScheme], &specifier[url::Position::AfterScheme..], - ) - .replace('@', "​@"); + ); if let Ok(jsr_req_ref) = JsrPackageReqReference::from_specifier(specifier) { diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 4eb425c1f9..691f7c91dd 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -31,6 +31,7 @@ use crate::tsc; use crate::tsc::ResolveArgs; use crate::util::path::relative_specifier; use crate::util::path::specifier_to_file_path; +use crate::util::path::to_percent_decoded_str; use dashmap::DashMap; use deno_ast::MediaType; @@ -543,6 +544,10 @@ impl TsServer { .and_then(|mut changes| { for changes in &mut changes { changes.normalize(&self.specifier_map)?; + for text_changes in &mut changes.text_changes { + text_changes.new_text = + to_percent_decoded_str(&text_changes.new_text); + } } Ok(changes) }) @@ -1611,7 +1616,41 @@ fn display_parts_to_string( link.name = Some(part.text.clone()); } } - _ => out.push(part.text.clone()), + _ => out.push( + // should decode percent-encoding string when hovering over the right edge of module specifier like below + // module "file:///path/to/πŸ¦•" + to_percent_decoded_str(&part.text), + // NOTE: The reason why an example above that lacks `.ts` extension is caused by the implementation of tsc itself. + // The request `tsc.request.getQuickInfoAtPosition` receives the payload from tsc host as follows. + // { + // text_span: { + // start: 19, + // length: 9, + // }, + // displayParts: + // [ + // { + // text: "module", + // kind: "keyword", + // target: null, + // }, + // { + // text: " ", + // kind: "space", + // target: null, + // }, + // { + // text: "\"file:///path/to/%F0%9F%A6%95\"", + // kind: "stringLiteral", + // target: null, + // }, + // ], + // documentation: [], + // tags: null, + // } + // + // related issue: https://github.com/denoland/deno/issues/16058 + ), } } @@ -5424,7 +5463,7 @@ mod tests { .get_edits_for_file_rename( snapshot, resolve_url("file:///b.ts").unwrap(), - resolve_url("file:///c.ts").unwrap(), + resolve_url("file:///πŸ¦•.ts").unwrap(), FormatCodeSettings::default(), UserPreferences::default(), ) @@ -5439,7 +5478,7 @@ mod tests { start: 8, length: 6, }, - new_text: "./c.ts".to_string(), + new_text: "./πŸ¦•.ts".to_string(), }], is_new_file: None, }] diff --git a/cli/util/path.rs b/cli/util/path.rs index b64fde6b9d..144676c015 100644 --- a/cli/util/path.rs +++ b/cli/util/path.rs @@ -156,11 +156,12 @@ pub fn relative_specifier( text.push('/'); } - Some(if text.starts_with("../") || text.starts_with("./") { + let text = if text.starts_with("../") || text.starts_with("./") { text } else { format!("./{text}") - }) + }; + Some(to_percent_decoded_str(&text)) } /// Gets a path with the specified file stem suffix. @@ -265,6 +266,24 @@ pub fn matches_pattern_or_exact_path( false } +/// For decoding percent-encodeing string +/// could be used for module specifier string literal of local modules, +/// or local file path to display `non-ASCII` characters correctly +/// # Examples +/// ``` +/// use crate::util::path::to_percent_decoded_str; +/// +/// let str = to_percent_decoded_str("file:///Users/path/to/%F0%9F%A6%95.ts"); +/// assert_eq!(str, "file:///Users/path/to/πŸ¦•.ts"); +/// ``` +pub fn to_percent_decoded_str(s: &str) -> String { + match percent_encoding::percent_decode_str(s).decode_utf8() { + Ok(s) => s.to_string(), + // when failed to decode, return the original string + Err(_) => s.to_string(), + } +} + #[cfg(test)] mod test { use super::*; @@ -457,4 +476,10 @@ mod test { PathBuf::from("/test_2.d.cts") ); } + + #[test] + fn test_to_percent_decoded_str() { + let str = to_percent_decoded_str("%F0%9F%A6%95"); + assert_eq!(str, "πŸ¦•"); + } } diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 852f56e9b3..5a81244fd4 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -2266,7 +2266,7 @@ fn lsp_hover_dependency() { "uri": "file:///a/file.ts", "languageId": "typescript", "version": 1, - "text": "import * as a from \"http://127.0.0.1:4545/xTypeScriptTypes.js\";\n// @deno-types=\"http://127.0.0.1:4545/type_definitions/foo.d.ts\"\nimport * as b from \"http://127.0.0.1:4545/type_definitions/foo.js\";\nimport * as c from \"http://127.0.0.1:4545/subdir/type_reference.js\";\nimport * as d from \"http://127.0.0.1:4545/subdir/mod1.ts\";\nimport * as e from \"data:application/typescript;base64,ZXhwb3J0IGNvbnN0IGEgPSAiYSI7CgpleHBvcnQgZW51bSBBIHsKICBBLAogIEIsCiAgQywKfQo=\";\nimport * as f from \"./file_01.ts\";\nimport * as g from \"http://localhost:4545/x/a/mod.ts\";\n\nconsole.log(a, b, c, d, e, f, g);\n" + "text": "import * as a from \"http://127.0.0.1:4545/xTypeScriptTypes.js\";\n// @deno-types=\"http://127.0.0.1:4545/type_definitions/foo.d.ts\"\nimport * as b from \"http://127.0.0.1:4545/type_definitions/foo.js\";\nimport * as c from \"http://127.0.0.1:4545/subdir/type_reference.js\";\nimport * as d from \"http://127.0.0.1:4545/subdir/mod1.ts\";\nimport * as e from \"data:application/typescript;base64,ZXhwb3J0IGNvbnN0IGEgPSAiYSI7CgpleHBvcnQgZW51bSBBIHsKICBBLAogIEIsCiAgQywKfQo=\";\nimport * as f from \"./file_01.ts\";\nimport * as g from \"http://localhost:4545/x/a/mod.ts\";\nimport * as h from \"./modπŸ¦•.ts\";\n\nconsole.log(a, b, c, d, e, f, g, h);\n" } }), ); @@ -2387,6 +2387,28 @@ fn lsp_hover_dependency() { } }) ); + let res = client.write_request( + "textDocument/hover", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + }, + "position": { "line": 8, "character": 28 } + }), + ); + assert_eq!( + res, + json!({ + "contents": { + "kind": "markdown", + "value": "**Resolved Dependency**\n\n**Code**: file​:///a/modπŸ¦•.ts\n" + }, + "range": { + "start": { "line": 8, "character": 19 }, + "end":{ "line": 8, "character": 30 } + } + }) + ); } // This tests for a regression covered by denoland/deno#12753 where the lsp was @@ -6637,7 +6659,7 @@ fn lsp_completions_auto_import() { client.initialize_default(); client.did_open(json!({ "textDocument": { - "uri": "file:///a/b.ts", + "uri": "file:///a/πŸ¦•.ts", "languageId": "typescript", "version": 1, "text": "export const foo = \"foo\";\n", @@ -6668,7 +6690,7 @@ fn lsp_completions_auto_import() { let req = json!({ "label": "foo", "labelDetails": { - "description": "./b.ts", + "description": "./πŸ¦•.ts", }, "kind": 6, "sortText": "οΏΏ16_0", @@ -6683,12 +6705,16 @@ fn lsp_completions_auto_import() { "specifier": "file:///a/file.ts", "position": 12, "name": "foo", - "source": "./b.ts", + "source": "./%F0%9F%A6%95.ts", + "specifierRewrite": [ + "./%F0%9F%A6%95.ts", + "./πŸ¦•.ts", + ], "data": { "exportName": "foo", "exportMapKey": "", - "moduleSpecifier": "./b.ts", - "fileName": "file:///a/b.ts" + "moduleSpecifier": "./%F0%9F%A6%95.ts", + "fileName": "file:///a/%F0%9F%A6%95.ts" }, "useCodeSnippet": false } @@ -6702,7 +6728,7 @@ fn lsp_completions_auto_import() { json!({ "label": "foo", "labelDetails": { - "description": "./b.ts", + "description": "./πŸ¦•.ts", }, "kind": 6, "detail": "const foo: \"foo\"", @@ -6717,7 +6743,7 @@ fn lsp_completions_auto_import() { "start": { "line": 0, "character": 0 }, "end": { "line": 0, "character": 0 } }, - "newText": "import { foo } from \"./b.ts\";\n\n" + "newText": "import { foo } from \"./πŸ¦•.ts\";\n\n" } ] })