ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
Currently, a symbol can have only one #include header attached, which
might not work well if the symbol can be imported via different #includes
depending
on where it's used. This patch stores multiple #include headers (with #
references)
for each symbol, so that CodeCompletion can suggest one candidate for each
distinct #include for the same symbol and rank them according to proximity and
popularity of #include headers. In order to preserve the order of overall
ranking
(i.e. ranking score without #include signals) and group candidates for the same
symbol
close by, we only increment the ranking score by a small amount based on the
relevance
and quality of #includes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51291
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/QualityTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===================================================================
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -53,7 +53,11 @@
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
MATCHER_P(IncludeHeader, P, "") {
- return arg.Detail && arg.Detail->IncludeHeader == P;
+ return (arg.IncludeHeaders.size() == 1) &&
+ (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
MATCHER_P(DeclRange, Pos, "") {
return std::tie(arg.CanonicalDeclaration.Start.Line,
@@ -696,6 +700,11 @@
Line: 1
Column: 1
IsIndexedForCodeCompletion: true
+IncludeHeaders:
+ - Header: 'include1'
+ References: 7
+ - Header: 'include2'
+ References: 3
Detail:
Documentation: 'Foo doc'
ReturnType: 'int'
@@ -730,6 +739,11 @@
Doc("Foo doc"), ReturnType("int"),
DeclURI("file:///path/foo.h"),
ForCodeCompletion(true))));
+ auto &Sym1 = *Symbols1.begin();
+ assert(Sym1.Detail);
+ EXPECT_THAT(Sym1.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
+ IncludeHeaderWithRef("include2", 3u)));
auto Symbols2 = symbolsFromYAML(YAML2);
EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
QName("clang::Foo2"), Labeled("Foo2-sig"),
@@ -751,9 +765,10 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
- IncludeHeader(TestHeaderURI))));
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI))));
+ EXPECT_THAT(Symbols.begin()->IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
}
#ifndef _WIN32
Index: unittests/clangd/QualityTests.cpp
===================================================================
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -200,7 +200,7 @@
TEST(QualityTests, SymbolRelevanceSignalsSanity) {
SymbolRelevanceSignals Default;
- EXPECT_EQ(Default.evaluate(), 1);
+ EXPECT_FLOAT_EQ(Default.evaluate(), 1.000015);
SymbolRelevanceSignals Forbidden;
Forbidden.Forbidden = true;
Index: unittests/clangd/IndexTests.cpp
===================================================================
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -243,6 +243,41 @@
EXPECT_EQ(M.Name, "right");
}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
+
+TEST(MergeTest, MergeIncludesOnDifferentDefinitions) {
+ Symbol L, R;
+ L.Name = "left";
+ R.Name = "right";
+ L.ID = R.ID = SymbolID("hello");
+ L.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("new", 1);
+
+ L.Definition.FileURI = "file:/left.h";
+
+ Symbol::Details Scratch;
+ // Only merge references of the same includes but do not merge new #includes.
+ Symbol M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u)));
+
+ // Definitions are the same.
+ R.Definition.FileURI = "file:/left.h";
+ M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u)));
+
+ // Also merge new #includes when definitions are different.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -177,7 +177,7 @@
Req.Query = "";
bool SeenSymbol = false;
M.fuzzyFind(Req, [&](const Symbol &Sym) {
- EXPECT_TRUE(Sym.Detail->IncludeHeader.empty());
+ EXPECT_TRUE(Sym.IncludeHeaders.empty());
SeenSymbol = true;
});
EXPECT_TRUE(SeenSymbol);
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -568,7 +568,8 @@
auto BarURI = URI::createFile(BarHeader).toString();
Symbol Sym = cls("ns::X");
Sym.CanonicalDeclaration.FileURI = BarURI;
- Scratch.IncludeHeader = BarURI;
+ Sym.IncludeHeaders.emplace_back(BarURI, 1);
+
Sym.Detail = &Scratch;
// Shoten include path based on search dirctory and insert.
auto Results = completions(Server,
@@ -602,7 +603,10 @@
auto BarURI = URI::createFile(BarHeader).toString();
SymX.CanonicalDeclaration.FileURI = BarURI;
SymY.CanonicalDeclaration.FileURI = BarURI;
- Scratch.IncludeHeader = "<bar>";
+
+ SymX.IncludeHeaders.emplace_back("<bar>", 1);
+ SymY.IncludeHeaders.emplace_back("<bar>", 1);
+
SymX.Detail = &Scratch;
SymY.Detail = &Scratch;
// Shoten include path based on search dirctory and insert.
@@ -1130,7 +1134,8 @@
Symbol::Details Detail;
std::string DeclFile = URI::createFile(testPath("foo")).toString();
NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
- Detail.IncludeHeader = "<foo>";
+ NoArgsGFunc.IncludeHeaders.emplace_back("<foo>", 1);
+
NoArgsGFunc.Detail = &Detail;
EXPECT_THAT(
completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
@@ -1782,6 +1787,20 @@
ASSERT_EQ(Reqs3.size(), 2u);
}
+TEST(CompletionTest, MultipleIncludeHeaders) {
+ // Differences in header-to-insert suppress bundling.
+ std::string DeclFile = URI::createFile(testPath("foo")).toString();
+ Symbol sym = func("Func");
+ sym.CanonicalDeclaration.FileURI = DeclFile;
+ sym.IncludeHeaders.emplace_back("<foo>", 1);
+ sym.IncludeHeaders.emplace_back("<bar>", 1);
+
+ EXPECT_THAT(
+ completions("Fun^", {sym}).Completions,
+ UnorderedElementsAre(AllOf(Named("Func"), InsertInclude("<foo>")),
+ AllOf(Named("Func"), InsertInclude("<bar>"))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/SymbolYAML.cpp
===================================================================
--- clangd/index/SymbolYAML.cpp
+++ clangd/index/SymbolYAML.cpp
@@ -10,11 +10,13 @@
#include "SymbolYAML.h"
#include "Index.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/raw_ostream.h"
LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(clang::clangd::Symbol)
+LLVM_YAML_IS_SEQUENCE_VECTOR(clang::clangd::Symbol::IncludeHeaderWithReferences)
namespace llvm {
namespace yaml {
@@ -70,7 +72,15 @@
static void mapping(IO &io, Symbol::Details &Detail) {
io.mapOptional("Documentation", Detail.Documentation);
io.mapOptional("ReturnType", Detail.ReturnType);
- io.mapOptional("IncludeHeader", Detail.IncludeHeader);
+ }
+};
+
+template <>
+struct MappingTraits<clang::clangd::Symbol::IncludeHeaderWithReferences> {
+ static void mapping(IO &io,
+ clang::clangd::Symbol::IncludeHeaderWithReferences &Inc) {
+ io.mapRequired("Header", Inc.IncludeHeader);
+ io.mapRequired("References", Inc.References);
}
};
@@ -112,6 +122,7 @@
false);
IO.mapOptional("Signature", Sym.Signature);
IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix);
+ IO.mapOptional("IncludeHeaders", Sym.IncludeHeaders);
IO.mapOptional("Detail", NDetail->Opt);
}
};
Index: clangd/index/SymbolCollector.cpp
===================================================================
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -403,10 +403,12 @@
SM.getExpansionLoc(MI->getDefinitionLoc()), Opts))
Include = std::move(*Header);
}
+ if (!Include.empty())
+ S.IncludeHeaders.emplace_back(Include, 1);
+
S.Signature = Signature;
S.CompletionSnippetSuffix = SnippetSuffix;
Symbol::Details Detail;
- Detail.IncludeHeader = Include;
S.Detail = &Detail;
Symbols.insert(S);
return true;
@@ -489,7 +491,8 @@
Symbol::Details Detail;
Detail.Documentation = Documentation;
Detail.ReturnType = ReturnType;
- Detail.IncludeHeader = Include;
+ if (!Include.empty())
+ S.IncludeHeaders.emplace_back(Include, 1);
S.Detail = &Detail;
S.Origin = Opts.Origin;
Index: clangd/index/Merge.cpp
===================================================================
--- clangd/index/Merge.cpp
+++ clangd/index/Merge.cpp
@@ -100,6 +100,11 @@
// Classes: this is the def itself. Functions: hopefully the header decl.
// If both did (or both didn't), continue to prefer L over R.
bool PreferR = R.Definition && !L.Definition;
+ // If L and R have definitions in different files (saw on different TUs),
+ // merge include headers from L and R, as they can both export the symbol.
+ bool MergeIncludes = !L.Definition.FileURI.empty() &&
+ !R.Definition.FileURI.empty() &&
+ (L.Definition.FileURI != R.Definition.FileURI);
Symbol S = PreferR ? R : L; // The target symbol we're merging into.
const Symbol &O = PreferR ? L : R; // The "other" less-preferred symbol.
@@ -114,6 +119,18 @@
S.Signature = O.Signature;
if (S.CompletionSnippetSuffix == "")
S.CompletionSnippetSuffix = O.CompletionSnippetSuffix;
+ for (const auto &OI : O.IncludeHeaders) {
+ bool Found = false;
+ for (auto &SI : S.IncludeHeaders) {
+ if (SI.IncludeHeader == OI.IncludeHeader) {
+ Found = true;
+ SI.References += OI.References;
+ break;
+ }
+ }
+ if (!Found && MergeIncludes)
+ S.IncludeHeaders.emplace_back(OI.IncludeHeader, OI.References);
+ }
if (O.Detail) {
if (S.Detail) {
@@ -123,8 +140,6 @@
Scratch->Documentation = O.Detail->Documentation;
if (Scratch->ReturnType == "")
Scratch->ReturnType = O.Detail->ReturnType;
- if (Scratch->IncludeHeader == "")
- Scratch->IncludeHeader = O.Detail->IncludeHeader;
S.Detail = Scratch;
} else
S.Detail = O.Detail;
Index: clangd/index/Index.h
===================================================================
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -16,7 +16,9 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/StringSaver.h"
#include <array>
#include <string>
@@ -200,6 +202,27 @@
/// (When snippets are disabled, the symbol name alone is used).
llvm::StringRef CompletionSnippetSuffix;
+ struct IncludeHeaderWithReferences {
+ IncludeHeaderWithReferences() = default;
+
+ IncludeHeaderWithReferences(llvm::StringRef IncludeHeader,
+ unsigned References)
+ : IncludeHeader(IncludeHeader), References(References) {}
+
+ /// This can be either a URI of the header to be #include'd
+ /// for this symbol, or a literal header quoted with <> or "" that is
+ /// suitable to be included directly. When it is a URI, the exact #include
+ /// path needs to be calculated according to the URI scheme.
+ ///
+ /// Note that the include header is a canonical include for the symbol and
+ /// can be different from FileURI in the CanonicalDeclaration.
+ llvm::StringRef IncludeHeader = "";
+ /// The number of translation units that reference this symbol via this
+ /// header. This number is only meaningful if aggregated in an index.
+ unsigned References = 0;
+ };
+ /// One Symbol can potentially be incuded via different headers.
+ llvm::SmallVector<IncludeHeaderWithReferences,1> IncludeHeaders;
/// Optional symbol details that are not required to be set. For example, an
/// index fuzzy match can return a large number of symbol candidates, and it
/// is preferable to send only core symbol information in the batched results
@@ -211,14 +234,6 @@
/// Type when this symbol is used in an expression. (Short display form).
/// e.g. return type of a function, or type of a variable.
llvm::StringRef ReturnType;
- /// This can be either a URI of the header to be #include'd for this symbol,
- /// or a literal header quoted with <> or "" that is suitable to be included
- /// directly. When this is a URI, the exact #include path needs to be
- /// calculated according to the URI scheme.
- ///
- /// This is a canonical include for the symbol and can be different from
- /// FileURI in the CanonicalDeclaration.
- llvm::StringRef IncludeHeader;
};
// Optional details of the symbol.
Index: clangd/index/Index.cpp
===================================================================
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -9,6 +9,7 @@
#include "Index.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/SHA1.h"
#include "llvm/Support/raw_ostream.h"
@@ -91,14 +92,16 @@
Intern(S.Signature);
Intern(S.CompletionSnippetSuffix);
+ for (auto &I : S.IncludeHeaders)
+ Intern(I.IncludeHeader);
+
if (S.Detail) {
// Copy values of StringRefs into arena.
auto *Detail = Arena.Allocate<Symbol::Details>();
*Detail = *S.Detail;
// Intern the actual strings.
Intern(Detail->Documentation);
Intern(Detail->ReturnType);
- Intern(Detail->IncludeHeader);
// Replace the detail pointer with our copy.
S.Detail = Detail;
}
Index: clangd/Quality.h
===================================================================
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -57,6 +57,8 @@
bool ReservedName = false; // __foo, _Foo are usually implementation details.
// FIXME: make these findable once user types _.
unsigned References = 0;
+ // Reference count for the include header of symbol, if the symbol has one.
+ unsigned IncludeReferences = 0;
enum SymbolCategory {
Unknown = 0,
@@ -90,6 +92,10 @@
/// This is used to calculate proximity between the index symbol and the
/// query.
llvm::StringRef SymbolURI;
+ /// There can be multiple candidates with different include headrs for the
+ /// same symbol. This is used to calculate proximity between the include
+ /// header of the index symbol and the query.
+ llvm::StringRef IncludeHeader;
/// Proximity between best declaration and the query. [0-1], 1 is closest.
/// FIXME: unify with index proximity score - signals should be
/// source-independent.
Index: clangd/Quality.cpp
===================================================================
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//
#include "Quality.h"
#include "FileDistance.h"
+#include "Headers.h"
#include "URI.h"
#include "index/Index.h"
#include "clang/AST/ASTContext.h"
@@ -18,6 +19,7 @@
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Sema/CodeCompleteConsumer.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/MathExtras.h"
@@ -185,6 +187,8 @@
ReservedName = ReservedName || isReserved(IndexResult.Name);
}
+static const float kIncludeHeaderScoreIncreaseUnit = 0.0001;
+
float SymbolQualitySignals::evaluate() const {
float Score = 1;
@@ -228,6 +232,12 @@
break;
}
+ // Increase score by [0, kIncludeHeaderScoreIncreaseUnit) based on the
+ // populatiry of the include header.
+ Score += kIncludeHeaderScoreIncreaseUnit /
+ (1 + std::exp(-IncludeReferences / 500.0)) -
+ kIncludeHeaderScoreIncreaseUnit / 2;
+
return Score;
}
@@ -349,6 +359,16 @@
if (NeedsFixIts)
Score *= 0.5;
+ // Increase score by [0, 1.5*kIncludeHeaderScoreIncreaseUnit] based on
+ // include proximity. Give extra boost for include proximity comparing to
+ // include quality so that unpopular include can still be ranked before very
+ // popular ones. Symbol with no include header or literal include is
+ // preferred.
+ Score += 1.5 * kIncludeHeaderScoreIncreaseUnit *
+ ((IncludeHeader.empty() || isLiteralInclude(IncludeHeader))
+ ? 1
+ : proximityScore(IncludeHeader, FileProximityMatch).first);
+
return Score;
}
@@ -415,3 +435,4 @@
} // namespace clangd
} // namespace clang
+
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -44,6 +44,7 @@
#include "clang/Sema/Sema.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
@@ -247,6 +248,7 @@
// We may have a result from Sema, from the index, or both.
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ llvm::Optional<Symbol::IncludeHeaderWithReferences> IncludeHeader;
// States whether this item is an override suggestion.
bool IsOverride = false;
@@ -285,8 +287,7 @@
}
llvm::Optional<llvm::StringRef> headerToInsertIfNotPresent() const {
- if (!IndexResult || !IndexResult->Detail ||
- IndexResult->Detail->IncludeHeader.empty())
+ if (!IncludeHeader)
return llvm::None;
if (SemaResult && SemaResult->Declaration) {
// Avoid inserting new #include if the declaration is found in the current
@@ -296,7 +297,7 @@
if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc())))
return llvm::None;
}
- return IndexResult->Detail->IncludeHeader;
+ return IncludeHeader->IncludeHeader;
}
using Bundle = llvm::SmallVector<CompletionCandidate, 4>;
@@ -382,7 +383,7 @@
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}",
C.IndexResult->CanonicalDeclaration.FileURI,
- C.IndexResult->Detail->IncludeHeader, FileName);
+ C.IncludeHeader->IncludeHeader, FileName);
}
}
@@ -1378,14 +1379,26 @@
C.IndexResult = IndexResult;
C.IsOverride = IsOverride;
C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult);
- if (auto OverloadSet = Opts.BundleOverloads ? C.overloadSet() : 0) {
- auto Ret = BundleLookup.try_emplace(OverloadSet, Bundles.size());
- if (Ret.second)
- Bundles.emplace_back();
- Bundles[Ret.first->second].push_back(std::move(C));
+ auto AddWithInclude =
+ [&](llvm::Optional<Symbol::IncludeHeaderWithReferences> Include) {
+ llvm::errs() << " === AddWithInclude: "
+ << (Include ? Include->IncludeHeader : "No") << "\n";
+ C.IncludeHeader = std::move(Include);
+ if (auto OverloadSet = Opts.BundleOverloads ? C.overloadSet() : 0) {
+ 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));
+ }
+ };
+ if (IndexResult && !IndexResult->IncludeHeaders.empty()) {
+ for (const auto &P : IndexResult->IncludeHeaders)
+ AddWithInclude(P);
} else {
- Bundles.emplace_back();
- Bundles.back().push_back(std::move(C));
+ AddWithInclude(llvm::None);
}
};
llvm::DenseSet<const Symbol *> UsedIndexResults;
@@ -1446,6 +1459,13 @@
Relevance.NameMatch = *FuzzyScore;
else
return;
+ if (First.IncludeHeader) {
+ llvm::errs() << " === addCandidate: include " << Relevance.IncludeHeader
+ << ", "
+ << " Refs: " << Quality.IncludeReferences << "\n";
+ Relevance.IncludeHeader = First.IncludeHeader->IncludeHeader;
+ Quality.IncludeReferences = First.IncludeHeader->References;
+ }
SymbolOrigin Origin = SymbolOrigin::Unknown;
bool FromIndex = false;
for (const auto &Candidate : Bundle) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
