[clangd] Propagate CodeActions in addition to Fixes for diagnostics

[kadircet]

Summary

Fixes only allow modifications to local file containing the diagnostics,
with CodeActions we'll be able to provide changes that can fix the issue
outside of the local file (e.g. build file/header modificiations).

As an implementation detail, we now covert all the fixes to codeactions
directly at publish time, rather than onCodeAction requests. So this
implies some extra copies on each diag release.

[njames93]

Emitting fixes for a diagnostic in another file seems dangerous, what's the intended use case for this?

[kadircet]

Emitting fixes for a diagnostic in another file seems dangerous, what's the intended use case for this?

one particular example is fixing layering violations in more strict build systems like bazel/blaze. for example if you #include "a.h" in a file, but build target of that particular file doesn't depend on a target exporting a.h clangd can generate an edit to update the relevant build target to have a dependency on a.h.

clang-tools-extra/clangd/Diagnostics.h

113

we keep broadcasting fixes and actions separately to keep embedders that can't handle codeactions happy.

we can start emitting just codeactions here by converting fixes to codeactions in this layer (all we need is fileuri, and we have it). then embedders can still iterate over all code actions and pick only the edits for mainfile while ignoring the rest.

[kadircet]

• Rebase

[kadircet]

• Update header

[kadircet]

ping :)

[sammccall]

Will the fixes/actions this enables really be available synchronously?

AFAIK the only reasons we attach fixes to diagnostics are:

• because clang fixes are generated at parse time and need to be stored somewhere
• for the benefit of embedders that need the fix content synchronously with the diagnostic
• to trigger the "fix available" text

The first two points don't apply if the fix content is too slow to compute synchronously. (And the third can be more simply solved with a boolean flag instead)

The alternative is just to discover these fixes through the code action flow (and allow modules to contribute code actions, which we should do anyway)

[kadircet]

In D96245#2602293, @sammccall wrote:

Will the fixes/actions this enables really be available synchronously?

The fixes stay as is today, but the action might be just a command that needs to be invoked by the client (so it is still synchronous, in a sense, but instead of an edit embedder just initiates another action on the clangdserver, which will synchronously do "something")

AFAIK the only reasons we attach fixes to diagnostics are:

• because clang fixes are generated at parse time and need to be stored somewhere
• for the benefit of embedders that need the fix content synchronously with the diagnostic
• to trigger the "fix available" text

The first two points don't apply if the fix content is too slow to compute synchronously. (And the third can be more simply solved with a boolean flag instead)
The alternative is just to discover these fixes through the code action flow (and allow modules to contribute code actions, which we should do anyway)

Agreed, I was going this way because currently there was no way to provide a code action to clangd::diagnostic apart from storing it in the fixes map while generating it. I suppose it is more sensible to just go with a new hook on modules that can provide a codeaction given a clangd::diagnostic. (and when the diagnostic is seen, the module can just set a boolean flag or increment some fix counter as you suggested).

Does std::vector<CodeAction> contributeActions(const CodeActionParams&); look like a good signature for the module hook?

[njames93]

One little curveball I'm still kind of hoping we can integrate clang-format into our codeAction workflow, in the hopes of addressing https://github.com/clangd/clangd/issues/429.
While we compute the edits eagerly we don't actually send them until the client request for codeActions so it may be possible to pipe them into clang-format then.

[sammccall]

In D96245#2602896, @kadircet wrote:

Agreed, I was going this way because currently there was no way to provide a code action to clangd::diagnostic apart from storing it in the fixes map while generating it. I suppose it is more sensible to just go with a new hook on modules that can provide a codeaction given a clangd::diagnostic.

I think this is what you meant, but just to be sure... there's no fundamental need in LSP to make any association between code actions and diagnostics until a codeAction request comes in.
There's no need to put anything into FixesMap. (We do have an extension/C++ API that exposes a different model, but as discussed offline I think we should just not expose this through the extension for now)

