feat(lsp): auto-import completions from byonm dependencies (#26680)

5 files changed, 179 insertions, 85 deletions

diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs
index 0769c9047..98215855c 100644
--- a/cli/lsp/analysis.rs
+++ b/cli/lsp/analysis.rs
@@ -12,7 +12,9 @@ use super::urls::url_to_uri;
 use crate::args::jsr_url;
 use crate::lsp::search::PackageSearchApi;
 use crate::tools::lint::CliLinter;
+use crate::util::path::relative_specifier;
 use deno_config::workspace::MappedResolution;
+use deno_graph::source::ResolutionMode;
 use deno_lint::diagnostic::LintDiagnosticRange;
 
 use deno_ast::SourceRange;
@@ -228,6 +230,7 @@ pub struct TsResponseImportMapper<'a> {
   documents: &'a Documents,
   maybe_import_map: Option<&'a ImportMap>,
   resolver: &'a LspResolver,
+  tsc_specifier_map: &'a tsc::TscSpecifierMap,
   file_referrer: ModuleSpecifier,
 }
 
@@ -236,12 +239,14 @@ impl<'a> TsResponseImportMapper<'a> {
     documents: &'a Documents,
     maybe_import_map: Option<&'a ImportMap>,
     resolver: &'a LspResolver,
+    tsc_specifier_map: &'a tsc::TscSpecifierMap,
     file_referrer: &ModuleSpecifier,
   ) -> Self {
     Self {
       documents,
       maybe_import_map,
       resolver,
+      tsc_specifier_map,
       file_referrer: file_referrer.clone(),
     }
   }
@@ -387,6 +392,11 @@ impl<'a> TsResponseImportMapper<'a> {
           }
         }
       }
+    } else if let Some(dep_name) = self
+      .resolver
+      .file_url_to_package_json_dep(specifier, Some(&self.file_referrer))
+    {
+      return Some(dep_name);
     }
 
     // check if the import map has this specifier
