Repository: denoland/deno
Pull request: fix(lsp): discard completions from non-exported npm files

Fixes #28676.

Changed files:
filename: cli/lsp/analysis.rs
patch: @@ -354,71 +354,75 @@ impl<'a> TsResponseImportMapper<'a> {
 
     if let Some(npm_resolver) = scoped_resolver.as_maybe_managed_npm_resolver()
     {
-      let in_npm_pkg = scoped_resolver
-        .as_in_npm_pkg_checker()
-        .in_npm_package(specifier);
-      if in_npm_pkg {
-        if let Ok(Some(pkg_id)) =
-          npm_resolver.resolve_pkg_id_from_specifier(specifier)
-        {
-          let pkg_reqs =
-            maybe_reverse_definitely_typed(&pkg_id, npm_resolver.resolution())
-              .unwrap_or_else(|| {
-                npm_resolver
-                  .resolution()
-                  .resolve_pkg_reqs_from_pkg_id(&pkg_id)
-              });
-          // check if any pkg reqs match what is found in an import map
-          if !pkg_reqs.is_empty() {
-            let sub_path = npm_resolver
-              .resolve_pkg_folder_from_pkg_id(&pkg_id)
-              .ok()
-              .and_then(|pkg_folder| {
-                self.resolve_package_path(specifier, &pkg_folder)
-              });
-            if let Some(import_map) = self.maybe_import_map {
-              let pkg_reqs = pkg_reqs.iter().collect::<HashSet<_>>();
-              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)
+      let match_specifier = || {
+        let in_npm_pkg = scoped_resolver
+          .as_in_npm_pkg_checker()
+          .in_npm_package(specifier);
+        if !in_npm_pkg {
+          return None;
+        }
+        let pkg_id = npm_resolver
+          .resolve_pkg_id_from_specifier(specifier)
+          .ok()??;
+        let pkg_reqs =
+          maybe_reverse_definitely_typed(&pkg_id, npm_resolver.resolution())
+            .unwrap_or_else(|| {
+              npm_resolver
+                .resolution()
+                .resolve_pkg_reqs_from_pkg_id(&pkg_id)
+            });
+        if pkg_reqs.is_empty() {
+          return None;
+        }
+        // check if any pkg reqs match what is found in an import map
+        let sub_path = npm_resolver
+          .resolve_pkg_folder_from_pkg_id(&pkg_id)
+          .ok()
+          .and_then(|pkg_folder| {
+            self.resolve_package_path(specifier, &pkg_folder)
+          })?;
+        let sub_path = Some(sub_path).filter(|s| !s.is_empty());
+        if let Some(import_map) = self.maybe_import_map {
+          let pkg_reqs = pkg_reqs.iter().collect::<HashSet<_>>();
+          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) {
+                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)
                   {
-                    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)
-                      {
-                        // keys that don't end in a slash can't be mapped to a subpath
-                        if entry.raw_key.ends_with('/')
-                          || key_sub_path.is_empty()
-                        {
-                          matches
-                            .push(format!("{}{}", entry.raw_key, key_sub_path));
-                        }
-                      }
+                    // keys that don't end in a slash can't be mapped to a subpath
+                    if entry.raw_key.ends_with('/') || key_sub_path.is_empty() {
+                      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
-            if let Some(pkg_req) = pkg_reqs.first() {
-              return Some(concat_npm_specifier(
-                "npm:",
-                pkg_req,
-                sub_path.as_deref(),
-              ));
             }
           }
+          // 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
+        if let Some(pkg_req) = pkg_reqs.first() {
+          return Some(concat_npm_specifier(
+            "npm:",
+            pkg_req,
+            sub_path.as_deref(),
+          ));
+        }
+        None
+      };
+      if let Some(result) = match_specifier() {
+        return Some(result);
       }
     } else if let Some(dep_name) =
       scoped_resolver.file_url_to_package_json_dep(specifier)
@@ -449,8 +453,11 @@ impl<'a> TsResponseImportMapper<'a> {
       .get_closest_package_json(&package_root_folder.join("package.json"))
       .ok()
       .flatten()?;
-    let root_folder = package_json.path.parent()?;
+    let Some(exports) = &package_json.exports else {
+      return Some("".to_string());
+    };
 
+    let root_folder = package_json.path.parent()?;
     let specifier_path = url_to_file_path(specifier).ok()?;
     let mut search_paths = vec![specifier_path.clone()];
     // TypeScript will provide a .js extension for quick fixes, so do
@@ -476,14 +483,10 @@ impl<'a> TsResponseImportMapper<'a> {
     }
 
     for search_path in search_paths {
-      if let Some(exports) = &package_json.exports {
-        if let Some(result) = try_reverse_map_package_json_exports(
-          root_folder,
-          &search_path,
-          exports,
-        ) {
-          return Some(result);
-        }
+      if let Some(result) =
+        try_reverse_map_package_json_exports(root_folder, &search_path, exports)
+      {
+        return Some(result);
       }
     }
 
@@ -628,16 +631,7 @@ fn try_reverse_map_package_json_exports(
     None
   }
 
-  let result = try_reverse_map_package_json_exports_inner(
-    root_path,
-    target_path,
-    exports,
-  )?;
-  if result.is_empty() {
-    None
-  } else {
-    Some(result)
-  }
+  try_reverse_map_package_json_exports_inner(root_path, target_path, exports)
 }
 
 /// For a set of tsc changes, can them for any that contain something that looks
@@ -1439,12 +1433,15 @@ mod tests {
       .unwrap(),
       "hooks"
     );
-    assert!(try_reverse_map_package_json_exports(
-      &PathBuf::from("/project/"),
-      &PathBuf::from("/project/src/index.d.ts"),
-      exports,
-    )
-    .is_none());
+    assert_eq!(
+      try_reverse_map_package_json_exports(
+        &PathBuf::from("/project/"),
+        &PathBuf::from("/project/src/index.d.ts"),
+        exports,
+      )
+      .unwrap(),
+      ""
+    );
     assert_eq!(
       try_reverse_map_package_json_exports(
         &PathBuf::from("/project/"),

filename: tests/integration/lsp_tests.rs
patch: @@ -8871,6 +8871,84 @@ fn lsp_npm_reexport_from_non_export() {
   client.shutdown();
 }
 
+// Regression test for https://github.com/denoland/deno/issues/28676.
+#[test]
+#[timeout(300_000)]
+fn lsp_npm_managed_import_map_no_duplicate_completions() {
+  let context = TestContextBuilder::new()
+    .use_http_server()
+    .use_temp_cwd()
+    .build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write(
+    "deno.json",
+    json!({
+      "imports": {
+        "chalk": "npm:chalk"
+      },
+    })
+    .to_string(),
+  );
+  let file = temp_dir.source_file(
+    "file.ts",
+    r#"
+      import {} from "chalk";
+      const colorInfo: ColorInfo = false;
+      console.log(colorInfo);
+    "#,
+  );
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  client.write_request(
+    "workspace/executeCommand",
+    json!({
+      "command": "deno.cache",
+      "arguments": [[], file.url()],
+    }),
+  );
+  client.read_diagnostics();
+  let list = client.get_completion_list(
+    file.uri().as_str(),
+    (2, 32),
+    json!({
+      "triggerKind": 2,
+      "triggerCharacter": ".",
+    }),
+  );
+  assert!(!list.is_incomplete);
+  // Get all matching entries, then assert that there's only one and it has the
+  // correct auto-import edit.
+  let items = list
+    .items
+    .iter()
+    .filter(|i| i.label == "ColorInfo")
+    .map(|i| client.write_request("completionItem/resolve", json!(i)))
+    .collect::<Vec<_>>();
+  assert_eq!(
+    json!(items),
+    json!([
+      {
+        "label": "ColorInfo",
+        "labelDetails": { "description": "chalk" },
+        "kind": 6,
+        "detail": "Update import from \"chalk\"\n\n(alias) type ColorInfo = false | ColorSupport\nexport ColorInfo",
+        "documentation": { "kind": "markdown", "value": "" },
+        "sortText": "\u{ffff}16_0",
+        "additionalTextEdits": [
+          {
+            "range": {
+              "start": { "line": 1, "character": 13 },
+              "end": { "line": 1, "character": 15 },
+            },
+            "newText": "{ ColorInfo }",
+          },
+        ],
+      },
+    ]),
+  );
+  client.shutdown();
+}
+
 #[test]
 #[timeout(300_000)]
 fn lsp_completions_using_decl() {

filename: tests/util/server/src/fs.rs
patch: @@ -20,6 +20,8 @@ use serde::Serialize;
 use url::Url;
 
 use crate::assertions::assert_wildcard_match;
+use crate::lsp::source_file;
+use crate::lsp::SourceFile;
 use crate::testdata_path;
 
 /// Characters that are left unencoded in a `Url` path but will be encoded in a
@@ -593,6 +595,15 @@ impl TempDir {
     self.target_path().join(path).write(text)
   }
 
+  pub fn source_file(
+    &self,
+    path: impl AsRef<Path>,
+    text: impl AsRef<str>,
+  ) -> SourceFile {
+    let path = self.target_path().join(path);
+    source_file(path, text)
+  }
+
   pub fn symlink_dir(
     &self,
     oldpath: impl AsRef<Path>,