fix(lsp): don't write lockfile during cache-on-save (#30733)

Author: nayeemrmn

cli/lsp/config.rs

@@ -555,7 +555,7 @@ pub struct WorkspaceSettings {
 
   /// Cache local modules and their dependencies on `textDocument/didSave`
   /// notifications corresponding to them.
-  #[serde(default)]
+  #[serde(default = "default_to_true")]
   pub cache_on_save: bool,
 
   /// Override the default stores used to validate certificates. This overrides
@@ -632,7 +632,7 @@ impl Default for WorkspaceSettings {
       disable_paths: vec![],
       enable_paths: None,
       cache: None,
-      cache_on_save: false,
+      cache_on_save: true,
       certificate_stores: None,
       config: None,
       import_map: None,
@@ -1433,7 +1433,7 @@ impl ConfigData {
         is_package_manager_subcommand: false,
         frozen_lockfile: None,
         lock_arg: None,
-        lockfile_skip_write: false,
+        lockfile_skip_write: true,
         node_modules_dir: Some(resolve_node_modules_dir_mode(
           &member_dir.workspace,
           byonm,
@@ -2146,7 +2146,7 @@ mod tests {
         disable_paths: vec![],
         enable_paths: None,
         cache: None,
-        cache_on_save: false,
+        cache_on_save: true,
         certificate_stores: None,
         config: None,
         import_map: None,

cli/lsp/language_server.rs

@@ -377,6 +377,7 @@ impl LanguageServer {
     specifiers: Vec<ModuleSpecifier>,
     referrer: ModuleSpecifier,
     force_global_cache: bool,
+    lockfile_skip_write: bool,
   ) -> LspResult<Option<Value>> {
     async fn create_graph_for_caching(
       factory: CliFactory,
@@ -439,6 +440,7 @@ impl LanguageServer {
       specifiers,
       referrer,
       force_global_cache,
+      lockfile_skip_write,
     );
 
     match prepare_cache_result {
@@ -458,7 +460,12 @@ impl LanguageServer {
 
         // now get the lock back to update with the new information
         *self.did_change_batch_queue.borrow_mut() = None;
-        self.inner.write().await.post_cache().await;
+        self
+          .inner
+          .write()
+          .await
+          .post_cache(!lockfile_skip_write)
+          .await;
         self.performance.measure(mark);
       }
       Err(err) => {
@@ -1254,7 +1261,9 @@ impl Inner {
             };
             specifier
           };
-          if let Err(err) = ls.cache(vec![specifier], referrer, false).await {
+          if let Err(err) =
+            ls.cache(vec![specifier], referrer, false, true).await
+          {
             lsp_warn!("{:#}", err);
           }
         });
@@ -1451,7 +1460,7 @@ impl Inner {
     self.task_queue.queue_task(Box::new(|ls: LanguageServer| {
       spawn(async move {
         if let Err(err) = ls
-          .cache(vec![], module.specifier.as_ref().clone(), false)
+          .cache(vec![], module.specifier.as_ref().clone(), false, true)
           .await
         {
           lsp_warn!(
@@ -4085,7 +4094,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
         serde_json::from_value(json!(params.arguments))
           .map_err(|err| LspError::invalid_params(err.to_string()))?;
       self
-        .cache(specifiers, referrer, options.force_global_cache)
+        .cache(specifiers, referrer, options.force_global_cache, false)
         .await
     } else if params.command == "deno.reloadImportRegistries" {
       *self.did_change_batch_queue.borrow_mut() = None;
@@ -4694,6 +4703,7 @@ impl Inner {
     specifiers: Vec<ModuleSpecifier>,
     referrer: ModuleSpecifier,
     force_global_cache: bool,
+    lockfile_skip_write: bool,
   ) -> Result<PrepareCacheResult, AnyError> {
     let config_data = self.config.tree.data_for_specifier(&referrer);
     let scope = config_data.map(|d| d.scope.clone());
@@ -4725,6 +4735,7 @@ impl Inner {
     let mut cli_factory = CliFactory::from_flags(Arc::new(Flags {
       internal: InternalFlags {
         cache_path: Some(self.cache.deno_dir().root.clone()),
+        lockfile_skip_write,
         ..Default::default()
       },
       ca_stores: workspace_settings.certificate_stores.clone(),
@@ -4776,9 +4787,22 @@ impl Inner {
   }
 
   #[cfg_attr(feature = "lsp-tracing", tracing::instrument(skip_all))]
-  async fn post_cache(&mut self) {
-    self.resolver.did_cache();
-    self.refresh_dep_info();
+  async fn post_cache(&mut self, did_write_lockfile: bool) {
+    if did_write_lockfile {
+      // Most of the refresh steps will happen in `did_change_watched_files()`,
+      // since the lockfile was written.
+      self.resolver.did_cache();
+      self.refresh_dep_info();
+    } else {
+      self.refresh_config_tree().await;
+      self.update_cache();
+      self.refresh_resolver().await;
+      self.refresh_compiler_options_resolver();
+      self.refresh_linter_resolver();
+      self.refresh_documents_config();
+      self.resolver.did_cache();
+      self.refresh_dep_info();
+    }
     self.project_changed(vec![], ProjectScopesChange::Config);
     self.ts_server.cleanup_semantic_cache(self.snapshot()).await;
     self.send_diagnostics_update();

tests/integration/lsp_tests.rs

@@ -6586,7 +6586,7 @@ fn lsp_cache_on_save() {
     .use_temp_cwd()
     .build();
   let temp_dir = context.temp_dir();
-  temp_dir.write(
+  let file = temp_dir.source_file(
     "file.ts",
     r#"
       import { printHello } from "http://localhost:4545/subdir/print_hello.ts";
@@ -6595,25 +6595,12 @@ fn lsp_cache_on_save() {
   );
   let mut client = context.new_lsp_command().build();
   client.initialize_default();
-  client.change_configuration(json!({
-    "deno": {
-      "enable": true,
-      "cacheOnSave": true,
-    },
-  }));
 
-  let diagnostics = client.did_open(json!({
-    "textDocument": {
-      "uri": url_to_uri(&temp_dir.url().join("file.ts").unwrap()).unwrap(),
-      "languageId": "typescript",
-      "version": 1,
-      "text": temp_dir.read_to_string("file.ts"),
-    }
-  }));
+  let diagnostics = client.did_open_file(&file);
   assert_eq!(
     diagnostics.messages_with_source("deno"),
     serde_json::from_value(json!({
-      "uri": url_to_uri(&temp_dir.url().join("file.ts").unwrap()).unwrap(),
+      "uri": file.uri(),
       "diagnostics": [{
         "range": {
           "start": { "line": 1, "character": 33 },
@@ -6630,10 +6617,13 @@ fn lsp_cache_on_save() {
     .unwrap()
   );
   client.did_save(json!({
-    "textDocument": { "uri": url_to_uri(&temp_dir.url().join("file.ts").unwrap()).unwrap() },
+    "textDocument": { "uri": file.uri() },
   }));
   client.handle_refresh_diagnostics_request();
-  assert_eq!(client.read_diagnostics().all(), vec![]);
+  assert_eq!(json!(client.read_diagnostics().all()), json!([]));
+
+  // Lockfiles should not be written from cache-on-save.
+  assert!(!temp_dir.path().join("deno.lock").exists());
 
   client.shutdown();
 }