- Make clangd's internal representation more aligned with the standard. We keep range and extra inlayhint kinds around, but don't serialize them on standard version.
- Have custom serialization for extension (ugly, but going to go away).
- Support both versions until clangd-17.
- Don't advertise extension if client has support for standard implementation.
- Log a warning at startup about extension being deprecated, if client doesn't have support.
Details
- Reviewers
sammccall - Commits
- rG3137ca80b9ef: [clangd] Support for standard inlayHint protocol
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This looks like the right approach to me, thanks for doing this!
What do you want to do about paddingLeft/paddingRight?
IIUC where we're currently sending label="foo: " we need to send label="foo:", paddingRight=true.
But updating these strings might have a blast radius in the tests, so it might be best to do it in a separate patch?
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
588 | I still feel a little uncomfortable with this because it cuts against the design of capabilities, and it's not clear what concrete problem it solves. Most likely it won't cause nor solve any problems. But it might lead to a lot of confusion. However it's up to you, I promise this is my last comment about it :-) | |
1215 | ||
1225 | Defining the extension JSON representation as a delta of the standard JSON representation seems brittle and it's hard to tell what the result is. | |
clang-tools-extra/clangd/Protocol.cpp | ||
1321–1322 | this can now be inlined into operator<< again, it's not being called anywhere else | |
1338 | We can't serialize our extension kinds, a new version of the protocol might come out that says 3 means nullability hints or something and existing versions of clangd are sending incorrect kinds. (probably the most regular thing to do is define json::Value toJSON(InlayHintKind) that returns int | null, but up to you) | |
clang-tools-extra/clangd/Protocol.h | ||
497 | nit: inlayHint, not hints (mostly confusing because arity differs between the two versions, and this is the wrong one) | |
1530–1531 | we almost always use the comments from LSP in this file. (And in the case of InlayHint, some of the differences are meaningful) | |
1567–1568 | this is an (unserialized) clangd extension | |
1575–1576 | this is no longer correct: whitespace should no longer be included and paddingLeft/paddingRight should be set instead. |
- Get rid of warning and not advertising when client has capability.
- Introduce paddingLeft/paddingRight, get rid of leading/trailing spaces in label, but re-introduce them when serializing for extension.
- Update documentation to reflect LSP docs.
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
588 |
agreed, i thought this was more of a capability when i was proposing it. but noticed that it wasn't while going into the details and rubbed me the wrong way as well. so dropping it. |
Great! If you didn't already, can you check with VSCode before landing?
This might catch protocol bits we missed, but also will make sure that the disablement of the legacy protocol in vscode-clangd is working as we expect.
clang-tools-extra/clangd/Protocol.cpp | ||
---|---|---|
1339 | I think "kind": null is formally invalid per spec, it needs to be omitted entirely (the difference between kind?: InlayHintKind and kind: InlayHintKind|null). I suspect it'll work fine in practice, but if you want to be really rigorous probably better add it conditionally | |
clang-tools-extra/clangd/Protocol.h | ||
1579 | nit: = false to avoid accidentally uninitialized members |
- Default initialize paddingLeft/Right
- Don't serialize kind when its null
- Run in vscode, seems to be working.
I still feel a little uncomfortable with this because it cuts against the design of capabilities, and it's not clear what concrete problem it solves.
Most likely it won't cause nor solve any problems. But it might lead to a lot of confusion.
(e.g. in nvim and some others, base capabilities are provided but customized by extensions/users. If one extension sets the standard capability and another tries to use clangd hints, or the user sets the wrong capability at first out of confusion, it seems hard to debug)
However it's up to you, I promise this is my last comment about it :-)