fix(lsp): handle byonm specifiers in jupyter notebooks (#21332)

Part of https://github.com/denoland/deno/issues/21308

cli/lsp/completions.rs

@@ -3,7 +3,6 @@
 use super::client::Client;
 use super::config::ConfigSnapshot;
 use super::config::WorkspaceSettings;
-use super::documents::file_like_to_file_specifier;
 use super::documents::Documents;
 use super::documents::DocumentsFilter;
 use super::lsp_custom;
@@ -375,16 +374,11 @@ fn get_local_completions(
   current: &str,
   range: &lsp::Range,
 ) -> Option<Vec<lsp::CompletionItem>> {
-  let base = match file_like_to_file_specifier(base) {
-    Some(s) => s,
-    None => base.clone(),
-  };
-
   if base.scheme() != "file" {
     return None;
   }
 
-  let mut base_path = specifier_to_file_path(&base).ok()?;
+  let mut base_path = specifier_to_file_path(base).ok()?;
   base_path.pop();
   let mut current_path = normalize_path(base_path.join(current));
   // if the current text does not end in a `/` then we are still selecting on
@@ -404,10 +398,10 @@ fn get_local_completions(
           let de = de.ok()?;
           let label = de.path().file_name()?.to_string_lossy().to_string();
           let entry_specifier = resolve_path(de.path().to_str()?, &cwd).ok()?;
-          if entry_specifier == base {
+          if entry_specifier == *base {
             return None;
           }
-          let full_text = relative_specifier(&base, &entry_specifier)?;
+          let full_text = relative_specifier(base, &entry_specifier)?;
           // this weeds out situations where we are browsing in the parent, but
           // we want to filter out non-matches when the completion is manually
           // invoked by the user, but still allows for things like `../src/../`

cli/lsp/config.rs

@@ -859,9 +859,6 @@ impl Config {
     specifier: &ModuleSpecifier,
   ) -> Option<&LanguageWorkspaceSettings> {
     let workspace_settings = self.workspace_settings_for_specifier(specifier);
-    if specifier.scheme() == "deno-notebook-cell" {
-      return Some(&workspace_settings.typescript);
-    }
     match MediaType::from_specifier(specifier) {
       MediaType::JavaScript
       | MediaType::Jsx

cli/lsp/documents.rs

@@ -92,15 +92,8 @@ static TSX_HEADERS: Lazy<HashMap<String, String>> = Lazy::new(|| {
     .collect()
 });
 
-pub const DOCUMENT_SCHEMES: [&str; 7] = [
-  "data",
-  "blob",
-  "file",
-  "http",
-  "https",
-  "untitled",
-  "deno-notebook-cell",
-];
+pub const DOCUMENT_SCHEMES: [&str; 5] =
+  ["data", "blob", "file", "http", "https"];
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum LanguageId {
@@ -263,29 +256,6 @@ impl AssetOrDocument {
   }
 }
 
-/// Convert a e.g. `deno-notebook-cell:` specifier to a `file:` specifier.
-/// ```rust
-/// assert_eq!(
-///   file_like_to_file_specifier(
-///     &Url::parse("deno-notebook-cell:/path/to/file.ipynb#abc").unwrap(),
-///   ),
-///   Some(Url::parse("file:///path/to/file.ipynb#abc").unwrap()),
-/// );
-pub fn file_like_to_file_specifier(specifier: &Url) -> Option<Url> {
-  if matches!(specifier.scheme(), "untitled" | "deno-notebook-cell") {
-    if let Ok(mut s) = ModuleSpecifier::parse(&format!(
-      "file://{}",
-      &specifier.as_str()
-        [url::quirks::internal_components(specifier).host_end as usize..],
-    )) {
-      s.query_pairs_mut()
-        .append_pair("scheme", specifier.scheme());
-      return Some(s);
-    }
-  }
-  None
-}
-
 #[derive(Debug, Default)]
 struct DocumentDependencies {
   deps: IndexMap<String, deno_graph::Dependency>,
@@ -302,38 +272,10 @@ impl DocumentDependencies {
   }
 
   pub fn from_module(module: &deno_graph::EsmModule) -> Self {
-    let mut deps = Self {
+    Self {
       deps: module.dependencies.clone(),
       maybe_types_dependency: module.maybe_types_dependency.clone(),
-    };
-    if file_like_to_file_specifier(&module.specifier).is_some() {
-      for (_, dep) in &mut deps.deps {
-        if let Resolution::Ok(resolved) = &mut dep.maybe_code {
-          if let Some(specifier) =
-            file_like_to_file_specifier(&resolved.specifier)
-          {
-            resolved.specifier = specifier;
-          }
-        }
-        if let Resolution::Ok(resolved) = &mut dep.maybe_type {
-          if let Some(specifier) =
-            file_like_to_file_specifier(&resolved.specifier)
-          {
-            resolved.specifier = specifier;
-          }
-        }
-      }
-      if let Some(dep) = &mut deps.maybe_types_dependency {
-        if let Resolution::Ok(resolved) = &mut dep.dependency {
-          if let Some(specifier) =
-            file_like_to_file_specifier(&resolved.specifier)
-          {
-            resolved.specifier = specifier;
-          }
-        }
-      }
     }
-    deps
   }
 }
 

cli/lsp/tsc.rs

@@ -3,7 +3,6 @@
 use super::analysis::CodeActionData;
 use super::code_lens;
 use super::config;
-use super::documents::file_like_to_file_specifier;
 use super::documents::AssetOrDocument;
 use super::documents::DocumentsFilter;
 use super::language_server;
@@ -257,9 +256,9 @@ impl TsServer {
         .map(|s| self.specifier_map.denormalize(&s))
         .collect::<Vec<String>>(),]),
     };
-    let diagnostics_map_ = self.request_with_cancellation::<HashMap<String, Vec<crate::tsc::Diagnostic>>>(snapshot, req, token).await?;
-    let mut diagnostics_map = HashMap::new();
-    for (mut specifier, mut diagnostics) in diagnostics_map_ {
+    let raw_diagnostics = self.request_with_cancellation::<HashMap<String, Vec<crate::tsc::Diagnostic>>>(snapshot, req, token).await?;
+    let mut diagnostics_map = HashMap::with_capacity(raw_diagnostics.len());
+    for (mut specifier, mut diagnostics) in raw_diagnostics {
       specifier = self.specifier_map.normalize(&specifier)?.to_string();
       for diagnostic in &mut diagnostics {
         normalize_diagnostic(diagnostic, &self.specifier_map)?;
@@ -3715,16 +3714,8 @@ impl TscSpecifierMap {
     if let Some(specifier) = self.denormalized_specifiers.get(original) {
       return specifier.to_string();
     }
-    let mut specifier = if let Some(s) = file_like_to_file_specifier(original) {
-      s.to_string()
-    } else {
-      original.to_string()
-    };
-    let media_type = if original.scheme() == "deno-notebook-cell" {
-      MediaType::TypeScript
-    } else {
-      MediaType::from_specifier(original)
-    };
+    let mut specifier = original.to_string();
+    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.
     if MediaType::from_path(Path::new(specifier.as_str())) != media_type {

cli/lsp/urls.rs

@@ -247,6 +247,8 @@ impl LspUrlMap {
           LspUrlKind::File => Url::from_file_path(path).unwrap(),
         });
       }
+    } else if let Some(s) = file_like_to_file_specifier(url) {
+      specifier = Some(s);
     } else if let Some(s) = from_deno_url(url) {
       specifier = Some(s);
     }
@@ -256,6 +258,30 @@ impl LspUrlMap {
   }
 }
 
+/// Convert a e.g. `deno-notebook-cell:` specifier to a `file:` specifier.
+/// ```rust
+/// assert_eq!(
+///   file_like_to_file_specifier(
+///     &Url::parse("deno-notebook-cell:/path/to/file.ipynb#abc").unwrap(),
+///   ),
+///   Some(Url::parse("file:///path/to/file.ipynb.ts?scheme=deno-notebook-cell#abc").unwrap()),
+/// );
+fn file_like_to_file_specifier(specifier: &Url) -> Option<Url> {
+  if matches!(specifier.scheme(), "untitled" | "deno-notebook-cell") {
+    if let Ok(mut s) = ModuleSpecifier::parse(&format!(
+      "file://{}",
+      &specifier.as_str()[deno_core::url::quirks::internal_components(specifier)
+        .host_end as usize..],
+    )) {
+      s.query_pairs_mut()
+        .append_pair("scheme", specifier.scheme());
+      s.set_path(&format!("{}.ts", s.path()));
+      return Some(s);
+    }
+  }
+  None
+}
+
 #[cfg(test)]
 mod tests {
   use super::*;
@@ -389,4 +415,28 @@ mod tests {
     let actual = map.normalize_url(&fixture, LspUrlKind::File);
     assert_eq!(actual, fixture);
   }
+
+  #[test]
+  fn test_file_like_to_file_specifier() {
+    assert_eq!(
+      file_like_to_file_specifier(
+        &Url::parse("deno-notebook-cell:/path/to/file.ipynb#abc").unwrap(),
+      ),
+      Some(
+        Url::parse(
+          "file:///path/to/file.ipynb.ts?scheme=deno-notebook-cell#abc"
+        )
+        .unwrap()
+      ),
+    );
+    assert_eq!(
+      file_like_to_file_specifier(
+        &Url::parse("untitled:/path/to/file.ipynb#123").unwrap(),
+      ),
+      Some(
+        Url::parse("file:///path/to/file.ipynb.ts?scheme=untitled#123")
+          .unwrap()
+      ),
+    );
+  }
 }

cli/tests/integration/lsp_tests.rs

@@ -10438,3 +10438,63 @@ fn lsp_import_unstable_bare_node_builtins_auto_discovered() {
 
   client.shutdown();
 }
+
+#[test]
+fn lsp_jupyter_byonm_diagnostics() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir = context.temp_dir().path();
+  temp_dir.join("package.json").write_json(&json!({
+    "dependencies": {
+      "@denotest/esm-basic": "*"
+    }
+  }));
+  temp_dir.join("deno.json").write_json(&json!({
+    "unstable": ["byonm"]
+  }));
+  context.run_npm("install");
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  let notebook_specifier = temp_dir.join("notebook.ipynb").uri_file();
+  let notebook_specifier = format!(
+    "{}#abc",
+    notebook_specifier
+      .to_string()
+      .replace("file://", "deno-notebook-cell:")
+  );
+  let diagnostics = client.did_open(json!({
+    "textDocument": {
+      "uri": notebook_specifier,
+      "languageId": "typescript",
+      "version": 1,
+      "text": "import { getValue, nonExistent } from '@denotest/esm-basic';\n console.log(getValue, nonExistent);",
+    },
+  }));
+  assert_eq!(
+    json!(diagnostics.all_messages()),
+    json!([
+      {
+        "uri": notebook_specifier,
+        "diagnostics": [
+          {
+            "range": {
+              "start": {
+                "line": 0,
+                "character": 19,
+              },
+              "end": {
+                "line": 0,
+                "character": 30,
+              },
+            },
+            "severity": 1,
+            "code": 2305,
+            "source": "deno-ts",
+            "message": "Module '\"@denotest/esm-basic\"' has no exported member 'nonExistent'.",
+          },
+        ],
+        "version": 1,
+      },
+    ])
+  );
+  client.shutdown();
+}