ArcsinX created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
ArcsinX requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
This patch fixes the following problem:
- open a file with references to the symbol `Foo`
- remove all references to `Foo` (from the dynamic index).
- `MergedIndex::refs()` result will contain positions of removed references
(from the static index).
Idea of this patch is to keep a set of files which were used during index build
inside the index.
Thus at processing the static index references we can check if the file of
processing reference is a part the dynamic index or not.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93393
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/Index.cpp
clang-tools-extra/clangd/index/Index.h
clang-tools-extra/clangd/index/MemIndex.cpp
clang-tools-extra/clangd/index/MemIndex.h
clang-tools-extra/clangd/index/Merge.cpp
clang-tools-extra/clangd/index/Merge.h
clang-tools-extra/clangd/index/ProjectAware.cpp
clang-tools-extra/clangd/index/dex/Dex.cpp
clang-tools-extra/clangd/index/dex/Dex.h
clang-tools-extra/clangd/index/remote/Client.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/DexTests.cpp
clang-tools-extra/clangd/unittests/IndexTests.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/TestFS.cpp
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -99,8 +99,9 @@
llvm::Expected<std::string>
getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
llvm::StringRef HintPath) const override {
- if (!HintPath.startswith(testRoot()))
- return error("Hint path doesn't start with test root: {0}", HintPath);
+ if (!HintPath.empty() && !HintPath.startswith(testRoot()))
+ return error("Hint path is not empty and doesn't start with {0}: {1}",
+ testRoot(), HintPath);
if (!Body.consume_front("/"))
return error("Body of an unittest: URI must start with '/'");
llvm::SmallString<16> Path(Body.begin(), Body.end());
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1237,6 +1237,9 @@
void relations(const RelationsRequest &Req,
llvm::function_ref<void(const SymbolID &, const Symbol &)>
Callback) const override {}
+
+ bool hasFile(llvm::StringRef) const override { return false; }
+
size_t estimateMemoryUsage() const override { return 0; }
} PIndex;
Results = rename({MainCode.point(),
@@ -1285,6 +1288,9 @@
void relations(const RelationsRequest &,
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override {}
+
+ bool hasFile(llvm::StringRef) const override { return false; }
+
size_t estimateMemoryUsage() const override { return 0; }
Ref ReturnedRef;
} DIndex(XRefInBarCC);
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -224,6 +224,19 @@
EXPECT_THAT(lookup(*I, SymbolID("ns::nonono")), UnorderedElementsAre());
}
+TEST(MemIndexTest, hasFile) {
+ SymbolSlab Symbols;
+ RefSlab Refs;
+ auto Size = Symbols.bytes() + Refs.bytes();
+ auto Data = std::make_pair(std::move(Symbols), std::move(Refs));
+ llvm::StringSet<> Files = {testPath("foo.cc"), testPath("bar.cc")};
+ MemIndex I(std::move(Data.first), std::move(Data.second), RelationSlab(),
+ std::move(Files), std::move(Data), Size);
+ EXPECT_TRUE(I.hasFile("unittest:///foo.cc"));
+ EXPECT_TRUE(I.hasFile("unittest:///bar.cc"));
+ EXPECT_FALSE(I.hasFile("unittest:///foobar.cc"));
+}
+
TEST(MemIndexTest, TemplateSpecialization) {
SymbolSlab::Builder B;
@@ -367,7 +380,7 @@
Test.Code = std::string(Test1Code.code());
Test.Filename = "test.cc";
auto AST = Test.build();
- Dyn.updateMain(Test.Filename, AST);
+ Dyn.updateMain(testPath(Test.Filename), AST);
// Build static index for test.cc.
Test.HeaderCode = HeaderCode;
@@ -375,7 +388,7 @@
Test.Filename = "test.cc";
auto StaticAST = Test.build();
// Add stale refs for test.cc.
- StaticIndex.updateMain(Test.Filename, StaticAST);
+ StaticIndex.updateMain(testPath(Test.Filename), StaticAST);
// Add refs for test2.cc
Annotations Test2Code(R"(class $Foo[[Foo]] {};)");
@@ -384,7 +397,7 @@
Test2.Code = std::string(Test2Code.code());
Test2.Filename = "test2.cc";
StaticAST = Test2.build();
- StaticIndex.updateMain(Test2.Filename, StaticAST);
+ StaticIndex.updateMain(testPath(Test2.Filename), StaticAST);
RefsRequest Request;
Request.IDs = {Foo.ID};
@@ -403,10 +416,46 @@
RefSlab::Builder Results2;
EXPECT_TRUE(
Merge.refs(Request, [&](const Ref &O) { Results2.insert(Foo.ID, O); }));
- EXPECT_THAT(std::move(Results2).build(),
- ElementsAre(Pair(
- _, ElementsAre(AnyOf(FileURI("unittest:///test.cc"),
- FileURI("unittest:///test2.cc"))))));
+
+ // Remove all refs for test.cc from dynamic index,
+ // merged index should not return results from static index for test.cc.
+ Test.Code = "";
+ AST = Test.build();
+ Dyn.updateMain(testPath(Test.Filename), AST);
+
+ Request.Limit = llvm::None;
+ RefSlab::Builder Results3;
+ EXPECT_FALSE(
+ Merge.refs(Request, [&](const Ref &O) { Results3.insert(Foo.ID, O); }));
+ EXPECT_THAT(std::move(Results3).build(),
+ ElementsAre(Pair(_, UnorderedElementsAre(AllOf(
+ RefRange(Test2Code.range("Foo")),
+ FileURI("unittest:///test2.cc"))))));
+}
+
+TEST(MergeIndexTest, HasFile) {
+ SymbolSlab DynSymbols;
+ RefSlab DynRefs;
+ auto DynSize = DynSymbols.bytes() + DynRefs.bytes();
+ auto DynData = std::make_pair(std::move(DynSymbols), std::move(DynRefs));
+ llvm::StringSet<> DynFiles = {testPath("foo.cc")};
+ MemIndex DynIndex(std::move(DynData.first), std::move(DynData.second),
+ RelationSlab(), std::move(DynFiles), std::move(DynData),
+ DynSize);
+ SymbolSlab StaticSymbols;
+ RefSlab StaticRefs;
+ auto StaticData =
+ std::make_pair(std::move(StaticSymbols), std::move(StaticRefs));
+ llvm::StringSet<> StaticFiles = {testPath("bar.cc")};
+ MemIndex StaticIndex(std::move(StaticData.first),
+ std::move(StaticData.second), RelationSlab(),
+ std::move(StaticFiles), std::move(StaticData),
+ StaticSymbols.bytes() + StaticRefs.bytes());
+ MergedIndex Merge(&DynIndex, &StaticIndex);
+
+ EXPECT_TRUE(Merge.hasFile("unittest:///foo.cc"));
+ EXPECT_TRUE(Merge.hasFile("unittest:///bar.cc"));
+ EXPECT_FALSE(Merge.hasFile("unittest:///foobar.cc"));
}
TEST(MergeIndexTest, NonDocumentation) {
Index: clang-tools-extra/clangd/unittests/DexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DexTests.cpp
+++ clang-tools-extra/clangd/unittests/DexTests.cpp
@@ -732,6 +732,19 @@
EXPECT_THAT(Results, UnorderedElementsAre(Child1.ID, Child2.ID));
}
+TEST(DexIndex, hasFile) {
+ SymbolSlab Symbols;
+ RefSlab Refs;
+ auto Size = Symbols.bytes() + Refs.bytes();
+ auto Data = std::make_pair(std::move(Symbols), std::move(Refs));
+ llvm::StringSet<> Files = {testPath("foo.cc"), testPath("bar.cc")};
+ Dex I(std::move(Data.first), std::move(Data.second), RelationSlab(),
+ std::move(Files), std::move(Data), Size);
+ EXPECT_TRUE(I.hasFile("unittest:///foo.cc"));
+ EXPECT_TRUE(I.hasFile("unittest:///bar.cc"));
+ EXPECT_FALSE(I.hasFile("unittest:///foobar.cc"));
+}
+
TEST(DexTest, PreferredTypesBoosting) {
auto Sym1 = symbol("t1");
Sym1.Type = "T1";
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1349,6 +1349,8 @@
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override {}
+ bool hasFile(llvm::StringRef) const override { return false; }
+
// This is incorrect, but IndexRequestCollector is not an actual index and it
// isn't used in production code.
size_t estimateMemoryUsage() const override { return 0; }
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===================================================================
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -152,6 +152,8 @@
});
}
+ bool hasFile(llvm::StringRef) const { return false; }
+
// IndexClient does not take any space since the data is stored on the
// server.
size_t estimateMemoryUsage() const override { return 0; }
Index: clang-tools-extra/clangd/index/dex/Dex.h
===================================================================
--- clang-tools-extra/clangd/index/dex/Dex.h
+++ clang-tools-extra/clangd/index/dex/Dex.h
@@ -67,6 +67,16 @@
this->BackingDataSize = BackingDataSize;
}
+ template <typename SymbolRange, typename RefsRange, typename RelationsRange,
+ typename FileRange, typename Payload>
+ Dex(SymbolRange &&Symbols, RefsRange &&Refs, RelationsRange &&Relations,
+ FileRange &&Files, Payload &&BackingData, size_t BackingDataSize)
+ : Dex(std::forward<SymbolRange>(Symbols), std::forward<RefsRange>(Refs),
+ std::forward<RelationsRange>(Relations),
+ std::forward<Payload>(BackingData), BackingDataSize) {
+ this->Files = std::forward<FileRange>(Files);
+ }
+
/// Builds an index from slabs. The index takes ownership of the slab.
static std::unique_ptr<SymbolIndex> build(SymbolSlab, RefSlab, RelationSlab);
@@ -84,6 +94,8 @@
llvm::function_ref<void(const SymbolID &, const Symbol &)>
Callback) const override;
+ bool hasFile(llvm::StringRef FileURI) const override;
+
size_t estimateMemoryUsage() const override;
private:
@@ -112,6 +124,8 @@
"RelationKind should be of same size as a uint8_t");
llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> Relations;
std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
+ // Set of files which were used during this index build.
+ llvm::StringSet<> Files;
// Size of memory retained by KeepAlive.
size_t BackingDataSize = 0;
};
Index: clang-tools-extra/clangd/index/dex/Dex.cpp
===================================================================
--- clang-tools-extra/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -313,6 +313,13 @@
}
}
+bool Dex::hasFile(llvm::StringRef FileURI) const {
+ auto Path = URI::resolve(FileURI);
+ if (!Path)
+ return false;
+ return Files.contains(*Path);
+}
+
size_t Dex::estimateMemoryUsage() const {
size_t Bytes = Symbols.size() * sizeof(const Symbol *);
Bytes += SymbolQuality.size() * sizeof(float);
Index: clang-tools-extra/clangd/index/ProjectAware.cpp
===================================================================
--- clang-tools-extra/clangd/index/ProjectAware.cpp
+++ clang-tools-extra/clangd/index/ProjectAware.cpp
@@ -54,6 +54,8 @@
llvm::function_ref<void(const SymbolID &, const Symbol &)>
Callback) const override;
+ bool hasFile(llvm::StringRef FileURI) const override;
+
ProjectAwareIndex(IndexFactory Gen) : Gen(std::move(Gen)) {}
private:
@@ -112,6 +114,13 @@
return Idx->relations(Req, Callback);
}
+bool ProjectAwareIndex::hasFile(llvm::StringRef FileURI) const {
+ trace::Span Tracer("ProjectAwareIndex::hasFile");
+ if (auto *Idx = getIndex())
+ return Idx->hasFile(FileURI);
+ return false;
+}
+
SymbolIndex *ProjectAwareIndex::getIndex() const {
const auto &C = Config::current();
if (!C.Index.External)
Index: clang-tools-extra/clangd/index/Merge.h
===================================================================
--- clang-tools-extra/clangd/index/Merge.h
+++ clang-tools-extra/clangd/index/Merge.h
@@ -45,6 +45,9 @@
void relations(const RelationsRequest &,
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override;
+ bool hasFile(llvm::StringRef FileURI) const override {
+ return Dynamic->hasFile(FileURI) || Static->hasFile(FileURI);
+ }
size_t estimateMemoryUsage() const override {
return Dynamic->estimateMemoryUsage() + Static->estimateMemoryUsage();
}
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -99,13 +99,7 @@
// and we can't reliably deduplicate them because offsets may differ slightly.
// We consider the dynamic index authoritative and report all its refs,
// and only report static index refs from other files.
- //
- // FIXME: The heuristic fails if the dynamic index contains a file, but all
- // refs were removed (we will report stale ones from the static index).
- // Ultimately we should explicit check which index has the file instead.
- llvm::StringSet<> DynamicIndexFileURIs;
More |= Dynamic->refs(Req, [&](const Ref &O) {
- DynamicIndexFileURIs.insert(O.Location.FileURI);
Callback(O);
assert(Remaining != 0);
--Remaining;
@@ -114,8 +108,8 @@
return More;
// We return less than Req.Limit if static index returns more refs for dirty
// files.
- bool StaticHadMore = Static->refs(Req, [&](const Ref &O) {
- if (DynamicIndexFileURIs.count(O.Location.FileURI))
+ bool StaticHadMore = Static->refs(Req, [&](const Ref &O) {
+ if (Dynamic->hasFile(O.Location.FileURI))
return; // ignore refs that have been seen from dynamic index.
if (Remaining == 0) {
More = true;
Index: clang-tools-extra/clangd/index/MemIndex.h
===================================================================
--- clang-tools-extra/clangd/index/MemIndex.h
+++ clang-tools-extra/clangd/index/MemIndex.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_MEMINDEX_H
#include "Index.h"
+#include "llvm/ADT/StringSet.h"
#include <mutex>
namespace clang {
@@ -44,6 +45,17 @@
this->BackingDataSize = BackingDataSize;
}
+ template <typename SymbolRange, typename RefRange, typename RelationRange,
+ typename FileRange, typename Payload>
+ MemIndex(SymbolRange &&Symbols, RefRange &&Refs, RelationRange &&Relations,
+ FileRange &&Files, Payload &&BackingData, size_t BackingDataSize)
+ : MemIndex(std::forward<SymbolRange>(Symbols),
+ std::forward<RefRange>(Refs),
+ std::forward<RelationRange>(Relations),
+ std::forward<Payload>(BackingData), BackingDataSize) {
+ this->Files = std::forward<FileRange>(Files);
+ }
+
/// Builds an index from slabs. The index takes ownership of the data.
static std::unique_ptr<SymbolIndex> build(SymbolSlab Symbols, RefSlab Refs,
RelationSlab Relations);
@@ -62,6 +74,8 @@
llvm::function_ref<void(const SymbolID &, const Symbol &)>
Callback) const override;
+ bool hasFile(llvm::StringRef FileURI) const override;
+
size_t estimateMemoryUsage() const override;
private:
@@ -73,6 +87,8 @@
static_assert(sizeof(RelationKind) == sizeof(uint8_t),
"RelationKind should be of same size as a uint8_t");
llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> Relations;
+ // Set of files which were used during this index build.
+ llvm::StringSet<> Files;
std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
// Size of memory retained by KeepAlive.
size_t BackingDataSize = 0;
Index: clang-tools-extra/clangd/index/MemIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/MemIndex.cpp
+++ clang-tools-extra/clangd/index/MemIndex.cpp
@@ -109,6 +109,13 @@
}
}
+bool MemIndex::hasFile(llvm::StringRef FileURI) const {
+ auto Path = URI::resolve(FileURI);
+ if (!Path)
+ return false;
+ return Files.contains(*Path);
+}
+
size_t MemIndex::estimateMemoryUsage() const {
return Index.getMemorySize() + Refs.getMemorySize() +
Relations.getMemorySize() + BackingDataSize;
Index: clang-tools-extra/clangd/index/Index.h
===================================================================
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -121,6 +121,9 @@
llvm::function_ref<void(const SymbolID &Subject, const Symbol &Object)>
Callback) const = 0;
+ /// Checks if the specified file was used to build this index.
+ virtual bool hasFile(llvm::StringRef FileURI) const = 0;
+
/// Returns estimated size of index (in bytes).
virtual size_t estimateMemoryUsage() const = 0;
};
@@ -145,6 +148,8 @@
llvm::function_ref<void(const SymbolID &, const Symbol &)>)
const override;
+ bool hasFile(llvm::StringRef FileURI) const override;
+
size_t estimateMemoryUsage() const override;
private:
Index: clang-tools-extra/clangd/index/Index.cpp
===================================================================
--- clang-tools-extra/clangd/index/Index.cpp
+++ clang-tools-extra/clangd/index/Index.cpp
@@ -76,6 +76,10 @@
return snapshot()->relations(R, CB);
}
+bool SwapIndex::hasFile(llvm::StringRef FileURI) const {
+ return snapshot()->hasFile(FileURI);
+}
+
size_t SwapIndex::estimateMemoryUsage() const {
return snapshot()->estimateMemoryUsage();
}
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -266,11 +266,14 @@
std::vector<std::shared_ptr<SymbolSlab>> SymbolSlabs;
std::vector<std::shared_ptr<RefSlab>> RefSlabs;
std::vector<std::shared_ptr<RelationSlab>> RelationSlabs;
+ llvm::StringSet<> Files;
std::vector<RefSlab *> MainFileRefs;
{
std::lock_guard<std::mutex> Lock(Mutex);
- for (const auto &FileAndSymbols : SymbolsSnapshot)
+ for (const auto &FileAndSymbols : SymbolsSnapshot) {
SymbolSlabs.push_back(FileAndSymbols.second);
+ Files.insert(FileAndSymbols.first());
+ }
for (const auto &FileAndRefs : RefsSnapshot) {
RefSlabs.push_back(FileAndRefs.second.Slab);
if (FileAndRefs.second.CountReferences)
@@ -372,14 +375,14 @@
case IndexType::Light:
return std::make_unique<MemIndex>(
llvm::make_pointee_range(AllSymbols), std::move(AllRefs),
- std::move(AllRelations),
+ std::move(AllRelations), std::move(Files),
std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
std::move(RefsStorage), std::move(SymsStorage)),
StorageSize);
case IndexType::Heavy:
return std::make_unique<dex::Dex>(
llvm::make_pointee_range(AllSymbols), std::move(AllRefs),
- std::move(AllRelations),
+ std::move(AllRelations), std::move(Files),
std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
std::move(RefsStorage), std::move(SymsStorage)),
StorageSize);
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
