Case-sensitive path handling is error prone on Win/Mac

clangd / clangd

clangd language server

https://clangd.llvm.org

Apache License 2.0

1.52k stars 63 forks source link

Case-sensitive path handling is error prone on Win/Mac #108

Open sam-mccall opened 5 years ago

sam-mccall commented 5 years ago

Windows and (most) Mac OS filesystems are case-insensitive but case-preserving.

We use std::string/StringRef to represent paths for the most part, and do comparisons with == and standard string hashing.

This means that if the same path gets into clangd with different case in two places, our comparisons will be incorrect. This affects at least:

set of open files (TUScheduler's file map)
dynamic index (FileSymbols)
background index (shards in memory are keyed by case-sensitive strings, filenames of shards on disk are case-insensitive, but contain case-sensitive hashes of directory paths)
global CDB: See #102 for a nasty example.

I think the best option is to have a Path class that has the correct semantics for the platform. On e.g. linux this would behave like std::string, on windows it would have case-insensitive ==/hash behavior. Then we get the correct semantics by using Path consistently.

sam-mccall commented 3 years ago

This came up again in #665. And again in https://github.com/ycm-core/YouCompleteMe/issues/3698

One annoyance with the Path idea is ownership: paths aren't cheap to copy, so I guess we need a PathRef class too, and the API got a bit out of hand when I tried it.

But this might combine well with an idea I had of interning pathnames in memory, if we can find a sensible way to do it across threads. Then we'd only have PathRefs to deal with.

HighCommander4 commented 3 years ago

This observation may only be tangentially relevant, but there's precedent for treating paths as distinct types in the Rust standard library.

EDIT: And, in fact, even in the C++17 standard library.

sam-mccall commented 3 years ago

Yeah, the details of those types probably don't actually work for us (e.g. C++'s paths are case-sensitive, and the method for checking equivalence does IO, reads through symlinks etc). But the idea of using a proper type is appealing.

https://reviews.llvm.org/D95824 is a sketch of path interning. As noted on that patch it's pretty painful to actually roll out such a change, and I'm wary of getting stuck halfway. So not sure if I want to push for this anytime soon.

(If we got rid of URIs in the index I think it becomes more compelling - having paths be cheap handles is more of a performance win if we're not converting the character data all the time anyway)

fifv commented 2 months ago

I met a problem caused by this.

When I rename function names in vscode, it failed with "Rename failed to apply edits". I open the rename preview and it can't open, says "The editor could not be opened due to an unexpected error: Resource not found in diff editor". I have tried diffrent versions of clangd, vscode-clangd extension with no luck.

Then I checked the verbose clangd log, saw the textDocument/rename replied with same file twice, one with D:/ and another one with d:/. Found https://github.com/clangd/clangd/issues/665 and https://github.com/clangd/clangd/issues/1592#issuecomment-1516506136 , I tried to replace all D:/ in compile_commands.json to d:/, the problem got fixed. But compile_commands.json will be overriden each time I configure cmake, I didn't find any option about this in cmake docs.

Any hope that this issuewill be resolved? With thanks.