[clangd] Support relatedInformation in diagnostics.

Summary: We already have the structure internally, we just need to expose it.

Reviewers: ilya-biryukov

Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D60267

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@358675 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 213 insertions, 69 deletions

diff --git a/clangd/ClangdLSPServer.cpp b/clangd/ClangdLSPServer.cpp
index 7d6f54e7..44357df7 100644
--- a/clangd/ClangdLSPServer.cpp
+++ b/clangd/ClangdLSPServer.cpp
@@ -348,6 +348,8 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
   CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets;
   DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes;
   DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory;
+  DiagOpts.EmitRelatedLocations =
+      Params.capabilities.DiagnosticRelatedInformation;
   if (Params.capabilities.WorkspaceSymbolKinds)
     SupportedSymbolKinds |= *Params.capabilities.WorkspaceSymbolKinds;
   if (Params.capabilities.CompletionItemKinds)
diff --git a/clangd/Diagnostics.cpp b/clangd/Diagnostics.cpp
index aee19258..a734f233 100644
--- a/clangd/Diagnostics.cpp
+++ b/clangd/Diagnostics.cpp
@@ -10,6 +10,7 @@
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "Compiler.h"
 #include "Logger.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/DiagnosticIDs.h"
@@ -175,9 +176,7 @@ std::string capitalize(std::string Message) {
 }
 
 /// Returns a message sent to LSP for the main diagnostic in \p D.
-/// The message includes all the notes with their corresponding locations.
-/// However, notes with fix-its are excluded as those usually only contain a
-/// fix-it message and just add noise if included in the message for diagnostic.
+/// This message may include notes, if they're not emited in some other way.
 /// Example output:
 ///
 ///     no matching function for call to 'foo'
@@ -186,29 +185,34 @@ std::string capitalize(std::string Message) {
 ///
 ///     dir1/dir2/dir3/../../dir4/header.h:12:23
 ///     note: candidate function not viable: requires 3 arguments
-std::string mainMessage(const Diag &D, bool DisplayFixesCount) {
+std::string mainMessage(const Diag &D, const ClangdDiagnosticOptions &Opts) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << D.Message;
-  if (DisplayFixesCount && !D.Fixes.empty())
+  if (Opts.DisplayFixesCount && !D.Fixes.empty())
     OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)";
-  for (auto &Note : D.Notes) {
-    OS << "\n\n";
-    printDiag(OS, Note);
-  }
+  // If notes aren't emitted as structured info, add them to the message.
+  if (!Opts.EmitRelatedLocations)
+    for (auto &Note : D.Notes) {
+      OS << "\n\n";
+      printDiag(OS, Note);
+    }
   OS.flush();
   return capitalize(std::move(Result));
 }
 
 /// Returns a message sent to LSP for the note of the main diagnostic.
-/// The message includes the main diagnostic to provide the necessary context
-/// for the user to understand the note.
-std::string noteMessage(const Diag &Main, const DiagBase &Note) {
+std::string noteMessage(const Diag &Main, const DiagBase &Note,
+                        const ClangdDiagnosticOptions &Opts) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << Note.Message;
-  OS << "\n\n";
-  printDiag(OS, Main);
+  // If the client doesn't support structured links between the note and the
+  // original diagnostic, then emit the main diagnostic to give context.
+  if (!Opts.EmitRelatedLocations) {
+    OS << "\n\n";
+    printDiag(OS, Main);
+  }
   OS.flush();
   return capitalize(std::move(Result));
 }
@@ -275,39 +279,54 @@ void toLSPDiags(
     return Res;
   };
 
-  {
-    clangd::Diagnostic Main = FillBasicFields(D);
-    Main.message = mainMessage(D, Opts.DisplayFixesCount);
-    if (!D.Name.empty())
-      Main.code = D.Name;
-    switch (D.Source) {
-    case Diag::Clang:
-      Main.source = "clang";
-      break;
-    case Diag::ClangTidy:
-      Main.source = "clang-tidy";
-      break;
-    case Diag::Unknown:
-      break;
-    }
-    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);
+  clangd::Diagnostic Main = FillBasicFields(D);
+  Main.code = D.Name;
+  switch (D.Source) {
+  case Diag::Clang:
+    Main.source = "clang";
+    break;
+  case Diag::ClangTidy:
+    Main.source = "clang-tidy";
+    break;
+  case Diag::Unknown:
+    break;
+  }
+  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;
 
-  for (auto &Note : D.Notes) {
-    if (!Note.InsideMainFile)
-      continue;
-    clangd::Diagnostic Res = FillBasicFields(Note);
-    Res.message = noteMessage(D, Note);
-    OutFn(std::move(Res), llvm::ArrayRef<Fix>());
+  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);
+
+  // 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 = FillBasicFields(Note);
+      Res.message = noteMessage(D, Note, Opts);
+      OutFn(std::move(Res), llvm::ArrayRef<Fix>());
+    }
 }
 
 int getSeverity(DiagnosticsEngine::Level L) {
@@ -396,6 +415,9 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     D.Message = Message.str();
     D.InsideMainFile = InsideMainFile;
     D.File = Info.getSourceManager().getFilename(Info.getLocation());
+    auto &SM = Info.getSourceManager();
+    D.AbsFile = getCanonicalPath(
+        SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM);
     D.Severity = DiagLevel;
     D.Category = DiagnosticIDs::getCategoryNameFromID(
                      DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
diff --git a/clangd/Diagnostics.h b/clangd/Diagnostics.h
index 1ff9fcf5..c8045807 100644
--- a/clangd/Diagnostics.h
+++ b/clangd/Diagnostics.h
@@ -30,6 +30,11 @@ struct ClangdDiagnosticOptions {
   /// diagnostics that are sent to the client.
   bool EmbedFixesInDiagnostics = false;
 
+  /// If true, Clangd uses the relatedInformation field to include other
+  /// locations (in particular attached notes).
+  /// Otherwise, these are flattened into the diagnostic message.
+  bool EmitRelatedLocations = false;
+
   /// If true, Clangd uses an LSP extension to send the diagnostic's
   /// category to the client. The category typically describes the compilation
   /// stage during which the issue was produced, e.g. "Semantic Issue" or "Parse
@@ -47,6 +52,9 @@ struct DiagBase {
   // Intended to be used only in error messages.
   // May be relative, absolute or even artifically 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;
diff --git a/clangd/Protocol.cpp b/clangd/Protocol.cpp
index 60c047b3..a8b1c437 100644
--- a/clangd/Protocol.cpp
+++ b/clangd/Protocol.cpp
@@ -277,6 +277,8 @@ bool fromJSON(const llvm::json::Value &Params, ClientCapabilities &R) {
         R.DiagnosticCategory = *CategorySupport;
       if (auto CodeActions = Diagnostics->getBoolean("codeActionsInline"))
         R.DiagnosticFixes = *CodeActions;
+      if (auto RelatedInfo = Diagnostics->getBoolean("relatedInformation"))
+        R.DiagnosticRelatedInformation = *RelatedInfo;
     }
     if (auto *Completion = TextDocument->getObject("completion")) {
       if (auto *Item = Completion->getObject("completionItem")) {
@@ -419,6 +421,13 @@ bool fromJSON(const llvm::json::Value &Params, DocumentSymbolParams &R) {
   return O && O.map("textDocument", R.textDocument);
 }
 
+llvm::json::Value toJSON(const DiagnosticRelatedInformation &DRI) {
+  return llvm::json::Object{
+    {"location", DRI.location},
+    {"message", DRI.message},
+  };
+}
+
 llvm::json::Value toJSON(const Diagnostic &D) {
   llvm::json::Object Diag{
       {"range", D.range},
@@ -433,6 +442,8 @@ llvm::json::Value toJSON(const Diagnostic &D) {
     Diag["code"] = D.code;
   if (!D.source.empty())
     Diag["source"] = D.source;
+  if (D.relatedInformation)
+    Diag["relatedInformation"] = *D.relatedInformation;
   return std::move(Diag);
 }
 
diff --git a/clangd/Protocol.h b/clangd/Protocol.h
index 5e8e7b4b..9a8d4ac7 100644
--- a/clangd/Protocol.h
+++ b/clangd/Protocol.h
@@ -366,6 +366,10 @@ struct ClientCapabilities {
   /// textDocument.publishDiagnostics.codeActionsInline.
   bool DiagnosticFixes = false;
 
+  /// 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
@@ -582,6 +586,18 @@ struct DocumentSymbolParams {
 };
 bool fromJSON(const llvm::json::Value &, DocumentSymbolParams &);
 
+
+/// Represents a related message and source code location for a diagnostic.
+/// This should be used to point to code locations that cause or related to a
+/// diagnostics, e.g when duplicating a symbol in a scope.
+struct DiagnosticRelatedInformation {
+  /// The location of this related diagnostic information.
+  Location location;
+  /// The message of this related diagnostic information.
+  std::string message;
+};
+llvm::json::Value toJSON(const DiagnosticRelatedInformation &);
+
 struct CodeAction;
 struct Diagnostic {
   /// The range at which the message applies.
@@ -601,6 +617,10 @@ struct Diagnostic {
   /// The diagnostic's message.
   std::string message;
 
+  /// An array of related diagnostic information, e.g. when symbol-names within
+  /// a scope collide all definitions can be marked via this property.
+  llvm::Optional<std::vector<DiagnosticRelatedInformation>> relatedInformation;
+
   /// 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
diff --git a/test/clangd/diagnostics-notes.test b/test/clangd/diagnostics-notes.test
new file mode 100644
index 00000000..c04c5cf0
--- /dev/null
+++ b/test/clangd/diagnostics-notes.test
@@ -0,0 +1,48 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"relatedInformation":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cc","languageId":"cpp","version":1,"text":"int x;\nint x;"}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "code": "redefinition",
+# CHECK-NEXT:        "message": "Redefinition of 'x'",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 5,
+# CHECK-NEXT:            "line": 1
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 4,
+# CHECK-NEXT:            "line": 1
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "relatedInformation": [
+# CHECK-NEXT:          {
+# CHECK-NEXT:            "location": {
+# CHECK-NEXT:              "range": {
+# CHECK-NEXT:                "end": {
+# CHECK-NEXT:                  "character": 5,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                },
+# CHECK-NEXT:                "start": {
+# CHECK-NEXT:                  "character": 4,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                }
+# CHECK-NEXT:              },
+# CHECK-NEXT:              "uri": "{{.*}}foo.cc"
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "message": "Previous definition is here"
+# CHECK-NEXT:          }
+# CHECK-NEXT:        ],
+# CHECK-NEXT:        "severity": 1,
+# CHECK-NEXT:        "source": "clang"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file://{{.*}}/foo.cc"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
diff --git a/unittests/clangd/DiagnosticsTests.cpp b/unittests/clangd/DiagnosticsTests.cpp
index 33b806de..a742c9d6 100644
--- a/unittests/clangd/DiagnosticsTests.cpp
+++ b/unittests/clangd/DiagnosticsTests.cpp
@@ -8,10 +8,14 @@
 
 #include "Annotations.h"
 #include "ClangdUnit.h"
+#include "Diagnostics.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "TestIndex.h"
+#include "TestFS.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
@@ -55,8 +59,12 @@ MATCHER_P3(Fix, Range, Replacement, Message,
 
 MATCHER_P(EqualToLSPDiag, LSPDiag,
           "LSP diagnostic " + llvm::to_string(LSPDiag)) {
-  return std::tie(arg.range, arg.severity, arg.message) ==
-         std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message);
+  if (toJSON(arg) != toJSON(LSPDiag)) {
+    *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
+                                      toJSON(LSPDiag), toJSON(arg)).str();
+    return false;
+  }
+  return true;
 }
 
 MATCHER_P(DiagSource, S, "") { return arg.Source == S; }
@@ -248,6 +256,11 @@ TEST(DiagnosticsTest, NoFixItInMacro) {
 }
 
 TEST(DiagnosticsTest, ToLSP) {
+  URIForFile MainFile =
+      URIForFile::canonicalize(testPath("foo/bar/main.cpp"), "");
+  URIForFile HeaderFile =
+      URIForFile::canonicalize(testPath("foo/bar/header.h"), "");
+
   clangd::Diag D;
   D.ID = clang::diag::err_enum_class_reference;
   D.Name = "enum_class_reference";
@@ -257,6 +270,7 @@ TEST(DiagnosticsTest, ToLSP) {
   D.InsideMainFile = true;
   D.Severity = DiagnosticsEngine::Error;
   D.File = "foo/bar/main.cpp";
+  D.AbsFile = MainFile.file();
 
   clangd::Note NoteInMain;
   NoteInMain.Message = "declared somewhere in the main file";
@@ -264,6 +278,8 @@ TEST(DiagnosticsTest, ToLSP) {
   NoteInMain.Severity = DiagnosticsEngine::Remark;
   NoteInMain.File = "../foo/bar/main.cpp";
   NoteInMain.InsideMainFile = true;
+  NoteInMain.AbsFile = MainFile.file();
+
   D.Notes.push_back(NoteInMain);
 
   clangd::Note NoteInHeader;
@@ -272,44 +288,37 @@ TEST(DiagnosticsTest, ToLSP) {
   NoteInHeader.Severity = DiagnosticsEngine::Note;
   NoteInHeader.File = "../foo/baz/header.h";
   NoteInHeader.InsideMainFile = false;
+  NoteInHeader.AbsFile = HeaderFile.file();
   D.Notes.push_back(NoteInHeader);
 
   clangd::Fix F;
   F.Message = "do something";
   D.Fixes.push_back(F);
 
-  auto MatchingLSP = [](const DiagBase &D, StringRef Message) {
-    clangd::Diagnostic Res;
-    Res.range = D.Range;
-    Res.severity = getSeverity(D.Severity);
-    Res.message = Message;
-    return Res;
-  };
-
   // Diagnostics should turn into these:
-  clangd::Diagnostic MainLSP =
-      MatchingLSP(D, R"(Something terrible happened (fix available)
+  clangd::Diagnostic MainLSP;
+  MainLSP.range = D.Range;
+  MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
+  MainLSP.code = "enum_class_reference";
+  MainLSP.source = "clang";
+  MainLSP.message = R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
 ../foo/baz/header.h:10:11:
-note: declared somewhere in the header file)");
+note: declared somewhere in the header file)";
 
-  clangd::Diagnostic NoteInMainLSP =
-      MatchingLSP(NoteInMain, R"(Declared somewhere in the main file
+  clangd::Diagnostic NoteInMainLSP;
+  NoteInMainLSP.range = NoteInMain.Range;
+  NoteInMainLSP.severity = getSeverity(DiagnosticsEngine::Remark);
+  NoteInMainLSP.message = R"(Declared somewhere in the main file
 
-main.cpp:2:3: error: something terrible happened)");
+main.cpp:2:3: error: something terrible happened)";
 
-  // Transform dianostics and check the results.
+  ClangdDiagnosticOptions Opts;
+  // Transform diagnostics and check the results.
   std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
-  toLSPDiags(D,
-#ifdef _WIN32
-             URIForFile::canonicalize("c:\\path\\to\\foo\\bar\\main.cpp",
-                                      /*TUPath=*/""),
-#else
-      URIForFile::canonicalize("/path/to/foo/bar/main.cpp", /*TUPath=*/""),
-#endif
-             ClangdDiagnosticOptions(),
+  toLSPDiags(D, MainFile, Opts,
              [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
                LSPDiags.push_back(
                    {std::move(LSPDiag),
@@ -324,6 +333,30 @@ main.cpp:2:3: error: something terrible happened)");
   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) {
+               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;
+  NoteInMainDRI.location.uri = MainFile;
+  MainLSP.relatedInformation = {NoteInMainDRI};
+  DiagnosticRelatedInformation NoteInHeaderDRI;
+  NoteInHeaderDRI.message = "Declared somewhere in the header file";
+  NoteInHeaderDRI.location.range = NoteInHeader.Range;
+  NoteInHeaderDRI.location.uri = HeaderFile;
+  MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeaderDRI};
+  EXPECT_THAT(
+      LSPDiags,
+      ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F)))));
 }
 
 struct SymbolWithHeader {