[clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.

[sammccall]

Summary

CodeAction provides us with a standard way of representing fixes inline, so
use it, replacing our existing ad-hoc extension.

After this, it's easy to serialize diagnostics using the structured
toJSON/Protocol.h mechanism rather than assembling JSON ad-hoc.

[hokein]

LGTM.

clangd/ClangdLSPServer.cpp

558

nit: we can reuse URI insteading of calling URIForFile again.

[sammccall]

Don't call URIForFile again, rebase.

[sammccall]

Thanks! @arphaman, any concerns about the change to extension format?

clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
   // We provide a code action for Fixes on the specified diagnostics.
   if (!DraftMgr.getDraft(Params.textDocument.uri.file()))
     return replyError(ErrorCode::InvalidParams,
                       "onCodeAction called for non-added file");
 
   std::vector<CodeAction> Actions;
   for (Diagnostic &D : Params.context.diagnostics) {
     for (auto &F : getFixes(Params.textDocument.uri.file(), D)) {
-      Actions.emplace_back();
-      Actions.back().title = F.Message;
-      Actions.back().kind = CodeAction::QUICKFIX_KIND;
+      Actions.push_back(toCodeAction(F, Params.textDocument.uri));
       Actions.back().diagnostics = {D};
-      Actions.back().edit.emplace();
-      Actions.back().edit->changes.emplace();
-      (*Actions.back().edit->changes)[Params.textDocument.uri.uri()] = {
-          F.Edits.begin(), F.Edits.end()};
     }
   }
 
   if (SupportsCodeAction)
     reply(json::Array(Actions));
   else {
     std::vector<Command> Commands;
     for (const auto &Action : Actions)
std::vector<Fix> ClangdLSPServer::getFixes(StringRef File,
   if (FixItsIter == DiagToFixItsMap.end())
     return {};
 
   return FixItsIter->second;
 }
 
 void ClangdLSPServer::onDiagnosticsReady(PathRef File,
                                          std::vector<Diag> Diagnostics) {
-  json::Array DiagnosticsJSON;
-
+  URIForFile URI(File);
+  std::vector<Diagnostic> LSPDiagnostics;
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
-    toLSPDiags(Diag, [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
-      json::Object LSPDiag({
-          {"range", Diag.range},
-          {"severity", Diag.severity},
-          {"message", Diag.message},
-      });
-      // LSP extension: embed the fixes in the diagnostic.
-      if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
-        json::Array ClangdFixes;
-        for (const auto &Fix : Fixes) {
-          WorkspaceEdit WE;
-          URIForFile URI{File};
-          WE.changes = {{URI.uri(), std::vector<TextEdit>(Fix.Edits.begin(),
-                                                          Fix.Edits.end())}};
-          ClangdFixes.push_back(
-              json::Object{{"edit", toJSON(WE)}, {"title", Fix.Message}});
-        }
-        LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
-      }
-      if (DiagOpts.SendDiagnosticCategory && !Diag.category.empty())
-        LSPDiag["category"] = Diag.category;
-      DiagnosticsJSON.push_back(std::move(LSPDiag));
-
+    toLSPDiags(Diag, URI, DiagOpts,
+               [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
                  auto &FixItsForDiagnostic = LocalFixIts[Diag];
                  llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+                 LSPDiagnostics.push_back(std::move(Diag));
                });
   }
 
   // Cache FixIts
   {
     // FIXME(ibiryukov): should be deleted when documents are removed
     std::lock_guard<std::mutex> Lock(FixItsMutex);
     FixItsMap[File] = LocalFixIts;
   }
 
   // Publish diagnostics.
   Transp.notify("textDocument/publishDiagnostics",
                 json::Object{
                     {"uri", URIForFile{File}},
-                    {"diagnostics", std::move(DiagnosticsJSON)},
+                    {"diagnostics", std::move(LSPDiagnostics)},
                 });
 }
 
 void ClangdLSPServer::reparseOpenedFiles() {
   for (const Path &FilePath : DraftMgr.getActiveFiles())
     Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                         WantDiagnostics::Auto);
 }

clangd/Diagnostics.h

 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
 /// 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 Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> 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
 /// clangd. It groups all notes for a diagnostic into a single Diag
 /// and filters out diagnostics that don't mention the main file (i.e. neither
 /// the diag itself nor its notes are in the main file).
 class StoreDiags : public DiagnosticConsumer {

clangd/Diagnostics.cpp

if (!D.Fixes.empty()) {
     for (auto &Fix : D.Fixes) {
       OS << Sep << Fix;
       Sep = ", ";
     }
   }
   return OS;
 }
 
+CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
+  CodeAction Action;
+  Action.title = F.Message;
+  Action.kind = CodeAction::QUICKFIX_KIND;
+  Action.edit.emplace();
+  Action.edit->changes.emplace();
+  (*Action.edit->changes)[File.uri()] = {F.Edits.begin(), F.Edits.end()};
+  return Action;
+}
+
 void toLSPDiags(
-    const Diag &D,
+    const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
   auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
     clangd::Diagnostic Res;
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
-    Res.category = D.Category;
     return Res;
   };
 
   {
     clangd::Diagnostic Main = FillBasicFields(D);
     Main.message = mainMessage(D);
+    if (Opts.EmbedFixesInDiagnostics) {
+      Main.codeActions.emplace();
+      for (const auto &Fix : D.Fixes)
+        Main.codeActions->push_back(toCodeAction(Fix, File));
+    }
+    if (Opts.SendDiagnosticCategory && !D.Category.empty())
+      Main.category = D.Category;
+
     OutFn(std::move(Main), D.Fixes);
   }
 
   for (auto &Note : D.Notes) {
     if (!Note.InsideMainFile)
       continue;
     clangd::Diagnostic Res = FillBasicFields(Note);
     Res.message = noteMessage(D, Note);

clangd/Protocol.h

 // This struct doesn't mirror LSP!
 // The protocol defines deeply nested structures for client capabilities.
 // Instead of mapping them all, this just parses out the bits we care about.
 struct ClientCapabilities {
   /// The supported set of SymbolKinds for workspace/symbol.
   /// workspace.symbol.symbolKind.valueSet
   llvm::Optional<SymbolKindBitset> WorkspaceSymbolKinds;
 
-  /// Whether the client accepts diagnostics with fixes attached using the
-  /// "clangd_fixes" extension.
-  /// textDocument.publishDiagnostics.clangdFixSupport
+  /// Whether the client accepts diagnostics with codeActions attached inline.
+  /// textDocument.publishDiagnostics.codeActionsInline.
   bool DiagnosticFixes = false;
 
   /// Whether the client accepts diagnostics with category attached to it
   /// using the "category" extension.
   /// textDocument.publishDiagnostics.categorySupport
   bool DiagnosticCategory = false;
 
   /// Client supports snippets as insert text.

 bool fromJSON(const llvm::json::Value &, DocumentFormattingParams &);
 
 struct DocumentSymbolParams {
   // The text document to find symbols in.
   TextDocumentIdentifier textDocument;
 };
 bool fromJSON(const llvm::json::Value &, DocumentSymbolParams &);
 
+struct CodeAction;
 struct Diagnostic {
   /// The range at which the message applies.
   Range range;
 
   /// The diagnostic's severity. Can be omitted. If omitted it is up to the
   /// client to interpret diagnostics as error, warning, info or hint.
   int severity = 0;
 
   /// The diagnostic's code. Can be omitted.
   /// Note: Not currently used by clangd
   // std::string code;
 
   /// A human-readable string describing the source of this
   /// diagnostic, e.g. 'typescript' or 'super lint'.
   /// Note: Not currently used by clangd
   // std::string source;
 
   /// The diagnostic's message.
   std::string message;
 
   /// The diagnostic's category. Can be omitted.
   /// An LSP extension that's used to send the name of the category over to the
   /// client. The category typically describes the compilation stage during
   /// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".
-  std::string category;
+  llvm::Optional<std::string> category;
+
+  /// Clangd extension: code actions related to this diagnostic.
+  /// Only with capability textDocument.publishDiagnostics.codeActionsInline.
+  /// (These actions can also be obtained using textDocument/codeAction).
+  llvm::Optional<std::vector<CodeAction>> codeActions;
 };
 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 comparsion to avoid
 /// any regression issues from LSP clients (e.g. VScode), see
 /// https://git.io/vbr29

clangd/Protocol.cpp

 bool fromJSON(const json::Value &Params, ClientCapabilities &R) {
   const json::Object *O = Params.getAsObject();
   if (!O)
     return false;
   if (auto *TextDocument = O->getObject("textDocument")) {
     if (auto *Diagnostics = TextDocument->getObject("publishDiagnostics")) {
       if (auto CategorySupport = Diagnostics->getBoolean("categorySupport"))
         R.DiagnosticCategory = *CategorySupport;
-      if (auto ClangdFixSupport = Diagnostics->getBoolean("clangdFixSupport"))
-        R.DiagnosticFixes = *ClangdFixSupport;
+      if (auto CodeActions = Diagnostics->getBoolean("codeActionsInline"))
+        R.DiagnosticFixes = *CodeActions;
     }
     if (auto *Completion = TextDocument->getObject("completion")) {
       if (auto *Item = Completion->getObject("completionItem")) {
         if (auto SnippetSupport = Item->getBoolean("snippetSupport"))
           R.CompletionSnippets = *SnippetSupport;
       }
       if (auto *ItemKind = Completion->getObject("completionItemKind")) {
         if (auto *ValueSet = ItemKind->get("valueSet")) {

 }
 
 llvm::json::Value toJSON(const Diagnostic &D) {
   json::Object Diag{
       {"range", D.range},
       {"severity", D.severity},
       {"message", D.message},
   };
-  // FIXME: this should be used for publishDiagnostics.
-  // FIXME: send category and fixes when appropriate.
+  if (D.category)
+    Diag["category"] = *D.category;
+  if (D.codeActions)
+    Diag["codeActions"] = D.codeActions;
   return std::move(Diag);
 }
 
 bool fromJSON(const json::Value &Params, Diagnostic &R) {
   json::ObjectMapper O(Params);
   if (!O || !O.map("range", R.range) || !O.map("message", R.message))
     return false;
   O.map("severity", R.severity);
+  O.map("category", R.category);
   return true;
 }
 
 bool fromJSON(const json::Value &Params, CodeActionContext &R) {
   json::ObjectMapper O(Params);
   return O && O.map("diagnostics", R.diagnostics);
 }
 

test/clangd/fixits-embed-in-diagnostic.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"clangdFixSupport":true}}},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"codeActionsInline":true}}},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
-#      CHECK:    "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:    "params": {
-# CHECK-NEXT:     "diagnostics": [
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "clangd_fixes": [
+# CHECK-NEXT:        "codeActions": [
 # CHECK-NEXT:          {
 # CHECK-NEXT:            "edit": {
 # CHECK-NEXT:              "changes": {
 # CHECK-NEXT:                "file://{{.*}}/foo.c": [
 # CHECK-NEXT:                  {
 # CHECK-NEXT:                    "newText": "struct",
 # CHECK-NEXT:                    "range": {
 # CHECK-NEXT:                      "end": {
 # CHECK-NEXT:                        "character": 22,
 # CHECK-NEXT:                        "line": 0
 # CHECK-NEXT:                      },
 # CHECK-NEXT:                      "start": {
 # CHECK-NEXT:                        "character": 17,
 # CHECK-NEXT:                        "line": 0
 # CHECK-NEXT:                      }
 # CHECK-NEXT:                    }
 # CHECK-NEXT:                  }
 # CHECK-NEXT:                ]
 # CHECK-NEXT:              }
 # CHECK-NEXT:            },
+# CHECK-NEXT:            "kind": "quickfix",
 # CHECK-NEXT:            "title": "change 'union' to 'struct'"
 # CHECK-NEXT:          }
 # CHECK-NEXT:        ],
 # CHECK-NEXT:        "message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 22,
 # CHECK-NEXT:            "line": 0

unittests/clangd/ClangdUnitTests.cpp

note: declared somewhere in the header file)");
 
   clangd::Diagnostic NoteInMainLSP =
       MatchingLSP(NoteInMain, R"(Declared somewhere in the main file
 
 main.cpp:2:3: error: something terrible happened)");
 
   // Transform dianostics and check the results.
   std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
-  toLSPDiags(D, [&](clangd::Diagnostic LSPDiag,
-                    llvm::ArrayRef<clangd::Fix> Fixes) {
-    LSPDiags.push_back({std::move(LSPDiag),
-                        std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
+  toLSPDiags(
+      D, URIForFile("/path/to/foo/bar/main.cpp"), ClangdDiagnosticOptions(),
+      [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef<clangd::Fix> Fixes) {
+        LSPDiags.push_back(
+            {std::move(LSPDiag),
       });
 
   EXPECT_THAT(
       LSPDiags,
       ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
                   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty())));
 }
 
 TEST(ClangdUnitTest, GetBeginningOfIdentifier) {