[clangd] Tweak "provides" hover card when symbols have the same name

[sammccall]

Summary

Previously for overloaded functions we'd show:

Provides: foo, bar bar bar bar

The symbol name is duplicated

==> only show unique names, since we're not displaying the signature

Commas are missing

==> fix the logic which was checking for "last element" by value
    (though after the above fix this bug is dead anyway)

While here, remove a redundant bounds check before take_front().

[VitaNuo]

clang-tools-extra/clangd/Hover.cpp

1539

What will this do if Front has an even number of elements? AFAIU the message will end in a comma then, which is not desirable.

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

3492

Why did you introduce this change? This is just a rendering test, and you seem to be feeding with input that should not be possible anymore (after this patch).

[sammccall]

clang-tools-extra/clangd/Hover.cpp

1539

llvm::interleave invokes the first callback for each element, and the second callback between subsequent elements. So this prints Front as a comma-separated list.
(It's very similar to the example in the llvm::interleave docs)

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

3492

The rendering logic was incorrect: if (Sym != Front.back()) was intending to test whether this was the last element, but instead was testing if it had the same text as the last element.

After fixing that bug I realized we probably didn't ever want to include overloads separately anyway, independent of any rendering issues.

But the rendering logic should still be fixed! (render() doesn't know there are no duplicates!)
And when fixing it I think it makes sense to test it - but I don't feel strongly, I can revert the test if you prefer.

[sammccall]

clang-tools-extra/clangd/Hover.cpp

1539

Oh, the docs are confusing.

Given a list [a b c d] the relevant pairs are [a b];[b c];[c d]. Each one gets a comma in the middle, there are three commas.

[VitaNuo]

Thank you!

clang-tools-extra/clangd/Hover.cpp

1539

Oh ok, I thought it was more basic, ie just applied the callbacks interchangeably. Thanks for the explanation!

clang-tools-extra/clangd/Hover.cpp

include_cleaner::walkUsed(
           return;
 
         UsedSymbols.insert(Ref.Target);
       });
 
   for (const auto &UsedSymbolDecl : UsedSymbols)
     HI.UsedSymbolNames.push_back(getSymbolName(UsedSymbolDecl));
   llvm::sort(HI.UsedSymbolNames);
+  HI.UsedSymbolNames.erase(
+      std::unique(HI.UsedSymbolNames.begin(), HI.UsedSymbolNames.end()),
+      HI.UsedSymbolNames.end());
 }
 
 } // namespace
 
 std::optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                   const format::FormatStyle &Style,
                                   const SymbolIndex *Index) {
   static constexpr trace::Metric HoverCountMetric(
markup::Document HoverInfo::present() const {
   }
 
   if (!UsedSymbolNames.empty()) {
     Output.addRuler();
     markup::Paragraph &P = Output.addParagraph();
     P.appendText("provides ");
 
     const std::vector<std::string>::size_type SymbolNamesLimit = 5;
-    auto Front =
-        llvm::ArrayRef(UsedSymbolNames)
-            .take_front(std::min(UsedSymbolNames.size(), SymbolNamesLimit));
-
-    for (const auto &Sym : Front) {
-      P.appendCode(Sym);
-      if (Sym != Front.back())
-        P.appendText(", ");
-    }
+    auto Front = llvm::ArrayRef(UsedSymbolNames).take_front(SymbolNamesLimit);
 
+    llvm::interleave(
+        Front, [&](llvm::StringRef Sym) { P.appendCode(Sym); },
+        [&] { P.appendText(", "); });
     if (UsedSymbolNames.size() > Front.size()) {
       P.appendText(" and ");
       P.appendText(std::to_string(UsedSymbolNames.size() - Front.size()));
       P.appendText(" more");
     }
   }
 
   return Output;

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

 
 TEST(Hover, UsedSymbols) {
   struct {
     const char *Code;
     const std::function<void(HoverInfo &)> ExpectedBuilder;
   } Cases[] = {{R"cpp(
                   #include ^"bar.h"
                   int fstBar = bar1();
+                  int another= bar1(0);
                   int sndBar = bar2();
                   Bar bar;
                   int macroBar = BAR;
                 )cpp",
                 [](HoverInfo &HI) {
                   HI.UsedSymbolNames = {"BAR", "Bar", "bar1", "bar2"};
                 }},
                {R"cpp(
                   #in^clude <vector>
                   std::vector<int> vec;
                 )cpp",
                 [](HoverInfo &HI) { HI.UsedSymbolNames = {"vector"}; }}};
   for (const auto &Case : Cases) {
     Annotations Code{Case.Code};
     SCOPED_TRACE(Code.code());
 
     TestTU TU;
     TU.Filename = "foo.cpp";
     TU.Code = Code.code();
     TU.AdditionalFiles["bar.h"] = guard(R"cpp(
                                           #define BAR 5
                                           int bar1();
                                           int bar2();
+                                          int bar1(double);
                                           class Bar {};
                                         )cpp");
     TU.AdditionalFiles["system/vector"] = guard(R"cpp(
       namespace std {
         template<typename>
         class vector{};
       }
     )cpp");
int foo = 3)",
             HI.Definition = "/usr/include/stdio.h";
           },
           R"(stdio.h
 
 /usr/include/stdio.h)",
       },
       {[](HoverInfo &HI) {
          HI.Name = "foo.h";
-         HI.UsedSymbolNames = {"Foo", "Bar", "Baz"};
+         HI.UsedSymbolNames = {"Foo", "Bar", "Bar"};
        },
        R"(foo.h
 
-provides Foo, Bar, Baz)"},
+provides Foo, Bar, Bar)"},
       {[](HoverInfo &HI) {
          HI.Name = "foo.h";
          HI.UsedSymbolNames = {"Foo", "Bar", "Baz", "Foobar", "Qux", "Quux"};
        },
        R"(foo.h
 
 provides Foo, Bar, Baz, Foobar, Qux and 1 more)"}};