[clangd] Deduplicate clang-tidy diagnostic messages.

[hokein]

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 as well.

[kadircet]

Could you also revert changes in LSPDiagnosticCompare in Protocol.h, that were introduced by rL363889? (or bring back fixits-duplication.test)

[hokein]

Revert the LSPDiagnosticCompare change in rL363889.

[hokein]

In D64127#1568093, @kadircet wrote:

Could you also revert changes in LSPDiagnosticCompare in Protocol.h, that were introduced by rL363889? (or bring back fixits-duplication.test)

Done, it looks like we don't need this change as we don't store all diagnostics of clang-tidy alias checks.

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

427 ↗

(On Diff #207760)

This sorting/partitioning seems a bit heavyweight... what about

DenseSet<pair<Range, Message>> SeenDiagnostics;
llvm::erase_if([&](const Diag &D) {
  return !SeenDiagnostics.try_emplace(D.Range, D.Message).second;
});

[hokein]

Address comment.

clang-tools-extra/clangd/Diagnostics.cpp

427 ↗

(On Diff #207760)

This is neat. DenseSet requires a hashvalue function of the value, which we don't have, switched to use set

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

427 ↗

(On Diff #207760)

Can we add one instead? It'd be nice to get rid of the operator< on range, use of set etc.
I can do this is a followup if you prefer.

[hokein]

clang-tools-extra/clangd/Diagnostics.cpp

427 ↗

(On Diff #207760)

I'd leave it to a follow-up.

clang-tools-extra/trunk/clangd/Diagnostics.cpp

if (Tidy != nullptr) {
         for (auto &Note : Diag.Notes)
           CleanMessage(Note.Message);
         for (auto &Fix : Diag.Fixes)
           CleanMessage(Fix.Message);
         continue;
       }
     }
   }
+  // 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);
 }
 
 void StoreDiags::BeginSourceFile(const LangOptions &Opts,
                                  const Preprocessor *) {
   LangOpts = Opts;
 }
 

clang-tools-extra/trunk/clangd/Protocol.h

struct Diagnostic {
   /// Only with capability textDocument.publishDiagnostics.codeActionsInline.
   /// (These actions can also be obtained using textDocument/codeAction).
   llvm::Optional<std::vector<CodeAction>> codeActions;
 };
 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);
   }
 };
 bool fromJSON(const llvm::json::Value &, Diagnostic &);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Diagnostic &);
 
 struct CodeActionContext {
   /// An array of diagnostics.

clang-tools-extra/trunk/clangd/test/fixits-duplication.test

-# 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"}
-

clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

TEST(DiagnosticsTest, DiagnosticPreamble) {
 
   auto TU = TestTU::withCode(Test.code());
   EXPECT_THAT(TU.build().getDiagnostics(),
               ElementsAre(::testing::AllOf(
                   Diag(Test.range(), "'not-found.h' file not found"),
                   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"]]
 
     #define $macrodef[[SQUARE]](X) (X)*(X)
     int $main[[main]]() {
       int y = 4;
       return SQUARE($macroarg[[++]]y);