Absolutize LSP and DAP paths more conservatively (#42482)

Andrew Farkas created 2 weeks ago

Fixes a regression caused by #42135 where LSP and DAP binaries weren't
being used from `PATH` env var

Now we absolutize the path if (path is relative AND (path has multiple
components OR path exists in worktree)).

- Relative paths with multiple components might not exist in the
worktree because they are ignored. Paths with a single component will at
least have an entry saying that they exist and are ignored.
- Relative paths with multiple components will never use the `PATH` env
var, so they can be safely absolutized

Release Notes:

- N/A

Change summary

crates/language/src/language.rs          |  1 
crates/project/src/debugger/dap_store.rs |  4 +-
crates/project/src/lsp_store.rs          |  7 ++++-
crates/project/src/project_tests.rs      | 29 ++++++++++++++++++++-----
crates/worktree/src/worktree.rs          | 21 ++++++++++++++++++
5 files changed, 52 insertions(+), 10 deletions(-)

Detailed changes

crates/language/src/language.rs

@@ -291,6 +291,7 @@ pub trait LspAdapterDelegate: Send + Sync {
     fn http_client(&self) -> Arc<dyn HttpClient>;
     fn worktree_id(&self) -> WorktreeId;
     fn worktree_root_path(&self) -> &Path;
+    fn resolve_executable_path(&self, path: PathBuf) -> PathBuf;
     fn update_status(&self, language: LanguageServerName, status: BinaryStatus);
     fn registered_lsp_adapters(&self) -> Vec<Arc<dyn LspAdapter>>;
     async fn language_server_download_dir(&self, name: &LanguageServerName) -> Option<Arc<Path>>;

crates/project/src/debugger/dap_store.rs

@@ -262,8 +262,8 @@ impl DapStore {
                 let user_installed_path = dap_settings.and_then(|s| match &s.binary {
                     DapBinary::Default => None,
                     DapBinary::Custom(binary) => {
-                        // if `binary` is absolute, `.join()` will keep it unmodified
-                        Some(worktree.read(cx).abs_path().join(PathBuf::from(binary)))
+                        let path = PathBuf::from(binary);
+                        Some(worktree.read(cx).resolve_executable_path(path))
                     }
                 });
                 let user_args = dap_settings.map(|s| s.args.clone());

crates/project/src/lsp_store.rs

@@ -573,8 +573,7 @@ impl LocalLspStore {
                 env.extend(settings.env.unwrap_or_default());
 
                 Ok(LanguageServerBinary {
-                    // if `path` is absolute, `.join()` will keep it unmodified
-                    path: delegate.worktree_root_path().join(path),
+                    path: delegate.resolve_executable_path(path),
                     env: Some(env),
                     arguments: settings
                         .arguments
@@ -13516,6 +13515,10 @@ impl LspAdapterDelegate for LocalLspAdapterDelegate {
         self.worktree.abs_path().as_ref()
     }
 
+    fn resolve_executable_path(&self, path: PathBuf) -> PathBuf {
+        self.worktree.resolve_executable_path(path)
+    }
+
     async fn shell_env(&self) -> HashMap<String, String> {
         let task = self.load_shell_env_task.clone();
         task.await.unwrap_or_default()

crates/project/src/project_tests.rs

@@ -1215,13 +1215,20 @@ async fn test_language_server_relative_path(cx: &mut gpui::TestAppContext) {
     let settings_json_contents = json!({
         "languages": {
             "Rust": {
-                "language_servers": ["my_fake_lsp"]
+                "language_servers": ["my_fake_lsp", "lsp_on_path"]
             }
         },
         "lsp": {
             "my_fake_lsp": {
                 "binary": {
-                    "path": path!("relative_path/to/my_fake_lsp_binary.exe").to_string(),
+                    // file exists, so this is treated as a relative path
+                    "path": path!(".relative_path/to/my_fake_lsp_binary.exe").to_string(),
+                }
+            },
+            "lsp_on_path": {
+                "binary": {
+                    // file doesn't exist, so it will fall back on PATH env var
+                    "path": path!("lsp_on_path.exe").to_string(),
                 }
             }
         },
@@ -1234,7 +1241,7 @@ async fn test_language_server_relative_path(cx: &mut gpui::TestAppContext) {
             ".zed": {
                 "settings.json": settings_json_contents.to_string(),
             },
-            "relative_path": {
+            ".relative_path": {
                 "to": {
                     "my_fake_lsp.exe": "",
                 },
@@ -1250,13 +1257,20 @@ async fn test_language_server_relative_path(cx: &mut gpui::TestAppContext) {
     let language_registry = project.read_with(cx, |project, _| project.languages().clone());
     language_registry.add(rust_lang());
 
-    let mut fake_rust_servers = language_registry.register_fake_lsp(
+    let mut my_fake_lsp = language_registry.register_fake_lsp(
         "Rust",
         FakeLspAdapter {
             name: "my_fake_lsp",
             ..Default::default()
         },
     );
+    let mut lsp_on_path = language_registry.register_fake_lsp(
+        "Rust",
+        FakeLspAdapter {
+            name: "lsp_on_path",
+            ..Default::default()
+        },
+    );
 
     cx.run_until_parked();
 
@@ -1268,11 +1282,14 @@ async fn test_language_server_relative_path(cx: &mut gpui::TestAppContext) {
         .await
         .unwrap();
 
-    let lsp_path = fake_rust_servers.next().await.unwrap().binary.path;
+    let lsp_path = my_fake_lsp.next().await.unwrap().binary.path;
     assert_eq!(
         lsp_path.to_string_lossy(),
-        path!("/the-root/relative_path/to/my_fake_lsp_binary.exe"),
+        path!("/the-root/.relative_path/to/my_fake_lsp_binary.exe"),
     );
+
+    let lsp_path = lsp_on_path.next().await.unwrap().binary.path;
+    assert_eq!(lsp_path.to_string_lossy(), path!("lsp_on_path.exe"));
 }
 
 #[gpui::test]

crates/worktree/src/worktree.rs

@@ -2384,6 +2384,27 @@ impl Snapshot {
             })
     }
 
+    /// Resolves a path to an executable using the following heuristics:
+    ///
+    /// 1. If the path is relative and contains more than one component,
+    ///    it is joined to the worktree root path.
+    /// 2. If the path is relative and exists in the worktree
+    ///    (even if falls under an exclusion filter),
+    ///    it is joined to the worktree root path.
+    /// 3. Otherwise the path is returned unmodified.
+    ///
+    /// Relative paths that do not exist in the worktree may
+    /// still be found using the `PATH` environment variable.
+    pub fn resolve_executable_path(&self, path: PathBuf) -> PathBuf {
+        if let Ok(rel_path) = RelPath::new(&path, self.path_style)
+            && (path.components().count() > 1 || self.entry_for_path(&rel_path).is_some())
+        {
+            self.abs_path().join(path)
+        } else {
+            path
+        }
+    }
+
     pub fn entry_for_id(&self, id: ProjectEntryId) -> Option<&Entry> {
         let entry = self.entries_by_id.get(&id, ())?;
         self.entry_for_path(&entry.path)