Workaround the drive letter casing issue on windows.

[raldone01]

Fixes: #726.

Lower case drive letters seem to be the cause of the issues.

Not being able to refactor on windows makes clangd borderline unusable.

The workaround can be removed once clangd/clangd#108 is fixed.
In the mean time I think this simple workaround should be quite robust and alleviates the pain.

Why I fixed it here:

Working around the clangd issue in the vscode extension is much easier than attempting to tame the beast that is clangd.
The impact of the workaround is also very small.

Some things I am uncertain about:

I don't know if clangd normalizes paths at all.
If it doesn't:

Maybe a setting should be added to decide between upper and lower case drive letter normalization on windows.
I have never observed lower case ones in compile_commands.json though.
So we should probably wait for complaints 😁 before adding the setting.

Note:

I changed the typescript target because I needed named capture groups.
Maybe it is also time to move on from commonjs to "module": "Node16",.

[JVApen]

Do we know where the lower case letter comes from? I'm a bit worried that this change fixes the symptoms instead of the underlying problem. If that's the case, clangd might know about the same file twice with different casing. I wouldn't be surprised to find out that more issues would popup.

[JVApen]

On our systems, using both Windows with vs code and CMake, it always uses the same casing, which I believe is upper case.

[raldone01]

I don't know the origin but probably cmake? It generates the compile commands after all.

Ideally clangd should normalise every path it receives/reads using some library. Then all these issues should go away. I am sure cpp has some sort of normalise function.

[raldone01]

On our systems, using both Windows with vs code and CMake, it always uses the same casing, which I believe is upper case.

So it works on your system. When the drive letter casing is already uppercase, my code should have no effect.

Did this branch cause issues on your systems?

[artem-kamyshev]

Any progress here or on the main issue? It's unfortunate that we as devs have to apply our own fixes to make the tooling work.

[ccll]

May I ask why this PR is not accepted?

From these historical discussions:

• Case-sensitive path handling is error prone on Win/Mac clangd#108
• Duplicates in response of "textDocument/references" request clangd#2195
• Renaming will fail under certain circumstances #687
• Renaming function failed across files #779

We can see that:

• CMake generates compile_commands.json with upper case "C:"
• vscode sends the file name with lower case "c:" to vscode-clangd
• clangd is confused with the two version and generates duplicated rename operations

So we either:

• let CMake generates lower case "c:"
• or let vscode sends upper case "C:"
• or let clangd deduplicate same file in different case
• or let vscode-clangd transform lower case "c:" to upper case "C:"

Is my understanding correct?

If so, @raldone01 the author of the PR might be right that vscode-clangd is the most easy solution here.

[HighCommander4]

I think the proper solution is this one:

let clangd deduplicate same file in different case

There is work in progress towards it at llvm/llvm-project#136439.

[Rawalanche]

I think the proper solution is this one:

let clangd deduplicate same file in different case

There is work in progress towards it at llvm/llvm-project#136439.

Hi,

what do you mean "work in progress towards"? The last activity on the issue is from half a year ago.

It's really confusing that people whom are working on the tools solving one of the most complicated problem set in programming, parsing semantics of a language, for arguably one of the hardest languages to parse (C++), haven't been able to figure out a reasonable solution for case sensitivy of a drive letter for over 5 years now (half a decade). And due to this, one of the most essential features of a language server - renaming of a symbol - does not work on by far the most popular OS - Windows.

[HighCommander4]

There is work in progress towards it at llvm/llvm-project#136439.

what do you mean "work in progress towards"? The last activity on the issue is from half a year ago

I just meant that someone has started work on this solution. You are right that there have not been updates in a while, suggesting the PR submitter may have abandoned this work.

It's really confusing that people whom are working on the tools solving one of the most complicated problem set in programming, parsing semantics of a language, for arguably one of the hardest languages to parse (C++), haven't been able to figure out a reasonable solution for case sensitivy of a drive letter for over 5 years now (half a decade). And due to this, one of the most essential features of a language server - renaming of a symbol - does not work on by far the most popular OS - Windows.

Clangd is an open source project. There is no paid team or anything working on it; all bug fixes and enhancements come from individual contributors, most of whom contribute on a volunteer basis (some are employed by their employers to solve specific problems for them).

Improvements to Windows support will happen if/when someone is motivated enough to contribute them.

Concretely, it looks like the patch mentioned above, llvm/llvm-project#136439, needs a new owner to pick it up and continue work on it. Consider this an invitation to the Windows user community to do so.

[raldone01]

I think this PR is a good and trivial hack until proper support arrives BUT:

It is up to the maintainers of this repository whether to merge this or not. So far there has been no actionable feedback for me.

