Repository: denoland/deno
Pull request: fix(lsp): auto-import from npm package exports with manual node_modules

Fixes #28412.

Changed files:
filename: cli/lsp/resolver.rs
patch: @@ -43,6 +43,7 @@ use node_resolver::cache::NodeResolutionThreadLocalCache;
 use node_resolver::DenoIsBuiltInNodeModuleChecker;
 use node_resolver::NodeResolutionKind;
 use node_resolver::NodeResolverOptions;
+use node_resolver::PackageJson;
 use node_resolver::PackageJsonThreadLocalCache;
 use node_resolver::ResolutionMode;
 
@@ -102,7 +103,7 @@ struct LspScopeResolver {
   redirect_resolver: Option<Arc<RedirectResolver>>,
   graph_imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>,
   dep_info: Arc<Mutex<Arc<ScopeDepInfo>>>,
-  package_json_deps_by_resolution: Arc<IndexMap<ModuleSpecifier, String>>,
+  package_json_dep_resolutions: Arc<PackageJsonDepResolutions>,
   config_data: Option<Arc<ConfigData>>,
 }
 
@@ -125,7 +126,7 @@ impl Default for LspScopeResolver {
       redirect_resolver: None,
       graph_imports: Default::default(),
       dep_info: Default::default(),
-      package_json_deps_by_resolution: Default::default(),
+      package_json_dep_resolutions: Default::default(),
       config_data: None,
     }
   }
@@ -186,48 +187,16 @@ impl LspScopeResolver {
         )
       })
       .unwrap_or_default();
-    let package_json_deps_by_resolution = (|| {
+    let package_json_dep_resolutions = (|| {
       let npm_pkg_req_resolver = npm_pkg_req_resolver.as_ref()?;
       let package_json = config_data?.maybe_pkg_json()?;
-      let referrer = package_json.specifier();
-      let dependencies = package_json.dependencies.as_ref()?;
-      let result = dependencies
-        .iter()
-        .flat_map(|(name, _)| {
-          let mut deps = Vec::with_capacity(2);
-          let Some(req_ref) =
-            NpmPackageReqReference::from_str(&format!("npm:{name}")).ok()
-          else {
-            return vec![];
-          };
-          for kind in [NodeResolutionKind::Types, NodeResolutionKind::Execution]
-          {
-            let Some(req) = npm_pkg_req_resolver
-              .resolve_req_reference(
-                &req_ref,
-                &referrer,
-                // todo(dsherret): this is wrong because it doesn't consider CJS referrers
-                ResolutionMode::Import,
-                kind,
-              )
-              .ok()
-            else {
-              continue;
-            };
-
-            let Some(url) = req.into_url().ok() else {
-              continue;
-            };
-            let specifier = into_specifier_and_media_type(Some(url)).0;
-            deps.push((specifier, name.clone()))
-          }
-          deps
-        })
-        .collect();
-      Some(result)
-    })();
-    let package_json_deps_by_resolution =
-      Arc::new(package_json_deps_by_resolution.unwrap_or_default());
+      Some(Arc::new(PackageJsonDepResolutions::new(
+        package_json,
+        npm_pkg_req_resolver,
+        &pkg_json_resolver,
+      )))
+    })()
+    .unwrap_or_default();
     Self {
       resolver: cli_resolver,
       in_npm_pkg_checker,
@@ -244,7 +213,7 @@ impl LspScopeResolver {
       redirect_resolver,
       graph_imports,
       dep_info: Default::default(),
-      package_json_deps_by_resolution,
+      package_json_dep_resolutions,
       config_data: config_data.cloned(),
     }
   }
@@ -320,9 +289,7 @@ impl LspScopeResolver {
       pkg_json_resolver: factory.pkg_json_resolver().clone(),
       graph_imports: self.graph_imports.clone(),
       dep_info: self.dep_info.clone(),
-      package_json_deps_by_resolution: self
-        .package_json_deps_by_resolution
-        .clone(),
+      package_json_dep_resolutions: self.package_json_dep_resolutions.clone(),
       config_data: self.config_data.clone(),
     })
   }
