LSP (clangd) actions open duplicate buffers

[jazzfool]

When I perform navigational code actions through the LSP (clangd), e.g., gd (goto definition), it opens a duplicate buffer with the absolute path rather than the path relative to pwd. This buffer is separate from the original buffer even though they refer to same file, so I can unknowingly write changes to this duplicate buffer, then overwrite those changes in the original buffer, which is frustrating since I end up losing my work.

For example, within buffer "src/code.cpp" -> gd -> opens buffer "C:/full/path/to/src/code.cpp" (cursor at the definition).

I expect the correct behavior to be for it to resolve the LSP's requested file path relative to pwd such that
a) It doesn't open the buffer with an absolute path if it's under pwd and
b) subsequently, doesn't open a duplicate buffer

[stabor705]

Is it possible that this is windows only issue? Have no problem like that on linux.

[jazzfool]

I tried this with Linux and yes, it does seem to be on Windows only, and specific to clangd

[pascalkuthe]

We do automatically canonicalize paths so I suspect that this is a windows issue. This looks a bit like #6182 but in this case it shouldn't apply because we are converting the URLs to paths (which ignore prefix casing). I suspect this is some special case where canonicalization either fails or returns inconsistent results. Is this some kind of specical path (network path or something else)? Perhaphs cannonicalization fails in some way. Could you share the exact path that you started helix in and the exact path clangd opens?

We hand-roll our own canonicalization because the one from standard library is somehwat of a footgun. I think we are missing case cannonicalization here tough so that might be the cause of the problem. I think we might need to switch to https://docs.rs/normpath/latest/normpath/index.html

[jazzfool]

I did some testing and can confirm it seems to be associated with path casing.

mkdir Test then in helix cd Q:\test, o file.cpp, gd, opens Q:\Test\file.cpp (notice the casing difference!)

However, if I instead in helix cd into cd Q:\Test, this issue does not occur.

So essentially you are correct, if the casing of pwd in helix differs from the filesystem, path canonicalization fails.

[pascalkuthe]

Thanks for confirming. The right way to fix this is to implement a better canonicalization strategy here:

helix/helix-core/src/path.rs

Line 42 in 4f066b1

pub fn get_normalized_path(path: &Path) -> PathBuf {

. Currently we never touch the filesystem and just so some "text" manipulation.

Howeber the way to correctly normalize a path is to obtain a file descriptor and to turn that file descriptor back into a path. Path equality is fundamentally file system dependent so even on windows different drives might have different canonicalization strategies.

Luckily this is exactly what the std Path::cannonicalize function does. We don't use it because:

• It returns weird looking (but technically more correct) paths on windows (called UNC paths)
• it doesn't work for paths that don't exist.

The first problem.can be resolved by using https://docs.rs/dunce/latest/dunce/fn.canonicalize.html instead of std cannonicalize (it would be great if the standard library adopted but not holding my breath for that). Under the hood it cannonicalizess the path with std and (on windows) it then simplifies the result back to a traditional path if possible.

For the second problem I think the right way to fix this is to: Walk up the pathsancestors with Path::ancestors and try to cannonicalization and if it succeeds return the result of canonicalization with the rest of the path (so the part that doesn't exist yet) normalized with our existing implementation joined on top.
So essentially cannonicalize the first parent that does exist (or the file itself if it exists) and then do pur current cannonicalization for the part of the path that doesn't exist yet and join the result on top of the cannonicalized base pathm

This implementation also has the added bonus of properly handeling softlinks which our currently strategy also doesn't do.

Should.not be hard to send a PR to implement what I described

[jazzfool]

I had a look through the relevant code and I don't think canonicalization is actually the issue here. The issue is just that strip_prefix (in get_relative_path) is case-sensitive and therefore fails if std::env::current_dir is cased differently to what the LSP returns. See my fix here: #6290