From what I gather they have no intention of merging this.

I also no longer use windows. So I will close this PR and unsubscribe.

Feel free to open a new PR.

[Rawalanche]

There is work in progress towards it at llvm/llvm-project#136439.

what do you mean "work in progress towards"? The last activity on the issue is from half a year ago

I just meant that someone has started work on this solution. You are right that there have not been updates in a while, suggesting the PR submitter may have abandoned this work.

It's really confusing that people whom are working on the tools solving one of the most complicated problem set in programming, parsing semantics of a language, for arguably one of the hardest languages to parse (C++), haven't been able to figure out a reasonable solution for case sensitivy of a drive letter for over 5 years now (half a decade). And due to this, one of the most essential features of a language server - renaming of a symbol - does not work on by far the most popular OS - Windows.

Clangd is an open source project. There is no paid team or anything working on it; all bug fixes and enhancements come from individual contributors, most of whom contribute on a volunteer basis (some are employed by their employers to solve specific problems for them).

Improvements to Windows support will happen if/when someone is motivated enough to contribute them.

Concretely, it looks like the patch mentioned above, llvm/llvm-project#136439, needs a new owner to pick it up and continue work on it. Consider this an invitation to the Windows user community to do so.

Well, the thing is that the community did step up to fix the problem. The very issue this thread is under, the workaround, solves the problem. And it solves it with about ~40 LOC in the single file.

The supposedly "proper" solution you've linked is over ~600 LOC changes over 50 files. You very well know no one is going to pick that up, given how open source development works.

So this fix being rejected on the grounds of solution not being proper enough practically means it's never going to happen.

What I find really puzzling is the indiference and apathy towards the quality of the project you are contributor of. When I work on something, I can't even remotely imagine being okay with integral feature of that project (in case of language server symbol renaming) not working on the most popular development platform. It would not let me sleep at night, especially if I knew the solution to unblock it, even if just temporarily, is just clicking an Accept button on one existing PR.

Instead, Clangd VSCode Extension will continue, probably in perpetuity, to not have working symbol rename, not because someone did not make patch, but just because it was rejected on principle.

The comment above the code chunk in this patch literally says:
"// Remove this workaround once clangd fixes the issue on their side: clangd/clangd#108"

It's not meant to replace proper fix, it's just meant to exist until the proper fix comes, which in case of clangd is probably never. So why not have symbol renaming at least working from now ideally forever, if it costs couple button clicks in the GitHub UI?

[HighCommander4]

just because it was rejected on principle

To be clear, this workaround patch wasn't rejected solely on principle; there were also technical concerns brought up in this comment.

That said, I will admit I haven't spent much time thinking through the technical concerns, and you've helped convince me that the proper fix being seemingly stalled makes it worth doing so.

the solution to unblock it, even if just temporarily, is just clicking an Accept button on one existing PR

It's not just a matter of clicking the Accept button. We have to think through the effects of the patch carefully, including its effects on users who may have different setups than those who have reported the PR to solve their problem.

So, let's reopen the PR. I'm not very experienced with Typescript or language clients so I'm not in the best position to evaluate this patch, but I can try, and maybe others can help as well.

For starters, is there some API documentation about LanguageClientOptions.uriConverters that would help me understand in what situations vscode will call these functions?

[Rawalanche]

just because it was rejected on principle

To be clear, this workaround patch wasn't rejected solely on principle; there were also technical concerns brought up in this comment.

That said, I will admit I haven't spent much time thinking through the technical concerns, and you've helped convince me that the proper fix being seemingly stalled makes it worth doing so.

the solution to unblock it, even if just temporarily, is just clicking an Accept button on one existing PR

It's not just a matter of clicking the Accept button. We have to think through the effects of the patch carefully, including its effects on users who may have different setups than those who have reported the PR to solve their problem.

So, let's reopen the PR. I'm not very experienced with Typescript or language clients so I'm not in the best position to evaluate this patch, but I can try, and maybe others can help as well.

For starters, is there some API documentation about LanguageClientOptions.uriConverters that would help me understand in what situations vscode will call these functions?

Thank you for reopening it.

I am a little bit confused about who owns the decision making regarding this extension...? It's VSCode extension, so it's obviously almost entirely TypeScript.

If you are someone who can approve pull requests but does not own the development of it, then do you happen to know who does?

As for your specific LanguageClientOptions.uriConverters question, I am also not very experienced with TypeScript/VSCode extension development either. I just do C++ game development and this extension is invaluable to me. I could just give you copy pasted Codex answer, which could be quite close to accurate, but I am guessing if you wanted AI generated answer, you would have probably done so yourself already.

