[clangd] Refine the workflow for diagnostic Fixits.

[hokein]

Summary

• No longer store the diagnostic fixits in the clangdLSPServer
• When propagating the fixit via the code action, we use the Diag information stored in the ParsedAST (in clangdServer.cpp)

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

675

this is tricky indeed and conceptually really hard to achieve inside ClangdServer layer.

what about keeping the cache in ClangdLSPServer, but changing the format? Similar to TweakRef, we now have a DiagRef, which is ClangdServer-native. So we can keep a cache from (FilePath, clangd::Diagnostic) to clangd::DiagRef, and pass those DiagRefs to ClangdServer and make sure we're doing the search for sure on the right domain here?

this also gives us the flexibility to change the definition of a DiagRef in the future.

687

we can have quick fix kind tweaks too (e.g. populate switch)

709–710

can you also move this counter to global scope and use it in ::codeAction too?

clang-tools-extra/clangd/ClangdServer.h

384

maybe a more structured output can be useful here:

struct CodeActionResult {
  std::string Version; // Version of content results belong to.
  struct QuickFix {
      DiagRef D; // Diagnostic being addressed by the fix.
      Fix F;
  };
  std::vector<QuickFix> QuickFixes;
  std::vector<TweakRef> TweakRefs;
};

[hokein]

address review comments.

clang-tools-extra/clangd/ClangdServer.cpp

675

I'm not a fan of keeping a cache in ClangdLSPServer, I'd like to remove it entirely.

What do you think about this alternative?

• pull out and exposed the mainMessage API from the toLSPDiags
• we add the ClangdDiagnosticOptions to the CodeActionInputs.
• when searching a diagnostic in ClangdServer.cpp, we check the equality by checking mainMessage(ClangdServerDiagnostic.message, Inputs.DiagOpts) == DiagRef.Message

The downside here is that we have to pay O(N * cost of mainMessage) to find a matched diagnostic.

687

oops, I weren't aware of that, removed.

709–710

