From fbfeedb68b593d0dbf3f0bfb0061939756da20b7 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 21 Jan 2024 17:19:10 -0500 Subject: [PATCH] fix(lsp): improved npm specifier to import map entry mapping (#22016) Upgrades to the latest deno_semver --- Cargo.lock | 30 ++++++----- Cargo.toml | 2 +- cli/Cargo.toml | 8 ++- cli/lsp/analysis.rs | 49 ++++++++++++++---- cli/tests/integration/lsp_tests.rs | 51 +++++++++++++++++++ .../1.0.0/dist/entry-a.js | 2 +- .../1.0.0/dist/entry-b.d.ts | 1 + .../1.0.0/dist/entry-b.js | 3 ++ .../types-exports-subpaths/1.0.0/package.json | 3 ++ 9 files changed, 120 insertions(+), 29 deletions(-) create mode 100644 cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.d.ts create mode 100644 cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.js diff --git a/Cargo.lock b/Cargo.lock index 528d242709..a2ad3f969d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -992,7 +992,7 @@ dependencies = [ "libz-sys", "log", "lsp-types", - "monch", + "monch 0.5.0", "napi_sym", "nix 0.26.2", "notify", @@ -1342,7 +1342,7 @@ dependencies = [ "import_map", "indexmap", "log", - "monch", + "monch 0.4.3", "once_cell", "parking_lot 0.12.1", "regex", @@ -1593,7 +1593,7 @@ dependencies = [ "deno_semver", "futures", "log", - "monch", + "monch 0.4.3", "serde", "thiserror", ] @@ -1678,11 +1678,11 @@ dependencies = [ [[package]] name = "deno_semver" -version = "0.5.1" +version = "0.5.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d2d3f7f5a3b2ace62b8fdede8585f5fdbd4e7dba9cb33fcaf0db54887316feaa" +checksum = "b49e14effd9df8ed261f7a1a34ac19bbaf0fa940c59bd19a6d8313cf41525e1c" dependencies = [ - "monch", + "monch 0.5.0", "once_cell", "serde", "thiserror", @@ -1691,14 +1691,14 @@ dependencies = [ [[package]] name = "deno_task_shell" -version = "0.14.0" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a94a6fd5d889087748f4794887f28756a01b718dae92a316db0951222231325a" +checksum = "fc8ddb4362a79ef3d264df363eef77c25c976964132bf672b682c5816ebdcfd1" dependencies = [ "anyhow", "futures", "glob", - "monch", + "monch 0.5.0", "os_pipe", "path-dedot", "tokio", @@ -3170,9 +3170,9 @@ checksum = "cb56e1aa765b4b4f3aadfab769793b7087bb03a4ea4920644a6d238e2df5b9ed" [[package]] name = "import_map" -version = "0.18.1" +version = "0.18.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1624c269d2ca7427d79471c8ba799abe9215e706cc0182d7b86fc856a35d565b" +checksum = "556779a5de0289679854a5344c22f466698fd2661f7170db90df7fa5013b281e" dependencies = [ "indexmap", "log", @@ -3733,6 +3733,12 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4519a88847ba2d5ead3dc53f1060ec6a571de93f325d9c5c4968147382b1cbc3" +[[package]] +name = "monch" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b52c1b33ff98142aecea13138bd399b68aa7ab5d9546c300988c345004001eea" + [[package]] name = "multimap" version = "0.8.3" @@ -6089,7 +6095,7 @@ dependencies = [ "lazy-regex", "libc", "lsp-types", - "monch", + "monch 0.5.0", "nix 0.26.2", "once_cell", "os_pipe", diff --git a/Cargo.toml b/Cargo.toml index 33a8904e13..c6bb7c52ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -117,7 +117,7 @@ libz-sys = { version = "1.1", default-features = false } log = "=0.4.20" lsp-types = "=0.94.1" # used by tower-lsp and "proposed" feature is unstable in patch releases memmem = "0.1.1" -monch = "=0.4.3" +monch = "=0.5.0" notify = "=5.0.0" num-bigint = { version = "0.4", features = ["rand"] } once_cell = "1.17.1" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ae0e0cde9b..07bdc69c82 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -64,10 +64,8 @@ deno_lint = { version = "=0.53.0", features = ["docs"] } deno_lockfile.workspace = true deno_npm = "=0.15.3" deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] } -# todo(dsherret): investigate https://github.com/denoland/deno_semver/commit/98f9174baef199809295077b3b68c9fa58defb9b causing -# lsp_completions_auto_import_and_quick_fix_with_import_map to fail when bumping this version -deno_semver = "=0.5.1" -deno_task_shell = "=0.14.0" +deno_semver = "=0.5.4" +deno_task_shell = "=0.14.3" eszip = "=0.57.0" napi_sym.workspace = true @@ -99,7 +97,7 @@ flate2.workspace = true fs3.workspace = true glob = "0.3.1" hex.workspace = true -import_map = { version = "=0.18.1", features = ["ext"] } +import_map = { version = "=0.18.2", features = ["ext"] } indexmap.workspace = true jsonc-parser = { version = "=0.23.0", features = ["serde"] } lazy-regex.workspace = true diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 5301fbeea0..6c6d7cab4a 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -25,12 +25,14 @@ use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::NpmResolver; use deno_runtime::deno_node::PathClean; use deno_runtime::permissions::PermissionsContainer; +use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use import_map::ImportMap; use once_cell::sync::Lazy; use regex::Regex; use std::cmp::Ordering; use std::collections::HashMap; +use std::collections::HashSet; use std::path::Path; use tower_lsp::lsp_types as lsp; use tower_lsp::lsp_types::Position; @@ -211,20 +213,31 @@ impl<'a> TsResponseImportMapper<'a> { if !pkg_reqs.is_empty() { let sub_path = self.resolve_package_path(specifier); if let Some(import_map) = self.maybe_import_map { - for pkg_req in &pkg_reqs { - let paths = vec![ - concat_npm_specifier("npm:", pkg_req, sub_path.as_deref()), - concat_npm_specifier("npm:/", pkg_req, sub_path.as_deref()), - ]; - for path in paths { - if let Some(mapped_path) = ModuleSpecifier::parse(&path) - .ok() - .and_then(|s| import_map.lookup(&s, referrer)) + let pkg_reqs = pkg_reqs.iter().collect::>(); + let mut matches = Vec::new(); + for entry in import_map.entries_for_referrer(referrer) { + if let Some(value) = entry.raw_value { + if let Ok(package_ref) = + NpmPackageReqReference::from_str(value) { - return Some(mapped_path); + if pkg_reqs.contains(package_ref.req()) { + let sub_path = sub_path.as_deref().unwrap_or(""); + let value_sub_path = package_ref.sub_path().unwrap_or(""); + if let Some(key_sub_path) = + sub_path.strip_prefix(value_sub_path) + { + matches + .push(format!("{}{}", entry.raw_key, key_sub_path)); + } + } } } } + // select the shortest match + matches.sort_by_key(|a| a.len()); + if let Some(matched) = matches.first() { + return Some(matched.to_string()); + } } // if not found in the import map, return the first pkg req @@ -267,6 +280,22 @@ impl<'a> TsResponseImportMapper<'a> { // a search for the .d.ts file instead if specifier_path.extension().and_then(|e| e.to_str()) == Some("js") { search_paths.insert(0, specifier_path.with_extension("d.ts")); + } else if let Some(file_name) = + specifier_path.file_name().and_then(|f| f.to_str()) + { + // In some other cases, typescript will provide the .d.ts extension, but the + // export might not have a .d.ts defined. In that case, look for the corresponding + // JavaScript file after not being able to find the .d.ts file. + if let Some(file_stem) = file_name.strip_suffix(".d.ts") { + search_paths + .push(specifier_path.with_file_name(format!("{}.js", file_stem))); + } else if let Some(file_stem) = file_name.strip_suffix(".d.cts") { + search_paths + .push(specifier_path.with_file_name(format!("{}.cjs", file_stem))); + } else if let Some(file_stem) = file_name.strip_suffix(".d.mts") { + search_paths + .push(specifier_path.with_file_name(format!("{}.mjs", file_stem))); + } } for search_path in search_paths { diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index d60d14beab..2fc3d29e82 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -6450,6 +6450,7 @@ fn lsp_completions_auto_import_and_quick_fix_with_import_map() { "imports": { "print_hello": "http://localhost:4545/subdir/print_hello.ts", "chalk": "npm:chalk@~5", + "nested/": "npm:/@denotest/types-exports-subpaths@1/nested/", "types-exports-subpaths/": "npm:/@denotest/types-exports-subpaths@1/" } }"#; @@ -6470,6 +6471,7 @@ fn lsp_completions_auto_import_and_quick_fix_with_import_map() { "import _test1 from 'npm:chalk@^5.0';\n", "import chalk from 'npm:chalk@~5';\n", "import chalk from 'npm:chalk@~5';\n", + "import {entryB} from 'npm:@denotest/types-exports-subpaths@1/nested/entry-b';\n", "import {printHello} from 'print_hello';\n", "\n", ), @@ -6483,6 +6485,7 @@ fn lsp_completions_auto_import_and_quick_fix_with_import_map() { "arguments": [ [ "npm:@denotest/types-exports-subpaths@1/client", + "npm:@denotest/types-exports-subpaths@1/nested/entry-b", "npm:chalk@^5.0", "npm:chalk@~5", "http://localhost:4545/subdir/print_hello.ts", @@ -6822,6 +6825,54 @@ fn lsp_completions_auto_import_and_quick_fix_with_import_map() { } }]) ); + + // try auto-import with npm package with sub-path on value side of import map + client.did_open(json!({ + "textDocument": { + "uri": "file:///a/nested_path.ts", + "languageId": "typescript", + "version": 1, + "text": "entry", + } + })); + let list = client.get_completion_list( + "file:///a/nested_path.ts", + (0, 5), + json!({ "triggerKind": 1 }), + ); + assert!(!list.is_incomplete); + let item = list + .items + .iter() + .find(|item| item.label == "entryB") + .unwrap(); + + let res = client.write_request("completionItem/resolve", item); + assert_eq!( + res, + json!({ + "label": "entryB", + "labelDetails": { + "description": "nested/entry-b", + }, + "kind": 3, + "detail": "function entryB(): \"b\"", + "documentation": { + "kind": "markdown", + "value": "" + }, + "sortText": "￿16_0", + "additionalTextEdits": [ + { + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "newText": "import { entryB } from \"nested/entry-b\";\n\n" + } + ] + }) + ); } #[test] diff --git a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-a.js b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-a.js index 070b1ccbd6..84f1f2c941 100644 --- a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-a.js +++ b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-a.js @@ -1,3 +1,3 @@ -export function entryC() { +export function entryA() { return 12; } diff --git a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.d.ts b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.d.ts new file mode 100644 index 0000000000..382d1995ee --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.d.ts @@ -0,0 +1 @@ +export function entryB(): "b"; diff --git a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.js b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.js new file mode 100644 index 0000000000..162d4f1908 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/dist/entry-b.js @@ -0,0 +1,3 @@ +export function entryB() { + return "b"; +} diff --git a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/package.json index 1690175609..cc43cf2ed7 100644 --- a/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/package.json +++ b/cli/tests/testdata/npm/registry/@denotest/types-exports-subpaths/1.0.0/package.json @@ -18,6 +18,9 @@ }, "./entry-a": { "import": "./dist/entry-a.js" + }, + "./nested/entry-b": { + "import": "./dist/entry-b.js" } } }