[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL358675: [clangd] Support relatedInformation in diagnostics.
(authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/Diagnostics.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/test/clangd/diagnostics-notes.test
clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
Index: clang-tools-extra/trunk/test/clangd/diagnostics-notes.test
===
--- clang-tools-extra/trunk/test/clangd/diagnostics-notes.test
+++ clang-tools-extra/trunk/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"}
Index: clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
+++ clang-tools-extra/trunk/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 =
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall added a comment. Discussed further offline - it's not clear that expressing the flattening as LSP diagnostic -> LSP diagnostic is better than the current Diag -> LSP diagnostic. So that followup probably won't happen, and there isn't that much to be gained from "unifying" the behavior in !AbsFile or File!=AbsFile cases. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60267/new/ https://reviews.llvm.org/D60267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
Comment at: clangd/Diagnostics.cpp:271
+ if (!Note.AbsFile) {
+log("Dropping note from unknown file: {0}", Note);
+continue;
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe `vlog`? This is what we use for dropped diagnostics, should probably
> > stick to the same level with dropped notes (even though the dropped notes
> > probably come up less often in practice).
> We seem to be dropping these only at related information case, what about
> flattening?
> Maybe we should get rid of them at that stage as well.
As with the other comment - you're right, and we'll drop these if we refactor -
I don't think there's any need to drop them now, though.
Comment at: clangd/Diagnostics.cpp:417
D.InsideMainFile = InsideMainFile;
D.File = Info.getSourceManager().getFilename(Info.getLocation());
+auto = Info.getSourceManager();
kadircet wrote:
> Do we still need the File field as well?
It's more readable than full path - e.g. if the main TU is foo.cc and includes
foo.h, this is "foo.h".
It's true that if we want to flatten as another pass, we're not going to make
use of this information. I'd rather change that in the flatten-as-another-pass
patch, so we can see whether the damage to output is worth the refactoring.
(And whether we want to explicitly compute a nice path somehow, etc)
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
kadircet added inline comments.
Comment at: clangd/Diagnostics.cpp:271
+ if (!Note.AbsFile) {
+log("Dropping note from unknown file: {0}", Note);
+continue;
ilya-biryukov wrote:
> Maybe `vlog`? This is what we use for dropped diagnostics, should probably
> stick to the same level with dropped notes (even though the dropped notes
> probably come up less often in practice).
We seem to be dropping these only at related information case, what about
flattening?
Maybe we should get rid of them at that stage as well.
Comment at: clangd/Diagnostics.cpp:417
D.InsideMainFile = InsideMainFile;
D.File = Info.getSourceManager().getFilename(Info.getLocation());
+auto = Info.getSourceManager();
Do we still need the File field as well?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall updated this revision to Diff 195706.
sammccall added a comment.
Propagate the capability to Diagnostics, add lit test.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
Files:
clangd/ClangdLSPServer.cpp
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/diagnostics-notes.test
unittests/clangd/DiagnosticsTests.cpp
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 , 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,
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall updated this revision to Diff 195707.
sammccall added a comment.
Remove accidental copy/paste in lit test.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
Files:
clangd/ClangdLSPServer.cpp
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/diagnostics-notes.test
unittests/clangd/DiagnosticsTests.cpp
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 , 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
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall updated this revision to Diff 195694.
sammccall marked an inline comment as done.
sammccall added a comment.
Rebase and address comments.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
Files:
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
unittests/clangd/DiagnosticsTests.cpp
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 , 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) {
+
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
sammccall marked 9 inline comments as done. sammccall added inline comments. Comment at: clangd/Diagnostics.cpp:280 + Main.relatedInformation->push_back(std::move(RelInfo)); +} } ilya-biryukov wrote: > NIT: maybe call `OutFn` and return here to avoid checking for > `EmitRelatedLocations` again? > Would arguably make the code simpler, although would require another call to > `OutFn(Main)` outside the if branch. Yeah, I don't really see this as an improvement - it reduces the nesting, but makes the relation of conditions to code more confusing, I think. Comment at: unittests/clangd/DiagnosticsTests.cpp:259 +#ifdef _WIN32 + "c:\\path\\to\\foo\\bar\\main.cpp", +#else ilya-biryukov wrote: > maybe use `testPath()` here to avoid PP directives? Done - this requires changing the actual paths but it doesn't seem to matter. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60267/new/ https://reviews.llvm.org/D60267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/Protocol.cpp:280
R.DiagnosticFixes = *CodeActions;
+ if (auto CodeActions = Diagnostics->getBoolean("relatedInformation"))
+R.DiagnosticRelatedInformation = *CodeActions;
kadircet wrote:
> `s/CodeActions/RelatedInfoSupport/`
copy-paste detected? :-)))
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
kadircet added inline comments.
Comment at: clangd/Protocol.cpp:280
R.DiagnosticFixes = *CodeActions;
+ if (auto CodeActions = Diagnostics->getBoolean("relatedInformation"))
+R.DiagnosticRelatedInformation = *CodeActions;
`s/CodeActions/RelatedInfoSupport/`
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/Diagnostics.cpp:271
+ if (!Note.AbsFile) {
+log("Dropping note from unknown file: {0}", Note);
+continue;
Maybe `vlog`? This is what we use for dropped diagnostics, should probably
stick to the same level with dropped notes (even though the dropped notes
probably come up less often in practice).
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/Diagnostics.cpp:185
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.
NIT: the comment looks open to misinterpretation, "no structured link" refers
to the fact the clients don't support it, but could be read that we don't know
which notes correspond to a main diagnostic.
Maybe rephrase to something link "If the client does not support structured
links …"?
Comment at: clangd/Diagnostics.cpp:280
+ Main.relatedInformation->push_back(std::move(RelInfo));
+}
}
NIT: maybe call `OutFn` and return here to avoid checking for
`EmitRelatedLocations` again?
Would arguably make the code simpler, although would require another call to
`OutFn(Main)` outside the if branch.
Comment at: clangd/SourceCode.cpp:310
+ if (!F) {
return None;
+ }
NIT: seems unrelated. Maybe revert?
Comment at: unittests/clangd/DiagnosticsTests.cpp:62
+return false;
+ if (toJSON(arg) != toJSON(LSPDiag)) {
+*result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
Maybe omit the `std::tie()`-based comparison altogether?
This would not change the semantics of the matcher, right?
Comment at: unittests/clangd/DiagnosticsTests.cpp:259
+#ifdef _WIN32
+ "c:\\path\\to\\foo\\bar\\main.cpp",
+#else
maybe use `testPath()` here to avoid PP directives?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60267/new/
https://reviews.llvm.org/D60267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.
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>> 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),
@@ -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 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 {
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
