fix(lsp): don't normalize urls in cache command params (#22182)

4 files changed, 128 insertions, 126 deletions

diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index 31bd5ba8f..573fb1eb4 100644
--- a/cli/lsp/language_server.rs
+++ b/cli/lsp/language_server.rs
@@ -284,7 +284,8 @@ impl LanguageServer {
   /// in the Deno cache, including any of their dependencies.
   pub async fn cache_request(
     &self,
-    params: Option<Value>,
+    specifiers: Vec<ModuleSpecifier>,
+    referrer: ModuleSpecifier,
   ) -> LspResult<Option<Value>> {
     async fn create_graph_for_caching(
       cli_options: CliOptions,
@@ -330,50 +331,44 @@ impl LanguageServer {
       Ok(())
     }
 
-    match params.map(serde_json::from_value) {
-      Some(Ok(params)) => {
-        // do as much as possible in a read, then do a write outside
-        let maybe_prepare_cache_result = {
-          let inner = self.0.read().await; // ensure dropped
-          match inner.prepare_cache(params) {
-            Ok(maybe_cache_result) => maybe_cache_result,
-            Err(err) => {
-              self
-                .0
-                .read()
-                .await
-                .client
-                .show_message(MessageType::WARNING, err);
-              return Err(LspError::internal_error());
-            }
-          }
-        };
-        if let Some(result) = maybe_prepare_cache_result {
-          let cli_options = result.cli_options;
-          let roots = result.roots;
-          let open_docs = result.open_docs;
-          let handle = spawn(async move {
-            create_graph_for_caching(cli_options, roots, open_docs).await
-          });
-          if let Err(err) = handle.await.unwrap() {
-            self
-              .0
-              .read()
-              .await
-              .client
-              .show_message(MessageType::WARNING, err);
-          }
-          // do npm resolution in a write—we should have everything
-          // cached by this point anyway
-          self.0.write().await.refresh_npm_specifiers().await;
-          // now refresh the data in a read
-          self.0.read().await.post_cache(result.mark).await;
+    // do as much as possible in a read, then do a write outside
+    let maybe_prepare_cache_result = {
+      let inner = self.0.read().await; // ensure dropped
+      match inner.prepare_cache(specifiers, referrer) {
+        Ok(maybe_cache_result) => maybe_cache_result,
+        Err(err) => {
+          self
+            .0
+            .read()
+            .await
+            .client
+            .show_message(MessageType::WARNING, err);
+          return Err(LspError::internal_error());
         }
-        Ok(Some(json!(true)))
       }
-      Some(Err(err)) => Err(LspError::invalid_params(err.to_string())),
-      None => Err(LspError::invalid_params("Missing parameters")),
+    };
+    if let Some(result) = maybe_prepare_cache_result {
+      let cli_options = result.cli_options;
+      let roots = result.roots;
+      let open_docs = result.open_docs;
+      let handle = spawn(async move {
+        create_graph_for_caching(cli_options, roots, open_docs).await
+      });
+      if let Err(err) = handle.await.unwrap() {
+        self
+          .0
+          .read()
+          .await
+          .client
+          .show_message(MessageType::WARNING, err);
+      }
+      // do npm resolution in a write—we should have everything
+      // cached by this point anyway
+      self.0.write().await.refresh_npm_specifiers().await;
+      // now refresh the data in a read
+      self.0.read().await.post_cache(result.mark).await;
     }
+    Ok(Some(json!(true)))
   }
 
   /// This request is only used by the lsp integration tests to
@@ -396,12 +391,6 @@ impl LanguageServer {
     Ok(Some(self.0.read().await.get_performance()))
   }
 
-  pub async fn reload_import_registries_request(
-    &self,
-  ) -> LspResult<Option<Value>> {
-    self.0.write().await.reload_import_registries().await
-  }
-
   pub async fn task_definitions(&self) -> LspResult<Vec<TaskDefinition>> {
     self.0.read().await.task_definitions()
   }
@@ -1081,24 +1070,18 @@ impl Inner {
               compiler_options_obj.get("jsxImportSource")
             {
               if let Some(jsx_import_source) = jsx_import_source.as_str() {
-                let cache_params = lsp_custom::CacheParams {
-                  referrer: TextDocumentIdentifier {
-                    uri: config_file.specifier.clone(),
-                  },
-                  uris: vec![TextDocumentIdentifier {
-                    uri: Url::parse(&format!(
-                      "data:application/typescript;base64,{}",
-                      base64::engine::general_purpose::STANDARD.encode(
-                        format!("import '{jsx_import_source}/jsx-runtime';")
-                      )
-                    ))
-                    .unwrap(),
-                  }],
-                };
+                let specifiers = vec![Url::parse(&format!(
+                  "data:application/typescript;base64,{}",
+                  base64::engine::general_purpose::STANDARD.encode(format!(
+                    "import '{jsx_import_source}/jsx-runtime';"
+                  ))
+                ))
+                .unwrap()];
+                let referrer = config_file.specifier.clone();
                 self.task_queue.queue_task(Box::new(|ls: LanguageServer| {
                   spawn(async move {
                     if let Err(err) =
-                      ls.cache_request(Some(json!(cache_params))).await
+                      ls.cache_request(specifiers, referrer).await
                     {
                       lsp_warn!("{}", err);
                     }
@@ -3222,24 +3205,13 @@ impl tower_lsp::LanguageServer for LanguageServer {
   ) -> LspResult<Option<Value>> {
     if params.command == "deno.cache" {
       let mut arguments = params.arguments.into_iter();
-      let uris = serde_json::to_value(arguments.next()).unwrap();
-      let uris: Vec<Url> = serde_json::from_value(uris)
+      let specifiers = serde_json::to_value(arguments.next()).unwrap();
+      let specifiers: Vec<Url> = serde_json::from_value(specifiers)
         .map_err(|err| LspError::invalid_params(err.to_string()))?;
       let referrer = serde_json::to_value(arguments.next()).unwrap();
       let referrer: Url = serde_json::from_value(referrer)
         .map_err(|err| LspError::invalid_params(err.to_string()))?;
-      self
-        .cache_request(Some(
-          serde_json::to_value(lsp_custom::CacheParams {
-            referrer: TextDocumentIdentifier { uri: referrer },
-            uris: uris
-              .into_iter()
-              .map(|uri| TextDocumentIdentifier { uri })
-              .collect(),
-          })
-          .expect("well formed json"),
-        ))
-        .await
+      self.cache_request(specifiers, referrer).await
     } else if params.command == "deno.reloadImportRegistries" {
       self.0.write().await.reload_import_registries().await
     } else {
@@ -3421,7 +3393,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
 
   async fn did_save(&self, params: DidSaveTextDocumentParams) {
     let uri = &params.text_document.uri;
-    {
+    let specifier = {
       let mut inner = self.0.write().await;
       let specifier = inner.url_map.normalize_url(uri, LspUrlKind::File);
       inner.documents.save(&specifier);
@@ -3438,18 +3410,10 @@ impl tower_lsp::LanguageServer for LanguageServer {
         Ok(path) if is_importable_ext(&path) => {}
         _ => return,
       }
-    }
-    if let Err(err) = self
-      .cache_request(Some(
-        serde_json::to_value(lsp_custom::CacheParams {
-          referrer: TextDocumentIdentifier { uri: uri.clone() },
-          uris: vec![TextDocumentIdentifier { uri: uri.clone() }],
-        })
-        .unwrap(),
-      ))
-      .await
-    {
-      lsp_warn!("Failed to cache \"{}\" on save: {}", uri.to_string(), err);
+      specifier
+    };
+    if let Err(err) = self.cache_request(vec![], specifier.clone()).await {
+      lsp_warn!("Failed to cache \"{}\" on save: {}", &specifier, err);
     }
   }
 
@@ -3693,22 +3657,17 @@ struct PrepareCacheResult {
 impl Inner {
   fn prepare_cache(
     &self,
-    params: lsp_custom::CacheParams,
+    specifiers: Vec<ModuleSpecifier>,
+    referrer: ModuleSpecifier,
   ) -> Result<Option<PrepareCacheResult>, AnyError> {
-    let referrer = self
-      .url_map
-      .normalize_url(&params.referrer.uri, LspUrlKind::File);
-    let mark = self.performance.mark_with_args("lsp.cache", &params);
-    let roots = if !params.uris.is_empty() {
-      params
-        .uris
-        .iter()
-        .map(|t| self.url_map.normalize_url(&t.uri, LspUrlKind::File))
-        .collect()
+    let mark = self
+      .performance
+      .mark_with_args("lsp.cache", (&specifiers, &referrer));
+    let roots = if !specifiers.is_empty() {
+      specifiers
     } else {
       vec![referrer]
     };
-
     let workspace_settings = self.config.workspace_settings();
     let mut cli_options = CliOptions::new(
       Flags {
diff --git a/cli/lsp/lsp_custom.rs b/cli/lsp/lsp_custom.rs
index a1fe392f1..637a88bda 100644
--- a/cli/lsp/lsp_custom.rs
+++ b/cli/lsp/lsp_custom.rs
@@ -4,28 +4,14 @@ use deno_core::serde::Deserialize;
 use deno_core::serde::Serialize;
 use tower_lsp::lsp_types as lsp;
 
-pub const CACHE_REQUEST: &str = "deno/cache";
 pub const PERFORMANCE_REQUEST: &str = "deno/performance";
 pub const TASK_REQUEST: &str = "deno/taskDefinitions";
-pub const RELOAD_IMPORT_REGISTRIES_REQUEST: &str =
-  "deno/reloadImportRegistries";
 pub const VIRTUAL_TEXT_DOCUMENT: &str = "deno/virtualTextDocument";
 pub const LATEST_DIAGNOSTIC_BATCH_INDEX: &str =
   "deno/internalLatestDiagnosticBatchIndex";
 
 #[derive(Debug, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
-pub struct CacheParams {
-  /// The document currently open in the editor.  If there are no `uris`
-  /// supplied, the referrer will be cached.
-  pub referrer: lsp::TextDocumentIdentifier,
-  /// Any documents that have been specifically asked to be cached via the
-  /// command.
-  pub uris: Vec<lsp::TextDocumentIdentifier>,
-}
-
-#[derive(Debug, Deserialize, Serialize)]
-#[serde(rename_all = "camelCase")]
 pub struct TaskDefinition {
   pub name: String,
   // TODO(nayeemrmn): Rename this to `command` in vscode_deno.
diff --git a/cli/lsp/mod.rs b/cli/lsp/mod.rs
index bc2323f12..c941a02a8 100644
--- a/cli/lsp/mod.rs
+++ b/cli/lsp/mod.rs
@@ -48,20 +48,10 @@ pub async fn start() -> Result<(), AnyError> {
       token.clone(),
     )
   })
-  // TODO(nayeemrmn): The extension has replaced this with the `deno.cache`
-  // command as of vscode_deno 3.21.0 / 2023.09.05. Remove this eventually.
-  .custom_method(lsp_custom::CACHE_REQUEST, LanguageServer::cache_request)
   .custom_method(
     lsp_custom::PERFORMANCE_REQUEST,
     LanguageServer::performance_request,
   )
-  // TODO(nayeemrmn): The extension has replaced this with the
-  // `deno.reloadImportRegistries` command as of vscode_deno
-  // 3.26.0 / 2023.10.10. Remove this eventually.
-  .custom_method(
-    lsp_custom::RELOAD_IMPORT_REGISTRIES_REQUEST,
-    LanguageServer::reload_import_registries_request,
-  )
   .custom_method(lsp_custom::TASK_REQUEST, LanguageServer::task_definitions)
   // TODO(nayeemrmn): Rename this to `deno/taskDefinitions` in vscode_deno and
   // remove this alias.
diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs
index 83ce5c98b..3de8802fa 100644
--- a/cli/tests/integration/lsp_tests.rs
+++ b/cli/tests/integration/lsp_tests.rs
@@ -4945,6 +4945,73 @@ fn lsp_cache_on_save() {
   client.shutdown();
 }
 
+// Regression test for https://github.com/denoland/deno/issues/22122.
+#[test]
+fn lsp_cache_then_definition() {
+  let context = TestContextBuilder::new()
+    .use_http_server()
+    .use_temp_cwd()
+    .build();
+  let temp_dir = context.temp_dir();
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  client.did_open(json!({
+    "textDocument": {
+      "uri": temp_dir.uri().join("file.ts").unwrap(),
+      "languageId": "typescript",
+      "version": 1,
+      "text": r#"import "http://localhost:4545/run/002_hello.ts";"#,
+    },
+  }));
+  // Prior to the fix, this would cause a faulty memoization that maps the
+  // URL "http://localhost:4545/run/002_hello.ts" to itself, preventing it from
+  // being reverse-mapped to "deno:/http/localhost%3A4545/run/002_hello.ts" on
+  // "textDocument/definition" request.
+  client.write_request(
+    "workspace/executeCommand",
+    json!({
+      "command": "deno.cache",
+      "arguments": [
+        ["http://localhost:4545/run/002_hello.ts"],
+        temp_dir.uri().join("file.ts").unwrap(),
+      ],
+    }),
+  );
+  let res = client.write_request(
+    "textDocument/definition",
+    json!({
+      "textDocument": { "uri": temp_dir.uri().join("file.ts").unwrap() },
+      "position": { "line": 0, "character": 8 },
+    }),
+  );
+  assert_eq!(
+    res,
+    json!([{
+      "targetUri": "deno:/http/localhost%3A4545/run/002_hello.ts",
+      "targetRange": {
+        "start": {
+          "line": 0,
+          "character": 0,
+        },
+        "end": {
+          "line": 1,
+          "character": 0,
+        },
+      },
+      "targetSelectionRange": {
+        "start": {
+          "line": 0,
+          "character": 0,
+        },
+        "end": {
+          "line": 1,
+          "character": 0,
+        },
+      },
+    }]),
+  );
+}
+
 #[test]
 fn lsp_code_actions_imports() {
   let context = TestContextBuilder::new().use_temp_cwd().build();