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 <[email protected]> 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": { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