Does std::vector<CodeAction> contributeActions(const CodeActionParams&); look like a good signature for the module hook?

A couple of issues:

• this will require modules to duplicate protocol details (e.g. CodeActionContext.only)
• many modules that want to provide code actions will want an AST, and we build one for the codeAction request, we should be able to share it
• it's not obvious how to use these without ClangdLSPServer if the CodeAction wraps a Command whose handler must be bound

We have the Tweak infrastructure that's basically designed to solve all these problems. It has some issues (like you can only return one fix per "class") but these are fixable and would be nice to have fixed in any case.
This means we'd be using a fixed LSP applyTweak command rather than binding a custom command for this action as planned, but it might work well?

Interface would just look like contributeTweaks(vector<unique_ptr<Tweak>>&) or something

WDYT

[kadircet]

abandoning in favor of D98498

clang-tools-extra/clangd/ClangdLSPServer.h

LSPBinder::OutgoingNotification<ProgressParams<WorkDoneProgressReport>>
       ReportWorkDoneProgress;
   LSPBinder::OutgoingNotification<ProgressParams<WorkDoneProgressEnd>>
       EndWorkDoneProgress;
 
   void applyEdit(WorkspaceEdit WE, llvm::json::Value Success,
                  Callback<llvm::json::Value> Reply);
 
   void bindMethods(LSPBinder &);
-  std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
+  std::vector<CodeAction> getFixes(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;
 
   /// Requests a reparse of currently opened files using their latest source.
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<Fix>, LSPDiagnosticCompare>
+  typedef std::map<clangd::Diagnostic, std::vector<CodeAction>,
+                   LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
   /// Caches FixIts per file and diagnostics
   llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
   // Last semantic-tokens response, for incremental requests.
   std::mutex SemanticTokensMutex;
   llvm::StringMap<SemanticTokens> LastSemanticTokens;
 
   // Most code should not deal with Transport, callMethod, notify directly.

clang-tools-extra/clangd/ClangdLSPServer.cpp

 #include "support/Context.h"
 #include "support/MemoryTree.h"
 #include "support/Trace.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/Version.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
auto KindAllowed = [Only(Params.context.only)](llvm::StringRef Kind) {
     });
   };
 
   // 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 &F : getFixes(File.file(), D)) {
