ilya-biryukov updated this revision to Diff 137589.
ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added a comment.
This is not final, there are still unadressed comments.
- Significantly simplified the implementation by removing the DiagList, etc.
- Addressed some of the rest of the comments.
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
@@ -42,7 +42,7 @@
class IgnoreDiagnostics : public DiagnosticsConsumer {
void onDiagnosticsReady(
- PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+ PathRef File, Tagged<std::vector<PersistentDiag>> 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<std::vector<PersistentDiag>>) {}
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(); });
+ [&](std::vector<PersistentDiag>) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) {
+ [&](std::vector<PersistentDiag> Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector<DiagWithFixIts> Diags) {
+ [&](std::vector<PersistentDiag> Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ [&](std::vector<PersistentDiag> 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) {
+ [&](std::vector<PersistentDiag> 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; });
+ [&](std::vector<PersistentDiag> 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; });
+ [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -191,7 +191,7 @@
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
[Nonce, &Mut, &TotalUpdates](
- llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
+ llvm::Optional<std::vector<PersistentDiag>> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
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 {}
+ PathRef File, Tagged<std::vector<PersistentDiag>> Diagnostics) override {}
};
// GMock helpers for matching completion items.
@@ -319,11 +319,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({});
@@ -547,7 +547,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,26 +20,29 @@
namespace clang {
namespace clangd {
using namespace llvm;
-void PrintTo(const DiagWithFixIts &D, std::ostream *O) {
+
+void PrintTo(const Diag &D, std::ostream *O) {
llvm::raw_os_ostream OS(*O);
- OS << D.Diag;
- if (!D.FixIts.empty()) {
+ if (!D.InsideMainFile)
+ OS << "[in " << D.File << "] ";
+ OS << D.Message;
+
+ if (D.Fixes.empty()) {
OS << " {";
const char *Sep = "";
- for (const auto &F : D.FixIts) {
- OS << Sep << F;
+ for (const auto &Edit : D.Fixes) {
+ OS << Sep << Edit;
Sep = ", ";
}
- OS << "}";
}
}
namespace {
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,18 +53,29 @@
return std::move(*AST);
}
+std::vector<Diag> buildDiags(llvm::StringRef Code,
+ std::vector<const char *> Flags = {}) {
+ auto Diags = build(Code, std::move(Flags)).getDiagnostics();
+ std::vector<Diag> Flattened;
+ for (auto &&D : Diags) {
+ Flattened.push_back(D.Main);
+ for (auto &&N : D.Notes)
+ Flattened.push_back(N);
+ }
+ return Flattened;
+}
+
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.Fixes.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 &&
- arg.FixIts.size() == 1 && arg.FixIts[0].range == Range &&
- arg.FixIts[0].newText == Replacement;
+ return arg.Range == Range && arg.Message == Message &&
+ arg.Fixes.size() == 1 && arg.Fixes[0].range == Range &&
+ arg.Fixes[0].newText == Replacement;
}
TEST(DiagnosticsTest, DiagnosticRanges) {
@@ -75,31 +89,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()),
+ 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()),
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"}),
ElementsAre(
// FIXME: ideally this would be one diagnostic with a named FixIt.
Diag(Test.range(), "return type of 'main' is not 'int'"),
@@ -121,7 +133,7 @@
#endif
)cpp");
EXPECT_THAT(
- build(Test.code()).getDiagnostics(),
+ buildDiags(Test.code()),
ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'")));
}
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -42,12 +42,10 @@
namespace {
-static bool diagsContainErrors(ArrayRef<DiagWithFixIts> Diagnostics) {
- for (const auto &DiagAndFixIts : Diagnostics) {
- // FIXME: severities returned by clangd should have a descriptive
- // diagnostic severity enum
- const int ErrorSeverity = 1;
- if (DiagAndFixIts.Diag.severity == ErrorSeverity)
+static bool diagsContainErrors(const std::vector<PersistentDiag> &Diagnostics) {
+ for (auto D : Diagnostics) {
+ if (D.Main.Severity == DiagnosticsEngine::Error ||
+ D.Main.Severity == DiagnosticsEngine::Fatal)
return true;
}
return false;
@@ -57,7 +55,7 @@
public:
void
onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ Tagged<std::vector<PersistentDiag>> Diagnostics) override {
bool HadError = diagsContainErrors(Diagnostics.Value);
std::lock_guard<std::mutex> Lock(Mutex);
@@ -84,7 +82,7 @@
public:
void
onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ Tagged<std::vector<PersistentDiag>> Diagnostics) override {
bool HadError = diagsContainErrors(Diagnostics.Value);
std::lock_guard<std::mutex> Lock(Mutex);
@@ -614,7 +612,7 @@
void onDiagnosticsReady(
PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ Tagged<std::vector<PersistentDiag>> Diagnostics) override {
StringRef FileIndexStr = llvm::sys::path::stem(File);
ASSERT_TRUE(FileIndexStr.consume_front("Foo"));
@@ -882,7 +880,7 @@
: StartSecondReparse(std::move(StartSecondReparse)) {}
void onDiagnosticsReady(PathRef,
- Tagged<std::vector<DiagWithFixIts>>) override {
+ Tagged<std::vector<PersistentDiag>>) override {
++Count;
std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());
ASSERT_TRUE(Lock.owns_lock())
@@ -945,8 +943,8 @@
auto FooCpp = testPath("foo.cpp");
const auto Code = R"cpp(
-#include "z.h"
#include "x.h"
+#include "z.h"
void f() {}
)cpp";
@@ -967,7 +965,7 @@
.contains((llvm::Twine("#include ") + Expected + "").str());
};
- EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"","\"y.h\""));
+ EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"", "\"y.h\""));
EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"\"Y.h\"", "\"Y.h\""));
EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
@@ -1000,8 +998,8 @@
auto Path = testPath("foo.cpp");
std::string Code = R"cpp(
-#include "y.h"
#include "x.h"
+#include "y.h"
void f( ) {}
)cpp";
Index: test/clangd/fixits.test
===================================================================
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -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"},{"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": [
@@ -91,7 +91,7 @@
# CHECK-NEXT: }
# CHECK-NEXT: ],
# CHECK-NEXT: "command": "clangd.applyFix",
-# CHECK-NEXT: "title": "Apply FixIt place parentheses around the assignment to silence this warning"
+# CHECK-NEXT: "title": "Apply FixIt: place parentheses around the assignment to silence this warning"
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
@@ -116,11 +116,11 @@
# CHECK-NEXT: }
# CHECK-NEXT: ],
# CHECK-NEXT: "command": "clangd.applyFix",
-# CHECK-NEXT: "title": "Apply FixIt use '==' to turn this assignment into an equality comparison"
+# CHECK-NEXT: "title": "Apply FixIt: use '==' to turn this assignment into an equality comparison"
# 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"},{"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",
@@ -161,7 +161,7 @@
# CHECK-NEXT: }
# CHECK-NEXT: ],
# CHECK-NEXT: "command": "clangd.applyFix",
-# CHECK-NEXT: "title": "Apply FixIt place parentheses around the assignment to silence this warning"
+# CHECK-NEXT: "title": "Apply FixIt: place parentheses around the assignment to silence this warning"
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
@@ -186,7 +186,7 @@
# CHECK-NEXT: }
# CHECK-NEXT: ],
# CHECK-NEXT: "command": "clangd.applyFix",
-# CHECK-NEXT: "title": "Apply FixIt use '==' to turn this assignment into an equality comparison"
+# CHECK-NEXT: "title": "Apply FixIt: use '==' to turn this assignment into an equality comparison"
# CHECK-NEXT: }
# CHECK-NEXT: ]
---
Index: test/clangd/extra-flags.test
===================================================================
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -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,
@@ -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
@@ -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
@@ -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(std::vector<PersistentDiag>)> 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(std::vector<PersistentDiag>)> 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;
};
@@ -212,7 +212,7 @@
void ASTWorker::update(
ParseInputs Inputs, WantDiagnostics WantDiags,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+ UniqueFunction<void(std::vector<PersistentDiag>)> OnUpdated) {
auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
FileInputs = Inputs;
auto Diags = AST.rebuild(std::move(Inputs));
@@ -449,7 +449,7 @@
void TUScheduler::update(
PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
- UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+ UniqueFunction<void(std::vector<PersistentDiag>)> 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(const SourceManager &SM, CharSourceRange R);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/SourceCode.cpp
===================================================================
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -48,5 +48,13 @@
return P;
}
+Range halfOpenToRange(const SourceManager &SM, CharSourceRange R) {
+ // Clang is 1-based, LSP uses 0-based indexes.
+ Position Begin = sourceLocToPosition(SM, R.getBegin());
+ Position End = sourceLocToPosition(SM, R.getEnd());
+
+ return {Begin, End};
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/Diagnostics.h
===================================================================
--- /dev/null
+++ clangd/Diagnostics.h
@@ -0,0 +1,90 @@
+//===--- 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 {
+
+struct Diag {
+ std::string Message;
+ // Intended to be used only in error messages.
+ // May be relative, absolute or even artifically constructed.
+ std::string File;
+ clangd::Range Range;
+ DiagnosticsEngine::Level Severity;
+ // Since File is only descriptive, we store a separate flag to distinguish
+ // diags from the main file.
+ bool InsideMainFile = false;
+ /// TextEdits from clang's fix-its.
+ llvm::SmallVector<TextEdit, 1> Fixes;
+};
+
+struct PersistentDiag {
+ /// Main diagnostic.
+ Diag Main;
+ /// Notes for Main.
+ std::vector<Diag> Notes;
+};
+
+/// 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 PersistentDiag &D,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn);
+
+/// StoreDiagsConsumer collects the diagnostics that can later be reported by
+/// clangd. It groups all notes for a diagnostic into a single PersistentDiag
+/// and filters out diagnostics that don't mention the main file (i.e. neither
+/// the diag itself nor its notes are in the main file).
+class StoreDiagsConsumer : public DiagnosticConsumer {
+public:
+ StoreDiagsConsumer(std::vector<PersistentDiag> &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();
+
+ std::vector<PersistentDiag> &Output;
+ llvm::Optional<LangOptions> LangOpts;
+ llvm::Optional<PersistentDiag> LastDiag;
+ /// Is any diag or note from LastDiag in the main file?
+ bool LastDiagMentionsMainFile = false;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clangd/Diagnostics.cpp
===================================================================
--- /dev/null
+++ clangd/Diagnostics.cpp
@@ -0,0 +1,290 @@
+//===--- 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 "Logger.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 {
+
+bool mentionsMainFile(const PersistentDiag &D) {
+ if (D.Main.InsideMainFile)
+ return true;
+ for (auto &N : D.Notes) {
+ if (N.InsideMainFile)
+ return true;
+ }
+ return false;
+}
+
+/// 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 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 halfOpenToRange(M, R);
+ }
+ // 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 halfOpenToRange(M, R);
+ }
+ // 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(M, R);
+}
+
+TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
+ const LangOptions &L) {
+ TextEdit Result;
+ Result.range =
+ halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L));
+ Result.newText = FixIt.CodeToInsert;
+ return Result;
+}
+
+bool isInsideMainFile(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;
+}
+
+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");
+}
+
+/// Prints a single diagnostic in a clang-like manner, the output includes
+/// location, severity and error message. An example of the output message is:
+///
+/// main.cpp:12:23: error: undeclared identifier
+///
+/// For main file we only print the basename and for all other files we print
+/// the filename on a separate line to provide a slightly more readable output
+/// in the editors:
+///
+/// dir1/dir2/dir3/../../dir4/header.h:12:23
+/// error: undeclared identifier
+void printDiag(llvm::raw_string_ostream &OS, const Diag &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;
+}
+
+/// 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.
+/// Example output:
+///
+/// no matching function for call to 'foo'
+///
+/// main.cpp:3:5: note: candidate function not viable: requires 2 arguments
+///
+/// dir1/dir2/dir3/../../dir4/header.h:12:23
+/// note: candidate function not viable: requires 3 arguments
+std::string mainMessage(const PersistentDiag &D) {
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ OS << D.Main.Message;
+ for (auto &Note : D.Notes) {
+ if (!Note.Fixes.empty()) {
+ // We don't add fix-it notes to main diag message, they add too much
+ // noise.
+ continue;
+ }
+ OS << "\n\n";
+ printDiag(OS, Note);
+ }
+ OS.flush();
+ return 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 Diag &Note) {
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ OS << Note.Message;
+ OS << "\n\n";
+ printDiag(OS, Main);
+ OS.flush();
+ return Result;
+}
+} // namespace
+
+void toLSPDiags(const PersistentDiag &D,
+ llvm::function_ref<void(DiagWithFixIts)> OutFn) {
+ auto FillBasicFields = [](const Diag &D) {
+ DiagWithFixIts Res;
+ Res.Diag.range = D.Range;
+ Res.Diag.severity = getSeverity(D.Severity);
+ Res.FixIts = D.Fixes;
+ return Res;
+ };
+
+ DiagWithFixIts Main = FillBasicFields(D.Main);
+ Main.Diag.message = mainMessage(D);
+ OutFn(std::move(Main));
+
+ for (auto &Note : D.Notes) {
+ DiagWithFixIts Res = FillBasicFields(Note);
+ Res.Diag.message = noteMessage(D.Main, Note);
+ OutFn(std::move(Res));
+ }
+}
+
+StoreDiagsConsumer::StoreDiagsConsumer(std::vector<PersistentDiag> &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 || !Info.hasSourceManager()) {
+ IgnoreDiagnostics::log(DiagLevel, Info);
+ return;
+ }
+
+ bool InsideMainFile = isInsideMainFile(Info);
+
+ auto BuildDiagData = [&]() -> Diag {
+ Diag D;
+ D.Range = diagnosticRange(Info, *LangOpts);
+ llvm::SmallString<64> Message;
+ Info.FormatDiagnostic(Message);
+ D.Message = Message.str();
+ D.InsideMainFile = InsideMainFile;
+ D.File = Info.getSourceManager().getFilename(Info.getLocation());
+ D.Severity = DiagLevel;
+
+ if (D.InsideMainFile) {
+ for (auto &FixIt : Info.getFixItHints())
+ D.Fixes.push_back(
+ toTextEdit(FixIt, Info.getSourceManager(), *LangOpts));
+ }
+ return D;
+ };
+
+ if (!isNote(DiagLevel)) {
+ // A new diagnostic.
+ flushLastDiag();
+
+ LastDiag = PersistentDiag();
+ LastDiag->Main = BuildDiagData();
+ } else {
+ // A clang note without fix-its, transformed to clangd::Note.
+ LastDiag->Notes.push_back(BuildDiagData());
+ }
+}
+
+void StoreDiagsConsumer::flushLastDiag() {
+ if (!LastDiag)
+ return;
+ if (mentionsMainFile(*LastDiag))
+ Output.push_back(std::move(*LastDiag));
+ else
+ log(Twine("Dropped diagnostic outside main file:") + LastDiag->Main.File +
+ ":" + LastDiag->Main.Message);
+ LastDiag.reset();
+}
+
+} // 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,18 @@
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,
+ std::vector<PersistentDiag> Diags,
InclusionLocations IncLocations);
PrecompiledPreamble Preamble;
std::vector<serialization::DeclID> TopLevelDeclIDs;
- std::vector<DiagWithFixIts> Diags;
+ std::vector<PersistentDiag> Diags;
InclusionLocations IncLocations;
};
@@ -96,7 +91,7 @@
/// this call might be expensive.
ArrayRef<const Decl *> getTopLevelDecls();
- const std::vector<DiagWithFixIts> &getDiagnostics() const;
+ const std::vector<PersistentDiag> &getDiagnostics() const;
/// Returns the esitmated size of the AST and the accessory structures, in
/// bytes. Does not include the size of the preamble.
@@ -108,7 +103,7 @@
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
std::vector<const Decl *> TopLevelDecls,
- std::vector<DiagWithFixIts> Diags, InclusionLocations IncLocations);
+ std::vector<PersistentDiag> Diags, InclusionLocations IncLocations);
private:
void ensurePreambleDeclsDeserialized();
@@ -125,7 +120,7 @@
std::unique_ptr<FrontendAction> Action;
// Data, stored after parsing.
- std::vector<DiagWithFixIts> Diags;
+ std::vector<PersistentDiag> Diags;
std::vector<const Decl *> TopLevelDecls;
bool PreambleDeclsDeserialized;
InclusionLocations IncLocations;
@@ -143,7 +138,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<std::vector<PersistentDiag>> 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(SourceMgr, FilenameRange),
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);
+ std::vector<PersistentDiag> 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,7 +280,7 @@
return TopLevelDecls;
}
-const std::vector<DiagWithFixIts> &ParsedAST::getDiagnostics() const {
+const std::vector<PersistentDiag> &ParsedAST::getDiagnostics() const {
return Diags;
}
@@ -417,7 +298,7 @@
PreambleData::PreambleData(PrecompiledPreamble Preamble,
std::vector<serialization::DeclID> TopLevelDeclIDs,
- std::vector<DiagWithFixIts> Diags,
+ std::vector<PersistentDiag> Diags,
InclusionLocations IncLocations)
: Preamble(std::move(Preamble)),
TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)),
@@ -427,7 +308,7 @@
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
std::vector<const Decl *> TopLevelDecls,
- std::vector<DiagWithFixIts> Diags,
+ std::vector<PersistentDiag> Diags,
InclusionLocations IncLocations)
: Preamble(std::move(Preamble)), Clang(std::move(Clang)),
Action(std::move(Action)), Diags(std::move(Diags)),
@@ -445,7 +326,7 @@
log("Created CppFile for " + FileName);
}
-llvm::Optional<std::vector<DiagWithFixIts>>
+llvm::Optional<std::vector<PersistentDiag>>
CppFile::rebuild(ParseInputs &&Inputs) {
log("Rebuilding file " + FileName + " with command [" +
Inputs.CompileCommand.Directory + "] " +
@@ -500,12 +381,11 @@
std::move(ContentsBuffer), PCHs, Inputs.FS);
}
- std::vector<DiagWithFixIts> Diagnostics;
+ std::vector<PersistentDiag> 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 = NewPreamble->Diags;
Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
NewAST->getDiagnostics().end());
}
@@ -557,7 +437,7 @@
trace::Span Tracer("Preamble");
SPAN_ATTACH(Tracer, "File", FileName);
- std::vector<DiagWithFixIts> PreambleDiags;
+ std::vector<PersistentDiag> PreambleDiags;
StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags);
IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -72,7 +72,7 @@
/// Called by ClangdServer when \p Diagnostics for \p File are ready.
virtual void
onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) = 0;
+ Tagged<std::vector<PersistentDiag>> Diagnostics) = 0;
};
class FileSystemProvider {
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -542,7 +542,7 @@
VFSTag Tag = std::move(TaggedFS.Tag);
auto Callback = [this, Version, FileStr,
- Tag](std::vector<DiagWithFixIts> Diags) {
+ Tag](std::vector<PersistentDiag> 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
@@ -46,9 +46,9 @@
private:
// Implement DiagnosticsConsumer.
- virtual void
+ void
onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override;
+ Tagged<std::vector<PersistentDiag>> 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}},
});
@@ -437,21 +445,22 @@
}
void ClangdLSPServer::onDiagnosticsReady(
- PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
+ PathRef File, Tagged<std::vector<PersistentDiag>> Diagnostics) {
json::ary DiagnosticsJSON;
DiagnosticToReplacementMap LocalFixIts; // Temporary storage
- for (auto &DiagWithFixes : Diagnostics.Value) {
- auto Diag = DiagWithFixes.Diag;
- DiagnosticsJSON.push_back(json::obj{
- {"range", Diag.range},
- {"severity", Diag.severity},
- {"message", Diag.message},
+ for (auto &Diag : Diagnostics.Value) {
+ toLSPDiags(Diag, [&](DiagWithFixIts D) {
+ DiagnosticsJSON.push_back(json::obj{
+ {"range", D.Diag.range},
+ {"severity", D.Diag.severity},
+ {"message", D.Diag.message},
+ });
+
+ auto &FixItsForDiagnostic = LocalFixIts[D.Diag];
+ std::copy(D.FixIts.begin(), D.FixIts.end(),
+ std::back_inserter(FixItsForDiagnostic));
});
- // We convert to Replacements to become independent of the SourceManager.
- auto &FixItsForDiagnostic = LocalFixIts[Diag];
- std::copy(DiagWithFixes.FixIts.begin(), DiagWithFixes.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
