Skip to content

Commit 545917c

Browse files
[clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (#111616)
`CallHierarchyIncomingCall::fromRanges` are interpreted as ranges in the same file as the `CallHierarchyItem` representing the caller (`CallHierarchyIncomingCall::from`). In C/C++, it's possible for the calls to be in a different file than the caller, as illustrated in the added test case. With this patch, such calls are dropped, rather than their ranges being incorrectly interpreted as pertaining to the wrong file.
1 parent 9270328 commit 545917c

File tree

2 files changed

+46
-4
lines changed

2 files changed

+46
-4
lines changed

clang-tools-extra/clangd/XRefs.cpp

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
#include "llvm/ADT/StringRef.h"
6464
#include "llvm/Support/Casting.h"
6565
#include "llvm/Support/Error.h"
66+
#include "llvm/Support/ErrorHandling.h"
6667
#include "llvm/Support/Path.h"
6768
#include "llvm/Support/raw_ostream.h"
6869
#include <optional>
@@ -2275,7 +2276,7 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
22752276
// Initially store the ranges in a map keyed by SymbolID of the caller.
22762277
// This allows us to group different calls with the same caller
22772278
// into the same CallHierarchyIncomingCall.
2278-
llvm::DenseMap<SymbolID, std::vector<Range>> CallsIn;
2279+
llvm::DenseMap<SymbolID, std::vector<Location>> CallsIn;
22792280
// We can populate the ranges based on a refs request only. As we do so, we
22802281
// also accumulate the container IDs into a lookup request.
22812282
LookupRequest ContainerLookup;
@@ -2285,7 +2286,7 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
22852286
elog("incomingCalls failed to convert location: {0}", Loc.takeError());
22862287
return;
22872288
}
2288-
CallsIn[R.Container].push_back(Loc->range);
2289+
CallsIn[R.Container].push_back(*Loc);
22892290

22902291
ContainerLookup.IDs.insert(R.Container);
22912292
});
@@ -2294,9 +2295,21 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
22942295
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
22952296
auto It = CallsIn.find(Caller.ID);
22962297
assert(It != CallsIn.end());
2297-
if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
2298+
if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
2299+
std::vector<Range> FromRanges;
2300+
for (const Location &L : It->second) {
2301+
if (L.uri != CHI->uri) {
2302+
// Call location not in same file as caller.
2303+
// This can happen in some edge cases. There's not much we can do,
2304+
// since the protocol only allows returning ranges interpreted as
2305+
// being in the caller's file.
2306+
continue;
2307+
}
2308+
FromRanges.push_back(L.range);
2309+
}
22982310
Results.push_back(
2299-
CallHierarchyIncomingCall{std::move(*CHI), std::move(It->second)});
2311+
CallHierarchyIncomingCall{std::move(*CHI), std::move(FromRanges)});
2312+
}
23002313
});
23012314
// Sort results by name of container.
23022315
llvm::sort(Results, [](const CallHierarchyIncomingCall &A,

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,35 @@ TEST(CallHierarchy, HierarchyOnVar) {
491491
fromRanges(Source.range("Callee")))));
492492
}
493493

494+
TEST(CallHierarchy, CallInDifferentFileThanCaller) {
495+
Annotations Header(R"cpp(
496+
#define WALDO void caller() {
497+
)cpp");
498+
Annotations Source(R"cpp(
499+
void call^ee();
500+
WALDO
501+
callee();
502+
}
503+
)cpp");
504+
auto TU = TestTU::withCode(Source.code());
505+
TU.HeaderCode = Header.code();
506+
auto AST = TU.build();
507+
auto Index = TU.index();
508+
509+
std::vector<CallHierarchyItem> Items =
510+
prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
511+
ASSERT_THAT(Items, ElementsAre(withName("callee")));
512+
513+
auto Incoming = incomingCalls(Items[0], Index.get());
514+
515+
// The only call site is in the source file, which is a different file from
516+
// the declaration of the function containing the call, which is in the
517+
// header. The protocol does not allow us to represent such calls, so we drop
518+
// them. (The call hierarchy item itself is kept.)
519+
EXPECT_THAT(Incoming,
520+
ElementsAre(AllOf(from(withName("caller")), fromRanges())));
521+
}
522+
494523
} // namespace
495524
} // namespace clangd
496525
} // namespace clang

0 commit comments

Comments
 (0)