@@ -559,9 +526,14 @@ impl LspResolver {
   ) -> Option<String> {
     let resolver = self.get_scope_resolver(file_referrer);
     resolver
-      .package_json_deps_by_resolution
-      .get(specifier)
-      .cloned()
+      .package_json_dep_resolutions
+      .package_ref_from_resolution(specifier)
+      .or_else(|| {
+        resolver
+          .node_resolver
+          .as_ref()?
+          .lookup_package_specifier_for_resolution(specifier)
+      })
   }
 
   pub fn deno_types_to_code_resolution(
@@ -655,6 +627,105 @@ pub struct ScopeDepInfo {
   pub has_node_specifier: bool,
 }
 
+#[derive(Debug, Default)]
+struct PackageJsonDepResolutions {
+  deps_by_resolution: IndexMap<ModuleSpecifier, String>,
+}
+
+impl PackageJsonDepResolutions {
+  fn new(
+    package_json: &PackageJson,
+    npm_pkg_req_resolver: &CliNpmReqResolver,
+    pkg_json_resolver: &CliPackageJsonResolver,
+  ) -> Self {
+    let referrer = package_json.specifier();
+    let Some(dependencies) = package_json.dependencies.as_ref() else {
+      return Self::default();
+    };
+    let mut result = Self::default();
+    for name in dependencies.keys() {
+      let Some(req_ref) =
+        NpmPackageReqReference::from_str(&format!("npm:{name}")).ok()
+      else {
+        continue;
+      };
+      let mut dep_package_json = None;
+      for kind in [NodeResolutionKind::Types, NodeResolutionKind::Execution] {
+        let Some(req) = npm_pkg_req_resolver
+          .resolve_req_reference(
+            &req_ref,
+            &referrer,
+            // todo(dsherret): this is wrong because it doesn't consider CJS referrers
+            ResolutionMode::Import,
+            kind,
+          )
+          .ok()
+        else {
+          continue;
+        };
+        let Some(url) = req.into_url().ok() else {
+          continue;
+        };
+        let specifier = into_specifier_and_media_type(Some(url)).0;
+        if dep_package_json.is_none() {
+          dep_package_json = (|| {
+            let path = url_to_file_path(&specifier).ok()?;
+            pkg_json_resolver.get_closest_package_json(&path).ok()?
+          })();
+        }
+        result.deps_by_resolution.insert(specifier, name.clone());
+      }
+      let export_keys = dep_package_json
+        .as_ref()
+        .and_then(|p| p.exports.as_ref())
+        .into_iter()
+        .flat_map(|e| e.keys());
+      for export_key in export_keys {
+        let Some(export_name) = export_key.strip_prefix("./") else {
+          continue;
+        };
+        // Wildcards are not supported here.
+        if export_name.chars().filter(|c| *c == '*').count() == 1 {
+          continue;
+        }
+        let package_ref = format!("{name}/{export_name}");
+        let Some(req_ref) =
+          NpmPackageReqReference::from_str(&format!("npm:{}", &package_ref))
+            .ok()
+        else {
+          continue;
+        };
+        for kind in [NodeResolutionKind::Types, NodeResolutionKind::Execution] {
+          let Some(req) = npm_pkg_req_resolver
+            .resolve_req_reference(
+              &req_ref,
+              &referrer,
+              // todo(dsherret): this is wrong because it doesn't consider CJS referrers
+              ResolutionMode::Import,
+              kind,
+            )
+            .ok()
+          else {
+            continue;
+          };
+          let Some(url) = req.into_url().ok() else {
+            continue;
+          };
+          let specifier = into_specifier_and_media_type(Some(url)).0;
+          result
+            .deps_by_resolution
+            .insert(specifier, package_ref.clone());
+        }
+      }
+    }
+    result
+  }
+
+  fn package_ref_from_resolution(&self, resolution: &Url) -> Option<String> {
+    self.deps_by_resolution.get(resolution).cloned()
+  }
+}
+
 #[derive(Default)]
 struct ResolverFactoryServices {
   cli_resolver: Deferred<Arc<CliResolver>>,
@@ -925,22 +996,25 @@ impl<'a> ResolverFactory<'a> {
       .node_resolver
       .get_or_init(|| {
         let npm_resolver = self.services.npm_resolver.as_ref()?;
-        Some(Arc::new(CliNodeResolver::new(
-          self.in_npm_pkg_checker().clone(),
-          DenoIsBuiltInNodeModuleChecker,
-          npm_resolver.clone(),
-          self.pkg_json_resolver.clone(),
-          self.node_resolution_sys.clone(),
-          NodeResolverOptions {
-            conditions_from_resolution_mode: Default::default(),
-            typescript_version: Some(
-              deno_semver::Version::parse_standard(
-                deno_lib::version::DENO_VERSION_INFO.typescript,
-              )
-              .unwrap(),
-            ),
-          },
-        )))
+        Some(Arc::new(
+          CliNodeResolver::new(
+            self.in_npm_pkg_checker().clone(),
+            DenoIsBuiltInNodeModuleChecker,
+            npm_resolver.clone(),
+            self.pkg_json_resolver.clone(),
+            self.node_resolution_sys.clone(),
+            NodeResolverOptions {
+              conditions_from_resolution_mode: Default::default(),
+              typescript_version: Some(
+                deno_semver::Version::parse_standard(
+                  deno_lib::version::DENO_VERSION_INFO.typescript,
+                )
+                .unwrap(),
+              ),
+            },
+          )
+          .with_package_resolution_lookup_cache(),
+        ))
       })
       .as_ref()
   }

filename: resolvers/node/resolution.rs
patch: @@ -7,6 +7,7 @@ use std::path::PathBuf;
 
 use anyhow::bail;
 use anyhow::Error as AnyError;
+use dashmap::DashMap;
 use deno_media_type::MediaType;
 use deno_package_json::PackageJson;
 use deno_path_util::url_to_file_path;
@@ -207,6 +208,7 @@ pub struct NodeResolver<
   sys: NodeResolutionSys<TSys>,
   conditions_from_resolution_mode: ConditionsFromResolutionMode,
   typescript_version: Option<Version>,
+  package_resolution_lookup_cache: Option<DashMap<Url, String>>,
 }
 
 impl<
@@ -238,6 +240,14 @@ impl<
       sys,
       conditions_from_resolution_mode: options.conditions_from_resolution_mode,
       typescript_version: options.typescript_version,
+      package_resolution_lookup_cache: None,
+    }
+  }
+
+  pub fn with_package_resolution_lookup_cache(self) -> Self {
+    Self {
+      package_resolution_lookup_cache: Some(Default::default()),
+      ..self
     }
   }
 
