[clangd] Propagate data in diagnostics

[sammccall]

clang-tools-extra/clangd/Diagnostics.h

59

Second sentence is confusing because of inverted sense. And really I don't think the reason is that we don't want to send extra data, but rather the fear that old clients will choke on it.

If we're *not* afraid of that, i.e. we think they'll just drop it on the floor, then I don't think we should bother to feature-detect it just so *we* can drop it on the floor.
Not sure how you feel about this, but it's pretty tempting to me...

78

Maybe call this OpaqueData or Data?
I like the comment but I'm not sure where this is ultimately set is so important at this level (and we might use it in other ways!)

clang-tools-extra/clangd/Protocol.h

851

The array representation seems a bit awkward in practice: it's ~always going to have 1 element (or none), so it seems like unnecessary wrapping.
I think the "natural" representation is something like {"fooTweakParam": 42} or {"fooTweakParams": {"x": 1, "y": 2}}.

This array prevents tweaks from clobbering each other but OTOH it means the data they read doesn't match the data they write. I think making data an Object and passing in a mutable ref to it will work fine in practice.

[kadircet]

• Ignore capability
• Store data as a json::Object rather than an Array.

[kadircet]

clang-tools-extra/clangd/Diagnostics.h

59

as discussed offline dropping this completely. since we don't really guard against adding "extra" properties to objects, and the worst that could happen is clangd won't surface a particular tweak on clients without support.

[sammccall]

clang-tools-extra/clangd/Diagnostics.h

78

Hmm, you've replaced the json::Array with an array of objects :-)
Any reason we can't just use llvm::json::Object here?

We don't really handle conflicts anyway, and I don't think having one tweak read another one's data out of this field is a real concern.

clang-tools-extra/clangd/Protocol.cpp

615

std::move is a fancy way to spell copy here, since Params is const.

json::Value(*Data) (or can you just use mapOptOrNull?)

clang-tools-extra/clangd/Protocol.h

413

this is now unused

clang-tools-extra/clangd/refactor/Tweak.h

71

unrelated?
(well not quite, but neither populated or used in this patch)

[kadircet]

clang-tools-extra/clangd/Diagnostics.h

78

oops, that wasn't the intention. changed to llvm::json::Object.

clang-tools-extra/clangd/Protocol.cpp

615

std::move is a fancy way to spell copy here, since Params is const.

right.. dropped that.

json::Value(*Data) (or can you just use mapOptOrNull?)

we can't mapOptOrNull as fromJSON doesn't have output specializations for JSON types. i don't think it is worth having them (i think, they would actually be confusing). but if you think it's worth, maybe we can have some identity mapper specialization.

clang-tools-extra/clangd/refactor/Tweak.h

71

yeah this was supposed to be a separate change but forgot to strip this bit off..

[kadircet]

• Address comments

clang-tools-extra/clangd/Diagnostics.h

 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/SourceMgr.h"
 #include <cassert>
 #include <functional>
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 namespace clang {
 namespace tidy {
 class ClangTidyContext;
 } // namespace tidy
 namespace clangd {
 
 struct ClangdDiagnosticOptions {
struct ClangdDiagnosticOptions {
   /// Issue".
   bool SendDiagnosticCategory = false;
 
   /// If true, Clangd will add a number of available fixes to the diagnostic's
   /// message.
   bool DisplayFixesCount = true;
 };
 
 /// Contains basic information about a diagnostic.
 struct DiagBase {
   std::string Message;
   // Intended to be used only in error messages.
   // May be relative, absolute or even artificially constructed.
   std::string File;
   // Absolute path to containing file, if available.
   llvm::Optional<std::string> AbsFile;
 
   clangd::Range Range;
   DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;
   std::string Category;
   // Since File is only descriptive, we store a separate flag to distinguish
   // diags from the main file.
   bool InsideMainFile = false;
   unsigned ID; // e.g. member of clang::diag, or clang-tidy assigned ID.
+  // Feature modules can make use of this field to propagate data from a
+  // diagnostic to a CodeAction request. Each module should only append to the
+  // list.
+  llvm::json::Object OpaqueData;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D);
 
 /// Represents a single fix-it that editor can apply to fix the error.
 struct Fix {
   /// Message for the fix-it.
   std::string Message;
   /// TextEdits from clang's fix-its. Must be non-empty.

clang-tools-extra/clangd/Diagnostics.cpp

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>());
     }
+
+  // FIXME: Get rid of the copies here by taking in a mutable clangd::Diag.
+  for (auto &Entry : D.OpaqueData)
+    Main.data.insert({Entry.first, Entry.second});
 }
 
 int getSeverity(DiagnosticsEngine::Level L) {
   switch (L) {
   case DiagnosticsEngine::Remark:
     return 4;
   case DiagnosticsEngine::Note:
     return 3;

clang-tools-extra/clangd/Protocol.h

struct ClientCapabilities {
   /// Whether the client accepts diagnostics with related locations.
   /// textDocument.publishDiagnostics.relatedInformation.
   bool DiagnosticRelatedInformation = 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.
   /// textDocument.completion.completionItem.snippetSupport
   bool CompletionSnippets = false;
 
   /// Client supports completions with additionalTextEdit near the cursor.
   /// This is a clangd extension. (LSP says this is for unrelated text only).
   /// textDocument.completion.editsNearCursor
   bool CompletionFixes = false;
struct Diagnostic {
   /// client. The category typically describes the compilation stage during
   /// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".
   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;
+
+  /// A data entry field that is preserved between a
+  /// `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

clang-tools-extra/clangd/Protocol.cpp

llvm::json::Value toJSON(const Diagnostic &D) {
   if (D.codeActions)
     Diag["codeActions"] = D.codeActions;
   if (!D.code.empty())
     Diag["code"] = D.code;
   if (!D.source.empty())
     Diag["source"] = D.source;
   if (D.relatedInformation)
     Diag["relatedInformation"] = *D.relatedInformation;
+  if (!D.data.empty())
+    Diag["data"] = llvm::json::Object(D.data);
   // FIXME: workaround for older gcc/clang
   return std::move(Diag);
 }
 
 bool fromJSON(const llvm::json::Value &Params, Diagnostic &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  return O && O.map("range", R.range) && O.map("message", R.message) &&
+  if (!O)
+    return false;
+  if (auto *Data = Params.getAsObject()->getObject("data"))
+    R.data = *Data;
+  return O.map("range", R.range) && O.map("message", R.message) &&
          mapOptOrNull(Params, "severity", R.severity, P) &&
          mapOptOrNull(Params, "category", R.category, P) &&
          mapOptOrNull(Params, "code", R.code, P) &&
          mapOptOrNull(Params, "source", R.source, P);
 }
 
 llvm::json::Value toJSON(const PublishDiagnosticsParams &PDP) {
   llvm::json::Object Result{

clang-tools-extra/clangd/refactor/Tweak.h

 
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/Index.h"
 #include "support/Path.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
struct Selection {
     SourceLocation Cursor;
     /// The begin offset of the selection
     unsigned SelectionBegin;
     /// The end offset of the selection
     unsigned SelectionEnd;
     /// The AST nodes that were selected.
     SelectionTree ASTSelection;
     // FIXME: provide a way to get sources and ASTs for other files.
   };
 
   struct Effect {
     /// A message to be displayed to the user.
     llvm::Optional<std::string> ShowMessage;
     FileEdits ApplyEdits;
 
     static Effect showMessage(StringRef S) {
       Effect E;