hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, mgrang, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
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 as well.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64127
Files:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/test/fixits-duplication.test
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -179,6 +179,43 @@
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");
+ // 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"]]
Index: clang-tools-extra/clangd/test/fixits-duplication.test
===================================================================
--- clang-tools-extra/clangd/test/fixits-duplication.test
+++ /dev/null
@@ -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/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -424,6 +424,23 @@
}
}
}
+ // 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).
+ auto ClangTidyDiagsBegin =
+ std::partition(Output.begin(), Output.end(),
+ [](const Diag &D) { return D.Source != Diag::ClangTidy; });
+ auto ClangTidyDiagsEnd = Output.end();
+ llvm::sort(ClangTidyDiagsBegin, ClangTidyDiagsEnd, [](const Diag &LHS,
+ const Diag &RHS) {
+ return std::tie(LHS.Range, LHS.Message) < std::tie(RHS.Range, RHS.Message);
+ });
+ auto Last = std::unique(ClangTidyDiagsBegin, ClangTidyDiagsEnd,
+ [](const Diag &LHS, const Diag &RHS) {
+ return std::tie(LHS.Range, LHS.Message) ==
+ std::tie(RHS.Range, RHS.Message);
+ });
+ Output.erase(Last, ClangTidyDiagsEnd);
return std::move(Output);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