I added a similar local counter in codeAction method, since this method is being deprecated, and removed eventually (it doesn't seem too make much sense to use a shared counter)

clang-tools-extra/clangd/ClangdServer.h

384

good idea, also keep symmetry with the CodeActionInputs

[hokein]

Use the mainMessage to find the corresponding ClangdServer diagnostics.

clang-tools-extra/clangd/ClangdServer.cpp

675

I updated the patch with the mainMessage approach, please take a look.

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

675

i believe this still runs into the same conceptual problems. we're trying to imitate the logic done by clangdlspserver when converting a clangd-native diagnostic into an lsp-diagnostic and all of this will break as soon as there's a change in that logic. so if we want to go down that path, the best option would be to call toLSPDiagevery time here and make sure we imitate that logic properly (e.g. right now we're relying on the ranges not being remapped because we only store fixes for the main file diagnostics).

but i feel like this is unnecessary cost both at runtime but also as code-complexity wise. this is ~the same as the data/args we publish with commands. it's just unfortunate that data field in diagnostic didn't exist pre LSP 3.16, hence we can't rely on it without breaking compatibility with some old clients (hopefully we can do that one day), hence instead of clients keeping this identifier around, I am merely suggesting to keep this alive on the server side. as conceptually this is the only layer that can do the mapping back and forth.

709–710

sorry i am confused. the counter has nothing to do with how we generated a tweak. it's merely counting number of times a tweak was returned. hence we don't need a new one (if a code action Bar was made available either through a call to ::enumerateTweaks or ::codeAction we should increment the same counter. clients won't be invoking both at the same time, they'll be using one or the other).

[hokein]

add LSPServer cache back and repurpose for lsp <=> naive diagnostic mapping, per offline discussion.

[hokein]

cleanup

[kadircet]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1013–1015

we're already making a copy of Params.context.diagnostics when creating the CB.

so you might as well have a std::map<ClangdServer::DiagRef, clangd::Diagnostic> RefsToDiags; and get rid of the extra index conversions.

clang-tools-extra/clangd/ClangdLSPServer.h

239

instead of storing clangd::Diagnostic as the key, can we have a DiagnosticKey struct here (we can have a helper diagKey(const clangd::Diagnostic&) -> DiagnosticKey)? to make sure we don't get into trouble if there are too many "big" diagnostics :)

that way we can also get rid of the custom comparator.

[hokein]

address comments

• introduce DiagKey for the caching map
• get rid of extra index conversion

[hokein]

more cleanup

[kadircet]

thanks!

clang-tools-extra/clangd/ClangdLSPServer.cpp

1063

no need for LSPDiags anymore, you can just use ToLSPDiags.begin()

clang-tools-extra/clangd/ClangdServer.cpp

671

nit: you can return an llvm::ArrayRef<Fix> here and get rid of one extra layer of nesting down below

[hokein]

address comments

clang-tools-extra/clangd/ClangdLSPServer.h

private:
   LSPBinder::OutgoingNotification<ProgressParams<WorkDoneProgressEnd>>
       EndWorkDoneProgress;
   LSPBinder::OutgoingMethod<NoParams, std::nullptr_t> SemanticTokensRefresh;
 
   void applyEdit(WorkspaceEdit WE, llvm::json::Value Success,
                  Callback<llvm::json::Value> Reply);
 
   void bindMethods(LSPBinder &, const ClientCapabilities &Caps);
-  std::vector<CodeAction> getFixes(StringRef File, const clangd::Diagnostic &D);
+  std::optional<ClangdServer::DiagRef> getDiagRef(StringRef File,
+                                                  const clangd::Diagnostic &D);
 
   /// Checks if completion request should be ignored. We need this due to the
   /// limitation of the LSP. Per LSP, a client sends requests for all "trigger
   /// character" we specify, but for '>' and ':' we need to check they actually
   /// produce '->' and '::', respectively.
   bool shouldRunCompletion(const CompletionParams &Params) const;
 
   void applyConfiguration(const ConfigurationSettings &Settings);
private:
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
   bool ShutdownRequestReceived = false;
 
   /// Used to indicate the ClangdLSPServer is being destroyed.
   std::atomic<bool> IsBeingDestroyed = {false};
 
-  std::mutex FixItsMutex;
-  typedef std::map<clangd::Diagnostic, std::vector<CodeAction>,
-                   LSPDiagnosticCompare>
-      DiagnosticToReplacementMap;
-  /// Caches FixIts per file and diagnostics
-  llvm::StringMap<DiagnosticToReplacementMap>
-      FixItsMap;
+  // 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).
+  std::mutex DiagRefMutex;
+  struct DiagKey {
+    clangd::Range Range;
+    std::string Message;
+    bool operator<(const DiagKey &Other) const {
+      return std::tie(Range, Message) < std::tie(Other.Range, Other.Message);
+    }
+  };
+  DiagKey toDiagKey(const clangd::Diagnostic &LSPDiag) {
+    return {LSPDiag.range, LSPDiag.message};
+  }
+  /// A map from LSP diagnostic to clangd-naive diagnostic.
+  typedef std::map<DiagKey, ClangdServer::DiagRef>
+      DiagnosticToDiagRefMap;
+  /// Caches the mapping LSP and clangd-naive diagnostics per file.
+  llvm::StringMap<DiagnosticToDiagRefMap>
+      DiagRefMap;
+
   // Last semantic-tokens response, for incremental requests.
   std::mutex SemanticTokensMutex;
   llvm::StringMap<SemanticTokens> LastSemanticTokens;
 
   // Most code should not deal with Transport, callMethod, notify directly.
   // Use LSPBinder to handle incoming and outgoing calls.
   clangd::Transport &Transp;
   class MessageHandler;

clang-tools-extra/clangd/ClangdLSPServer.cpp

 #include "llvm/Support/JSON.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
 #include <functional>
+#include <map>
 #include <memory>
 #include <mutex>
 #include <optional>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {

 }
 
 void ClangdLSPServer::onDocumentDidClose(
     const DidCloseTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
   Server->removeDocument(File);
 
   {
-    std::lock_guard<std::mutex> Lock(FixItsMutex);
-    FixItsMap.erase(File);
+    std::lock_guard<std::mutex> Lock(DiagRefMutex);
+    DiagRefMap.erase(File);
   }
   {
     std::lock_guard<std::mutex> HLock(SemanticTokensMutex);
     LastSemanticTokens.erase(File);
   }
   // clangd will not send updates for this file anymore, so we empty out the
   // list of diagnostics shown on the client (e.g. in the "Problems" pane of
   // VSCode). Note that this cannot race with actual diagnostics responses
static std::optional<Command> asCommand(const CodeAction &Action) {
   Cmd.title = Action.title;
   if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
     Cmd.title = "Apply fix: " + Cmd.title;
   return Cmd;
 }
 
 void ClangdLSPServer::onCodeAction(const CodeActionParams &Params,
                                    Callback<llvm::json::Value> Reply) {
   URIForFile File = Params.textDocument.uri;
-  // Checks whether a particular CodeActionKind is included in the response.
-  auto KindAllowed = [Only(Params.context.only)](llvm::StringRef Kind) {
-    if (Only.empty())
-      return true;
-    return llvm::any_of(Only, [&](llvm::StringRef Base) {
-      return Kind.consume_front(Base) && (Kind.empty() || Kind.startswith("."));
-    });
-  };
+  std::map<ClangdServer::DiagRef, clangd::Diagnostic> ToLSPDiags;
+  ClangdServer::CodeActionInputs Inputs;
 
-  // We provide a code action for Fixes on the specified diagnostics.
-  std::vector<CodeAction> FixIts;
-  if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
-    for (const Diagnostic &D : Params.context.diagnostics) {
-      for (auto &CA : getFixes(File.file(), D)) {
-        FixIts.push_back(CA);
-        FixIts.back().diagnostics = {D};
-      }
-    }
+  for (const auto& LSPDiag : Params.context.diagnostics) {
+    if (auto DiagRef = getDiagRef(File.file(), LSPDiag)) {
+      ToLSPDiags[*DiagRef] = LSPDiag;
+      Inputs.Diagnostics.push_back(*DiagRef);
+  Inputs.File = File.file();
+  Inputs.Selection = Params.range;
+  Inputs.RequestedActionKinds = Params.context.only;
+  Inputs.TweakFilter = [this](const Tweak &T) {
+    return Opts.TweakFilter(T);
+  };
+  auto CB = [this,
+             Reply = std::move(Reply),
+             ToLSPDiags = std::move(ToLSPDiags), File,
+             Selection = Params.range](
+                llvm::Expected<ClangdServer::CodeActionResult> Fixits) mutable {
+    if (!Fixits)
+      return Reply(Fixits.takeError());
+    std::vector<CodeAction> CAs;
+    auto Version = decodeVersion(Fixits->Version);
+    for (const auto &QF : Fixits->QuickFixes) {
+      CAs.push_back(toCodeAction(QF.F, File, Version, SupportsDocumentChanges,
+                                 SupportsChangeAnnotation));
+      if (auto It = ToLSPDiags.find(QF.Diag);
+          It != ToLSPDiags.end()) {
+        CAs.back().diagnostics = {It->second};
       }
-
-  // Now enumerate the semantic code actions.
-  auto ConsumeActions =
-      [Diags = Params.context.diagnostics, Reply = std::move(Reply), File,
-       Selection = Params.range, FixIts = std::move(FixIts), this](
-          llvm::Expected<std::vector<ClangdServer::TweakRef>> Tweaks) mutable {
-        if (!Tweaks)
-          return Reply(Tweaks.takeError());
-
-        std::vector<CodeAction> Actions = std::move(FixIts);
-        Actions.reserve(Actions.size() + Tweaks->size());
-        for (const auto &T : *Tweaks)
-          Actions.push_back(toCodeAction(T, File, Selection));
+    }
+    for (const auto &TR : Fixits->TweakRefs)
+      CAs.push_back(toCodeAction(TR, File, Selection));
 
-        // If there's exactly one quick-fix, call it "preferred".
-        // We never consider refactorings etc as preferred.
-        CodeAction *OnlyFix = nullptr;
-        for (auto &Action : Actions) {
-          if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
-            if (OnlyFix) {
-              OnlyFix = nullptr;
-              break;
-            }
-            OnlyFix = &Action;
-          }
-        }
-        if (OnlyFix) {
-          OnlyFix->isPreferred = true;
-          if (Diags.size() == 1 && Diags.front().range == Selection)
-            OnlyFix->diagnostics = {Diags.front()};
-        }
+    for (auto &Action : CAs) {
+      if (ToLSPDiags.size() == 1 &&
+          ToLSPDiags.begin()->second.range == Selection)
+        OnlyFix->diagnostics = {ToLSPDiags.begin()->second};
 
-        if (SupportsCodeAction)
-          return Reply(llvm::json::Array(Actions));
-        std::vector<Command> Commands;
-        for (const auto &Action : Actions) {
-          if (auto Command = asCommand(Action))
-            Commands.push_back(std::move(*Command));
-        }
-        return Reply(llvm::json::Array(Commands));
-      };
-  Server->enumerateTweaks(
-      File.file(), Params.range,
-      [this, KindAllowed(std::move(KindAllowed))](const Tweak &T) {
-        return Opts.TweakFilter(T) && KindAllowed(T.kind());
-      },
-      std::move(ConsumeActions));
+      return Reply(llvm::json::Array(CAs));
+    for (const auto &Action : CAs) {
+  Server->codeAction(Inputs, std::move(CB));
 }
 
 void ClangdLSPServer::onCompletion(const CompletionParams &Params,
                                    Callback<CompletionList> Reply) {
   if (!shouldRunCompletion(Params)) {
     // Clients sometimes auto-trigger completions in undesired places (e.g.
     // 'a >^ '), we return empty results in those cases.
     vlog("ignored auto-triggered completion, preceding char did not match");
bool ClangdLSPServer::run() {
   return CleanExit && ShutdownRequestReceived;
 }
 
 void ClangdLSPServer::profile(MemoryTree &MT) const {
   if (Server)
     Server->profile(MT.child("clangd_server"));
 }
 
-std::vector<CodeAction> ClangdLSPServer::getFixes(llvm::StringRef File,
-                                                  const clangd::Diagnostic &D) {
-  std::lock_guard<std::mutex> Lock(FixItsMutex);
-  auto DiagToFixItsIter = FixItsMap.find(File);
-  if (DiagToFixItsIter == FixItsMap.end())
-    return {};
-
-  const auto &DiagToFixItsMap = DiagToFixItsIter->second;
-  auto FixItsIter = DiagToFixItsMap.find(D);
-  if (FixItsIter == DiagToFixItsMap.end())
-    return {};
+std::optional<ClangdServer::DiagRef>
+ClangdLSPServer::getDiagRef(StringRef File, const clangd::Diagnostic &D) {
+  std::lock_guard<std::mutex> Lock(DiagRefMutex);
+  auto DiagToDiagRefIter = DiagRefMap.find(File);
+  if (DiagToDiagRefIter == DiagRefMap.end())
+    return std::nullopt;
+
+  const auto &DiagToDiagRefMap = DiagToDiagRefIter->second;
+  auto FixItsIter = DiagToDiagRefMap.find(toDiagKey(D));
+  if (FixItsIter == DiagToDiagRefMap.end())
+    return std::nullopt;
 
   return FixItsIter->second;
 }
 
 // A completion request is sent when the user types '>' or ':', but we only
 // want to trigger on '->' and '::'. We check the preceding text to make
 // sure it matches what we expected.
 // Running the lexer here would be more robust (e.g. we can detect comments
bool ClangdLSPServer::shouldRunCompletion(
   return allowImplicitCompletion(*Code, *Offset);
 }
 
 void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                                          llvm::ArrayRef<Diag> Diagnostics) {
   PublishDiagnosticsParams Notification;
   Notification.version = decodeVersion(Version);
   Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
-  DiagnosticToReplacementMap LocalFixIts; // Temporary storage
+  DiagnosticToDiagRefMap LocalDiagMap; // Temporary storage
   for (auto &Diag : Diagnostics) {
     toLSPDiags(Diag, Notification.uri, DiagOpts,
-               [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+               [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef<Fix> Fixes) {
+                 if (DiagOpts.EmbedFixesInDiagnostics) {
                    std::vector<CodeAction> CodeActions;
                    for (const auto &Fix : Fixes)
-                   CodeActions.push_back(toCodeAction(Fix, Notification.uri,
-                                                      Notification.version,
-                                                      SupportsDocumentChanges,
-                                                      SupportsChangeAnnotation));
-
-                 if (DiagOpts.EmbedFixesInDiagnostics) {
-                   Diag.codeActions.emplace(CodeActions);
-                   if (Diag.codeActions->size() == 1)
-                     Diag.codeActions->front().isPreferred = true;
+                     CodeActions.push_back(toCodeAction(
+                         Fix, Notification.uri, Notification.version,
+                         SupportsDocumentChanges, SupportsChangeAnnotation));
+                   LSPDiag.codeActions.emplace(std::move(CodeActions));
+                   if (LSPDiag.codeActions->size() == 1)
+                     LSPDiag.codeActions->front().isPreferred = true;
                  }
-                 LocalFixIts[Diag] = std::move(CodeActions);
-                 Notification.diagnostics.push_back(std::move(Diag));
+                 LocalDiagMap[toDiagKey(LSPDiag)] = {Diag.Range, Diag.Message};
+                 Notification.diagnostics.push_back(std::move(LSPDiag));
                });
   }
 
-  // Cache FixIts
+  // Cache DiagRefMap
   {
-    std::lock_guard<std::mutex> Lock(FixItsMutex);
-    FixItsMap[File] = LocalFixIts;
+    std::lock_guard<std::mutex> Lock(DiagRefMutex);
+    DiagRefMap[File] = LocalDiagMap;
   }
 
   // Send a notification to the LSP client.
   PublishDiagnostics(Notification);
 }
 
 void ClangdLSPServer::onInactiveRegionsReady(
     PathRef File, std::vector<Range> InactiveRegions) {

clang-tools-extra/clangd/ClangdServer.h

 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
 #include <memory>
 #include <optional>
 #include <string>
-#include <type_traits>
-#include <utility>
+#include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 /// Manages a collection of source files and derived data (ASTs, indexes),
 /// and provides language-aware features such as code completion.
 ///
 /// The primary client is ClangdLSPServer which exposes these features via
public:
   void rename(PathRef File, Position Pos, llvm::StringRef NewName,
               const RenameOptions &Opts, Callback<RenameResult> CB);
 
   struct TweakRef {
     std::string ID;    /// ID to pass for applyTweak.
     std::string Title; /// A single-line message to show in the UI.
     llvm::StringLiteral Kind;
   };
+
+  // Ref to the clangd::Diag.
+  struct DiagRef {
+    Range Range;
+    std::string Message;
+    bool operator==(const DiagRef &Other) const {
+      return std::tie(Range, Message) == std::tie(Other.Range, Other.Message);
+    }
+    bool operator<(const DiagRef &Other) const {
+      return std::tie(Range, Message) < std::tie(Other.Range, Other.Message);
+    }
+  };
+
+  struct CodeActionInputs {
+    std::string File;
+    Range Selection;
+
+    /// Requested kind of actions to return.
+    std::vector<std::string> RequestedActionKinds;
+
+    /// Diagnostics attached to the code action request.
+    std::vector<DiagRef> Diagnostics;
+
+    /// Tweaks where Filter returns false will not be checked or included.
+    std::function<bool(const Tweak &)> TweakFilter;
+  };
+  struct CodeActionResult {
+    std::string Version;
+    struct QuickFix {
+      DiagRef Diag;
+      Fix F;
+    };
+    std::vector<QuickFix> QuickFixes;
+    std::vector<TweakRef> TweakRefs;
+  };
+  /// Surface code actions (quick-fixes for diagnostics, or available code
+  /// tweaks) for a given range in a file.
+  void codeAction(const CodeActionInputs &Inputs,
+                  Callback<CodeActionResult> CB);
+
   /// Enumerate the code tweaks available to the user at a specified point.
   /// Tweaks where Filter returns false will not be checked or included.
+  /// Deprecated, use codeAction instead.
   void enumerateTweaks(PathRef File, Range Sel,
                        llvm::unique_function<bool(const Tweak &)> Filter,
                        Callback<std::vector<TweakRef>> CB);
 
   /// Apply the code tweak with a specified \p ID.
   void applyTweak(PathRef File, Range Sel, StringRef ID,
                   Callback<Tweak::Effect> CB);
 

clang-tools-extra/clangd/ClangdServer.cpp

 #include <algorithm>
 #include <chrono>
 #include <future>
 #include <memory>
 #include <mutex>
 #include <optional>
 #include <string>
 #include <type_traits>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+// Tracks number of times a tweak has been offered.
+static constexpr trace::Metric TweakAvailable(
+    "tweak_available", trace::Metric::Counter, "tweak_id");
+
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
   UpdateIndexCallbacks(FileIndex *FIndex,
                        ClangdServer::Callbacks *ServerCallbacks,
                        const ThreadsafeFS &TFS, AsyncTaskRunner *Tasks,
                        bool CollectInactiveRegions,
                        const ClangdServer::Options &Opts)
       : FIndex(FIndex), ServerCallbacks(ServerCallbacks),
SelectionTree::createEach(
         Result.push_back(std::make_unique<Tweak::Selection>(
             AST.Inputs.Index, AST.AST, *Begin, *End, std::move(T), FS));
         return false;
       });
   assert(!Result.empty() && "Expected at least one SelectionTree");
   return std::move(Result);
 }
 
+void ClangdServer::codeAction(const CodeActionInputs &Params,
+                              Callback<CodeActionResult> CB) {
+  auto Action = [Params, CB = std::move(CB),
+                 FeatureModules(this->FeatureModules)](
+                    Expected<InputsAndAST> InpAST) mutable {
+    if (!InpAST)
+      return CB(InpAST.takeError());
+    auto KindAllowed =
+        [Only(Params.RequestedActionKinds)](llvm::StringRef Kind) {
+          if (Only.empty())
+            return true;
+          return llvm::any_of(Only, [&](llvm::StringRef Base) {
+            return Kind.consume_front(Base) &&
+                   (Kind.empty() || Kind.startswith("."));
+          });
+        };
+
+    CodeActionResult Result;
+    Result.Version = InpAST->AST.version().str();
+    if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
+      auto FindMatchedFixes =
+          [&InpAST](const DiagRef &DR) -> llvm::ArrayRef<Fix> {
+        for (const auto &Diag : InpAST->AST.getDiagnostics())
+          if (Diag.Range == DR.Range && Diag.Message == DR.Message)
+            return Diag.Fixes;
+        return {};
+      };
+      for (const auto &Diag : Params.Diagnostics)
+        for (const auto &Fix : FindMatchedFixes(Diag))
+          Result.QuickFixes.push_back({Diag, Fix});
+    }
+
+    // Collect Tweaks
+    auto Selections = tweakSelection(Params.Selection, *InpAST, /*FS=*/nullptr);
+    if (!Selections)
+      return CB(Selections.takeError());
+    // Don't allow a tweak to fire more than once across ambiguous selections.
+    llvm::DenseSet<llvm::StringRef> PreparedTweaks;
+    auto DeduplicatingFilter = [&](const Tweak &T) {
+      return KindAllowed(T.kind()) && Params.TweakFilter(T) &&
+             !PreparedTweaks.count(T.id());
+    };
+    for (const auto &Sel : *Selections) {
+      for (auto &T : prepareTweaks(*Sel, DeduplicatingFilter, FeatureModules)) {
+        Result.TweakRefs.push_back(TweakRef{T->id(), T->title(), T->kind()});
+        PreparedTweaks.insert(T->id());
+        TweakAvailable.record(1, T->id());
+      }
+    }
+    CB(std::move(Result));
+  };
+
+  WorkScheduler->runWithAST("codeAction", Params.File, std::move(Action),
+                            Transient);
+}
+
 void ClangdServer::enumerateTweaks(
     PathRef File, Range Sel, llvm::unique_function<bool(const Tweak &)> Filter,
     Callback<std::vector<TweakRef>> CB) {
-  // Tracks number of times a tweak has been offered.
-  static constexpr trace::Metric TweakAvailable(
-      "tweak_available", trace::Metric::Counter, "tweak_id");
   auto Action = [Sel, CB = std::move(CB), Filter = std::move(Filter),
                  FeatureModules(this->FeatureModules)](
                     Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto Selections = tweakSelection(Sel, *InpAST, /*FS=*/nullptr);
     if (!Selections)
       return CB(Selections.takeError());
     std::vector<TweakRef> Res;

clang-tools-extra/clangd/Protocol.h

struct Diagnostic {
   /// `textDocument/publishDiagnostics` notification
   /// and `textDocument/codeAction` request.
   /// Mutating users should associate their data with a unique key they can use
   /// to retrieve later on.
   llvm::json::Object data;
 };
 llvm::json::Value toJSON(const Diagnostic &);
 
-/// A LSP-specific comparator used to find diagnostic in a container like
-/// std:map.
-/// We only use the required fields of Diagnostic to do the comparison to avoid
-/// any regression issues from LSP clients (e.g. VScode), see
-/// https://git.io/vbr29
-struct LSPDiagnosticCompare {
-  bool operator()(const Diagnostic &LHS, const Diagnostic &RHS) const {
-    return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message);
-  }
-};
 bool fromJSON(const llvm::json::Value &, Diagnostic &, llvm::json::Path);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Diagnostic &);
 
 struct PublishDiagnosticsParams {
   /// The URI for which diagnostic information is reported.
   URIForFile uri;
   /// An array of diagnostic information items.
   std::vector<Diagnostic> diagnostics;