@@ -457,19 +467,36 @@ impl<'a> TsResponseImportMapper<'a> {
     specifier: &str,
     referrer: &ModuleSpecifier,
   ) -> Option<String> {
-    if let Ok(specifier) = referrer.join(specifier) {
-      if let Some(specifier) = self.check_specifier(&specifier, referrer) {
-        return Some(specifier);
-      }
-    }
-    let specifier = specifier.strip_suffix(".js").unwrap_or(specifier);
-    for ext in SUPPORTED_EXTENSIONS {
-      let specifier_with_ext = format!("{specifier}{ext}");
-      if self
-        .documents
-        .contains_import(&specifier_with_ext, referrer)
+    let specifier_stem = specifier.strip_suffix(".js").unwrap_or(specifier);
+    let specifiers = std::iter::once(Cow::Borrowed(specifier)).chain(
+      SUPPORTED_EXTENSIONS
+        .iter()
+        .map(|ext| Cow::Owned(format!("{specifier_stem}{ext}"))),
+    );
+    for specifier in specifiers {
+      if let Some(specifier) = self
+        .resolver
+        .as_graph_resolver(Some(&self.file_referrer))
+        .resolve(
+          &specifier,
+          &deno_graph::Range {
+            specifier: referrer.clone(),
+            start: deno_graph::Position::zeroed(),
+            end: deno_graph::Position::zeroed(),
+          },
+          ResolutionMode::Types,
+        )
+        .ok()
+        .and_then(|s| self.tsc_specifier_map.normalize(s.as_str()).ok())
+        .filter(|s| self.documents.exists(s, Some(&self.file_referrer)))
       {
-        return Some(specifier_with_ext);
+        if let Some(specifier) = self
+          .check_specifier(&specifier, referrer)
+          .or_else(|| relative_specifier(referrer, &specifier))
+          .filter(|s| !s.contains("/node_modules/"))
+        {
+          return Some(specifier);
+        }
       }
     }
     None
@@ -559,8 +586,9 @@ fn try_reverse_map_package_json_exports(
 pub fn fix_ts_import_changes(
   referrer: &ModuleSpecifier,
   changes: &[tsc::FileTextChanges],
-  import_mapper: &TsResponseImportMapper,
+  language_server: &language_server::Inner,
 ) -> Result<Vec<tsc::FileTextChanges>, AnyError> {
+  let import_mapper = language_server.get_ts_response_import_mapper(referrer);
   let mut r = Vec::new();
   for change in changes {
     let mut text_changes = Vec::new();
@@ -605,7 +633,7 @@ pub fn fix_ts_import_changes(
 fn fix_ts_import_action<'a>(
   referrer: &ModuleSpecifier,
   action: &'a tsc::CodeFixAction,
-  import_mapper: &TsResponseImportMapper,
+  language_server: &language_server::Inner,
 ) -> Option<Cow<'a, tsc::CodeFixAction>> {
   if !matches!(
     action.fix_name.as_str(),
@@ -621,6 +649,7 @@ fn fix_ts_import_action<'a>(
   let Some(specifier) = specifier else {
     return Some(Cow::Borrowed(action));
   };
+  let import_mapper = language_server.get_ts_response_import_mapper(referrer);
   if let Some(new_specifier) =
     import_mapper.check_unresolved_specifier(specifier, referrer)
   {
@@ -728,7 +757,7 @@ pub fn ts_changes_to_edit(
   }))
 }
 
-#[derive(Debug, Deserialize)]
+#[derive(Debug, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct CodeActionData {
   pub specifier: ModuleSpecifier,
@@ -998,11 +1027,8 @@ impl CodeActionCollection {
         "The action returned from TypeScript is unsupported.",
       ));
     }
-    let Some(action) = fix_ts_import_action(
-      specifier,
-      action,
-      &language_server.get_ts_response_import_mapper(specifier),
-    ) else {
+    let Some(action) = fix_ts_import_action(specifier, action, language_server)
+    else {
       return Ok(());
     };
     let edit = ts_changes_to_edit(&action.changes, language_server)?;
@@ -1051,10 +1077,12 @@ impl CodeActionCollection {
     specifier: &ModuleSpecifier,
     diagnostic: &lsp::Diagnostic,
   ) {
-    let data = Some(json!({
-      "specifier": specifier,
-      "fixId": action.fix_id,
-    }));
+    let data = action.fix_id.as_ref().map(|fix_id| {
+      json!(CodeActionData {
+        specifier: specifier.clone(),
+        fix_id: fix_id.clone(),
+      })
+    });
     let title = if let Some(description) = &action.fix_all_description {
       description.clone()
     } else {
diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs
index 8609aed05..ce13c3215 100644
--- a/cli/lsp/documents.rs
+++ b/cli/lsp/documents.rs
@@ -1059,34 +1059,6 @@ impl Documents {
     self.cache.is_valid_file_referrer(specifier)
   }
 
-  /// Return `true` if the provided specifier can be resolved to a document,
-  /// otherwise `false`.
-  pub fn contains_import(
-    &self,
-    specifier: &str,
-    referrer: &ModuleSpecifier,
-  ) -> bool {
-    let file_referrer = self.get_file_referrer(referrer);
-    let maybe_specifier = self
-      .resolver
-      .as_graph_resolver(file_referrer.as_deref())
-      .resolve(
-        specifier,
-        &deno_graph::Range {
-          specifier: referrer.clone(),
-          start: deno_graph::Position::zeroed(),
-          end: deno_graph::Position::zeroed(),
-        },
-        ResolutionMode::Types,
-      )
-      .ok();
-    if let Some(import_specifier) = maybe_specifier {
-      self.exists(&import_specifier, file_referrer.as_deref())
-    } else {
-      false
-    }
-  }
-
   pub fn resolve_document_specifier(
     &self,
     specifier: &ModuleSpecifier,
diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index 61a02f036..2554fa34b 100644
--- a/cli/lsp/language_server.rs
+++ b/cli/lsp/language_server.rs
@@ -1837,7 +1837,7 @@ impl Inner {
         fix_ts_import_changes(
           &code_action_data.specifier,
           &combined_code_actions.changes,
-          &self.get_ts_response_import_mapper(&code_action_data.specifier),
+          self,
         )
         .map_err(|err| {
           error!("Unable to remap changes: {:#}", err);
@@ -1890,7 +1890,7 @@ impl Inner {
         refactor_edit_info.edits = fix_ts_import_changes(
           &action_data.specifier,
           &refactor_edit_info.edits,
-          &self.get_ts_response_import_mapper(&action_data.specifier),
+          self,
         )
         .map_err(|err| {
           error!("Unable to remap changes: {:#}", err);
@@ -1921,7 +1921,8 @@ impl Inner {
         // todo(dsherret): this should probably just take the resolver itself
         // as the import map is an implementation detail
         .and_then(|d| d.resolver.maybe_import_map()),
-      self.resolver.as_ref(),
+      &self.resolver,
+      &self.ts_server.specifier_map,
       file_referrer,
     )
   }
@@ -2284,7 +2285,11 @@ impl Inner {
             .into(),
           scope.cloned(),
         )
-        .await;
+        .await
+        .unwrap_or_else(|err| {
+          error!("Unable to get completion info from TypeScript: {:#}", err);
+          None
+        });
 
       if let Some(completions) = maybe_completion_info {
         response = Some(
diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs
index 9ce76005e..f5df24d57 100644
--- a/cli/lsp/resolver.rs
+++ b/cli/lsp/resolver.rs
@@ -74,6 +74,7 @@ struct LspScopeResolver {
   pkg_json_resolver: Option<Arc<PackageJsonResolver>>,
   redirect_resolver: Option<Arc<RedirectResolver>>,
   graph_imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>,
+  package_json_deps_by_resolution: Arc<IndexMap<ModuleSpecifier, String>>,
   config_data: Option<Arc<ConfigData>>,
 }
 
@@ -88,6 +89,7 @@ impl Default for LspScopeResolver {
       pkg_json_resolver: None,
       redirect_resolver: None,
       graph_imports: Default::default(),
+      package_json_deps_by_resolution: Default::default(),
       config_data: None,
     }
   }
@@ -165,6 +167,33 @@ impl LspScopeResolver {
         )
       })
       .unwrap_or_default();
+    let package_json_deps_by_resolution = (|| {
+      let node_resolver = node_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 req_ref =
+            NpmPackageReqReference::from_str(&format!("npm:{name}")).ok()?;
+          let specifier = into_specifier_and_media_type(Some(
+            node_resolver
+              .resolve_req_reference(
+                &req_ref,
+                &referrer,
+                NodeResolutionMode::Types,
+              )
+              .ok()?,
+          ))
+          .0;
+          Some((specifier, name.clone()))
+        })
+        .collect();
+      Some(result)
+    })();
+    let package_json_deps_by_resolution =
+      Arc::new(package_json_deps_by_resolution.unwrap_or_default());
     Self {
       cjs_tracker: lsp_cjs_tracker,
       graph_resolver,
@@ -174,6 +203,7 @@ impl LspScopeResolver {
       pkg_json_resolver: Some(pkg_json_resolver),
       redirect_resolver,
       graph_imports,
+      package_json_deps_by_resolution,
       config_data: config_data.cloned(),
     }
   }
@@ -216,6 +246,9 @@ impl LspScopeResolver {
       redirect_resolver: self.redirect_resolver.clone(),
       pkg_json_resolver: Some(pkg_json_resolver),
       graph_imports: self.graph_imports.clone(),
+      package_json_deps_by_resolution: self
+        .package_json_deps_by_resolution
+        .clone(),
       config_data: self.config_data.clone(),
     })
   }
@@ -407,6 +440,18 @@ impl LspResolver {
     )))
   }
 
+  pub fn file_url_to_package_json_dep(
+    &self,
+    specifier: &ModuleSpecifier,
+    file_referrer: Option<&ModuleSpecifier>,
+  ) -> Option<String> {
+    let resolver = self.get_scope_resolver(file_referrer);
+    resolver
+      .package_json_deps_by_resolution
+      .get(specifier)
+      .cloned()
+  }
+
   pub fn in_node_modules(&self, specifier: &ModuleSpecifier) -> bool {
     fn has_node_modules_dir(specifier: &ModuleSpecifier) -> bool {
       // consider any /node_modules/ directory as being in the node_modules
diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs
index 5fcdb3575..6f63ced5b 100644
--- a/cli/lsp/tsc.rs
+++ b/cli/lsp/tsc.rs
@@ -236,7 +236,7 @@ pub struct TsServer {
   performance: Arc<Performance>,
   sender: mpsc::UnboundedSender<Request>,
   receiver: Mutex<Option<mpsc::UnboundedReceiver<Request>>>,
-  specifier_map: Arc<TscSpecifierMap>,
+  pub specifier_map: Arc<TscSpecifierMap>,
   inspector_server: Mutex<Option<Arc<InspectorServer>>>,
   pending_change: Mutex<Option<PendingChange>>,
 }
@@ -882,20 +882,22 @@ impl TsServer {
     options: GetCompletionsAtPositionOptions,
     format_code_settings: FormatCodeSettings,
     scope: Option<ModuleSpecifier>,
-  ) -> Option<CompletionInfo> {
+  ) -> Result<Option<CompletionInfo>, AnyError> {
     let req = TscRequest::GetCompletionsAtPosition(Box::new((
       self.specifier_map.denormalize(&specifier),
       position,
       options,
       format_code_settings,
     )));
-    match self.request(snapshot, req, scope).await {
-      Ok(maybe_info) => maybe_info,
-      Err(err) => {
-        log::error!("Unable to get completion info from TypeScript: {:#}", err);
-        None
-      }
-    }
+    self
+      .request::<Option<CompletionInfo>>(snapshot, req, scope)
+      .await
+      .map(|mut info| {
+        if let Some(info) = &mut info {
+          info.normalize(&self.specifier_map);
+        }
+        info
+      })
   }
 
   pub async fn get_completion_details(
@@ -3642,6 +3644,12 @@ pub struct CompletionInfo {
 }
 
 impl CompletionInfo {
+  fn normalize(&mut self, specifier_map: &TscSpecifierMap) {
+    for entry in &mut self.entries {
+      entry.normalize(specifier_map);
+    }
+  }
+
   pub fn as_completion_response(
     &self,
     line_index: Arc<LineIndex>,
@@ -3703,11 +3711,17 @@ pub struct CompletionItemData {
 
 #[derive(Debug, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
-struct CompletionEntryDataImport {
+struct CompletionEntryDataAutoImport {
   module_specifier: String,
   file_name: String,
 }
 
+#[derive(Debug)]
+pub struct CompletionNormalizedAutoImportData {
+  raw: CompletionEntryDataAutoImport,
+  normalized: ModuleSpecifier,
+}
+
 #[derive(Debug, Default, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct CompletionEntry {
@@ -3740,9 +3754,28 @@ pub struct CompletionEntry {
   is_import_statement_completion: Option<bool>,
   #[serde(skip_serializing_if = "Option::is_none")]
   data: Option<Value>,
+  /// This is not from tsc, we add it for convenience during normalization.
+  /// Represents `self.data.file_name`, but normalized.
+  #[serde(skip)]
+  auto_import_data: Option<CompletionNormalizedAutoImportData>,
 }
 
 impl CompletionEntry {
+  fn normalize(&mut self, specifier_map: &TscSpecifierMap) {
+    let Some(data) = &self.data else {
+      return;
+    };
+    let Ok(raw) =
+      serde_json::from_value::<CompletionEntryDataAutoImport>(data.clone())
+    else {
+      return;
+    };
+    if let Ok(normalized) = specifier_map.normalize(&raw.file_name) {
+      self.auto_import_data =
+        Some(CompletionNormalizedAutoImportData { raw, normalized });
+    }
+  }
+
   fn get_commit_characters(
     &self,
     info: &CompletionInfo,
@@ -3891,25 +3924,24 @@ impl CompletionEntry {
 
     if let Some(source) = &self.source {
       let mut display_source = source.clone();
-      if let Some(data) = &self.data {
-        if let Ok(import_data) =
-          serde_json::from_value::<CompletionEntryDataImport>(data.clone())
+      if let Some(import_data) = &self.auto_import_data {
+        if let Some(new_module_specifier) = language_server
+          .get_ts_response_import_mapper(specifier)
+          .check_specifier(&import_data.normalized, specifier)
+          .or_else(|| relative_specifier(specifier, &import_data.normalized))
         {
-          if let Ok(import_specifier) = resolve_url(&import_data.file_name) {
-            if let Some(new_module_specifier) = language_server
-              .get_ts_response_import_mapper(specifier)
-              .check_specifier(&import_specifier, specifier)
-              .or_else(|| relative_specifier(specifier, &import_specifier))
-            {
-              display_source.clone_from(&new_module_specifier);
-              if new_module_specifier != import_data.module_specifier {
-                specifier_rewrite =
-                  Some((import_data.module_specifier, new_module_specifier));
-              }
-            } else if source.starts_with(jsr_url().as_str()) {
-              return None;
-            }
+          if new_module_specifier.contains("/node_modules/") {
+            return None;
+          }
+          display_source.clone_from(&new_module_specifier);
+          if new_module_specifier != import_data.raw.module_specifier {
+            specifier_rewrite = Some((
+              import_data.raw.module_specifier.clone(),
+              new_module_specifier,
+            ));
           }
+        } else if source.starts_with(jsr_url().as_str()) {
+          return None;
         }
       }
       // We want relative or bare (import-mapped or otherwise) specifiers to
@@ -4212,6 +4244,13 @@ impl TscSpecifierMap {
       return specifier.to_string();
     }
     let mut specifier = original.to_string();
+    if specifier.contains("/node_modules/.deno/")
+      && !specifier.contains("/node_modules/@types/node/")
+    {
+      // The ts server doesn't give completions from files in
+      // `node_modules/.deno/`. We work around it like this.
+      specifier = specifier.replace("/node_modules/", "/$node_modules/");
+    }
     let media_type = MediaType::from_specifier(original);
     // If the URL-inferred media type doesn't correspond to tsc's path-inferred
     // media type, force it to be the same by appending an extension.
@@ -4329,7 +4368,7 @@ fn op_is_cancelled(state: &mut OpState) -> bool {
 fn op_is_node_file(state: &mut OpState, #[string] path: String) -> bool {
   let state = state.borrow::<State>();
   let mark = state.performance.mark("tsc.op.op_is_node_file");
-  let r = match ModuleSpecifier::parse(&path) {
+  let r = match state.specifier_map.normalize(path) {
     Ok(specifier) => state.state_snapshot.resolver.in_node_modules(&specifier),
     Err(_) => false,
   };
@@ -4609,7 +4648,10 @@ fn op_script_names(state: &mut OpState) -> ScriptNames {
   for doc in &docs {
     let specifier = doc.specifier();
     let is_open = doc.is_open();
-    if is_open || specifier.scheme() == "file" {
+    if is_open
+      || (specifier.scheme() == "file"
+        && !state.state_snapshot.resolver.in_node_modules(specifier))
+    {
       let script_names = doc
         .scope()
         .and_then(|s| result.by_scope.get_mut(s))
@@ -6035,6 +6077,7 @@ mod tests {
         Some(temp_dir.url()),
       )
       .await
+      .unwrap()
       .unwrap();
     assert_eq!(info.entries.len(), 22);
     let details = ts_server
@@ -6194,6 +6237,7 @@ mod tests {
         Some(temp_dir.url()),
       )
       .await
+      .unwrap()
       .unwrap();
     let entry = info
       .entries