So I guess this leaves us with having to find someone in charge of this extension who could make a judgement whether the patch is appropriate or not. I am sorry for the confusion, I originally thought you were the owner/creator of it.

[HighCommander4]

I am a little bit confused about who owns the decision making regarding this extension...? It's VSCode extension, so it's obviously almost entirely TypeScript.

If you are someone who can approve pull requests but does not own the development of it, then do you happen to know who does?

I'm a clangd user who started contributing to the clangd (server) codebase and more recently became one of its maintainers. I'm subscribed to this repository because a lot of issues which are actually on the server side get filed here, and I try to help where I can, but as mentioned I don't have particular domain experience in Typescript or vscode APIs.

The original authors (of both clangd and vscode-clangd) appear to have mostly moved on to other things. (For the record, I don't think they had particular domain experience in Typescript either. I believe they were also primarily C++ developers, who learned enough Typescript to write this plugin because clangd needed a vscode client due to vscode being the most popular LSP-based editor.)

I think it would be fair to say that vscode-clangd does not currently have official owners, at least not any who are active. If someone reading this thread has an employer who would be willing to take ownership of it by employing someone to maintain it (which, for a project of this size, would only require a fraction of a full-time job), or if someone reading this has relevant experience with Typescript and vscode APIs and is interested in taking on maintenance on a volunteer basis, please get in touch.

[HighCommander4]

Also, just wanted to mention one other thing since I see it hasn't come up in this thread: there is an existing workaround to this issue, which is to ensure that your compile_commands.json file contains paths with lowercase drive letters (post-process it with a script if necessary). It may be a bit annoying to do, but presumably better than nothing.

[JVApen]

I had some personal issues to attend to that took up too much of my time. Jumping in again, I'm worried about a snowball effect of this change, where it fixes one thing and breaks the next. Given we still don't understand why the casing is different, I'm still unable to reproduce. I've tried many different things and can't get CMake to generate lower case drives.

I think the safe option would be to include an escape hatch in this patch. I'm OK with having this change active by default, though I would prefer having an option to disable it. When a related issue pops up, we can ask the user to disable this.

I think the defacto owner of this repo is @HighCommander4, so leaving it up to him to decide if this option is required for landing this change.

[HighCommander4]

I'm still unable to reproduce. I've tried many different things and can't get CMake to generate lower case drives.

I believe for affected users the inconsistency is the other way around: CMake generates compilation databases with uppercase drive names, but vscode sends lowercase ones.

So, if your compilation database contains uppercase drive names, and you don't experience this problem, that suggests that for you, vscode is also sending uppercase drive names.

It would be good to understand what determines/affects what vscode sends.

[HighCommander4]

is there some API documentation about LanguageClientOptions.uriConverters that would help me understand in what situations vscode will call these functions?

@raldone01 or @JVApen, can you answer this?

[JVApen]

Not an expert, though after some searches I found a bug (microsoft/vscode-languageserver-node#228) that indicates that is should be called for every conversion linked to the protocol.

The API for it seems straight forward, so I suspect that every request should pass by this.

I do wonder if the clangd extension needs to be checked for usages, though a first glance gave me the following:

vscode-clangd/src/ast.ts

Line 45 in dd98501

const converter = this.context.client.code2ProtocolConverter;

So it seems it is already in use.

[Rawalanche]

I don't think it should be relevant where the compile commands come from. Mine don't have lowercase letters either, but they come from UBT (Unreal Build Tool), not CMake. Despite the compile_commands.json having uppercase drive letters, for some reason, the band aid patch above does make symbol rename work.

[HighCommander4]

Ok, I think I understand the idea behind the patch: to intercept URLs on their way from vscode to clangd, and change the driver letters to uppercase, anticipating that this will make them consistent with the paths clangd gets from elsewhere (mainly the compilation database) when they weren't before.

I do have concerns about this happening by default, precisely because compilation databases can be generated by a variety of tools, and some of them may generate them with lowercase paths. Clangd currently works for users of such tools, and this patch would break it.

A way forward that does seem feasible is the following:

• Add a new setting clangd.windowsPathNormalization, with three possible values: uppercase, lowercase, and none. Adjust the URI converter being added in the patch to respect this setting.
• Initially, keep the default at none. Users who experience this issue now have a relatively easy workaround, of changing the setting to uppercase.
• Subsequently, on the theory that most users have compilation databases with uppercase paths, we can try changing the default to uppercase and see what type of feedback we get. I'd want this change to stay on the pre-release channel for a while before shipping it on the stable release channel.

[nitrix]

Running into this issue myself, I'd happily be a guinea pig for clangd.windowsPathNormalization set to uppercase and report my experience. If it's smooth sailing, maybe a separate PR can advocate for it becoming the default.