-        FixIts.push_back(toCodeAction(F, Params.textDocument.uri));
+        FixIts.emplace_back(std::move(F));
         FixIts.back().diagnostics = {D};
       }
     }
   }
 
   // Now enumerate the semantic code actions.
   auto ConsumeActions =
       [Reply = std::move(Reply), File, Code = std::move(*Code),
bool ClangdLSPServer::run() {
   return CleanExit && ShutdownRequestReceived;
 }
 
 void ClangdLSPServer::profile(MemoryTree &MT) const {
   if (Server)
     Server->profile(MT.child("clangd_server"));
 }
 
-std::vector<Fix> ClangdLSPServer::getFixes(llvm::StringRef File,
+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())

 void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                                          std::vector<Diag> Diagnostics) {
   PublishDiagnosticsParams Notification;
   Notification.version = decodeVersion(Version);
   Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
     toLSPDiags(Diag, Notification.uri, DiagOpts,
-               [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+               [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes,
+                   llvm::ArrayRef<CodeAction> Actions) {
                  auto &FixItsForDiagnostic = LocalFixIts[Diag];
-                 llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+                 llvm::copy(Actions, std::back_inserter(FixItsForDiagnostic));
+                 for (const auto &Fix : Fixes) {
+                   FixItsForDiagnostic.emplace_back(
+                       toCodeAction(Fix, Notification.uri));
+                 }
                  Notification.diagnostics.push_back(std::move(Diag));
                });
   }
 
   // Cache FixIts
   {
     std::lock_guard<std::mutex> Lock(FixItsMutex);
     FixItsMap[File] = LocalFixIts;

clang-tools-extra/clangd/Diagnostics.h

enum DiagSource {
     Clang,
     ClangTidy,
     ClangdConfig,
   } Source = Unknown;
   /// Elaborate on the problem, usually pointing to a related piece of code.
   std::vector<Note> Notes;
   /// *Alternative* fixes for this diagnostic, one should be chosen.
   std::vector<Fix> Fixes;
+  std::vector<CodeAction> Actions;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
 Diag toDiag(const llvm::SMDiagnostic &, Diag::DiagSource Source);
 
 /// Conversion to LSP diagnostics. Formats the error message of each diagnostic
 /// to include all its notes. Notes inside main file are also provided as
 /// separate diagnostics with their corresponding fixits. Notes outside main
 /// file do not have a corresponding LSP diagnostic, but can still be included
 /// as part of their main diagnostic's message.
-void toLSPDiags(
-    const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
-    llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn);
+void toLSPDiags(const Diag &D, const URIForFile &File,
+                const ClangdDiagnosticOptions &Opts,
+                llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>,
+                                        llvm::ArrayRef<CodeAction>)>
+                    OutFn);
 
 /// Convert from Fix to LSP CodeAction.
 CodeAction toCodeAction(const Fix &D, const URIForFile &File);
 
 /// Convert from clang diagnostic level to LSP severity.
 int getSeverity(DiagnosticsEngine::Level L);
 
 /// StoreDiags collects the diagnostics that can later be reported by

clang-tools-extra/clangd/Diagnostics.cpp

if (D.getRanges().empty())
     Result.Range = {{D.getLineNo() - 1, D.getColumnNo()},
                     {D.getLineNo() - 1, D.getColumnNo()}};
   else
     Result.Range = {{D.getLineNo() - 1, (int)D.getRanges().front().first},
                     {D.getLineNo() - 1, (int)D.getRanges().front().second}};
   return Result;
 }
 
-void toLSPDiags(
-    const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
-    llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
+void toLSPDiags(const Diag &D, const URIForFile &File,
+                const ClangdDiagnosticOptions &Opts,
+                llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>,
+                                        llvm::ArrayRef<CodeAction>)>
+                    OutFn) {
   clangd::Diagnostic Main;
   Main.severity = getSeverity(D.Severity);
 
   // Main diagnostic should always refer to a range inside main file. If a
   // diagnostic made it so for, it means either itself or one of its notes is
   // inside main file.
   if (D.InsideMainFile) {
     Main.range = D.Range;
case Diag::ClangTidy:
     break;
   case Diag::ClangdConfig:
     Main.source = "clangd-config";
     break;
   case Diag::Unknown:
     break;
   }
   if (Opts.EmbedFixesInDiagnostics) {
-    Main.codeActions.emplace();
+    Main.codeActions = D.Actions;
     for (const auto &Fix : D.Fixes)
       Main.codeActions->push_back(toCodeAction(Fix, File));
     if (Main.codeActions->size() == 1)
       Main.codeActions->front().isPreferred = true;
   }
   if (Opts.SendDiagnosticCategory && !D.Category.empty())
     Main.category = D.Category;
 
   Main.message = mainMessage(D, Opts);
   if (Opts.EmitRelatedLocations) {
     Main.relatedInformation.emplace();
     for (auto &Note : D.Notes) {
       if (!Note.AbsFile) {
         vlog("Dropping note from unknown file: {0}", Note);
         continue;
       }
       DiagnosticRelatedInformation RelInfo;
       RelInfo.location.range = Note.Range;
       RelInfo.location.uri =
           URIForFile::canonicalize(*Note.AbsFile, File.file());
       RelInfo.message = noteMessage(D, Note, Opts);
       Main.relatedInformation->push_back(std::move(RelInfo));
     }
   }
-  OutFn(std::move(Main), D.Fixes);
+  OutFn(std::move(Main), D.Fixes, D.Actions);
 
   // If we didn't emit the notes as relatedLocations, emit separate diagnostics
   // so the user can find the locations easily.
   if (!Opts.EmitRelatedLocations)
     for (auto &Note : D.Notes) {
       if (!Note.InsideMainFile)
         continue;
       clangd::Diagnostic Res;
       Res.severity = getSeverity(Note.Severity);
       Res.range = Note.Range;
       Res.message = noteMessage(D, Note, Opts);
-      OutFn(std::move(Res), llvm::ArrayRef<Fix>());
+      OutFn(std::move(Res), {}, {});
     }
 }
 
 int getSeverity(DiagnosticsEngine::Level L) {
   switch (L) {
   case DiagnosticsEngine::Remark:
     return 4;
   case DiagnosticsEngine::Note:
if (Tidy != nullptr) {
         continue;
       }
     }
   }
   // Deduplicate clang-tidy diagnostics -- some clang-tidy checks may emit
   // duplicated messages due to various reasons (e.g. the check doesn't handle
   // template instantiations well; clang-tidy alias checks).
   std::set<std::pair<Range, std::string>> SeenDiags;
