[clang-tools-extra] r365204 - [clangd] Deduplicate clang-tidy diagnostic messages.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 5 05:57:56 PDT 2019
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"]]
More information about the cfe-commits
mailing list