Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lsp: Use Path instead of String for path handling #22762

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JunkuiZhang
Copy link
Contributor

During my work on PR #22616, while trying to fix the test_reporting_fs_changes_to_language_servers test case, I noticed that we are currently handling paths using String in some places. However, this approach causes issues on Windows.

This draft PR modifies rebuild_watched_paths_inner and glob_literal_prefix. For example, take the glob_literal_prefix function modified in this PR:

assert_eq!(
    glob_literal_prefix("node_modules/**/*.js"), 
    "node_modules"
);    // This works on Unix, fails on Windows

assert_eq!(
    glob_literal_prefix("node_modules\\**\\*.js"), 
    "node_modules"
);    // This works on Windows

assert_eq!(
    glob_literal_prefix("node_modules\\**/*.js"), 
    "node_modules"
);    // This fails on Windows

The current implementation treats path as String and relies on \ as the path separator on Windows, but on Windows, both / and \ can be used as separators. This means that node_modules\**/*.js is also a valid path representation.

There are two potential solutions to this issue:

  1. Continue handling paths with String, and on Windows, replace all / with \.
  2. Use Path for path handling, which is the solution implemented in this PR.

Advantages of Solution 1:

  • Simple and direct.

Advantages of Solution 2:

  • More robust, especially in handling strip_prefix.

Currently, the logic for removing a path prefix looks like this:

let path = "/some/path/to/file.rs";
let parent = "/some/path/to";
// remove prefix
let file = path.strip_prefix(parent).unwrap();    // which is `/file.rs`
let file = file.strip_prefix("/").unwrap();

However, using Path simplifies this process and makes it more robust:

let path = Path::new("C:/path/to/src/main.rs");
let parent = Path::new("C:/path/to/src"); 
let file = path.strip_prefix(&parent).unwrap(); // which is `main.rs`

let path = Path::new("C:\\path\\to/src/main.rs");
let parent = Path::new("C:/path/to\\src\\"); 
let file = path.strip_prefix(&parent).unwrap(); // which is `main.rs`

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 7, 2025
@maxdeviant maxdeviant changed the title Lsp: Use Path instead of String for path handling lsp: Use Path instead of String for path handling Jan 7, 2025
Comment on lines +2592 to +2596
let path = if base_uri.components().next().is_none() {
// TODO:
// why just return the root path?
// how should we handle this on Windows?
Arc::from(Path::new("/"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I dont know how to handle this on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant