This change extends the 'textDocument/publishDiagnostics' notification sent from Clangd to the client. The extension can be enabled using the '-fixit-usage=embed-in-diagnostic' argument. When it's enabled, Clangd sends out the fixits associated with the appropriate diagnostic in the body of the 'publicDiagnostics' notification.
Details
- Reviewers
jkorous sammccall ilya-biryukov - Commits
- rG8626d36f8c54: [clangd] extend the publishDiagnostics response to send back fixits to the…
rL339454: [clangd] extend the publishDiagnostics response to send back fixits to the
rCTE339454: [clangd] extend the publishDiagnostics response to send back fixits to the
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
Couple of thoughts. (Technically I'm out on leave so will let Jan/Ilya
review implementation and happy with whatever you decide)
Enabling
- negotiating LSP extensions is probably better done in the "capabilities"
message exchange than as a command-line flag. Generally, we want this
extension on if the *client* is aware of it. Roughly, the client
capabilities are owned by the client, and the flags are owned by the *user*.
- for simplicity, we could always enable this, unless we really think the
message size is a problem, or are worried about conflicts with future LSP
versions
Naming
- elsewhere in clangd we settled on calling a "Fix" what clang calls a
"FixItHint". The latter is long/awkward/jargon, and often gets shortened to
"FixIt" which isn't obviously a noun. The former mostly has its plain
English meaning. I'd prefer "fix" in the protocol/flags, for the same
reasons.
- obviously feel free to give these any name you prefer in your UI!
+1 Sam's suggestion of configuring this during initial LSP handshake, rather than as a command-line flag.
We could put it into ClientCapabilities or into initializationOptions. The latter has type 'any' in LSP, so it seems to be most in-line with the protocol to put clangd-specific config options there, but not opposed to having this in other structs as an extension either.
- Client now can request the fixes using a 'clientCapabilities/textDocument/publishDiagnostics' extension.
- Use 'Fixes' instead of 'Fixits' in code.
clangd/ClangdLSPServer.cpp | ||
---|---|---|
441 | Maybe remove this parameter now that we configure these via LSP? |
LGTM with a small NIT.
clangd/ClangdLSPServer.cpp | ||
---|---|---|
507 | NIT: maybe avoid . and use something like clangd_fixes? having dots in the object keys might be awkward if someone tries to use those from javascript. (not that we have these use-cases now, but still) |
Maybe remove this parameter now that we configure these via LSP?
We don't set it to anything other than the default anyway.