[clang-tools-extra] [clangd] Use Diagnostic.data for stable fix-it cache key (PR #198073)
Tanner Bennett via cfe-commits
cfe-commits at lists.llvm.org
Sat May 16 01:09:44 PDT 2026
https://github.com/NSExceptional updated https://github.com/llvm/llvm-project/pull/198073
>From 5cbfcb7c2218232006896046f95721069e557e33 Mon Sep 17 00:00:00 2001
From: Tanner Bennett <tannerbennett at me.com>
Date: Sat, 16 May 2026 03:09:11 -0500
Subject: [PATCH] [clangd] Use Diagnostic.data for stable fix-it cache key
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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.
---
clang-tools-extra/clangd/ClangdLSPServer.cpp | 31 ++++++++++++++++
clang-tools-extra/clangd/ClangdLSPServer.h | 30 ++++++++++++----
.../clangd/test/diagnostic-category.test | 3 ++
.../clangd/test/diagnostics-no-tidy.test | 3 ++
.../test/did-change-configuration-params.test | 3 ++
.../clangd/test/execute-command.test | 3 ++
.../test/fixits-codeaction-data-key.test | 35 +++++++++++++++++++
.../fixits-codeaction-documentchanges.test | 3 ++
.../clangd/test/fixits-codeaction.test | 3 ++
.../test/fixits-command-documentchanges.test | 3 ++
.../clangd/test/fixits-command.test | 3 ++
.../test/fixits-embed-in-diagnostic.test | 3 ++
.../test/include-cleaner-batch-fix.test | 12 +++++++
13 files changed, 129 insertions(+), 6 deletions(-)
create mode 100644 clang-tools-extra/clangd/test/fixits-codeaction-data-key.test
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 04f58ab6446d1..f038b4c285f73 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -936,6 +936,7 @@ void ClangdLSPServer::onDocumentDidClose(
{
std::lock_guard<std::mutex> Lock(DiagRefMutex);
DiagRefMap.erase(File);
+ DiagIdRefMap.erase(File);
}
{
std::lock_guard<std::mutex> HLock(SemanticTokensMutex);
@@ -1770,6 +1771,22 @@ void ClangdLSPServer::profile(MemoryTree &MT) const {
std::optional<ClangdServer::DiagRef>
ClangdLSPServer::getDiagRef(StringRef File, const clangd::Diagnostic &D) {
std::lock_guard<std::mutex> Lock(DiagRefMutex);
+
+ // Prefer the id-based lookup. Robust against clients that mutate the
+ // displayed message (e.g. stripping clangd's "(fix available)" suffix).
+ if (auto Id = diagFixIdFromData(D)) {
+ auto IdIter = DiagIdRefMap.find(File);
+ if (IdIter != DiagIdRefMap.end()) {
+ auto FixItsIter = IdIter->second.find(*Id);
+ if (FixItsIter != IdIter->second.end())
+ return FixItsIter->second;
+ }
+ // Fall through to legacy lookup — the id was missing from cache, which
+ // can happen if the file was reparsed between publish and codeAction.
+ }
+
+ // Fallback: (range, message) key. Used for clients that drop the data
+ // field on round-trip, and as a backup when the id is unknown.
auto DiagToDiagRefIter = DiagRefMap.find(File);
if (DiagToDiagRefIter == DiagRefMap.end())
return std::nullopt;
@@ -1811,6 +1828,7 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
Notification.version = decodeVersion(Version);
Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
DiagnosticToDiagRefMap LocalDiagMap; // Temporary storage
+ DiagIdToDiagRefMap LocalDiagIdMap;
for (auto &Diag : Diagnostics) {
toLSPDiags(Diag, Notification.uri, DiagOpts,
[&](clangd::Diagnostic LSPDiag, llvm::ArrayRef<Fix> Fixes) {
@@ -1824,6 +1842,18 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
if (LSPDiag.codeActions->size() == 1)
LSPDiag.codeActions->front().isPreferred = true;
}
+ // Stamp a unique id into Diagnostic.data only when there's
+ // at least one Fix-It associated. We only need to look this
+ // diagnostic up at codeAction time if it carries a fix, so
+ // gating keeps the wire format unchanged for the common
+ // non-fixable case and avoids churn in unrelated tests.
+ // The id keys an alternate cache so codeAction lookup stays
+ // robust even if the client mutates message text for display.
+ if (!Fixes.empty()) {
+ int64_t Id = NextDiagFixId.fetch_add(1);
+ LSPDiag.data[DiagDataIdKey] = Id;
+ LocalDiagIdMap[Id] = {Diag.Range, Diag.Message};
+ }
LocalDiagMap[toDiagKey(LSPDiag)] = {Diag.Range, Diag.Message};
Notification.diagnostics.push_back(std::move(LSPDiag));
});
@@ -1833,6 +1863,7 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
{
std::lock_guard<std::mutex> Lock(DiagRefMutex);
DiagRefMap[File] = std::move(LocalDiagMap);
+ DiagIdRefMap[File] = std::move(LocalDiagIdMap);
}
// Send a notification to the LSP client.
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 6ada3fd9e6e47..f62e17f9bb2bc 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -243,10 +243,18 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
/// Used to indicate the ClangdLSPServer is being destroyed.
std::atomic<bool> IsBeingDestroyed = {false};
- // FIXME: The caching is a temporary solution to get corresponding clangd
- // diagnostic from a LSP diagnostic.
- // Ideally, ClangdServer can generate an identifier for each diagnostic,
- // emit them via the LSP's data field (which was newly added in LSP 3.16).
+ // The cache maps each published LSP diagnostic back to the underlying
+ // clangd::Diag so we can look up its Fix-Its when the client sends a
+ // codeAction request later.
+ //
+ // Preferred path: each published Diagnostic gets a unique id stamped into
+ // its `data` field under the "clangdFixId" key. LSP spec (3.16+) preserves
+ // `data` between publishDiagnostics and codeAction's `context.diagnostics`,
+ // so we look up by id and the lookup is immune to clients that mutate
+ // `message` for display (e.g. capitalizing, stripping suffixes).
+ //
+ // Fallback path: legacy (range, message) key, retained for clients that
+ // drop `data` round-trip.
std::mutex DiagRefMutex;
struct DiagKey {
clangd::Range Rng;
@@ -255,15 +263,25 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
return std::tie(Rng, Message) < std::tie(Other.Rng, Other.Message);
}
};
+ static constexpr llvm::StringLiteral DiagDataIdKey = "clangdFixId";
DiagKey toDiagKey(const clangd::Diagnostic &LSPDiag) {
return {LSPDiag.range, LSPDiag.message};
}
+ /// Reads the clangdFixId from a diagnostic's `data` field, if present.
+ static std::optional<int64_t>
+ diagFixIdFromData(const clangd::Diagnostic &LSPDiag) {
+ return LSPDiag.data.getInteger(DiagDataIdKey);
+ }
/// A map from LSP diagnostic to clangd-naive diagnostic.
typedef std::map<DiagKey, ClangdServer::DiagRef>
DiagnosticToDiagRefMap;
+ typedef std::map<int64_t, ClangdServer::DiagRef> DiagIdToDiagRefMap;
/// Caches the mapping LSP and clangd-naive diagnostics per file.
- llvm::StringMap<DiagnosticToDiagRefMap>
- DiagRefMap;
+ /// Both maps are populated on every publish; lookup tries IdMap first.
+ llvm::StringMap<DiagnosticToDiagRefMap> DiagRefMap;
+ llvm::StringMap<DiagIdToDiagRefMap> DiagIdRefMap;
+ /// Monotonically-increasing fix-it id stamped into Diagnostic.data.
+ std::atomic<int64_t> NextDiagFixId = {1};
// Last semantic-tokens response, for incremental requests.
std::mutex SemanticTokensMutex;
diff --git a/clang-tools-extra/clangd/test/diagnostic-category.test b/clang-tools-extra/clangd/test/diagnostic-category.test
index 654c2c0b46556..2bf845102927a 100644
--- a/clang-tools-extra/clangd/test/diagnostic-category.test
+++ b/clang-tools-extra/clangd/test/diagnostic-category.test
@@ -8,6 +8,9 @@
# CHECK-NEXT: {
# CHECK-NEXT: "category": "Semantic Issue",
# CHECK-NEXT: "code": "use_with_wrong_tag",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Use of 'Point' with tag type that does not match previous declaration (fix available)\n\nfoo.c:1:8: note: previous use is here",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/diagnostics-no-tidy.test b/clang-tools-extra/clangd/test/diagnostics-no-tidy.test
index 9341275b6c213..8383ca59f457a 100644
--- a/clang-tools-extra/clangd/test/diagnostics-no-tidy.test
+++ b/clang-tools-extra/clangd/test/diagnostics-no-tidy.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wmain-return-type",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Return type of 'main' is not 'int' (fix available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/did-change-configuration-params.test b/clang-tools-extra/clangd/test/did-change-configuration-params.test
index 08c7b4bcb57ad..ea0516387989c 100644
--- a/clang-tools-extra/clangd/test/did-change-configuration-params.test
+++ b/clang-tools-extra/clangd/test/did-change-configuration-params.test
@@ -27,6 +27,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wuninitialized",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Variable 'i' is uninitialized when used here (fix available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/execute-command.test b/clang-tools-extra/clangd/test/execute-command.test
index b4bd48b784f20..cf1bc8f9f04c1 100644
--- a/clang-tools-extra/clangd/test/execute-command.test
+++ b/clang-tools-extra/clangd/test/execute-command.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wparentheses",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-codeaction-data-key.test b/clang-tools-extra/clangd/test/fixits-codeaction-data-key.test
new file mode 100644
index 0000000000000..033de2da2af5b
--- /dev/null
+++ b/clang-tools-extra/clangd/test/fixits-codeaction-data-key.test
@@ -0,0 +1,35 @@
+# Verifies that when an LSP client mutates the diagnostic message for display
+# (e.g. stripping clangd's "(fixes available)" suffix) but preserves the
+# `data` field per the LSP 3.16 spec, clangd still associates the diagnostic
+# with its cached fix-its on textDocument/codeAction.
+#
+# clangd assigns fix-it ids monotonically from 1 on each process start, so the
+# first published fix-bearing diagnostic gets clangdFixId 1.
+#
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main(int i, char **a) { if (i = 2) {}}"}}}
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK: "clangdFixId": 1
+---
+# A code action request whose `context.diagnostics[0].message` has been
+# mutated by the client (no "(fixes available)" suffix), but whose `data`
+# carries the clangdFixId from the publishDiagnostics above. The id-based
+# lookup should match and return the fix-its.
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses", "code": "-Wparentheses", "source": "clang", "data": {"clangdFixId": 1}}]}}}
+# CHECK: "id": 2,
+# CHECK: "kind": "quickfix",
+# CHECK-NEXT: "title": "place parentheses around the assignment to silence this warning"
+---
+# Negative control: same mutated message WITHOUT the data field — the legacy
+# (range, message) lookup misses because the message no longer matches what
+# clangd cached, so no fix-its are returned.
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses", "code": "-Wparentheses", "source": "clang"}]}}}
+# CHECK: "id": 3,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": []
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
diff --git a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
index 7a591319a7405..eb83573719c53 100644
--- a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
+++ b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wparentheses",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-codeaction.test b/clang-tools-extra/clangd/test/fixits-codeaction.test
index 75d0fb012ffc8..05fec1b8d912f 100644
--- a/clang-tools-extra/clangd/test/fixits-codeaction.test
+++ b/clang-tools-extra/clangd/test/fixits-codeaction.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wparentheses",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-command-documentchanges.test b/clang-tools-extra/clangd/test/fixits-command-documentchanges.test
index cd636c4df387a..1ef04d7938a0d 100644
--- a/clang-tools-extra/clangd/test/fixits-command-documentchanges.test
+++ b/clang-tools-extra/clangd/test/fixits-command-documentchanges.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wparentheses",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-command.test b/clang-tools-extra/clangd/test/fixits-command.test
index 62b5a6152d2cf..3c9a5c4afd04d 100644
--- a/clang-tools-extra/clangd/test/fixits-command.test
+++ b/clang-tools-extra/clangd/test/fixits-command.test
@@ -7,6 +7,9 @@
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "code": "-Wparentheses",
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test b/clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
index 26d3820d04322..878efdae3aba4 100644
--- a/clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ b/clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -33,6 +33,9 @@
# CHECK-NEXT: "title": "change 'union' to 'struct'"
# CHECK-NEXT: }
# CHECK-NEXT: ],
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Use of 'Point' with tag type that does not match previous declaration (fix available)\n\nfoo.c:1:8: note: previous use is here",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
index 5a87a87e2f63a..1c70c55fec715 100644
--- a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
@@ -39,6 +39,9 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "{{.*}}"
# CHECK-NEXT: },
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "No header providing \"Foo\" is directly included (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -58,6 +61,9 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "{{.*}}"
# CHECK-NEXT: },
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "No header providing \"Bar\" is directly included (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -77,6 +83,9 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "{{.*}}"
# CHECK-NEXT: },
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Included header all1.h is not used directly (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -99,6 +108,9 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "{{.*}}"
# CHECK-NEXT: },
+# CHECK-NEXT: "data": {
+# CHECK-NEXT: "clangdFixId": {{[0-9]+}}
+# CHECK-NEXT: },
# CHECK-NEXT: "message": "Included header all2.h is not used directly (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
More information about the cfe-commits
mailing list