sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric.
Herald added a project: clang.
We already have the structure internally, we just need to expose it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D60267
Files:
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
unittests/clangd/DiagnosticsTests.cpp
Index: unittests/clangd/DiagnosticsTests.cpp
===================================================================
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -8,6 +8,8 @@
#include "Annotations.h"
#include "ClangdUnit.h"
+#include "Diagnostics.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "TestIndex.h"
#include "TestTU.h"
@@ -54,8 +56,15 @@
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 (std::tie(arg.range, arg.severity, arg.message) !=
+ std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message))
+ return false;
+ 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, Source, "") { return arg.S == Source; }
@@ -245,12 +254,28 @@
}
TEST(DiagnosticsTest, ToLSP) {
+ URIForFile MainFile = URIForFile::canonicalize(
+#ifdef _WIN32
+ "c:\\path\\to\\foo\\bar\\main.cpp",
+#else
+ "/path/to/foo/bar/main.cpp",
+#endif
+ /*TUPath=*/"");
+ URIForFile HeaderFile = URIForFile::canonicalize(
+#ifdef _WIN32
+ "c:\\path\\to\\foo\\bar\\header.h",
+#else
+ "/path/to/foo/bar/header.h",
+#endif
+ /*TUPath=*/"");
+
clangd::Diag D;
D.Message = "something terrible happened";
D.Range = {pos(1, 2), pos(3, 4)};
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";
@@ -258,6 +283,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;
@@ -266,6 +293,7 @@
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;
@@ -294,16 +322,10 @@
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),
@@ -314,6 +336,30 @@
LSPDiags,
ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty())));
+
+ // 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 {
Index: clangd/SourceCode.cpp
===================================================================
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -306,8 +306,9 @@
llvm::Optional<std::string> getCanonicalPath(const FileEntry *F,
const SourceManager &SourceMgr) {
- if (!F)
+ if (!F) {
return None;
+ }
llvm::SmallString<128> FilePath = F->getName();
if (!llvm::sys::path::is_absolute(FilePath)) {
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.
@@ -603,6 +619,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<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
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 CodeActions = Diagnostics->getBoolean("relatedInformation"))
+ R.DiagnosticRelatedInformation = *CodeActions;
}
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},
@@ -429,6 +438,8 @@
Diag["category"] = *D.category;
if (D.codeActions)
Diag["codeActions"] = D.codeActions;
+ if (D.relatedInformation)
+ Diag["relatedInformation"] = *D.relatedInformation;
return std::move(Diag);
}
Index: clangd/Diagnostics.h
===================================================================
--- clangd/Diagnostics.h
+++ clangd/Diagnostics.h
@@ -27,6 +27,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
@@ -44,6 +49,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<std::string> AbsFile;
+
clangd::Range Range;
DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;
std::string Category;
Index: clangd/Diagnostics.cpp
===================================================================
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -9,6 +9,7 @@
#include "Diagnostics.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
@@ -150,9 +151,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'
@@ -161,29 +160,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 there's no structured link 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));
}
@@ -250,27 +254,43 @@
return Res;
};
- {
- clangd::Diagnostic Main = FillBasicFields(D);
- Main.message = mainMessage(D, Opts.DisplayFixesCount);
- 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);
+ 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) {
+ log("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) {
@@ -320,6 +340,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()))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
