[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

llvm-svn: 365204

4 files changed, 48 insertions, 230 deletions

diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 0bc3bae3d96..88bd6fdcad1 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -424,6 +424,13 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
       }
     }
   }
+  // 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);
 }
 
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 6f9b7a9470d..af8b903dea6 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -668,17 +668,11 @@ llvm::json::Value toJSON(const Diagnostic &);
 
 /// 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);
   }
 };
diff --git a/clang-tools-extra/clangd/test/fixits-duplication.test b/clang-tools-extra/clangd/test/fixits-duplication.test
deleted file mode 100644
index cef2b49ea7d..00000000000
--- a/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"}
-
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 939cd5e8e5e..71aa148d6b6 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -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"]]