Index: clang-tools-extra/trunk/clangd/Diagnostics.cpp =================================================================== --- clang-tools-extra/trunk/clangd/Diagnostics.cpp +++ clang-tools-extra/trunk/clangd/Diagnostics.cpp @@ -424,6 +424,13 @@ } } } + // 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> SeenDiags; + llvm::erase_if(Output, [&](const Diag& D) { + return !SeenDiags.emplace(D.Range, D.Message).second; + }); return std::move(Output); } Index: clang-tools-extra/trunk/clangd/Protocol.h =================================================================== --- clang-tools-extra/trunk/clangd/Protocol.h +++ clang-tools-extra/trunk/clangd/Protocol.h @@ -668,17 +668,11 @@ /// 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); } }; Index: clang-tools-extra/trunk/clangd/test/fixits-duplication.test =================================================================== --- clang-tools-extra/trunk/clangd/test/fixits-duplication.test +++ clang-tools-extra/trunk/clangd/test/fixits-duplication.test @@ -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"} - Index: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp =================================================================== --- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp +++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp @@ -179,6 +179,44 @@ 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 + 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"]]