ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, sammccall.
Herald added subscribers: jkorous-apple, mgorny, klimek.
The new implementation attaches notes to diagnostic message and shows
the original diagnostics in the message of the note.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File, Tagged<DiagList> Diagnostics) override {
+ }
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional<std::vector<DiagWithFixIts>>) {}
+void ignoreUpdate(llvm::Optional<DiagList>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector<DiagWithFixIts>) { Ready.wait(); });
+ [&](DiagList) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector<DiagWithFixIts> Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -189,15 +189,14 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
- S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
- EXPECT_THAT(Context::current().get(NonceKey),
- Pointee(Nonce));
-
- std::lock_guard<std::mutex> Lock(Mut);
- ++TotalUpdates;
- });
+ S.update(
+ File, Inputs, WantDiagnostics::Auto,
+ [Nonce, &Mut, &TotalUpdates](llvm::Optional<DiagList> Diags) {
+ EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
+
+ std::lock_guard<std::mutex> Lock(Mut);
+ ++TotalUpdates;
+ });
}
{
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
-using ::testing::Field;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File, Tagged<DiagList> Diagnostics) override {
+ }
};
// GMock helpers for matching completion items.
@@ -321,11 +321,11 @@
};
// We used to test every combination of options, but that got too slow (2^N).
auto Flags = {
- &clangd::CodeCompleteOptions::IncludeMacros,
- &clangd::CodeCompleteOptions::IncludeBriefComments,
- &clangd::CodeCompleteOptions::EnableSnippets,
- &clangd::CodeCompleteOptions::IncludeCodePatterns,
- &clangd::CodeCompleteOptions::IncludeIneligibleResults,
+ &clangd::CodeCompleteOptions::IncludeMacros,
+ &clangd::CodeCompleteOptions::IncludeBriefComments,
+ &clangd::CodeCompleteOptions::EnableSnippets,
+ &clangd::CodeCompleteOptions::IncludeCodePatterns,
+ &clangd::CodeCompleteOptions::IncludeIneligibleResults,
};
// Test default options.
Test({});
@@ -551,7 +551,6 @@
auto WithoutIndex = runCodeComplete(Server, File, Test.point(), Opts).Value;
EXPECT_THAT(WithoutIndex.items,
UnorderedElementsAre(Named("local"), Named("preamble")));
-
}
TEST(CompletionTest, DynamicIndexMultiFile) {
Index: unittests/clangd/ClangdUnitTests.cpp
===================================================================
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -7,8 +7,8 @@
//
//===----------------------------------------------------------------------===//
-#include "ClangdUnit.h"
#include "Annotations.h"
+#include "ClangdUnit.h"
#include "TestFS.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
@@ -20,9 +20,11 @@
namespace clang {
namespace clangd {
using namespace llvm;
-void PrintTo(const DiagWithFixIts &D, std::ostream *O) {
+void PrintTo(const PersistentDiag &D, std::ostream *O) {
llvm::raw_os_ostream OS(*O);
- OS << D.Diag;
+ if (!D.InsideMainFile)
+ OS << "[in " << D.File << "] ";
+ OS << D.Message;
if (!D.FixIts.empty()) {
OS << " {";
const char *Sep = "";
@@ -38,8 +40,8 @@
using testing::ElementsAre;
// FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code, std::vector<const char*> Flags = {}) {
- std::vector<const char*> Cmd = {"clang", "main.cpp"};
+ParsedAST build(StringRef Code, std::vector<const char *> Flags = {}) {
+ std::vector<const char *> Cmd = {"clang", "main.cpp"};
Cmd.insert(Cmd.begin() + 1, Flags.begin(), Flags.end());
auto CI = createInvocationFromCommandLine(Cmd);
auto Buf = MemoryBuffer::getMemBuffer(Code);
@@ -50,16 +52,20 @@
return std::move(*AST);
}
+DiagList buildDiags(llvm::StringRef Code,
+ std::vector<const char *> Flags = {}) {
+ return build(Code, std::move(Flags)).getDiagnostics();
+}
+
MATCHER_P2(Diag, Range, Message,
"Diagnostic at " + llvm::to_string(Range) + " = [" + Message + "]") {
- return arg.Diag.range == Range && arg.Diag.message == Message &&
- arg.FixIts.empty();
+ return arg.Range == Range && arg.Message == Message && arg.FixIts.empty();
}
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
- return arg.Diag.range == Range && arg.Diag.message == Message &&
+ return arg.Range == Range && arg.Message == Message &&
arg.FixIts.size() == 1 && arg.FixIts[0].range == Range &&
arg.FixIts[0].newText == Replacement;
}
@@ -75,31 +81,29 @@
$unk[[unknown]]();
}
)cpp");
- llvm::errs() << Test.code();
- EXPECT_THAT(
- build(Test.code()).getDiagnostics(),
- ElementsAre(
- // This range spans lines.
- Fix(Test.range("typo"), "foo",
- "use of undeclared identifier 'goo'; did you mean 'foo'?"),
- // This is a pretty normal range.
- Diag(Test.range("decl"), "'foo' declared here"),
- // This range is zero-width, and at the end of a line.
- Fix(Test.range("semicolon"), ";",
- "expected ';' after expression"),
- // This range isn't provided by clang, we expand to the token.
- Diag(Test.range("unk"),
- "use of undeclared identifier 'unknown'")));
+ llvm::errs() << Test.code();
+ EXPECT_THAT(
+ buildDiags(Test.code()).rawDiags(),
+ ElementsAre(
+ // This range spans lines.
+ Fix(Test.range("typo"), "foo",
+ "use of undeclared identifier 'goo'; did you mean 'foo'?"),
+ // This is a pretty normal range.
+ Diag(Test.range("decl"), "'foo' declared here"),
+ // This range is zero-width, and at the end of a line.
+ Fix(Test.range("semicolon"), ";", "expected ';' after expression"),
+ // This range isn't provided by clang, we expand to the token.
+ Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
}
TEST(DiagnosticsTest, FlagsMatter) {
Annotations Test("[[void]] main() {}");
EXPECT_THAT(
- build(Test.code()).getDiagnostics(),
+ buildDiags(Test.code()).rawDiags(),
ElementsAre(Fix(Test.range(), "int", "'main' must return 'int'")));
// Same code built as C gets different diagnostics.
EXPECT_THAT(
- build(Test.code(), {"-x", "c"}).getDiagnostics(),
+ buildDiags(Test.code(), {"-x", "c"}).rawDiags(),
ElementsAre(
// FIXME: ideally this would be one diagnostic with a named FixIt.
Diag(Test.range(), "return type of 'main' is not 'int'"),
@@ -121,7 +125,7 @@
#endif
)cpp");
EXPECT_THAT(
- build(Test.code()).getDiagnostics(),
+ buildDiags(Test.code()).rawDiags(),
ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'")));
}
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -42,22 +42,20 @@
namespace {
-static bool diagsContainErrors(ArrayRef<DiagWithFixIts> Diagnostics) {
- for (const auto &DiagAndFixIts : Diagnostics) {
+static bool diagsContainErrors(const DiagList &Diagnostics) {
+ for (auto DiagAndNotes : Diagnostics) {
// FIXME: severities returned by clangd should have a descriptive
// diagnostic severity enum
- const int ErrorSeverity = 1;
- if (DiagAndFixIts.Diag.severity == ErrorSeverity)
+ if (DiagAndNotes.main().Severity == DiagnosticsEngine::Error ||
+ DiagAndNotes.main().Severity == DiagnosticsEngine::Fatal)
return true;
}
return false;
}
class ErrorCheckingDiagConsumer : public DiagnosticsConsumer {
public:
- void
- onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ void onDiagnosticsReady(PathRef File, Tagged<DiagList> Diagnostics) override {
bool HadError = diagsContainErrors(Diagnostics.Value);
std::lock_guard<std::mutex> Lock(Mutex);
@@ -82,9 +80,7 @@
/// least one error.
class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
public:
- void
- onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ void onDiagnosticsReady(PathRef File, Tagged<DiagList> Diagnostics) override {
bool HadError = diagsContainErrors(Diagnostics.Value);
std::lock_guard<std::mutex> Lock(Mutex);
@@ -635,9 +631,8 @@
public:
TestDiagConsumer() : Stats(FilesCount, FileStat()) {}
- void onDiagnosticsReady(
- PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ void onDiagnosticsReady(PathRef File,
+ Tagged<DiagList> Diagnostics) override {
StringRef FileIndexStr = llvm::sys::path::stem(File);
ASSERT_TRUE(FileIndexStr.consume_front("Foo"));
@@ -907,8 +902,7 @@
NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
: StartSecondReparse(std::move(StartSecondReparse)) {}
- void onDiagnosticsReady(PathRef,
- Tagged<std::vector<DiagWithFixIts>>) override {
+ void onDiagnosticsReady(PathRef, Tagged<DiagList>) override {
++Count;
std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());
ASSERT_TRUE(Lock.owns_lock())
Index: test/clangd/fixits.test
===================================================================
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -6,7 +6,7 @@
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 37,
@@ -20,7 +20,7 @@
# CHECK-NEXT: "severity": 2
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning",
+# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 35,
@@ -34,7 +34,7 @@
# CHECK-NEXT: "severity": 3
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison",
+# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 35,
@@ -51,7 +51,7 @@
# CHECK-NEXT: "uri": "file://{{.*}}/foo.c"
# CHECK-NEXT: }
---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}}
# CHECK: "id": 2,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -120,7 +120,7 @@
# CHECK-NEXT: }
# CHECK-NEXT: ]
---
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}}
# Make sure unused "code" and "source" fields ignored gracefully
# CHECK: "id": 3,
# CHECK-NEXT: "jsonrpc": "2.0",
Index: test/clangd/extra-flags.test
===================================================================
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -6,7 +6,7 @@
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here",
+# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here\n\nfoo.c:1:19: note: initialize the variable 'i' to silence this warning",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 28,
@@ -20,7 +20,7 @@
# CHECK-NEXT: "severity": 2
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning",
+# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 19,
@@ -42,7 +42,7 @@
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here",
+# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here\n\nfoo.c:1:19: note: initialize the variable 'i' to silence this warning",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 28,
@@ -56,7 +56,7 @@
# CHECK-NEXT: "severity": 2
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning",
+# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 19,
Index: test/clangd/execute-command.test
===================================================================
--- test/clangd/execute-command.test
+++ test/clangd/execute-command.test
@@ -6,7 +6,7 @@
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 37,
@@ -20,7 +20,7 @@
# CHECK-NEXT: "severity": 2
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning",
+# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 35,
@@ -34,7 +34,7 @@
# CHECK-NEXT: "severity": 3
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison",
+# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 35,
Index: test/clangd/diagnostics.test
===================================================================
--- test/clangd/diagnostics.test
+++ test/clangd/diagnostics.test
@@ -6,7 +6,7 @@
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "return type of 'main' is not 'int'",
+# CHECK-NEXT: "message": "return type of 'main' is not 'int'\n\nfoo.c:1:1: note: change return type to 'int'",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 4,
@@ -20,7 +20,7 @@
# CHECK-NEXT: "severity": 2
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "message": "change return type to 'int'",
+# CHECK-NEXT: "message": "change return type to 'int'\n\nfoo.c:1:1: warning: return type of 'main' is not 'int'",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 4,
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -63,7 +63,7 @@
/// \p File was not part of it before.
/// FIXME(ibiryukov): remove the callback from this function.
void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
+ UniqueFunction<void(DiagList)> OnUpdated);
/// Remove \p File from the list of tracked files and schedule removal of its
/// resources.
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -85,7 +85,7 @@
~ASTWorker();
void update(ParseInputs Inputs, WantDiagnostics,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
+ UniqueFunction<void(DiagList)> OnUpdated);
void runWithAST(llvm::StringRef Name,
UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action);
bool blockUntilIdle(Deadline Timeout) const;
@@ -135,8 +135,8 @@
// Result of getUsedBytes() after the last rebuild or read of AST.
std::size_t LastASTSize; /* GUARDED_BY(Mutex) */
// Set to true to signal run() to finish processing.
- bool Done; /* GUARDED_BY(Mutex) */
- std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
+ bool Done; /* GUARDED_BY(Mutex) */
+ std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
};
@@ -210,9 +210,8 @@
#endif
}
-void ASTWorker::update(
- ParseInputs Inputs, WantDiagnostics WantDiags,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
+ UniqueFunction<void(DiagList)> OnUpdated) {
auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
FileInputs = Inputs;
auto Diags = AST.rebuild(std::move(Inputs));
@@ -447,9 +446,9 @@
return true;
}
-void TUScheduler::update(
- PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+void TUScheduler::update(PathRef File, ParseInputs Inputs,
+ WantDiagnostics WantDiags,
+ UniqueFunction<void(DiagList)> OnUpdated) {
std::unique_ptr<FileData> &FD = Files[File];
if (!FD) {
// Create a new worker to process the AST-related tasks.
Index: clangd/SourceCode.h
===================================================================
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -30,6 +30,10 @@
/// Turn a SourceLocation into a [line, column] pair.
Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
+// Converts a half-open clang source range to an LSP range.
+// Note that clang also uses closed source ranges, which this can't handle!
+Range halfOpenToRange(CharSourceRange R, const SourceManager &M);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/SourceCode.cpp
===================================================================
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -48,5 +48,19 @@
return P;
}
+Range halfOpenToRange(CharSourceRange R, const SourceManager &M) {
+ // Clang is 1-based, LSP uses 0-based indexes.
+ Position Begin;
+ Begin.line = static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1;
+ Begin.character =
+ static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1;
+
+ Position End;
+ End.line = static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1;
+ End.character = static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1;
+
+ return {Begin, End};
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/Diagnostics.h
===================================================================
--- /dev/null
+++ clangd/Diagnostics.h
@@ -0,0 +1,156 @@
+//===--- Diagnostics.h ------------------------------------------*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
+
+#include "Path.h"
+#include "Protocol.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include <cassert>
+#include <string>
+
+namespace clang {
+namespace clangd {
+
+/// A single diagnostic stored in DiagList. StringRefs in the diag are owned by
+/// DiagList.
+struct PersistentDiag {
+ llvm::StringRef Message;
+ // Intended to be used only in error messages.
+ // May be relative, absolute or even artifically constructed.
+ llvm::StringRef File;
+ clangd::Range Range;
+ DiagnosticsEngine::Level Severity;
+ llvm::SmallVector<TextEdit, 1> FixIts;
+ // Since File is only descriptive, we store a separate flag to distinguish
+ // diags from the main file.
+ bool InsideMainFile = false;
+};
+
+class DiagWithNotes {
+public:
+ DiagWithNotes(llvm::ArrayRef<PersistentDiag> DiagAndNotes);
+
+ PersistentDiag main() const;
+ llvm::ArrayRef<PersistentDiag> notes() const;
+ llvm::ArrayRef<PersistentDiag> all() const;
+
+private:
+ // First item is the diagnostic, the rest are notes for it.
+ llvm::ArrayRef<PersistentDiag> DiagAndNotes;
+};
+
+class DiagNotesIterator;
+
+/// A list of diagnostics with notes attached to them. Owns strings referenced
+/// by fields of PersistentDiag.
+class DiagList {
+public:
+ DiagList() = default;
+
+ DiagList(const DiagList &Other);
+ DiagList &operator=(const DiagList &Other);
+ DiagList(DiagList &&) = default;
+ DiagList &operator=(DiagList &&) = default;
+
+ // Add \p Diag to DiagList and intern all strings used inside it. If \p Diag
+ // is a note it will be attached to the latest added non-note diagnostic.
+ void add(PersistentDiag &Diag);
+ // Add all diagnostics from \p Other.
+ void insert(const DiagList &Other);
+
+ std::size_t getUsedBytes() const;
+
+ DiagNotesIterator begin() const;
+ DiagNotesIterator end() const;
+
+ /// A raw list of all stored diagnostics and notes. Prefer to iterate over
+ /// grouped values using begin() and end().
+ llvm::ArrayRef<PersistentDiag> rawDiags() const;
+
+private:
+ /// Stores concatenated list of [diag, note1, note2, ...], where each note
+ /// corresponds to diag.
+ std::vector<PersistentDiag> RawDiags;
+ /// Owns strins used in Message and Filename fields of PersistentDiag.
+ llvm::StringSet<> Intern;
+};
+
+class DiagNotesIterator {
+private:
+ friend class DiagList;
+ DiagNotesIterator(llvm::ArrayRef<PersistentDiag> RawDiags);
+
+public:
+ DiagWithNotes operator*() const;
+
+ DiagNotesIterator &operator++();
+ DiagNotesIterator operator++(int);
+
+ friend bool operator==(const DiagNotesIterator &LHS,
+ const DiagNotesIterator &RHS);
+ friend bool operator!=(const DiagNotesIterator &LHS,
+ const DiagNotesIterator &RHS);
+
+private:
+ const PersistentDiag *Diag;
+ const PersistentDiag *NotesEnd;
+ const PersistentDiag *End;
+};
+
+/// An LSP diagnostic with FixIts.
+struct DiagWithFixIts {
+ clangd::Diagnostic Diag;
+ llvm::SmallVector<TextEdit, 1> FixIts;
+};
+
+/// Conventional conversion to LSP diagnostics. Formats the error message of
+/// each diagnostic to include all its notes. Notes inside main file are also
+/// provided as separate diagnostics with their corresponding fixits.
+/// Notes outside main file do not have a corresponding LSP diagnostic, but can
+/// still be included as part of their main diagnostic's message.
+void toLSPDiags(const DiagList &Diags,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn);
+void toLSPDiags(const DiagWithNotes &D,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn);
+
+class StoreDiagsConsumer : public DiagnosticConsumer {
+public:
+ StoreDiagsConsumer(DiagList &Output);
+
+ void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
+ void EndSourceFile() override;
+ void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic &Info) override;
+
+private:
+ bool shouldIgnore(DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic &Info);
+
+ void flushLastDiag();
+ void addToOutput(const StoredDiagnostic &D);
+
+ DiagList &Output;
+ llvm::Optional<LangOptions> LangOpts;
+ /// Either empty or contains a diagnostic as a first element and the rest are
+ /// the first element's notes.
+ std::vector<StoredDiagnostic> LastDiagAndNotes;
+ /// Is any diag in LastDiagAndNotes in the main file?
+ bool LastDiagMentionsMainFile = false;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clangd/Diagnostics.cpp
===================================================================
--- /dev/null
+++ clangd/Diagnostics.cpp
@@ -0,0 +1,348 @@
+//===--- Diagnostics.cpp ----------------------------------------*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+
+#include "Diagnostics.h"
+#include "Compiler.h"
+#include "SourceCode.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/Support/Capacity.h"
+#include "llvm/Support/Path.h"
+#include <algorithm>
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Convert from clang diagnostic level to LSP severity.
+int getSeverity(DiagnosticsEngine::Level L) {
+ switch (L) {
+ case DiagnosticsEngine::Remark:
+ return 4;
+ case DiagnosticsEngine::Note:
+ return 3;
+ case DiagnosticsEngine::Warning:
+ return 2;
+ case DiagnosticsEngine::Fatal:
+ case DiagnosticsEngine::Error:
+ return 1;
+ case DiagnosticsEngine::Ignored:
+ return 0;
+ }
+ llvm_unreachable("Unknown diagnostic level!");
+}
+
+// Checks whether a location is within a half-open range.
+// Note that clang also uses closed source ranges, which this can't handle!
+bool locationInRange(SourceLocation L, CharSourceRange R,
+ const SourceManager &M) {
+ assert(R.isCharRange());
+ if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
+ M.getFileID(R.getBegin()) != M.getFileID(L))
+ return false;
+ return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
+}
+
+// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
+// LSP needs a single range.
+Range diagnosticRange(const StoredDiagnostic &D, const LangOptions &L) {
+ auto &M = D.getLocation().getManager();
+ auto Loc = M.getFileLoc(D.getLocation());
+ // Accept the first range that contains the location.
+ for (const auto &CR : D.getRanges()) {
+ auto R = Lexer::makeFileCharRange(CR, M, L);
+ if (locationInRange(Loc, R, M))
+ return halfOpenToRange(R, M);
+ }
+ // The range may be given as a fixit hint instead.
+ for (const auto &F : D.getFixIts()) {
+ auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
+ if (locationInRange(Loc, R, M))
+ return halfOpenToRange(R, M);
+ }
+ // If no suitable range is found, just use the token at the location.
+ auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
+ if (!R.isValid()) // Fall back to location only, let the editor deal with it.
+ R = CharSourceRange::getCharRange(Loc);
+ return halfOpenToRange(R, M);
+}
+
+TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
+ const LangOptions &L) {
+ TextEdit Result;
+ Result.range =
+ halfOpenToRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M);
+ Result.newText = FixIt.CodeToInsert;
+ return Result;
+}
+
+bool isInsdieMainFile(const clang::Diagnostic &D) {
+ if (!D.hasSourceManager())
+ return false;
+
+ auto Loc = D.getLocation();
+ return Loc.isValid() && D.getSourceManager().isInMainFile(Loc);
+}
+
+bool isNote(DiagnosticsEngine::Level L) {
+ return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
+}
+
+const PersistentDiag *findNotesEnd(llvm::ArrayRef<PersistentDiag> RawDiags) {
+ if (RawDiags.empty())
+ return RawDiags.end();
+
+ assert(!isNote(RawDiags.front().Severity));
+ return std::find_if(
+ RawDiags.begin() + 1, RawDiags.end(),
+ [](const PersistentDiag &D) { return !isNote(D.Severity); });
+}
+
+llvm::StringRef diagLeveltoString(DiagnosticsEngine::Level Lvl) {
+ switch (Lvl) {
+ case DiagnosticsEngine::Ignored:
+ return "ignored";
+ case DiagnosticsEngine::Note:
+ return "note";
+ case DiagnosticsEngine::Remark:
+ return "remark";
+ case DiagnosticsEngine::Warning:
+ return "warning";
+ case DiagnosticsEngine::Error:
+ return "error";
+ case DiagnosticsEngine::Fatal:
+ return "fatal error";
+ }
+ llvm_unreachable("unhandled DiagnosticsEngine::Level");
+}
+
+void printDiag(llvm::raw_string_ostream &OS, const PersistentDiag &D) {
+ if (D.InsideMainFile) {
+ // Paths to main files are often taken from compile_command.json, where they
+ // are typically absolute. To reduce noise we print only basename for them,
+ // it should not be confusing and saves space.
+ OS << llvm::sys::path::filename(D.File) << ":";
+ } else {
+ OS << D.File << ":";
+ }
+ // Note +1 to line and character. clangd::Range is zero-based, but when
+ // printing for users we want one-based indexes.
+ auto Pos = D.Range.start;
+ OS << (Pos.line + 1) << ":" << (Pos.character + 1) << ":";
+ // The non-main-file paths are often too long, putting them on a separate
+ // line improves readability.
+ if (D.InsideMainFile)
+ OS << " ";
+ else
+ OS << "\n";
+ OS << diagLeveltoString(D.Severity) << ": " << D.Message;
+}
+
+std::string presentMainMessage(const DiagWithNotes &D) {
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ OS << D.main().Message;
+ for (auto &Note : D.notes()) {
+ OS << "\n\n";
+ printDiag(OS, Note);
+ }
+ OS.flush();
+ return Result;
+}
+
+std::string presentNoteMessage(const PersistentDiag &Main,
+ const PersistentDiag &Note) {
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ OS << Note.Message;
+ OS << "\n\n";
+ printDiag(OS, Main);
+ OS.flush();
+ return Result;
+}
+} // namespace
+
+DiagWithNotes::DiagWithNotes(llvm::ArrayRef<PersistentDiag> DiagAndNotes)
+ : DiagAndNotes(DiagAndNotes) {
+ assert(!DiagAndNotes.empty());
+ assert(!isNote(DiagAndNotes.front().Severity));
+ assert(
+ std::all_of(DiagAndNotes.begin() + 1, DiagAndNotes.end(),
+ [](const PersistentDiag &D) { return isNote(D.Severity); }));
+}
+
+PersistentDiag DiagWithNotes::main() const { return DiagAndNotes.front(); }
+
+llvm::ArrayRef<PersistentDiag> DiagWithNotes::notes() const {
+ return DiagAndNotes.drop_front();
+}
+
+llvm::ArrayRef<PersistentDiag> DiagWithNotes::all() const {
+ return DiagAndNotes;
+}
+
+void toLSPDiags(const DiagWithNotes &D,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn) {
+ auto PreBuild = [](const PersistentDiag &D) {
+ DiagWithFixIts Res;
+ Res.Diag.range = D.Range;
+ Res.Diag.severity = getSeverity(D.Severity);
+ Res.FixIts = D.FixIts;
+ return Res;
+ };
+
+ DiagWithFixIts Main = PreBuild(D.main());
+ Main.Diag.message = presentMainMessage(D);
+ OutFn(std::move(Main));
+
+ for (auto &Note : D.notes()) {
+ DiagWithFixIts Res = PreBuild(Note);
+ Res.Diag.message = presentNoteMessage(D.main(), Note);
+ OutFn(std::move(Res));
+ }
+}
+
+DiagList::DiagList(const DiagList &Other) { insert(Other); }
+
+DiagList &DiagList::operator=(const DiagList &Other) {
+ Intern.clear();
+ std::vector<PersistentDiag>().swap(RawDiags);
+
+ insert(Other);
+ return *this;
+}
+
+void DiagList::add(PersistentDiag &Diag) {
+ assert((!isNote(Diag.Severity) || !RawDiags.empty()) &&
+ "Adding a note without prior diagnostics");
+
+ Diag.Message = Intern.insert(Diag.Message).first->getKey();
+ Diag.File = Intern.insert(Diag.File).first->getKey();
+
+ RawDiags.push_back(Diag);
+}
+
+void DiagList::insert(const DiagList &Other) {
+ for (auto D : Other.RawDiags)
+ add(D);
+}
+
+std::size_t DiagList::getUsedBytes() const {
+ // FIXME(ibiryukov): we do not account for strings inside Intern and FixIts
+ // inside Intern.
+ return llvm::capacity_in_bytes(RawDiags);
+}
+
+DiagNotesIterator DiagList::begin() const {
+ return DiagNotesIterator(RawDiags);
+}
+
+DiagNotesIterator DiagList::end() const {
+ return DiagNotesIterator(
+ llvm::makeArrayRef(RawDiags.data() + RawDiags.size(), 0));
+}
+
+llvm::ArrayRef<PersistentDiag> DiagList::rawDiags() const { return RawDiags; }
+
+void toLSPDiags(const DiagList &Diags,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn) {
+ for (auto D : Diags)
+ toLSPDiags(D, OutFn);
+}
+
+DiagNotesIterator::DiagNotesIterator(llvm::ArrayRef<PersistentDiag> RawDiags)
+ : Diag(RawDiags.begin()), End(RawDiags.end()) {
+ NotesEnd = findNotesEnd(RawDiags);
+}
+
+DiagWithNotes DiagNotesIterator::operator*() const {
+ assert(Diag != End);
+ return DiagWithNotes(llvm::makeArrayRef(Diag, NotesEnd));
+}
+
+DiagNotesIterator &DiagNotesIterator::operator++() {
+ Diag = NotesEnd;
+ NotesEnd = findNotesEnd(llvm::makeArrayRef(Diag, End));
+ return *this;
+}
+
+DiagNotesIterator DiagNotesIterator::operator++(int) {
+ auto Copy = *this;
+ ++(*this);
+ return Copy;
+}
+
+bool operator==(const DiagNotesIterator &LHS, const DiagNotesIterator &RHS) {
+ assert(LHS.End == RHS.End);
+ return LHS.Diag == RHS.Diag;
+}
+
+bool operator!=(const DiagNotesIterator &LHS, const DiagNotesIterator &RHS) {
+ assert(LHS.End == RHS.End);
+ return !(LHS == RHS);
+}
+
+StoreDiagsConsumer::StoreDiagsConsumer(DiagList &Output) : Output(Output) {}
+
+void StoreDiagsConsumer::BeginSourceFile(const LangOptions &Opts,
+ const Preprocessor *) {
+ LangOpts = Opts;
+}
+
+void StoreDiagsConsumer::EndSourceFile() {
+ flushLastDiag();
+ LangOpts = llvm::None;
+}
+
+void StoreDiagsConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic &Info) {
+ DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+
+ if (!LangOpts) {
+ IgnoreDiagnostics::log(DiagLevel, Info);
+ return;
+ }
+ if (!isNote(DiagLevel))
+ flushLastDiag();
+
+ LastDiagAndNotes.push_back(StoredDiagnostic(DiagLevel, Info));
+ LastDiagMentionsMainFile = LastDiagMentionsMainFile || isInsdieMainFile(Info);
+}
+
+void StoreDiagsConsumer::flushLastDiag() {
+ if (LastDiagMentionsMainFile) {
+ for (const StoredDiagnostic &D : LastDiagAndNotes)
+ addToOutput(D);
+ }
+ LastDiagAndNotes.clear();
+ LastDiagMentionsMainFile = false;
+}
+
+void StoreDiagsConsumer::addToOutput(const StoredDiagnostic &D) {
+ assert(LangOpts);
+ assert(D.getLocation().isValid());
+ assert(D.getLocation().hasManager());
+
+ FullSourceLoc Loc = D.getLocation();
+ auto &SourceMgr = Loc.getManager();
+
+ PersistentDiag Result;
+ Result.File = SourceMgr.getFilename(Loc.getFileLoc());
+ Result.Message = D.getMessage();
+ Result.Range = diagnosticRange(D, *LangOpts);
+ Result.Severity = D.getLevel();
+ Result.InsideMainFile = SourceMgr.isInMainFile(Loc);
+ for (const FixItHint &Fix : D.getFixIts())
+ Result.FixIts.push_back(toTextEdit(Fix, SourceMgr, *LangOpts));
+
+ Output.add(Result);
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/ClangdUnit.h
===================================================================
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -10,6 +10,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
+#include "Diagnostics.h"
#include "Function.h"
#include "Path.h"
#include "Protocol.h"
@@ -39,24 +40,17 @@
namespace clangd {
-/// A diagnostic with its FixIts.
-struct DiagWithFixIts {
- clangd::Diagnostic Diag;
- llvm::SmallVector<TextEdit, 1> FixIts;
-};
-
using InclusionLocations = std::vector<std::pair<Range, Path>>;
// Stores Preamble and associated data.
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble,
std::vector<serialization::DeclID> TopLevelDeclIDs,
- std::vector<DiagWithFixIts> Diags,
- InclusionLocations IncLocations);
+ DiagList Diags, InclusionLocations IncLocations);
PrecompiledPreamble Preamble;
std::vector<serialization::DeclID> TopLevelDeclIDs;
- std::vector<DiagWithFixIts> Diags;
+ DiagList Diags;
InclusionLocations IncLocations;
};
@@ -96,7 +90,7 @@
/// this call might be expensive.
ArrayRef<const Decl *> getTopLevelDecls();
- const std::vector<DiagWithFixIts> &getDiagnostics() const;
+ const DiagList &getDiagnostics() const;
/// Returns the esitmated size of the AST and the accessory structures, in
/// bytes. Does not include the size of the preamble.
@@ -107,8 +101,8 @@
ParsedAST(std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
- std::vector<const Decl *> TopLevelDecls,
- std::vector<DiagWithFixIts> Diags, InclusionLocations IncLocations);
+ std::vector<const Decl *> TopLevelDecls, DiagList Diags,
+ InclusionLocations IncLocations);
private:
void ensurePreambleDeclsDeserialized();
@@ -125,7 +119,7 @@
std::unique_ptr<FrontendAction> Action;
// Data, stored after parsing.
- std::vector<DiagWithFixIts> Diags;
+ DiagList Diags;
std::vector<const Decl *> TopLevelDecls;
bool PreambleDeclsDeserialized;
InclusionLocations IncLocations;
@@ -143,7 +137,7 @@
/// Rebuild the AST and the preamble.
/// Returns a list of diagnostics or llvm::None, if an error occured.
- llvm::Optional<std::vector<DiagWithFixIts>> rebuild(ParseInputs &&Inputs);
+ llvm::Optional<DiagList> rebuild(ParseInputs &&Inputs);
/// Returns the last built preamble.
const std::shared_ptr<const PreambleData> &getPreamble() const;
/// Returns the last built AST.
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -9,7 +9,9 @@
#include "ClangdUnit.h"
#include "Compiler.h"
+#include "Diagnostics.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -81,22 +83,6 @@
std::vector<const Decl *> TopLevelDecls;
};
-// Converts a half-open clang source range to an LSP range.
-// Note that clang also uses closed source ranges, which this can't handle!
-Range toRange(CharSourceRange R, const SourceManager &M) {
- // Clang is 1-based, LSP uses 0-based indexes.
- Position Begin;
- Begin.line = static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1;
- Begin.character =
- static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1;
-
- Position End;
- End.line = static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1;
- End.character = static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1;
-
- return {Begin, End};
-}
-
class InclusionLocationsCollector : public PPCallbacks {
public:
InclusionLocationsCollector(SourceManager &SourceMgr,
@@ -115,7 +101,7 @@
if (SourceMgr.isInMainFile(SR.getBegin())) {
// Only inclusion directives in the main file make sense. The user cannot
// select directives not in the main file.
- IncLocations.emplace_back(toRange(FilenameRange, SourceMgr),
+ IncLocations.emplace_back(halfOpenToRange(FilenameRange, SourceMgr),
File->tryGetRealPathName());
}
}
@@ -170,113 +156,6 @@
SourceManager *SourceMgr = nullptr;
};
-/// Convert from clang diagnostic level to LSP severity.
-static int getSeverity(DiagnosticsEngine::Level L) {
- switch (L) {
- case DiagnosticsEngine::Remark:
- return 4;
- case DiagnosticsEngine::Note:
- return 3;
- case DiagnosticsEngine::Warning:
- return 2;
- case DiagnosticsEngine::Fatal:
- case DiagnosticsEngine::Error:
- return 1;
- case DiagnosticsEngine::Ignored:
- return 0;
- }
- llvm_unreachable("Unknown diagnostic level!");
-}
-
-// Checks whether a location is within a half-open range.
-// Note that clang also uses closed source ranges, which this can't handle!
-bool locationInRange(SourceLocation L, CharSourceRange R,
- const SourceManager &M) {
- assert(R.isCharRange());
- if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
- M.getFileID(R.getBegin()) != M.getFileID(L))
- return false;
- return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
-}
-
-// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
-// LSP needs a single range.
-Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
- auto &M = D.getSourceManager();
- auto Loc = M.getFileLoc(D.getLocation());
- // Accept the first range that contains the location.
- for (const auto &CR : D.getRanges()) {
- auto R = Lexer::makeFileCharRange(CR, M, L);
- if (locationInRange(Loc, R, M))
- return toRange(R, M);
- }
- // The range may be given as a fixit hint instead.
- for (const auto &F : D.getFixItHints()) {
- auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
- if (locationInRange(Loc, R, M))
- return toRange(R, M);
- }
- // If no suitable range is found, just use the token at the location.
- auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
- if (!R.isValid()) // Fall back to location only, let the editor deal with it.
- R = CharSourceRange::getCharRange(Loc);
- return toRange(R, M);
-}
-
-TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
- const LangOptions &L) {
- TextEdit Result;
- Result.range = toRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M);
- Result.newText = FixIt.CodeToInsert;
- return Result;
-}
-
-llvm::Optional<DiagWithFixIts> toClangdDiag(const clang::Diagnostic &D,
- DiagnosticsEngine::Level Level,
- const LangOptions &LangOpts) {
- if (!D.hasSourceManager() || !D.getLocation().isValid() ||
- !D.getSourceManager().isInMainFile(D.getLocation())) {
- IgnoreDiagnostics::log(Level, D);
- return llvm::None;
- }
-
- SmallString<64> Message;
- D.FormatDiagnostic(Message);
-
- DiagWithFixIts Result;
- Result.Diag.range = diagnosticRange(D, LangOpts);
- Result.Diag.severity = getSeverity(Level);
- Result.Diag.message = Message.str();
- for (const FixItHint &Fix : D.getFixItHints())
- Result.FixIts.push_back(toTextEdit(Fix, D.getSourceManager(), LangOpts));
- return std::move(Result);
-}
-
-class StoreDiagsConsumer : public DiagnosticConsumer {
-public:
- StoreDiagsConsumer(std::vector<DiagWithFixIts> &Output) : Output(Output) {}
-
- // Track language options in case we need to expand token ranges.
- void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override {
- LangOpts = Opts;
- }
-
- void EndSourceFile() override { LangOpts = llvm::None; }
-
- void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
- const clang::Diagnostic &Info) override {
- DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
-
- if (LangOpts)
- if (auto D = toClangdDiag(Info, DiagLevel, *LangOpts))
- Output.push_back(std::move(*D));
- }
-
-private:
- std::vector<DiagWithFixIts> &Output;
- llvm::Optional<LangOptions> LangOpts;
-};
-
} // namespace
void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
@@ -289,8 +168,8 @@
std::unique_ptr<llvm::MemoryBuffer> Buffer,
std::shared_ptr<PCHContainerOperations> PCHs,
IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
- std::vector<DiagWithFixIts> ASTDiags;
- StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
+ DiagList ASTDiags;
+ StoreDiagsConsumer UnitDiagsConsumer(ASTDiags);
const PrecompiledPreamble *PreamblePCH =
Preamble ? &Preamble->Preamble : nullptr;
@@ -328,6 +207,8 @@
// UnitDiagsConsumer is local, we can not store it in CompilerInstance that
// has a longer lifetime.
Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
+ // CompilerInstance won't run this callback, do it directly.
+ UnitDiagsConsumer.EndSourceFile();
std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls();
return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
@@ -399,35 +280,29 @@
return TopLevelDecls;
}
-const std::vector<DiagWithFixIts> &ParsedAST::getDiagnostics() const {
- return Diags;
-}
+const DiagList &ParsedAST::getDiagnostics() const { return Diags; }
std::size_t ParsedAST::getUsedBytes() const {
auto &AST = getASTContext();
- // FIXME(ibiryukov): we do not account for the dynamically allocated part of
- // SmallVector<FixIt> inside each Diag.
return AST.getASTAllocatedMemory() + AST.getSideTableAllocatedMemory() +
- ::getUsedBytes(TopLevelDecls) + ::getUsedBytes(Diags);
+ ::getUsedBytes(TopLevelDecls) + Diags.getUsedBytes();
}
const InclusionLocations &ParsedAST::getInclusionLocations() const {
return IncLocations;
}
PreambleData::PreambleData(PrecompiledPreamble Preamble,
std::vector<serialization::DeclID> TopLevelDeclIDs,
- std::vector<DiagWithFixIts> Diags,
- InclusionLocations IncLocations)
+ DiagList Diags, InclusionLocations IncLocations)
: Preamble(std::move(Preamble)),
TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)),
IncLocations(std::move(IncLocations)) {}
ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
- std::vector<const Decl *> TopLevelDecls,
- std::vector<DiagWithFixIts> Diags,
+ std::vector<const Decl *> TopLevelDecls, DiagList Diags,
InclusionLocations IncLocations)
: Preamble(std::move(Preamble)), Clang(std::move(Clang)),
Action(std::move(Action)), Diags(std::move(Diags)),
@@ -445,8 +320,7 @@
log("Created CppFile for " + FileName);
}
-llvm::Optional<std::vector<DiagWithFixIts>>
-CppFile::rebuild(ParseInputs &&Inputs) {
+llvm::Optional<DiagList> CppFile::rebuild(ParseInputs &&Inputs) {
log("Rebuilding file " + FileName + " with command [" +
Inputs.CompileCommand.Directory + "] " +
llvm::join(Inputs.CompileCommand.CommandLine, " "));
@@ -500,14 +374,12 @@
std::move(ContentsBuffer), PCHs, Inputs.FS);
}
- std::vector<DiagWithFixIts> Diagnostics;
+ DiagList Diagnostics;
if (NewAST) {
// Collect diagnostics from both the preamble and the AST.
if (NewPreamble)
- Diagnostics.insert(Diagnostics.begin(), NewPreamble->Diags.begin(),
- NewPreamble->Diags.end());
- Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
- NewAST->getDiagnostics().end());
+ Diagnostics.insert(NewPreamble->Diags);
+ Diagnostics.insert(NewAST->getDiagnostics());
}
if (ASTCallback && NewAST) {
trace::Span Tracer("Running ASTCallback");
@@ -557,7 +429,7 @@
trace::Span Tracer("Preamble");
SPAN_ATTACH(Tracer, "File", FileName);
- std::vector<DiagWithFixIts> PreambleDiags;
+ DiagList PreambleDiags;
StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags);
IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -70,9 +70,8 @@
virtual ~DiagnosticsConsumer() = default;
/// Called by ClangdServer when \p Diagnostics for \p File are ready.
- virtual void
- onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) = 0;
+ virtual void onDiagnosticsReady(PathRef File,
+ Tagged<DiagList> Diagnostics) = 0;
};
class FileSystemProvider {
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -527,7 +527,7 @@
VFSTag Tag = std::move(TaggedFS.Tag);
auto Callback = [this, Version, FileStr,
- Tag](std::vector<DiagWithFixIts> Diags) {
+ Tag](DiagList Diags) {
// We need to serialize access to resulting diagnostics to avoid calling
// `onDiagnosticsReady` in the wrong order.
std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
Index: clangd/ClangdLSPServer.h
===================================================================
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -50,9 +50,7 @@
private:
// Implement DiagnosticsConsumer.
- virtual void
- onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override;
+ void onDiagnosticsReady(PathRef File, Tagged<DiagList> Diagnostics) override;
// Implement ProtocolCallbacks.
void onInitialize(InitializeParams &Params) override;
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -8,6 +8,7 @@
//===---------------------------------------------------------------------===//
#include "ClangdLSPServer.h"
+#include "Diagnostics.h"
#include "JSONRPCDispatcher.h"
#include "SourceCode.h"
#include "URI.h"
@@ -304,6 +305,12 @@
}
void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
+ auto GetFixitMessage = [](llvm::StringRef Message) -> llvm::StringRef {
+ // VSCode does not really print messages with newlines nicely.
+ // And we put notes into diagnostic messages, which are not useful for FixIt
+ // messages shown to the users.
+ return Message.take_until([](char C) { return C == '\n'; });
+ };
// We provide a code action for each diagnostic at the requested location
// which has FixIts available.
auto Code = Server.getDocument(Params.textDocument.uri.file());
@@ -318,7 +325,8 @@
WorkspaceEdit WE;
WE.changes = {{Params.textDocument.uri.uri(), std::move(Edits)}};
Commands.push_back(json::obj{
- {"title", llvm::formatv("Apply FixIt {0}", D.message)},
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
{"arguments", {WE}},
});
@@ -441,23 +449,22 @@
return FixItsIter->second;
}
-void ClangdLSPServer::onDiagnosticsReady(
- PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
+void ClangdLSPServer::onDiagnosticsReady(PathRef File,
+ Tagged<DiagList> Diagnostics) {
json::ary DiagnosticsJSON;
DiagnosticToReplacementMap LocalFixIts; // Temporary storage
- for (auto &DiagWithFixes : Diagnostics.Value) {
- auto Diag = DiagWithFixes.Diag;
+ toLSPDiags(Diagnostics.Value, [&](DiagWithFixIts D) {
DiagnosticsJSON.push_back(json::obj{
- {"range", Diag.range},
- {"severity", Diag.severity},
- {"message", Diag.message},
+ {"range", D.Diag.range},
+ {"severity", D.Diag.severity},
+ {"message", D.Diag.message},
});
- // We convert to Replacements to become independent of the SourceManager.
- auto &FixItsForDiagnostic = LocalFixIts[Diag];
- std::copy(DiagWithFixes.FixIts.begin(), DiagWithFixes.FixIts.end(),
+
+ auto &FixItsForDiagnostic = LocalFixIts[D.Diag];
+ std::copy(D.FixIts.begin(), D.FixIts.end(),
std::back_inserter(FixItsForDiagnostic));
- }
+ });
// Cache FixIts
{
Index: clangd/CMakeLists.txt
===================================================================
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -11,6 +11,7 @@
CompileArgsCache.cpp
Compiler.cpp
Context.cpp
+ Diagnostics.cpp
DraftStore.cpp
FuzzyMatch.cpp
GlobalCompilationDatabase.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
