Author: hokein Date: Fri Jul 5 05:57:56 2019 New Revision: 365204 URL: http://llvm.org/viewvc/llvm-project?rev=365204&view=rev Log: [clangd] Deduplicate clang-tidy diagnostic messages.
Summary: Clang-tidy checks may emit duplicated messages (clang-tidy tool deduplicate them in its custom diagnostic consumer), and we may show multiple duplicated diagnostics in the UI, which is really bad. This patch makes clangd do the deduplication, and revert the change rL363889. Reviewers: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, mgrang, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D64127 Removed: clang-tools-extra/trunk/clangd/test/fixits-duplication.test Modified: clang-tools-extra/trunk/clangd/Diagnostics.cpp clang-tools-extra/trunk/clangd/Protocol.h clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp Modified: clang-tools-extra/trunk/clangd/Diagnostics.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Diagnostics.cpp?rev=365204&r1=365203&r2=365204&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/Diagnostics.cpp (original) +++ clang-tools-extra/trunk/clangd/Diagnostics.cpp Fri Jul 5 05:57:56 2019 @@ -424,6 +424,13 @@ std::vector<Diag> StoreDiags::take(const } } } + // Deduplicate clang-tidy diagnostics -- some clang-tidy checks may emit + // duplicated messages due to various reasons (e.g. the check doesn't handle + // template instantiations well; clang-tidy alias checks). + std::set<std::pair<Range, std::string>> SeenDiags; + llvm::erase_if(Output, [&](const Diag& D) { + return !SeenDiags.emplace(D.Range, D.Message).second; + }); return std::move(Output); } Modified: clang-tools-extra/trunk/clangd/Protocol.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.h?rev=365204&r1=365203&r2=365204&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/Protocol.h (original) +++ clang-tools-extra/trunk/clangd/Protocol.h Fri Jul 5 05:57:56 2019 @@ -668,17 +668,11 @@ llvm::json::Value toJSON(const Diagnosti /// A LSP-specific comparator used to find diagnostic in a container like /// std:map. -/// We only use as many fields of Diagnostic as is needed to make distinct -/// diagnostics unique in practice, to avoid regression issues from LSP clients -/// (e.g. VScode), see https://git.io/vbr29 +/// 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 { - if (!LHS.code.empty() && !RHS.code.empty()) { - // If the code is known for both, use the code to diambiguate, as e.g. - // two checkers could produce diagnostics with the same range and message. - return std::tie(LHS.range, LHS.message, LHS.code) < - std::tie(RHS.range, RHS.message, RHS.code); - } return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message); } }; Removed: clang-tools-extra/trunk/clangd/test/fixits-duplication.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/test/fixits-duplication.test?rev=365203&view=auto ============================================================================== --- clang-tools-extra/trunk/clangd/test/fixits-duplication.test (original) +++ clang-tools-extra/trunk/clangd/test/fixits-duplication.test (removed) @@ -1,221 +0,0 @@ -# RUN: clangd -lit-test -clang-tidy-checks=modernize-use-nullptr,hicpp-use-nullptr -tweaks="" < %s | FileCheck -strict-whitespace %s -{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}} ---- -{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"void foo() { char* p = 0; }"}}} -# CHECK: "method": "textDocument/publishDiagnostics", -# CHECK-NEXT: "params": { -# CHECK-NEXT: "diagnostics": [ -# CHECK-NEXT: { -# CHECK-NEXT: "code": "hicpp-use-nullptr", -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "code": "modernize-use-nullptr", -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: } -# CHECK-NEXT: ], -# CHECK-NEXT: "uri": "file:///{{.*}}/foo.cpp" -# CHECK-NEXT: } ---- -{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "hicpp-use-nullptr", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "modernize-use-nullptr", "source": "clang-tidy"}]}}} -# CHECK: "id": 2, -# CHECK-NEXT: "jsonrpc": "2.0", -# CHECK-NEXT: "result": [ -# CHECK-NEXT: { -# CHECK-NEXT: "diagnostics": [ -# CHECK-NEXT: { -# CHECK-NEXT: "code": "hicpp-use-nullptr", -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: } -# CHECK-NEXT: ], -# CHECK-NEXT: "edit": { -# CHECK-NEXT: "changes": { -# CHECK-NEXT: "file://{{.*}}/foo.cpp": [ -# CHECK-NEXT: { -# CHECK-NEXT: "newText": "nullptr", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: ] -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "kind": "quickfix", -# CHECK-NEXT: "title": "change '0' to 'nullptr'" -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "diagnostics": [ -# CHECK-NEXT: { -# CHECK-NEXT: "code": "modernize-use-nullptr", -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: } -# CHECK-NEXT: ], -# CHECK-NEXT: "edit": { -# CHECK-NEXT: "changes": { -# CHECK-NEXT: "file://{{.*}}/foo.cpp": [ -# CHECK-NEXT: { -# CHECK-NEXT: "newText": "nullptr", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: ] -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "kind": "quickfix", -# CHECK-NEXT: "title": "change '0' to 'nullptr'" -# CHECK-NEXT: } -# CHECK-NEXT: ] ---- -{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"}]}}} -# CHECK: "id": 3, -# CHECK-NEXT: "jsonrpc": "2.0", -# CHECK-NEXT: "result": [ -# CHECK-NEXT: { -# CHECK-NEXT: "diagnostics": [ -# CHECK-NEXT: { -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: } -# CHECK-NEXT: ], -# CHECK-NEXT: "edit": { -# CHECK-NEXT: "changes": { -# CHECK-NEXT: "file://{{.*}}/foo.cpp": [ -# CHECK-NEXT: { -# CHECK-NEXT: "newText": "nullptr", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: ] -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "kind": "quickfix", -# CHECK-NEXT: "title": "change '0' to 'nullptr'" -# CHECK-NEXT: }, -# CHECK-NEXT: { -# CHECK-NEXT: "diagnostics": [ -# CHECK-NEXT: { -# CHECK-NEXT: "message": "Use nullptr (fix available)", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "severity": 2, -# CHECK-NEXT: "source": "clang-tidy" -# CHECK-NEXT: } -# CHECK-NEXT: ], -# CHECK-NEXT: "edit": { -# CHECK-NEXT: "changes": { -# CHECK-NEXT: "file://{{.*}}/foo.cpp": [ -# CHECK-NEXT: { -# CHECK-NEXT: "newText": "nullptr", -# CHECK-NEXT: "range": { -# CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 24, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: }, -# CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 23, -# CHECK-NEXT: "line": 0 -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: } -# CHECK-NEXT: ] -# CHECK-NEXT: } -# CHECK-NEXT: }, -# CHECK-NEXT: "kind": "quickfix", -# CHECK-NEXT: "title": "change '0' to 'nullptr'" -# CHECK-NEXT: } -# CHECK-NEXT: ] ---- -{"jsonrpc":"2.0","id":4,"method":"shutdown"} ---- -{"jsonrpc":"2.0","method":"exit"} - Modified: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=365204&r1=365203&r2=365204&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp (original) +++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp Fri Jul 5 05:57:56 2019 @@ -179,6 +179,44 @@ TEST(DiagnosticsTest, DiagnosticPreamble DiagSource(Diag::Clang), DiagName("pp_file_not_found")))); } +TEST(DiagnosticsTest, DeduplicatedClangTidyDiagnostics) { + Annotations Test(R"cpp( + float foo = [[0.1f]]; + )cpp"); + auto TU = TestTU::withCode(Test.code()); + // Enable alias clang-tidy checks, these check emits the same diagnostics + // (except the check name). + TU.ClangTidyChecks = "-*, readability-uppercase-literal-suffix, " + "hicpp-uppercase-literal-suffix"; + // Verify that we filter out the duplicated diagnostic message. + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre(::testing::AllOf( + Diag(Test.range(), + "floating point literal has suffix 'f', which is not uppercase"), + DiagSource(Diag::ClangTidy)))); + + Test = Annotations(R"cpp( + template<typename T> + void func(T) { + float f = [[0.3f]]; + } + void k() { + func(123); + func(2.0); + } + )cpp"); + TU.Code = Test.code(); + // The check doesn't handle template instantiations which ends up emitting + // duplicated messages, verify that we deduplicate them. + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre(::testing::AllOf( + Diag(Test.range(), + "floating point literal has suffix 'f', which is not uppercase"), + DiagSource(Diag::ClangTidy)))); +} + TEST(DiagnosticsTest, ClangTidy) { Annotations Test(R"cpp( #include $deprecated[["assert.h"]] _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
