fix(lsp): handle watched files events from symlinked config files (#19898)

Related to denoland/vscode_deno#784

Author: dsherret

cli/lsp/config.rs

@@ -425,12 +425,19 @@ pub struct Settings {
   pub workspace: WorkspaceSettings,
 }
 
+#[derive(Debug)]
+struct WithCanonicalizedSpecifier<T> {
+  /// Stored canonicalized specifier, which is used for file watcher events.
+  canonicalized_specifier: ModuleSpecifier,
+  file: T,
+}
+
 /// Contains the config file and dependent information.
 #[derive(Debug)]
 struct LspConfigFileInfo {
-  config_file: ConfigFile,
+  config_file: WithCanonicalizedSpecifier<ConfigFile>,
   /// An optional deno.lock file, which is resolved relative to the config file.
-  maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
+  maybe_lockfile: Option<WithCanonicalizedSpecifier<Arc<Mutex<Lockfile>>>>,
   /// The canonicalized node_modules directory, which is found relative to the config file.
   maybe_node_modules_dir: Option<PathBuf>,
   excluded_paths: Vec<Url>,
@@ -469,14 +476,42 @@ impl Config {
   }
 
   pub fn maybe_config_file(&self) -> Option<&ConfigFile> {
-    self.maybe_config_file_info.as_ref().map(|c| &c.config_file)
+    self
+      .maybe_config_file_info
+      .as_ref()
+      .map(|c| &c.config_file.file)
+  }
+
+  /// Canonicalized specifier of the config file, which should only be used for
+  /// file watcher events. Otherwise, prefer using the non-canonicalized path
+  /// as the rest of the CLI does for config files.
+  pub fn maybe_config_file_canonicalized_specifier(
+    &self,
+  ) -> Option<&ModuleSpecifier> {
+    self
+      .maybe_config_file_info
+      .as_ref()
+      .map(|c| &c.config_file.canonicalized_specifier)
   }
 
   pub fn maybe_lockfile(&self) -> Option<&Arc<Mutex<Lockfile>>> {
     self
       .maybe_config_file_info
       .as_ref()
-      .and_then(|c| c.maybe_lockfile.as_ref())
+      .and_then(|c| c.maybe_lockfile.as_ref().map(|l| &l.file))
+  }
+
+  /// Canonicalized specifier of the lockfile, which should only be used for
+  /// file watcher events. Otherwise, prefer using the non-canonicalized path
+  /// as the rest of the CLI does for config files.
+  pub fn maybe_lockfile_canonicalized_specifier(
+    &self,
+  ) -> Option<&ModuleSpecifier> {
+    self.maybe_config_file_info.as_ref().and_then(|c| {
+      c.maybe_lockfile
+        .as_ref()
+        .map(|l| &l.canonicalized_specifier)
+    })
   }
 
   pub fn clear_config_file(&mut self) {
@@ -485,7 +520,17 @@ impl Config {
 
   pub fn set_config_file(&mut self, config_file: ConfigFile) {
     self.maybe_config_file_info = Some(LspConfigFileInfo {
-      maybe_lockfile: resolve_lockfile_from_config(&config_file),
+      maybe_lockfile: resolve_lockfile_from_config(&config_file).map(
+        |lockfile| {
+          let path = canonicalize_path_maybe_not_exists(&lockfile.filename)
+            .unwrap_or_else(|_| lockfile.filename.clone());
+          WithCanonicalizedSpecifier {
+            canonicalized_specifier: ModuleSpecifier::from_file_path(path)
+              .unwrap(),
+            file: Arc::new(Mutex::new(lockfile)),
+          }
+        },
+      ),
       maybe_node_modules_dir: resolve_node_modules_dir(&config_file),
       excluded_paths: config_file
         .to_files_config()
@@ -496,7 +541,16 @@ impl Config {
         .into_iter()
         .filter_map(|path| ModuleSpecifier::from_file_path(path).ok())
         .collect(),
-      config_file,
+      config_file: WithCanonicalizedSpecifier {
+        canonicalized_specifier: config_file
+          .specifier
+          .to_file_path()
+          .ok()
+          .and_then(|p| canonicalize_path_maybe_not_exists(&p).ok())
+          .and_then(|p| ModuleSpecifier::from_file_path(p).ok())
+          .unwrap_or_else(|| config_file.specifier.clone()),
+        file: config_file,
+      },
     });
   }
 
@@ -739,9 +793,7 @@ fn specifier_enabled(
     .unwrap_or_else(|| settings.workspace.enable)
 }
 
-fn resolve_lockfile_from_config(
-  config_file: &ConfigFile,
-) -> Option<Arc<Mutex<Lockfile>>> {
+fn resolve_lockfile_from_config(config_file: &ConfigFile) -> Option<Lockfile> {
   let lockfile_path = match config_file.resolve_lockfile_path() {
     Ok(Some(value)) => value,
     Ok(None) => return None,
@@ -769,15 +821,13 @@ fn resolve_node_modules_dir(config_file: &ConfigFile) -> Option<PathBuf> {
   canonicalize_path_maybe_not_exists(&node_modules_dir).ok()
 }
 
-fn resolve_lockfile_from_path(
-  lockfile_path: PathBuf,
-) -> Option<Arc<Mutex<Lockfile>>> {
+fn resolve_lockfile_from_path(lockfile_path: PathBuf) -> Option<Lockfile> {
   match Lockfile::new(lockfile_path, false) {
     Ok(value) => {
       if let Ok(specifier) = ModuleSpecifier::from_file_path(&value.filename) {
         lsp_log!("  Resolved lock file: \"{}\"", specifier);
       }
-      Some(Arc::new(Mutex::new(value)))
+      Some(value)
     }
     Err(err) => {
       lsp_warn!("Error loading lockfile: {:#}", err);

cli/lsp/language_server.rs

@@ -1458,18 +1458,8 @@ impl Inner {
     &mut self,
     params: DidChangeWatchedFilesParams,
   ) {
-    fn has_lockfile_changed(
-      lockfile: &Lockfile,
-      changed_urls: &HashSet<Url>,
-    ) -> bool {
-      let lockfile_path = lockfile.filename.clone();
-      let Ok(specifier) = ModuleSpecifier::from_file_path(&lockfile_path) else {
-        return false;
-      };
-      if !changed_urls.contains(&specifier) {
-        return false;
-      }
-      match Lockfile::new(lockfile_path, false) {
+    fn has_lockfile_content_changed(lockfile: &Lockfile) -> bool {
+      match Lockfile::new(lockfile.filename.clone(), false) {
         Ok(new_lockfile) => {
           // only update if the lockfile has changed
           FastInsecureHasher::hash(lockfile)
@@ -1482,6 +1472,53 @@ impl Inner {
       }
     }
 
+    fn has_config_changed(config: &Config, changes: &HashSet<Url>) -> bool {
+      // Check the canonicalized specifier here because file watcher
+      // changes will be for the canonicalized path in vscode, but also check the
+      // non-canonicalized specifier in order to please the tests and handle
+      // a client that might send that instead.
+      if config
+        .maybe_config_file_canonicalized_specifier()
+        .map(|s| changes.contains(s))
+        .unwrap_or(false)
+      {
+        return true;
+      }
+      match config.maybe_config_file() {
+        Some(file) => {
+          if changes.contains(&file.specifier) {
+            return true;
+          }
+        }
+        None => {
+          // check for auto-discovery
+          if changes.iter().any(|url| {
+            url.path().ends_with("/deno.json")
+              || url.path().ends_with("/deno.jsonc")
+          }) {
+            return true;
+          }
+        }
+      }
+
+      // if the lockfile has changed, reload the config as well
+      if let Some(lockfile) = config.maybe_lockfile() {
+        let lockfile_matches = config
+          .maybe_lockfile_canonicalized_specifier()
+          .map(|s| changes.contains(s))
+          .or_else(|| {
+            ModuleSpecifier::from_file_path(&lockfile.lock().filename)
+              .ok()
+              .map(|s| changes.contains(&s))
+          })
+          .unwrap_or(false);
+        lockfile_matches && has_lockfile_content_changed(&lockfile.lock())
+      } else {
+        // check for auto-discovery
+        changes.iter().any(|url| url.path().ends_with("/deno.lock"))
+      }
+    }
+
     let mark = self
       .performance
       .mark("did_change_watched_files", Some(&params));
@@ -1493,23 +1530,7 @@ impl Inner {
       .collect();
 
     // if the current deno.json has changed, we need to reload it
-    let has_config_changed = match self.config.maybe_config_file() {
-      Some(config_file) => changes.contains(&config_file.specifier),
-      None => {
-        // check for auto-discovery
-        changes.iter().any(|url| {
-          url.path().ends_with("/deno.json")
-            || url.path().ends_with("/deno.jsonc")
-        })
-      }
-    } || match self.config.maybe_lockfile() {
-      Some(lockfile) => has_lockfile_changed(&lockfile.lock(), &changes),
-      None => {
-        // check for auto-discovery
-        changes.iter().any(|url| url.path().ends_with("/deno.lock"))
-      }
-    };
-    if has_config_changed {
+    if has_config_changed(&self.config, &changes) {
       if let Err(err) = self.update_config_file().await {
         self.client.show_message(MessageType::WARNING, err);
       }

cli/tests/integration/lsp_tests.rs

@@ -544,6 +544,72 @@ fn lsp_import_map_config_file_auto_discovered() {
   client.shutdown();
 }
 
+#[test]
+fn lsp_import_map_config_file_auto_discovered_symlink() {
+  let context = TestContextBuilder::new()
+    // DO NOT COPY THIS CODE. Very rare case where we want to force the temp
+    // directory on the CI to not be a symlinked directory because we are
+    // testing a scenario with a symlink to a non-symlink in the same directory
+    // tree. Generally you want to ensure your code works in symlinked directories
+    // so don't use this unless you have a similar scenario.
+    .temp_dir_path(std::env::temp_dir().canonicalize().unwrap())
+    .use_temp_cwd()
+    .build();
+  let temp_dir = context.temp_dir();
+  temp_dir.create_dir_all("lib");
+  temp_dir.write("lib/b.ts", r#"export const b = "b";"#);
+
+  let mut client = context.new_lsp_command().capture_stderr().build();
+  client.initialize_default();
+
+  // now create a symlink in the current directory to a subdir/deno.json
+  // and ensure the watched files notification still works
+  temp_dir.create_dir_all("subdir");
+  temp_dir.write("subdir/deno.json", r#"{ }"#);
+  temp_dir.symlink_file(
+    temp_dir.path().join("subdir").join("deno.json"),
+    temp_dir.path().join("deno.json"),
+  );
+  client.did_change_watched_files(json!({
+    "changes": [{
+      // the client will give a watched file changed event for the symlink's target
+      "uri": temp_dir.path().join("subdir/deno.json").canonicalize().uri(),
+      "type": 2
+    }]
+  }));
+
+  // this will discover the deno.json in the root
+  let search_line = format!(
+    "Auto-resolved configuration file: \"{}\"",
+    temp_dir.uri().join("deno.json").unwrap().as_str()
+  );
+  client.wait_until_stderr_line(|line| line.contains(&search_line));
+
+  // now open a file which will cause a diagnostic because the import map is empty
+  let diagnostics = client.did_open(json!({
+    "textDocument": {
+      "uri": temp_dir.uri().join("a.ts").unwrap(),
+      "languageId": "typescript",
+      "version": 1,
+      "text": "import { b } from \"/~/b.ts\";\n\nconsole.log(b);\n"
+    }
+  }));
+  assert_eq!(diagnostics.all().len(), 1);
+
+  // update the import map to have the imports now
+  temp_dir.write("subdir/deno.json", r#"{ "imports": { "/~/": "./lib/" } }"#);
+  client.did_change_watched_files(json!({
+    "changes": [{
+      // now still say that the target path has changed
+      "uri": temp_dir.path().join("subdir/deno.json").canonicalize().uri(),
+      "type": 2
+    }]
+  }));
+  assert_eq!(client.read_diagnostics().all().len(), 0);
+
+  client.shutdown();
+}
+
 #[test]
 fn lsp_deno_task() {
   let context = TestContextBuilder::new().use_temp_cwd().build();

test_util/src/builders.rs

@@ -19,7 +19,6 @@ use crate::env_vars_for_npm_tests_no_sync_download;
 use crate::fs::PathRef;
 use crate::http_server;
 use crate::lsp::LspClientBuilder;
-use crate::new_deno_dir;
 use crate::pty::Pty;
 use crate::strip_ansi_codes;
 use crate::testdata_path;
@@ -36,6 +35,7 @@ pub struct TestContextBuilder {
   /// to the temp folder and runs the test from there. This is useful when
   /// the test creates files in the testdata directory (ex. a node_modules folder)
   copy_temp_dir: Option<String>,
+  temp_dir_path: Option<PathBuf>,
   cwd: Option<String>,
   envs: HashMap<String, String>,
   deno_exe: Option<PathRef>,
@@ -50,6 +50,11 @@ impl TestContextBuilder {
     Self::new().use_http_server().add_npm_env_vars()
   }
 
+  pub fn temp_dir_path(mut self, path: impl AsRef<Path>) -> Self {
+    self.temp_dir_path = Some(path.as_ref().to_path_buf());
+    self
+  }
+
   pub fn use_http_server(mut self) -> Self {
     self.use_http_server = true;
     self
@@ -117,9 +122,13 @@ impl TestContextBuilder {
   }
 
   pub fn build(&self) -> TestContext {
-    let deno_dir = new_deno_dir(); // keep this alive for the test
+    let temp_dir_path = self
+      .temp_dir_path
+      .clone()
+      .unwrap_or_else(std::env::temp_dir);
+    let deno_dir = TempDir::new_in(&temp_dir_path);
     let temp_dir = if self.use_separate_deno_dir {
-      TempDir::new()
+      TempDir::new_in(&temp_dir_path)
     } else {
       deno_dir.clone()
     };

test_util/src/fs.rs

@@ -324,6 +324,10 @@ impl TempDir {
     Self::new_inner(&std::env::temp_dir(), Some(prefix))
   }
 
+  pub fn new_in(parent_dir: &Path) -> Self {
+    Self::new_inner(parent_dir, None)
+  }
+
   pub fn new_with_path(path: &Path) -> Self {
     Self(Arc::new(TempDirInner::Path(PathRef(path.to_path_buf()))))
   }

test_util/src/lsp.rs

@@ -641,16 +641,24 @@ impl LspClient {
       .stderr_lines_rx
       .as_ref()
       .expect("must setup with client_builder.capture_stderr()");
+    let mut found_lines = Vec::new();
     while Instant::now() < timeout_time {
       if let Ok(line) = lines_rx.try_recv() {
         if condition(&line) {
           return;
         }
+        found_lines.push(line);
       }
       std::thread::sleep(Duration::from_millis(20));
     }
 
-    panic!("Timed out.")
+    eprintln!("==== STDERR OUTPUT ====");
+    for line in found_lines {
+      eprintln!("{}", line)
+    }
+    eprintln!("== END STDERR OUTPUT ==");
+
+    panic!("Timed out waiting on condition.")
   }
 
   pub fn initialize_default(&mut self) {