Merge 9d1adccf62 into 1315b7e2b1

helix-core/src/uri.rs

@@ -80,7 +80,7 @@ impl std::error::Error for UrlConversionError {}
 fn convert_url_to_uri(url: &url::Url) -> Result<Uri, UrlConversionErrorKind> {
     if url.scheme() == "file" {
         url.to_file_path()
-            .map(|path| Uri::File(helix_stdx::path::normalize(path).into()))
+            .map(|path| Uri::File(helix_stdx::path::canonicalize(path).into()))
             .map_err(|_| UrlConversionErrorKind::UnableToConvert)
     } else {
         Err(UrlConversionErrorKind::UnsupportedScheme)

helix-lsp/src/client.rs

@@ -72,8 +72,12 @@ impl Client {
         doc_path: Option<&std::path::PathBuf>,
         may_support_workspace: bool,
     ) -> bool {
-        let (workspace, workspace_is_cwd) = find_workspace();
+        let (workspace_path, workspace_is_cwd) = find_workspace();
-        let workspace = path::normalize(workspace);
+        // Attempt to canonicalize the workspace path. Fallback to original if error.
+        let workspace = match helix_stdx::path::canonicalize(&workspace_path) {
+            Ok(p) => p,
+            Err(_) => workspace_path, // Or handle error more explicitly, e.g. return false
+        };
         let root = find_lsp_workspace(
             doc_path
                 .and_then(|x| x.parent().and_then(|x| x.to_str()))

helix-lsp/src/lib.rs

@@ -872,7 +872,7 @@ fn start_client(
     enable_snippets: bool,
 ) -> Result<NewClient, StartupError> {
     let (workspace, workspace_is_cwd) = helix_loader::find_workspace();
-    let workspace = path::normalize(workspace);
+    let workspace = helix_stdx::path::canonicalize(&workspace).map_err(|err| StartupError::Error(anyhow::anyhow!("failed to canonicalize workspace {:?}: {}", workspace, err)))?;
     let root = find_lsp_workspace(
         doc_path
             .and_then(|x| x.parent().and_then(|x| x.to_str()))
@@ -885,8 +885,8 @@ fn start_client(
 
     // `root_uri` and `workspace_folder` can be empty in case there is no workspace
     // `root_url` can not, use `workspace` as a fallback
-    let root_path = root.clone().unwrap_or_else(|| workspace.clone());
+    let root_path = root.clone().unwrap_or_else(|| workspace.clone()); // workspace is now canonical
-    let root_uri = root.and_then(|root| lsp::Url::from_file_path(root).ok());
+    let root_uri = root.as_ref().and_then(|r| lsp::Url::from_file_path(r).ok()); // root is now canonical
 
     if let Some(globset) = &ls_config.required_root_patterns {
         if !root_path
@@ -956,46 +956,62 @@ pub fn find_lsp_workspace(
     workspace: &Path,
     workspace_is_cwd: bool,
 ) -> Option<PathBuf> {
-    let file = std::path::Path::new(file);
+    let file_path = std::path::Path::new(file);
-    let mut file = if file.is_absolute() {
+    let initial_file_path = if file_path.is_absolute() {
-        file.to_path_buf()
+        file_path.to_path_buf()
     } else {
         let current_dir = helix_stdx::env::current_working_dir();
-        current_dir.join(file)
+        current_dir.join(file_path)
     };
-    file = path::normalize(&file);
 
+    // Attempt to canonicalize the initial file path. If it fails, we might not be able to proceed.
+    // For now, let's return None if canonicalization of the base file path fails.
+    let file = match helix_stdx::path::canonicalize(&initial_file_path) {
+        Ok(p) => p,
+        Err(_) => return None, // Or handle error more explicitly
+    };
+
+    // Workspace itself should be canonical. The caller (start_client) ensures this.
     if !file.starts_with(workspace) {
         return None;
     }
 
-    let mut top_marker = None;
+    let mut top_marker_path = None;
     for ancestor in file.ancestors() {
+        // Canonicalize ancestor before joining and checking for markers/root_dirs
+        // However, ancestor itself is already part of a canonical path `file`.
+        // The paths joined (marker, root_dir) should be relative to a canonical ancestor.
+        // And `workspace.join(root_dir)` should be canonicalized for comparison.
+
         if root_markers
             .iter()
             .any(|marker| ancestor.join(marker).exists())
         {
-            top_marker = Some(ancestor);
+            // Store the canonical ancestor if a marker is found
+            top_marker_path = Some(ancestor.to_path_buf());
         }
 
-        if root_dirs
+        if root_dirs.iter().any(|root_dir| {
-            .iter()
+            match helix_stdx::path::canonicalize(&workspace.join(root_dir)) {
-            .any(|root_dir| path::normalize(workspace.join(root_dir)) == ancestor)
+                Ok(canonical_root_dir) => canonical_root_dir == ancestor,
-        {
+                Err(_) => false,
-            // if the worskapce is the cwd do not search any higher for workspaces
+            }
-            // but specify
+        }) {
-            return Some(top_marker.unwrap_or(workspace).to_owned());
+            // If a root_dir matches the current ancestor, decide what to return.
+            // The path returned must be canonical.
+            return top_marker_path.or_else(|| Some(ancestor.to_path_buf()));
         }
+
         if ancestor == workspace {
-            // if the workspace is the CWD, let the LSP decide what the workspace
+            // If we've reached the workspace root.
-            // is
+            // `top_marker_path` would be a canonical path if set.
-            return top_marker
+            // `workspace` is already canonical.
-                .or_else(|| (!workspace_is_cwd).then_some(workspace))
+            return top_marker_path
-                .map(Path::to_owned);
+                .or_else(|| (!workspace_is_cwd).then_some(workspace.to_path_buf()));
         }
     }
 
-    debug_assert!(false, "workspace must be an ancestor of <file>");
+    debug_assert!(false, "workspace must be an ancestor of <file> or logic error in loop");
     None
 }
 

helix-term/tests/integration.rs

@@ -22,4 +22,5 @@ mod test {
     mod languages;
     mod movement;
     mod splits;
+    mod lsp;
 }

helix-term/tests/lsp.rs

@@ -0,0 +1,84 @@
+#[cfg(all(feature = "integration", unix))] // Keep cfg for consistency, though it won't be an integration test
+mod test {
+    use std::{fs, path::PathBuf, os::unix::fs::symlink};
+    use tempfile::tempdir;
+    use indoc::indoc;
+    use helix_lsp::lsp::Url; // Only Url needed for this simplified test
+
+    // Note: This is no longer an async tokio test.
+    // Note: This does not use AppBuilder or Application.
+    #[test]
+    fn verify_symlink_canonicalization_for_uri() -> anyhow::Result<()> {
+        println!("--- Test verify_symlink_canonicalization_for_uri started ---");
+
+        // 1. Create a temporary directory for test files.
+        let temp_dir = tempdir()?;
+        let dir_path = temp_dir.path();
+        println!("Temporary directory created: {:?}", dir_path);
+
+        // 2. Inside this directory:
+        //    a. Create original_file.py
+        let original_file_path = dir_path.join("original_file.py");
+        let python_content = indoc! {r#"
+            def my_function():
+                pass
+
+            my_function()
+        "#};
+        fs::write(&original_file_path, python_content)?;
+        println!("original_file.py created at: {:?}", original_file_path);
+
+        //    b. Create a symlink linked_file.py pointing to original_file.py
+        let linked_file_path = dir_path.join("linked_file.py");
+        symlink(&original_file_path, &linked_file_path)?;
+        println!("linked_file.py created at: {:?}, pointing to {:?}", linked_file_path, original_file_path);
+
+        // Core Logic Verification:
+        // Get the canonical path for both the original and the symlinked file.
+        let canonical_original_path = original_file_path.canonicalize()?;
+        let canonical_linked_path = linked_file_path.canonicalize()?;
+        println!("Canonical original path: {:?}", canonical_original_path);
+        println!("Canonical linked path: {:?}", canonical_linked_path);
+
+        // Assert that the canonical paths are the same.
+        assert_eq!(canonical_original_path, canonical_linked_path, "Canonical paths of original and symlink should be identical.");
+        println!("Assertion 1 passed: Canonical paths are identical.");
+
+        // Convert these canonical paths to file URIs.
+        let uri_from_original = Url::from_file_path(canonical_original_path).map_err(|_| anyhow::anyhow!("Failed to create URI from original path"))?;
+        let uri_from_linked = Url::from_file_path(canonical_linked_path).map_err(|_| anyhow::anyhow!("Failed to create URI from linked path"))?;
+        println!("URI from original's canonical path: {:?}", uri_from_original);
+        println!("URI from linked file's canonical path: {:?}", uri_from_linked);
+        
+        // Assert that the URIs are the same.
+        assert_eq!(uri_from_original, uri_from_linked, "URIs from canonical paths should be identical.");
+        println!("Assertion 2 passed: URIs from canonical paths are identical.");
+        
+        // Also, check if creating a URI from the non-canonical symlink path,
+        // and then canonicalizing the path from *that* URI (if possible, though Url doesn't directly do that),
+        // would match. The key is that `helix_stdx::path::canonicalize` should be used *before* Uri creation
+        // as per the original subtask that modified `convert_url_to_uri`.
+
+        // The original change was: Uri::File(helix_stdx::path::canonicalize(path).into())
+        // So, if we simulate this:
+        // 1. Path comes from url.to_file_path() - this would be /path/to/linked_file.py
+        // 2. Then helix_stdx::path::canonicalize is applied to it.
+        
+        let path_from_symlink_url = linked_file_path; // Simulating url.to_file_path() for the symlink
+        let canonicalized_path_for_uri_construction = helix_stdx::path::canonicalize(path_from_symlink_url).map_err(|e| anyhow::anyhow!("helix_stdx::path::canonicalize failed: {}",e))?;
+        println!("Path from symlink after helix_stdx::path::canonicalize: {:?}", canonicalized_path_for_uri_construction);
+
+        assert_eq!(canonicalized_path_for_uri_construction, canonical_original_path, "helix_stdx::path::canonicalize(symlink_path) should yield original's canonical path.");
+        println!("Assertion 3 passed: helix_stdx::path::canonicalize(symlink_path) is correct.");
+
+        let constructed_uri = Url::from_file_path(canonicalized_path_for_uri_construction).map_err(|_| anyhow::anyhow!("Failed to create URI from stdx canonicalized path"))?;
+        assert_eq!(constructed_uri, uri_from_original, "URI constructed using helix_stdx::path::canonicalize should match original's canonical URI.");
+        println!("Assertion 4 passed: Final URI construction matches.");
+
+
+        // Clean up the temporary directory
+        temp_dir.close()?;
+        println!("--- Test verify_symlink_canonicalization_for_uri finished ---");
+        Ok(())
+    }
+}