[clangd] Use Diagnostic.data for stable fix-it cache key#198073
Draft
NSExceptional wants to merge 1 commit into
Draft
[clangd] Use Diagnostic.data for stable fix-it cache key#198073NSExceptional wants to merge 1 commit into
NSExceptional wants to merge 1 commit into
Conversation
clangd's existing fix-it cache keys each cached diagnostic by `(range, message)`. The header has a long-standing FIXME noting this is brittle, and indeed it breaks down whenever an LSP client mutates the displayed message for the Problems panel — e.g. capitalizing the first letter or stripping clangd's "(fix available)" suffix — and then echoes that mutated diagnostic back in `context.diagnostics` on a textDocument/codeAction request. The (range, message) lookup misses and no fixes are returned, even though clangd has them cached. LSP 3.16 introduced `Diagnostic.data`, *"preserved between a textDocument/publishDiagnostics notification and textDocument/ codeAction request"*. Use it. This patch: - Assigns a monotonically-increasing `clangdFixId` to each published diagnostic that carries at least one Fix-It, stamped into `Diagnostic.data["clangdFixId"]`. - Maintains a parallel `DiagIdRefMap: file -> id -> DiagRef` keyed by that id. - In `getDiagRef`, looks up by id first; falls back to the legacy `(range, message)` key for clients that drop `data` on round-trip. Gating on `!Fixes.empty()` keeps the wire format unchanged for the common non-fixable diagnostic case and avoids churn in tests that don't deal with fix-its. Tests that *do* check fix-bearing publishDiagnostics output have been updated. A new lit test (`fixits-codeaction-data-key.test`) verifies that: - The id-based lookup returns fix-its even when the request's `context.diagnostics[0].message` has been mutated. - A control request with mutated message and no data returns `[]` (legacy behavior, confirming the new path is what's doing the work). Local: 1383 unit tests and the touched lit tests all pass.
NSExceptional
force-pushed
the
clangd-stable-fixit-key
branch
from
18d08d8 to
5cbfcb7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
clangd matches diagnostics in
textDocument/codeAction'scontext.diagnosticsagainst its fix-it cache by(range, message)(ClangdLSPServer.h—struct DiagKey). The header itself flags this as a FIXME:The brittleness shows up in practice whenever an LSP client mutates the displayed message for its Problems panel — capitalizing, stripping clangd's
(fix available)suffix, etc. — and then echoes that mutated diagnostic back on a code-action request. The (range, message) lookup misses and no quickfixes are returned, even though clangd has them cached.Concrete example: vscode-swift's
DiagnosticsManagerstrips(fix available)for display. As a result, Objective-CAdd method implementationquickfixes for-Wincomplete-implementationnever appear in VS Code, despite clang emitting the FixIt and clangd surfacing it correctly. (Tracked at swiftlang/sourcekit-lsp#2186.)Change
clangdFixIdintoDiagnostic.datafor every published diagnostic that carries at least one Fix-It.DiagIdRefMap: file → id → DiagRef.getDiagRef, look up by id first; fall back to the legacy(range, message)key when the client dropsdataon round-trip.Gating on
!Fixes.empty()keeps the wire format unchanged for non-fixable diagnostics, which keeps the test churn limited to the tests that already check fix-bearing diagnostic output.Backwards compatibility
Diagnostic.datais opaque per LSP spec — clients are expected to preserve it untouched between publish and request. The fallback path means clients that don't preserve it still behave exactly as before. The newdatapayload (one int64 per fix-bearing diagnostic) is small.Tests
fixits-codeaction-data-key.test— verifies the id-based lookup returns fix-its even when the client has mutated the diagnostic message, and a control with mutated message + no data returns[].datafield.Marked draft to let CI exercise the build on the other platforms before promoting.
Yes, I authored this with my $200 Claude plan, I hope that's okay — it is the only way I can find the time to contribute features and bug fixes to the projects I love