@@ -264,7 +274,9 @@ impl<
       return Ok(NodeResolution::BuiltIn(specifier.to_string()));
     }
 
+    let mut specifier_is_url = false;
     if let Ok(url) = Url::parse(specifier) {
+      specifier_is_url = true;
       if url.scheme() == "data" {
         return Ok(NodeResolution::Module(UrlOrPath::Url(url)));
       }
@@ -309,6 +321,21 @@ impl<
 
     let url_or_path =
       self.finalize_resolution(url, resolved_kind, Some(&referrer))?;
+    let maybe_cache_resolution = || {
+      let package_resolution_lookup_cache =
+        self.package_resolution_lookup_cache.as_ref()?;
+      if specifier_is_url
+        || specifier.starts_with("./")
+        || specifier.starts_with("../")
+        || specifier.starts_with("/")
+      {
+        return None;
+      }
+      let url = url_or_path.clone().into_url().ok()?;
+      package_resolution_lookup_cache.insert(url, specifier.to_string());
+      Some(())
+    };
+    maybe_cache_resolution();
     let resolve_response = NodeResolution::Module(url_or_path);
     // TODO(bartlomieju): skipped checking errors for commonJS resolution and
     // "preserveSymlinksMain"/"preserveSymlinks" options.
@@ -473,6 +500,17 @@ impl<
     }
   }
 
+  pub fn lookup_package_specifier_for_resolution(
+    &self,
+    url: &Url,
+  ) -> Option<String> {
+    self
+      .package_resolution_lookup_cache
+      .as_ref()?
+      .get(url)
+      .map(|r| r.value().clone())
+  }
+
   fn module_not_found_ext_suggestion(
     &self,
     path: &Path,

filename: tests/integration/lsp_tests.rs
patch: @@ -9883,7 +9883,7 @@ fn lsp_completions_npm() {
 
 #[test]
 #[timeout(300_000)]
-fn lsp_auto_imports_npm_auto() {
+fn lsp_auto_import_npm_export_node_modules_dir_auto() {
   let context = TestContextBuilder::for_npm().use_temp_cwd().build();
   let temp_dir_path = context.temp_dir().path();
   temp_dir_path.join("deno.json").write_json(&json!({
@@ -9945,6 +9945,79 @@ fn lsp_auto_imports_npm_auto() {
   client.shutdown();
 }
 
+#[test]
+#[timeout(300_000)]
+fn lsp_auto_import_npm_export_node_modules_dir_manual() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write(
+    "deno.json",
+    json!({
+      "nodeModulesDir": "manual",
+    })
+    .to_string(),
+  );
+  temp_dir.write(
+    "package.json",
+    json!({
+      "dependencies": {
+        "preact": "*",
+      },
+    })
+    .to_string(),
+  );
+  temp_dir.write(
+    "mod.ts",
+    "import { useEffect } from 'preact/hooks'; console.log(useEffect);",
+  );
+  context.run_npm("install");
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  let diagnostics = client
+    .did_open(json!({
+      "textDocument": {
+        "uri": temp_dir.url().join("file.ts").unwrap(),
+        "languageId": "typescript",
+        "version": 1,
+        "text": "useEffect",
+      }
+    }))
+    .all();
+  assert_eq!(diagnostics.len(), 1);
+  let diagnostic = diagnostics.first().unwrap();
+  let res = client.write_request(
+    "textDocument/codeAction",
+    json!({
+      "textDocument": {
+        "uri": temp_dir.url().join("file.ts").unwrap(),
+      },
+      "range": {
+        "start": { "line": 0, "character": 0 },
+        "end": { "line": 0, "character": 9 },
+      },
+      "context": {
+        "diagnostics": [diagnostic],
+        "only": ["quickfix"],
+      },
+    }),
+  );
+  assert_eq!(
+    res
+      .as_array()
+      .unwrap()
+      .first()
+      .unwrap()
+      .as_object()
+      .unwrap()
+      .get("title")
+      .unwrap()
+      .as_str()
+      .unwrap(),
+    "Add import from \"preact/hooks\"",
+  );
+  client.shutdown();
+}
+
 // Regression test for https://github.com/denoland/deno/issues/23869.
 #[test]
 #[timeout(300_000)]