Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
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:

```rust
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:

```rust
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:

```rust
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

---------

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>
This commit is contained in:
张小白 2025-01-20 03:07:39 +08:00 • committed by GitHub
parent 7578834d77
commit 7302be8ebd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 198 additions and 150 deletions
Download patch file Download diff file Expand all files Collapse all files

84
crates/project/src/project_tests.rs
View file

@ -781,11 +781,19 @@ async fn test_managing_language_servers(cx: &mut gpui::TestAppContext) {
#[gpui::test]
async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppContext) {
fn add_root_for_windows(path: &str) -> String {
if cfg!(windows) {
format!("C:{}", path)
} else {
path.to_string()
}
}
init_test(cx);
let fs = FakeFs::new(cx.executor());
fs.insert_tree(
"/the-root",
add_root_for_windows("/the-root"),
json!({
".gitignore": "target\n",
"src": {
@ -813,7 +821,7 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon
)
.await;
let project = Project::test(fs.clone(), ["/the-root".as_ref()], cx).await;
let project = Project::test(fs.clone(), [add_root_for_windows("/the-root").as_ref()], cx).await;
let language_registry = project.read_with(cx, |project, _| project.languages().clone());
language_registry.add(rust_lang());
let mut fake_servers = language_registry.register_fake_lsp(
@ -829,7 +837,7 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon
// Start the language server by opening a buffer with a compatible file extension.
let _ = project
.update(cx, |project, cx| {
project.open_local_buffer_with_lsp("/the-root/src/a.rs", cx)
project.open_local_buffer_with_lsp(add_root_for_windows("/the-root/src/a.rs"), cx)
})
.await
.unwrap();
@ -869,21 +877,21 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon
lsp::DidChangeWatchedFilesRegistrationOptions {
watchers: vec![
lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String(
"/the-root/Cargo.toml".to_string(),
),
glob_pattern: lsp::GlobPattern::String(add_root_for_windows(
"/the-root/Cargo.toml",
)),
kind: None,
},
lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String(
"/the-root/src/*.{rs,c}".to_string(),
),
glob_pattern: lsp::GlobPattern::String(add_root_for_windows(
"/the-root/src/*.{rs,c}",
)),
kind: None,
},
lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String(
"/the-root/target/y/**/*.rs".to_string(),
),
glob_pattern: lsp::GlobPattern::String(add_root_for_windows(
"/the-root/target/y/**/*.rs",
)),
kind: None,
},
],
@ -936,21 +944,36 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon
// Perform some file system mutations, two of which match the watched patterns,
// and one of which does not.
fs.create_file("/the-root/src/c.rs".as_ref(), Default::default())
.await
.unwrap();
fs.create_file("/the-root/src/d.txt".as_ref(), Default::default())
.await
.unwrap();
fs.remove_file("/the-root/src/b.rs".as_ref(), Default::default())
.await
.unwrap();
fs.create_file("/the-root/target/x/out/x2.rs".as_ref(), Default::default())
.await
.unwrap();
fs.create_file("/the-root/target/y/out/y2.rs".as_ref(), Default::default())
.await
.unwrap();
fs.create_file(
add_root_for_windows("/the-root/src/c.rs").as_ref(),
Default::default(),
)
.await
.unwrap();
fs.create_file(
add_root_for_windows("/the-root/src/d.txt").as_ref(),
Default::default(),
)
.await
.unwrap();
fs.remove_file(
add_root_for_windows("/the-root/src/b.rs").as_ref(),
Default::default(),
)
.await
.unwrap();
fs.create_file(
add_root_for_windows("/the-root/target/x/out/x2.rs").as_ref(),
Default::default(),
)
.await
.unwrap();
fs.create_file(
add_root_for_windows("/the-root/target/y/out/y2.rs").as_ref(),
Default::default(),
)
.await
.unwrap();
// The language server receives events for the FS mutations that match its watch patterns.
cx.executor().run_until_parked();
@ -958,15 +981,16 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon
&*file_changes.lock(),
&[
lsp::FileEvent {
uri: lsp::Url::from_file_path("/the-root/src/b.rs").unwrap(),
uri: lsp::Url::from_file_path(add_root_for_windows("/the-root/src/b.rs")).unwrap(),
typ: lsp::FileChangeType::DELETED,
},
lsp::FileEvent {
uri: lsp::Url::from_file_path("/the-root/src/c.rs").unwrap(),
uri: lsp::Url::from_file_path(add_root_for_windows("/the-root/src/c.rs")).unwrap(),
typ: lsp::FileChangeType::CREATED,
},
lsp::FileEvent {
uri: lsp::Url::from_file_path("/the-root/target/y/out/y2.rs").unwrap(),
uri: lsp::Url::from_file_path(add_root_for_windows("/the-root/target/y/out/y2.rs"))
.unwrap(),
typ: lsp::FileChangeType::CREATED,
},
]