[clangd] Bundle code completion items when the include paths differ, but resolve to the same file.

[adamcz]

Summary

This can happen when, for example, merging results from an external
index that generates IncludeHeaders with full URI rather than just
literal include.

[adamcz]

-bracket

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

187

(sorry for the late drive by comment)

if this returns an error it needs to be consumed to ensure we don't crash (destructor of Expected asserts)

but apart from that, this introduces lots of new string manipulations for every completion item, might have bad impact on latency especially when the include paths are long and require heap allocations. something to keep in mind once this hits production (and maybe we can choose to approximate or not do it at all if it turns out to be problematic)

clang-tools-extra/clangd/CodeComplete.cpp

struct CompletionCandidate {
   // We may have a result from Sema, from the index, or both.
   const CodeCompletionResult *SemaResult = nullptr;
   const Symbol *IndexResult = nullptr;
   const RawIdentifier *IdentifierResult = nullptr;
   llvm::SmallVector<llvm::StringRef, 1> RankedIncludeHeaders;
 
   // Returns a token identifying the overload set this is part of.
   // 0 indicates it's not part of any overload set.
-  size_t overloadSet(const CodeCompleteOptions &Opts) const {
+  size_t overloadSet(const CodeCompleteOptions &Opts, llvm::StringRef FileName,
+                     IncludeInserter *Inserter) const {
     if (!Opts.BundleOverloads.getValueOr(false))
       return 0;
+
+    // Depending on the index implementation, we can see different header
+    // strings (literal or URI) mapping to the same file. We still want to
+    // bundle those, so we must resolve the header to be included here.
+    std::string HeaderForHash;
+    if (Inserter)
+      if (auto Header = headerToInsertIfAllowed(Opts))
+        if (auto HeaderFile = toHeaderFile(*Header, FileName))
+          if (auto Spelled =
+                  Inserter->calculateIncludePath(*HeaderFile, FileName))
+            HeaderForHash = *Spelled;
+
     llvm::SmallString<256> Scratch;
     if (IndexResult) {
       switch (IndexResult->SymInfo.Kind) {
       case index::SymbolKind::ClassMethod:
       case index::SymbolKind::InstanceMethod:
       case index::SymbolKind::StaticMethod:
 #ifndef NDEBUG
         llvm_unreachable("Don't expect members from index in code completion");
 #else
         LLVM_FALLTHROUGH;
 #endif
       case index::SymbolKind::Function:
         // We can't group overloads together that need different #includes.
         // This could break #include insertion.
         return llvm::hash_combine(
             (IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
-            headerToInsertIfAllowed(Opts).getValueOr(""));
+            HeaderForHash);
       default:
         return 0;
       }
     }
     if (SemaResult) {
       // We need to make sure we're consistent with the IndexResult case!
       const NamedDecl *D = SemaResult->Declaration;
       if (!D || !D->isFunctionOrFunctionTemplate())
         return 0;
       {
         llvm::raw_svector_ostream OS(Scratch);
         D->printQualifiedName(OS);
       }
-      return llvm::hash_combine(Scratch,
-                                headerToInsertIfAllowed(Opts).getValueOr(""));
+      return llvm::hash_combine(Scratch, HeaderForHash);
     }
     assert(IdentifierResult);
     return 0;
   }
 
   // The best header to include if include insertion is allowed.
   llvm::Optional<llvm::StringRef>
   headerToInsertIfAllowed(const CodeCompleteOptions &Opts) const {
auto AddToBundles = [&](const CodeCompletionResult *SemaResult,
         C.Name = IndexResult->Name;
         C.RankedIncludeHeaders = getRankedIncludes(*C.IndexResult);
       } else if (C.SemaResult) {
         C.Name = Recorder->getName(*SemaResult);
       } else {
         assert(IdentifierResult);
         C.Name = IdentifierResult->Name;
       }
-      if (auto OverloadSet = C.overloadSet(Opts)) {
+      if (auto OverloadSet = C.overloadSet(
+              Opts, FileName, Inserter ? Inserter.getPointer() : nullptr)) {
         auto Ret = BundleLookup.try_emplace(OverloadSet, Bundles.size());
         if (Ret.second)
           Bundles.emplace_back();
         Bundles[Ret.first->second].push_back(std::move(C));
       } else {
         Bundles.emplace_back();
         Bundles.back().push_back(std::move(C));
       }

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

TEST(CompletionTest, OverloadBundling) {
   // For now we just return one of the doc strings arbitrarily.
   ASSERT_TRUE(A.Documentation);
   EXPECT_THAT(
       A.Documentation->asPlainText(),
       AnyOf(HasSubstr("Overload with int"), HasSubstr("Overload with bool")));
   EXPECT_EQ(A.SnippetSuffix, "($0)");
 }
 
+TEST(CompletionTest, OverloadBundlingSameFileDifferentURI) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.BundleOverloads = true;
+
+  Symbol SymX = sym("ns::X", index::SymbolKind::Function, "@F@\\0#");
+  Symbol SymY = sym("ns::X", index::SymbolKind::Function, "@F@\\0#I#");
+  std::string BarHeader = testPath("bar.h");
+  auto BarURI = URI::create(BarHeader).toString();
+  SymX.CanonicalDeclaration.FileURI = BarURI.c_str();
+  SymY.CanonicalDeclaration.FileURI = BarURI.c_str();
+  // The include header is different, but really it's the same file.
+  SymX.IncludeHeaders.emplace_back("\"bar.h\"", 1);
+  SymY.IncludeHeaders.emplace_back(BarURI.c_str(), 1);
+
+  auto Results = completions("void f() { ::ns::^ }", {SymX, SymY}, Opts);
+  // Expect both results are bundled, despite the different-but-same
+  // IncludeHeader.
+  ASSERT_EQ(1u, Results.Completions.size());
+  const auto &R = Results.Completions.front();
+  EXPECT_EQ("X", R.Name);
+  EXPECT_EQ(2u, R.BundleSize);
+}
+
 TEST(CompletionTest, DocumentationFromChangedFileCrash) {
   MockFS FS;
   auto FooH = testPath("foo.h");
   auto FooCpp = testPath("foo.cpp");
   FS.Files[FooH] = R"cpp(
     // this is my documentation comment.
     int func();
   )cpp";