ilya-biryukov updated this revision to Diff 137987. ilya-biryukov marked 14 inline comments as done. ilya-biryukov added a comment.
- Addressed review 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/Protocol.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<std::vector<Diag>> 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<Diag>>) {} 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<Diag>) { Ready.wait(); }); S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes, - [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; }); + [&](std::vector<Diag> Diags) { ++CallbackCount; }); S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto, - [&](std::vector<DiagWithFixIts> Diags) { + [&](std::vector<Diag> 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<Diag> 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<Diag> 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<Diag> 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<Diag> 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<Diag> Diags) { ++CallbackCount; }); } EXPECT_EQ(2, CallbackCount); } @@ -190,8 +190,8 @@ { WithContextValue WithNonce(NonceKey, ++Nonce); S.update(File, Inputs, WantDiagnostics::Auto, - [Nonce, &Mut, &TotalUpdates]( - llvm::Optional<std::vector<DiagWithFixIts>> Diags) { + [Nonce, &Mut, + &TotalUpdates](llvm::Optional<std::vector<Diag>> 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 {} + void onDiagnosticsReady(PathRef File, + Tagged<std::vector<Diag>> 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,24 @@ namespace clang { namespace clangd { using namespace llvm; -void PrintTo(const DiagWithFixIts &D, std::ostream *O) { - llvm::raw_os_ostream OS(*O); - OS << D.Diag; - if (!D.FixIts.empty()) { - OS << " {"; - const char *Sep = ""; - for (const auto &F : D.FixIts) { - OS << Sep << F; - Sep = ", "; - } - OS << "}"; - } -} namespace { using testing::ElementsAre; +using testing::Field; +using testing::IsEmpty; +using testing::Pair; + +testing::Matcher<const Diag &> WithFix(testing::Matcher<Fix> FixMatcher) { + return Field(&Diag::Fixes, ElementsAre(FixMatcher)); +} + +testing::Matcher<const Diag &> WithNote(testing::Matcher<Note> NoteMatcher) { + return Field(&Diag::Notes, ElementsAre(NoteMatcher)); +} // 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 +48,37 @@ return std::move(*AST); } +std::vector<Diag> 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(); + "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { + return arg.Range == Range && arg.Message == Message; } 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.Message == Message && arg.Edits.size() == 1 && + arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement; +} + +// Helper function to make tests shorter. +Position pos(int line, int character) { + Position Res; + Res.line = line; + Res.character = character; + return Res; +}; + +/// Matches diagnostic that has exactly one fix with the same range and message +/// as the diagnostic itself. +testing::Matcher<const clangd::Diag &> DiagWithEqualFix(clangd::Range Range, + std::string Replacement, + std::string Message) { + return AllOf(Diag(Range, Message), WithFix(Fix(Range, Replacement, Message))); } TEST(DiagnosticsTest, DiagnosticRanges) { @@ -75,35 +92,34 @@ $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. + AllOf(DiagWithEqualFix( + Test.range("typo"), "foo", + "use of undeclared identifier 'goo'; did you mean 'foo'?"), + // This is a pretty normal range. + WithNote(Diag(Test.range("decl"), "'foo' declared here"))), + // This range is zero-width, and at the end of a line. + DiagWithEqualFix(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(), - ElementsAre(Fix(Test.range(), "int", "'main' must return 'int'"))); + EXPECT_THAT(buildDiags(Test.code()), + ElementsAre(DiagWithEqualFix(Test.range(), "int", + "'main' must return 'int'"))); // Same code built as C gets different diagnostics. EXPECT_THAT( - build(Test.code(), {"-x", "c"}).getDiagnostics(), - ElementsAre( - // FIXME: ideally this would be one diagnostic with a named FixIt. + buildDiags(Test.code(), {"-x", "c"}), + ElementsAre(AllOf( Diag(Test.range(), "return type of 'main' is not 'int'"), - Fix(Test.range(), "int", "change return type to 'int'"))); + WithFix(Fix(Test.range(), "int", "change return type to 'int'"))))); } TEST(DiagnosticsTest, Preprocessor) { @@ -121,10 +137,71 @@ #endif )cpp"); EXPECT_THAT( - build(Test.code()).getDiagnostics(), + buildDiags(Test.code()), ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); } +TEST(DiagnosticsTest, ToLSP) { + clangd::Diag D; + D.Message = "something terrible happened"; + D.Range = {pos(1, 2), pos(3, 4)}; + D.InsideMainFile = true; + D.Severity = DiagnosticsEngine::Error; + D.File = "foo/bar/main.cpp"; + + clangd::Note NoteInMain; + NoteInMain.Message = "declared somewhere in the main file"; + NoteInMain.Range = {pos(5, 6), pos(7, 8)}; + NoteInMain.Severity = DiagnosticsEngine::Remark; + NoteInMain.File = "../foo/bar/main.cpp"; + NoteInMain.InsideMainFile = true; + D.Notes.push_back(NoteInMain); + + clangd::Note NoteInHeader; + NoteInHeader.Message = "declared somewhere in the header file"; + NoteInHeader.Range = {pos(9, 10), pos(11, 12)}; + NoteInHeader.Severity = DiagnosticsEngine::Note; + NoteInHeader.File = "../foo/baz/header.h"; + NoteInHeader.InsideMainFile = false; + D.Notes.push_back(NoteInHeader); + + clangd::Fix F; + F.Message = "do something"; + D.Fixes.push_back(F); + + auto MatchingLSP = [](const DiagBase &D, llvm::StringRef Message) { + clangd::Diagnostic Res; + Res.range = D.Range; + Res.severity = getSeverity(D.Severity); + Res.message = Message; + return Res; + }; + + // Diagnostics should turn into these: + clangd::Diagnostic MainLSP = MatchingLSP(D, R"(something terrible happened + +main.cpp:6:7: remark: declared somewhere in the main file + +../foo/baz/header.h:10:11: +note: declared somewhere in the header file)"); + + clangd::Diagnostic NoteInMainLSP = + MatchingLSP(NoteInMain, R"(declared somewhere in the main file + +main.cpp:2:3: error: something terrible happened)"); + + // Transform dianostics and check the results. + std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags; + toLSPDiags(D, [&](clangd::Diagnostic LSPDiag, + llvm::ArrayRef<clangd::Fix> Fixes) { + LSPDiags.push_back({std::move(LSPDiag), + std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())}); + }); + + EXPECT_THAT(LSPDiags, ElementsAre(Pair(MainLSP, ElementsAre(F)), + Pair(NoteInMainLSP, IsEmpty()))); +} + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -42,22 +42,19 @@ 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<Diag> &Diagnostics) { + for (auto D : Diagnostics) { + if (D.Severity == DiagnosticsEngine::Error || + D.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<std::vector<Diag>> Diagnostics) override { bool HadError = diagsContainErrors(Diagnostics.Value); std::lock_guard<std::mutex> Lock(Mutex); @@ -82,9 +79,8 @@ /// least one error. class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: - void - onDiagnosticsReady(PathRef File, - Tagged<std::vector<DiagWithFixIts>> Diagnostics) override { + void onDiagnosticsReady(PathRef File, + Tagged<std::vector<Diag>> Diagnostics) override { bool HadError = diagsContainErrors(Diagnostics.Value); std::lock_guard<std::mutex> Lock(Mutex); @@ -612,9 +608,8 @@ public: TestDiagConsumer() : Stats(FilesCount, FileStat()) {} - void onDiagnosticsReady( - PathRef File, - Tagged<std::vector<DiagWithFixIts>> Diagnostics) override { + void onDiagnosticsReady(PathRef File, + Tagged<std::vector<Diag>> Diagnostics) override { StringRef FileIndexStr = llvm::sys::path::stem(File); ASSERT_TRUE(FileIndexStr.consume_front("Foo")); @@ -881,8 +876,7 @@ NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse) : StartSecondReparse(std::move(StartSecondReparse)) {} - void onDiagnosticsReady(PathRef, - Tagged<std::vector<DiagWithFixIts>>) override { + void onDiagnosticsReady(PathRef, Tagged<std::vector<Diag>>) override { ++Count; std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t()); ASSERT_TRUE(Lock.owns_lock()) @@ -945,8 +939,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 +961,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 +994,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 @@ -18,40 +18,12 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "severity": 2 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 34, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 34, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 # CHECK-NEXT: } # CHECK-NEXT: ], # 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"}]}}} # CHECK: "id": 2, # CHECK-NEXT: "jsonrpc": "2.0", # CHECK-NEXT: "result": [ @@ -91,7 +63,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 fix: place parentheses around the assignment to silence this warning" # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "arguments": [ @@ -116,11 +88,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 fix: 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"}]}}} # Make sure unused "code" and "source" fields ignored gracefully # CHECK: "id": 3, # CHECK-NEXT: "jsonrpc": "2.0", @@ -161,7 +133,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 fix: place parentheses around the assignment to silence this warning" # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "arguments": [ @@ -186,7 +158,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 fix: 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 @@ -18,20 +18,6 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "severity": 2 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 19, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 18, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 # CHECK-NEXT: } # CHECK-NEXT: ], # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" @@ -54,20 +40,6 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "severity": 2 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 19, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 18, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 # CHECK-NEXT: } # CHECK-NEXT: ], # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" Index: test/clangd/execute-command.test =================================================================== --- test/clangd/execute-command.test +++ test/clangd/execute-command.test @@ -18,34 +18,6 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "severity": 2 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 34, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 34, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 # CHECK-NEXT: } # CHECK-NEXT: ], # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" Index: test/clangd/diagnostics.test =================================================================== --- test/clangd/diagnostics.test +++ test/clangd/diagnostics.test @@ -18,20 +18,6 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "severity": 2 -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "message": "change return type to 'int'", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 4, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 0, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 3 # CHECK-NEXT: } # CHECK-NEXT: ], # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" 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<Diag>)> 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<Diag>)> 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(std::vector<Diag>)> 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(std::vector<Diag>)> 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/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -163,6 +163,12 @@ /// empty string. std::string newText; }; +inline bool operator==(const TextEdit &LHS, const TextEdit &RHS) { + return std::tie(LHS.range, LHS.newText) == std::tie(RHS.range, RHS.newText); +} +inline bool operator!=(const TextEdit &LHS, const TextEdit &RHS) { + return !(LHS == RHS); +} bool fromJSON(const json::Expr &, TextEdit &); json::Expr toJSON(const TextEdit &); llvm::raw_ostream &operator<<(llvm::raw_ostream &, const TextEdit &); @@ -410,13 +416,21 @@ /// The diagnostic's message. std::string message; }; +inline bool operator==(const Diagnostic &LHS, const Diagnostic &RHS) { + return std::tie(LHS.range, LHS.severity, LHS.message) == + std::tie(RHS.range, RHS.severity, RHS.message); +} +inline bool operator!=(const Diagnostic &LHS, const Diagnostic &RHS) { + return !(LHS == RHS); +} + /// A LSP-specific comparator used to find diagnostic in a container like /// std:map. /// We only use the required fields of Diagnostic to do the comparsion to avoid /// any regression issues from LSP clients (e.g. VScode), see /// https://git.io/vbr29 struct LSPDiagnosticCompare { - bool operator()(const Diagnostic& LHS, const Diagnostic& RHS) const { + bool operator()(const Diagnostic &LHS, const Diagnostic &RHS) const { return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message); } }; Index: clangd/Diagnostics.h =================================================================== --- /dev/null +++ clangd/Diagnostics.h @@ -0,0 +1,110 @@ +//===--- 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 { + +/// Contains basic information about a diagnostic. +struct DiagBase { + 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 = DiagnosticsEngine::Note; + // Since File is only descriptive, we store a separate flag to distinguish + // diags from the main file. + bool InsideMainFile = false; +}; +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D); + +/// Represents a single fix-it that editor can apply to fix the error. +struct Fix { + /// Message for the fix-it. + std::string Message; + /// TextEdits from clang's fix-its. Must be non-empty. + llvm::SmallVector<TextEdit, 1> Edits; +}; +inline bool operator==(const clangd::Fix &LHS, const clangd::Fix &RHS) { + return LHS.Message == RHS.Message && + llvm::makeArrayRef(LHS.Edits).equals(RHS.Edits); +} +inline bool operator!=(const clangd::Fix &LHS, const clangd::Fix &RHS) { + return !(LHS == RHS); +} +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F); + +/// Represents a note for the diagnostic. Severity of notes can only be 'note' +/// or 'remark'. +struct Note : DiagBase {}; + +/// A top-level diagnostic that may have Notes and Fixes. +struct Diag : DiagBase { + /// Elaborate on the problem, usually pointing to a related piece of code. + std::vector<Note> Notes; + /// *Alternative* fixes for this diagnostic, one should be chosen. + std::vector<Fix> Fixes; +}; +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D); + +/// 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 Diag &D, + llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn); + +/// Convert from clang diagnostic level to LSP severity. +int getSeverity(DiagnosticsEngine::Level L); + +/// StoreDiags collects the diagnostics that can later be reported by +/// clangd. It groups all notes for a diagnostic into a single Diag +/// 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 StoreDiags : public DiagnosticConsumer { +public: + std::vector<Diag> take(); + + 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<Diag> Output; + llvm::Optional<LangOptions> LangOpts; + llvm::Optional<Diag> 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,364 @@ +//===--- 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 Diag &D) { + if (D.InsideMainFile) + return true; + // Fixes are always in the main file. + if (!D.Fixes.empty()) + return true; + for (auto &N : D.Notes) { + if (N.InsideMainFile) + return true; + } + return false; +} + +// 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 SourceLocation Loc, const SourceManager &M) { + return Loc.isValid() && M.isInMainFile(Loc); +} + +bool isInsideMainFile(const clang::Diagnostic &D) { + if (!D.hasSourceManager()) + return false; + + return isInsideMainFile(D.getLocation(), D.getSourceManager()); +} + +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 DiagBase &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 Diag &D) { + std::string Result; + llvm::raw_string_ostream OS(Result); + OS << D.Message; + for (auto &Note : D.Notes) { + 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 DiagBase &Note) { + std::string Result; + llvm::raw_string_ostream OS(Result); + OS << Note.Message; + OS << "\n\n"; + printDiag(OS, Main); + OS.flush(); + return Result; +} +} // namespace + +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D) { + if (!D.InsideMainFile) + OS << "[in " << D.File << "] "; + return OS << D.Message; +} + +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F) { + OS << F.Message << " {"; + const char *Sep = ""; + for (const auto &Edit : F.Edits) { + OS << Sep << Edit; + Sep = ", "; + } + return OS << "}"; +} + +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D) { + OS << static_cast<const DiagBase &>(D); + if (!D.Notes.empty()) { + OS << ", notes: {"; + const char *Sep = ""; + for (auto &Note : D.Notes) { + OS << Sep << Note; + Sep = ", "; + } + OS << "}"; + } + if (!D.Fixes.empty()) { + OS << ", fixes: {"; + const char *Sep = ""; + for (auto &Fix : D.Fixes) { + OS << Sep << Fix; + Sep = ", "; + } + } + return OS; +} + +void toLSPDiags( + const Diag &D, + llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) { + auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic { + clangd::Diagnostic Res; + Res.range = D.Range; + Res.severity = getSeverity(D.Severity); + return Res; + }; + + { + clangd::Diagnostic Main = FillBasicFields(D); + Main.message = mainMessage(D); + OutFn(std::move(Main), D.Fixes); + } + + for (auto &Note : D.Notes) { + if (!Note.InsideMainFile) + continue; + clangd::Diagnostic Res = FillBasicFields(Note); + Res.message = noteMessage(D, Note); + OutFn(std::move(Res), llvm::ArrayRef<Fix>()); + } +} + +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!"); +} + +std::vector<Diag> StoreDiags::take() { return std::move(Output); } + +void StoreDiags::BeginSourceFile(const LangOptions &Opts, + const Preprocessor *) { + LangOpts = Opts; +} + +void StoreDiags::EndSourceFile() { + flushLastDiag(); + LangOpts = llvm::None; +} + +void StoreDiags::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 FillDiagBase = [&](DiagBase &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; + return D; + }; + + auto AddFix = [&]() -> bool { + assert(!Info.getFixItHints().empty() && + "diagnostic does not have attached fix-its"); + if (!InsideMainFile) + return false; + + llvm::SmallVector<TextEdit, 1> Edits; + for (auto &FixIt : Info.getFixItHints()) { + if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), + Info.getSourceManager())) + return false; + Edits.push_back(toTextEdit(FixIt, Info.getSourceManager(), *LangOpts)); + } + + llvm::SmallString<64> Message; + Info.FormatDiagnostic(Message); + LastDiag->Fixes.push_back(Fix{Message.str(), std::move(Edits)}); + return true; + }; + + if (!isNote(DiagLevel)) { + // Handle the new main diagnostic. + flushLastDiag(); + + LastDiag = Diag(); + FillDiagBase(*LastDiag); + + if (!Info.getFixItHints().empty()) + AddFix(); + } else { + // Handle a note to an existing diagnostic. + if (!LastDiag) { + assert(false && "Adding a note without main diagnostic"); + IgnoreDiagnostics::log(DiagLevel, Info); + return; + } + + if (!Info.getFixItHints().empty()) { + // A clang note with fix-it is not a separate diagnostic in clangd. We + // attach it as a Fix to the main diagnostic instead. + if (!AddFix()) + IgnoreDiagnostics::log(DiagLevel, Info); + } else { + // A clang note without fix-its corresponds to clangd::Note. + Note N; + FillDiagBase(N); + + LastDiag->Notes.push_back(std::move(N)); + } + } +} + +void StoreDiags::flushLastDiag() { + if (!LastDiag) + return; + if (mentionsMainFile(*LastDiag)) + Output.push_back(std::move(*LastDiag)); + else + log(Twine("Dropped diagnostic outside main file:") + LastDiag->File + ":" + + LastDiag->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,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); + std::vector<Diag> Diags, InclusionLocations IncLocations); PrecompiledPreamble Preamble; std::vector<serialization::DeclID> TopLevelDeclIDs; - std::vector<DiagWithFixIts> Diags; + std::vector<Diag> Diags; InclusionLocations IncLocations; }; @@ -96,7 +90,7 @@ /// this call might be expensive. ArrayRef<const Decl *> getTopLevelDecls(); - const std::vector<DiagWithFixIts> &getDiagnostics() const; + const std::vector<Diag> &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, std::vector<Diag> Diags, + InclusionLocations IncLocations); private: void ensurePreambleDeclsDeserialized(); @@ -125,7 +119,7 @@ std::unique_ptr<FrontendAction> Action; // Data, stored after parsing. - std::vector<DiagWithFixIts> Diags; + std::vector<Diag> 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<std::vector<Diag>> 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,14 +168,13 @@ std::unique_ptr<llvm::MemoryBuffer> Buffer, std::shared_ptr<PCHContainerOperations> PCHs, IntrusiveRefCntPtr<vfs::FileSystem> VFS) { - std::vector<DiagWithFixIts> ASTDiags; - StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags); - const PrecompiledPreamble *PreamblePCH = Preamble ? &Preamble->Preamble : nullptr; - auto Clang = prepareCompilerInstance( - std::move(CI), PreamblePCH, std::move(Buffer), std::move(PCHs), - std::move(VFS), /*ref*/ UnitDiagsConsumer); + + StoreDiags ASTDiags; + auto Clang = + prepareCompilerInstance(std::move(CI), PreamblePCH, std::move(Buffer), + std::move(PCHs), std::move(VFS), ASTDiags); if (!Clang) return llvm::None; @@ -328,10 +206,12 @@ // 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. + ASTDiags.EndSourceFile(); std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls(); return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action), - std::move(ParsedDecls), std::move(ASTDiags), + std::move(ParsedDecls), ASTDiags.take(), std::move(IncLocations)); } @@ -399,14 +279,12 @@ return TopLevelDecls; } -const std::vector<DiagWithFixIts> &ParsedAST::getDiagnostics() const { - return Diags; -} +const std::vector<Diag> &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. + // Message and Fixes inside each diagnostic. return AST.getASTAllocatedMemory() + AST.getSideTableAllocatedMemory() + ::getUsedBytes(TopLevelDecls) + ::getUsedBytes(Diags); } @@ -417,7 +295,7 @@ PreambleData::PreambleData(PrecompiledPreamble Preamble, std::vector<serialization::DeclID> TopLevelDeclIDs, - std::vector<DiagWithFixIts> Diags, + std::vector<Diag> Diags, InclusionLocations IncLocations) : Preamble(std::move(Preamble)), TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)), @@ -427,8 +305,7 @@ std::unique_ptr<CompilerInstance> Clang, std::unique_ptr<FrontendAction> Action, std::vector<const Decl *> TopLevelDecls, - std::vector<DiagWithFixIts> Diags, - InclusionLocations IncLocations) + std::vector<Diag> Diags, InclusionLocations IncLocations) : Preamble(std::move(Preamble)), Clang(std::move(Clang)), Action(std::move(Action)), Diags(std::move(Diags)), TopLevelDecls(std::move(TopLevelDecls)), PreambleDeclsDeserialized(false), @@ -445,8 +322,7 @@ log("Created CppFile for " + FileName); } -llvm::Optional<std::vector<DiagWithFixIts>> -CppFile::rebuild(ParseInputs &&Inputs) { +llvm::Optional<std::vector<Diag>> CppFile::rebuild(ParseInputs &&Inputs) { log("Rebuilding file " + FileName + " with command [" + Inputs.CompileCommand.Directory + "] " + llvm::join(Inputs.CompileCommand.CommandLine, " ")); @@ -500,12 +376,11 @@ std::move(ContentsBuffer), PCHs, Inputs.FS); } - std::vector<DiagWithFixIts> Diagnostics; + std::vector<Diag> 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,11 +432,10 @@ trace::Span Tracer("Preamble"); SPAN_ATTACH(Tracer, "File", FileName); - std::vector<DiagWithFixIts> PreambleDiags; - StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags); + StoreDiags PreambleDiagnostics; IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine = CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(), - &PreambleDiagnosticsConsumer, false); + &PreambleDiagnostics, false); // Skip function bodies when building the preamble to speed up building // the preamble and make it smaller. @@ -584,7 +458,7 @@ return std::make_shared<PreambleData>( std::move(*BuiltPreamble), SerializedDeclsCollector.takeTopLevelDeclIDs(), - std::move(PreambleDiags), + PreambleDiagnostics.take(), SerializedDeclsCollector.takeInclusionLocations()); } else { log("Could not build a preamble for file " + Twine(FileName)); 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<std::vector<Diag>> Diagnostics) = 0; }; class FileSystemProvider { Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -541,8 +541,7 @@ Path FileStr = File.str(); VFSTag Tag = std::move(TaggedFS.Tag); - auto Callback = [this, Version, FileStr, - Tag](std::vector<DiagWithFixIts> Diags) { + auto Callback = [this, Version, FileStr, Tag](std::vector<Diag> 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,8 @@ private: // Implement DiagnosticsConsumer. - virtual void - onDiagnosticsReady(PathRef File, - Tagged<std::vector<DiagWithFixIts>> Diagnostics) override; + void onDiagnosticsReady(PathRef File, + Tagged<std::vector<Diag>> Diagnostics) override; // Implement ProtocolCallbacks. void onInitialize(InitializeParams &Params) override; @@ -74,7 +73,7 @@ void onHover(TextDocumentPositionParams &Params) override; void onChangeConfiguration(DidChangeConfigurationParams &Params) override; - std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D); + std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D); JSONOutput &Out; /// Used to indicate that the 'shutdown' request was received from the @@ -87,8 +86,7 @@ bool IsDone = false; std::mutex FixItsMutex; - typedef std::map<clangd::Diagnostic, std::vector<TextEdit>, - LSPDiagnosticCompare> + typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare> DiagnosticToReplacementMap; /// Caches FixIts per file and diagnostics llvm::StringMap<DiagnosticToReplacementMap> FixItsMap; 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" @@ -313,12 +314,12 @@ json::ary Commands; for (Diagnostic &D : Params.context.diagnostics) { - auto Edits = getFixIts(Params.textDocument.uri.file(), D); - if (!Edits.empty()) { + for (auto &F : getFixes(Params.textDocument.uri.file(), D)) { WorkspaceEdit WE; + std::vector<TextEdit> Edits(F.Edits.begin(), F.Edits.end()); 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 fix: {0}", F.Message)}, {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}, {"arguments", {WE}}, }); @@ -421,8 +422,8 @@ return ShutdownRequestReceived; } -std::vector<TextEdit> ClangdLSPServer::getFixIts(StringRef File, - const clangd::Diagnostic &D) { +std::vector<Fix> ClangdLSPServer::getFixes(StringRef File, + const clangd::Diagnostic &D) { std::lock_guard<std::mutex> Lock(FixItsMutex); auto DiagToFixItsIter = FixItsMap.find(File); if (DiagToFixItsIter == FixItsMap.end()) @@ -437,21 +438,22 @@ } void ClangdLSPServer::onDiagnosticsReady( - PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) { + PathRef File, Tagged<std::vector<Diag>> 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, [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) { + DiagnosticsJSON.push_back(json::obj{ + {"range", Diag.range}, + {"severity", Diag.severity}, + {"message", Diag.message}, + }); + + auto &FixItsForDiagnostic = LocalFixIts[Diag]; + std::copy(Fixes.begin(), Fixes.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 @@ -12,6 +12,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