-  llvm::erase_if(Output, [&](const Diag& D) {
+  llvm::erase_if(Output, [&](const Diag &D) {
     return !SeenDiags.emplace(D.Range, D.Message).second;
   });
   return std::move(Output);
 }
 
 void StoreDiags::BeginSourceFile(const LangOptions &Opts,
                                  const Preprocessor *PP) {
   LangOpts = Opts;

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

note: declared somewhere in the header file)";
   NoteInMainLSP.message = R"(Declared somewhere in the main file
 
 main.cpp:2:3: error: something terrible happened)";
 
   ClangdDiagnosticOptions Opts;
   // Transform diagnostics and check the results.
   std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
   toLSPDiags(D, MainFile, Opts,
-             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes,
+                 ArrayRef<CodeAction>) {
                LSPDiags.push_back(
                    {std::move(LSPDiag),
                     std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
              });
 
   EXPECT_THAT(
       LSPDiags,
       ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
                   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty())));
   EXPECT_EQ(LSPDiags[0].first.code, "undeclared_var_use");
   EXPECT_EQ(LSPDiags[0].first.source, "clang");
   EXPECT_EQ(LSPDiags[1].first.code, "");
   EXPECT_EQ(LSPDiags[1].first.source, "");
 
   // Same thing, but don't flatten notes into the main list.
   LSPDiags.clear();
   Opts.EmitRelatedLocations = true;
   toLSPDiags(D, MainFile, Opts,
-             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes,
+                 ArrayRef<CodeAction>) {
                LSPDiags.push_back(
                    {std::move(LSPDiag),
                     std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
              });
   MainLSP.message = "Something terrible happened (fix available)";
   DiagnosticRelatedInformation NoteInMainDRI;
   NoteInMainDRI.message = "Declared somewhere in the main file";
   NoteInMainDRI.location.range = NoteInMain.Range;
TEST(ToLSPDiag, RangeIsInMain) {
   D.Range = {pos(1, 2), pos(3, 4)};
   D.Notes.emplace_back();
   Note &N = D.Notes.back();
   N.Range = {pos(2, 3), pos(3, 4)};
 
   D.InsideMainFile = true;
   N.InsideMainFile = false;
   toLSPDiags(D, {}, Opts,
-             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
-               EXPECT_EQ(LSPDiag.range, D.Range);
-             });
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>,
+                 ArrayRef<CodeAction>) { EXPECT_EQ(LSPDiag.range, D.Range); });
 
   D.InsideMainFile = false;
   N.InsideMainFile = true;
   toLSPDiags(D, {}, Opts,
-             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
-               EXPECT_EQ(LSPDiag.range, N.Range);
-             });
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>,
+                 ArrayRef<CodeAction>) { EXPECT_EQ(LSPDiag.range, N.Range); });
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang