Index: clangd/Diagnostics.h =================================================================== --- clangd/Diagnostics.h +++ clangd/Diagnostics.h @@ -30,6 +30,11 @@ /// 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 @@ // 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 AbsFile; + clangd::Range Range; DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note; std::string Category; Index: clangd/Diagnostics.cpp =================================================================== --- clangd/Diagnostics.cpp +++ 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 @@ } /// 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 @@ /// /// 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 @@ 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()); + 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()); + } } int getSeverity(DiagnosticsEngine::Level L) { @@ -396,6 +415,9 @@ 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())) Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -366,6 +366,10 @@ /// 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 @@ }; 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 @@ /// 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> 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 Index: clangd/Protocol.cpp =================================================================== --- clangd/Protocol.cpp +++ clangd/Protocol.cpp @@ -277,6 +277,8 @@ 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 @@ 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 @@ Diag["code"] = D.code; if (!D.source.empty()) Diag["source"] = D.source; + if (D.relatedInformation) + Diag["relatedInformation"] = *D.relatedInformation; return std::move(Diag); } Index: unittests/clangd/DiagnosticsTests.cpp =================================================================== --- unittests/clangd/DiagnosticsTests.cpp +++ 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_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, 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 @@ 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 @@ 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 @@ 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>> 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 Fixes) { LSPDiags.push_back( {std::move(LSPDiag), @@ -324,6 +333,30 @@ 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 Fixes) { + LSPDiags.push_back( + {std::move(LSPDiag), + std::vector(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 {