[clangd] Deduplicate inlay hints

[nridge]

Summary

Duplicates can sometimes appear due to e.g. explicit template
instantiations

[nridge]

I will note that I considered handling this in a way specific to explicit template instantiations.

However, while I do not have a reduced testcase for it, I've also run into duplicate hints in the context of macros.

So, it seemed easier to deduplicate in general; there is no justification for showing the same hint multiple times in the same location.

[kadircet]

I am not aware of how the intersecting hints are handled in the implementation on the client side nor in the proposal today. After this patch we might still produce them if for whatever reason there are different kinds of hints for the same range. Is this OK?

clang-tools-extra/clangd/InlayHints.cpp

372

nit: You can just do llvm::sort(Results)

[nridge]

In D110051#3009378, @kadircet wrote:

I am not aware of how the intersecting hints are handled in the implementation on the client side nor in the proposal today. After this patch we might still produce them if for whatever reason there are different kinds of hints for the same range. Is this OK?

The behaviour with the duplicated hints in vscode was that they appeared one after the other, e.g. : int : int : int. So, wrong but not catastrophic.

I don't think we're likely to get a parameter hint and a type hint at the same location, but if you're concerned, I could revise the patch to ignore the hint kind during the comparison.

[nridge]

address nit

[kadircet]

LGTM, thanks!

I don't think we're likely to get a parameter hint and a type hint at the same location, but if you're concerned, I could revise the patch to ignore the hint kind during the comparison.

That's hard call to make, there are probably good arguments for:

• keeping them all
• keeping only one
• dropping them all.

I was just worried about breaking clients. We should probably account for this in the spec, as by probably forbidding any intersecting hints completely for a better UX (as discussed with @sammccall offline).

clang-tools-extra/clangd/InlayHints.cpp

private:
 
   static const size_t TypeNameLimit = 32;
 };
 
 std::vector<InlayHint> inlayHints(ParsedAST &AST) {
   std::vector<InlayHint> Results;
   InlayHintVisitor Visitor(Results, AST);
   Visitor.TraverseAST(AST.getASTContext());
+
+  // De-duplicate hints. Duplicates can sometimes occur due to e.g. explicit
+  // template instantiations.
+  llvm::sort(Results);
+  Results.erase(std::unique(Results.begin(), Results.end()), Results.end());
+
   return Results;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Protocol.h

struct InlayHint {
 
   /// The type of hint.
   InlayHintKind kind;
 
   /// The label that is displayed in the editor.
   std::string label;
 };
 llvm::json::Value toJSON(const InlayHint &);
+bool operator==(const InlayHint &, const InlayHint &);
+bool operator<(const InlayHint &, const InlayHint &);
 
 struct ReferenceContext {
   /// Include the declaration of the current symbol.
   bool includeDeclaration = false;
 };
 
 struct ReferenceParams : public TextDocumentPositionParams {
   ReferenceContext context;

clang-tools-extra/clangd/Protocol.cpp

llvm::json::Value toJSON(InlayHintKind K) {
   }
   llvm_unreachable("Unknown clang.clangd.InlayHintKind");
 }
 
 llvm::json::Value toJSON(const InlayHint &H) {
   return llvm::json::Object{
       {"range", H.range}, {"kind", H.kind}, {"label", H.label}};
 }
+bool operator==(const InlayHint &A, const InlayHint &B) {
+  return std::tie(A.kind, A.range, A.label) ==
+         std::tie(B.kind, B.range, B.label);
+}
+bool operator<(const InlayHint &A, const InlayHint &B) {
+  return std::tie(A.kind, A.range, A.label) <
+         std::tie(B.kind, B.range, B.label);
+}
 
 static const char *toString(OffsetEncoding OE) {
   switch (OE) {
   case OffsetEncoding::UTF8:
     return "utf-8";
   case OffsetEncoding::UTF16:
     return "utf-16";
   case OffsetEncoding::UTF32:

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

assertTypeHints(R"cpp(
     template <typename, typename = int>
     struct A {};
     A<float> foo();
     auto $var[[var]] = foo();
   )cpp",
                   ExpectedHint{": A<float>", "var"});
 }
 
+TEST(TypeHints, Deduplication) {
+  assertTypeHints(R"cpp(
+    template <typename T>
+    void foo() {
+      auto $var[[var]] = 42;
+    }
+    template void foo<int>();
+    template void foo<float>();
+  )cpp",
+                  ExpectedHint{": int", "var"});
+}
+
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
 //  - auto x = static_cast<TypeName>(...);  // and other built-in casts
 
 // Annoyances for which a heuristic is not obvious:
 //  - auto x = llvm::dyn_cast<LongTypeName>(y);  // and similar
 //  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
 //    (For this one, perhaps we should omit type hints that start
 //     with a double underscore.)
 
 } // namespace
 } // namespace clangd
-} // namespace clang
+} // namespace clang