[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Sat Nov 16 22:39:00 PST 2024
https://github.com/HighCommander4 updated https://github.com/llvm/llvm-project/pull/111616
>From 3d0c93948072cd2412a656a38f48de3d6e23e996 Mon Sep 17 00:00:00 2001
From: timon-ul <timon.ulrich at advantest.com>
Date: Sun, 3 Nov 2024 07:38:25 +0100
Subject: [PATCH 1/3] Support call hierarchy for fields and non-local variables
(#113900)
Fixes https://github.com/clangd/clangd/issues/1308
---
clang-tools-extra/clangd/XRefs.cpp | 5 ++-
.../clangd/unittests/CallHierarchyTests.cpp | 45 +++++++++++++++++++
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index f94cadeffaa298..70107e8a5b1517 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -2238,7 +2238,10 @@ prepareCallHierarchy(ParsedAST &AST, Position Pos, PathRef TUPath) {
for (const NamedDecl *Decl : getDeclAtPosition(AST, *Loc, {})) {
if (!(isa<DeclContext>(Decl) &&
cast<DeclContext>(Decl)->isFunctionOrMethod()) &&
- Decl->getKind() != Decl::Kind::FunctionTemplate)
+ Decl->getKind() != Decl::Kind::FunctionTemplate &&
+ !(Decl->getKind() == Decl::Kind::Var &&
+ !cast<VarDecl>(Decl)->isLocalVarDecl()) &&
+ Decl->getKind() != Decl::Kind::Field)
continue;
if (auto CHI = declToCallHierarchyItem(*Decl, AST.tuPath()))
Result.emplace_back(std::move(*CHI));
diff --git a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
index 6fa76aa6094bf2..b2278ff12735dc 100644
--- a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -446,6 +446,51 @@ TEST(CallHierarchy, CallInLocalVarDecl) {
AllOf(from(withName("caller3")), fromRanges(Source.range("call3")))));
}
+TEST(CallHierarchy, HierarchyOnField) {
+ // Tests that the call hierarchy works on fields.
+ Annotations Source(R"cpp(
+ struct Vars {
+ int v^ar1 = 1;
+ };
+ void caller() {
+ Vars values;
+ values.$Callee[[var1]];
+ }
+ )cpp");
+ TestTU TU = TestTU::withCode(Source.code());
+ auto AST = TU.build();
+ auto Index = TU.index();
+
+ std::vector<CallHierarchyItem> Items =
+ prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
+ ASSERT_THAT(Items, ElementsAre(withName("var1")));
+ auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
+ ASSERT_THAT(IncomingLevel1,
+ ElementsAre(AllOf(from(withName("caller")),
+ fromRanges(Source.range("Callee")))));
+}
+
+TEST(CallHierarchy, HierarchyOnVar) {
+ // Tests that the call hierarchy works on non-local variables.
+ Annotations Source(R"cpp(
+ int v^ar = 1;
+ void caller() {
+ $Callee[[var]];
+ }
+ )cpp");
+ TestTU TU = TestTU::withCode(Source.code());
+ auto AST = TU.build();
+ auto Index = TU.index();
+
+ std::vector<CallHierarchyItem> Items =
+ prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
+ ASSERT_THAT(Items, ElementsAre(withName("var")));
+ auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
+ ASSERT_THAT(IncomingLevel1,
+ ElementsAre(AllOf(from(withName("caller")),
+ fromRanges(Source.range("Callee")))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
>From 8e1e1995344e511b846c500b8cc4eca02012c6b3 Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Fri, 11 Oct 2024 09:00:39 -0700
Subject: [PATCH 2/3] [clangd] Simplify code with *Map::operator[] (NFC)
(#111939)
---
clang-tools-extra/clangd/Headers.cpp | 4 ++--
clang-tools-extra/clangd/XRefs.cpp | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 75f8668e7bef06..b537417bd10568 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -75,8 +75,8 @@ class IncludeStructure::RecordHeaders : public PPCallbacks {
IDs.push_back(HID);
}
}
- Out->MainFileIncludesBySpelling.try_emplace(Inc.Written)
- .first->second.push_back(Out->MainFileIncludes.size() - 1);
+ Out->MainFileIncludesBySpelling[Inc.Written].push_back(
+ Out->MainFileIncludes.size() - 1);
}
// Record include graph (not just for main-file includes)
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 70107e8a5b1517..4fd11307857ff8 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -2285,8 +2285,7 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
elog("incomingCalls failed to convert location: {0}", Loc.takeError());
return;
}
- auto It = CallsIn.try_emplace(R.Container, std::vector<Range>{}).first;
- It->second.push_back(Loc->range);
+ CallsIn[R.Container].push_back(Loc->range);
ContainerLookup.IDs.insert(R.Container);
});
>From 8b263d78924aef850f24d7d7199fe30852174917 Mon Sep 17 00:00:00 2001
From: Nathan Ridge <zeratul976 at hotmail.com>
Date: Tue, 8 Oct 2024 21:43:55 -0400
Subject: [PATCH 3/3] [clangd] Harden incomingCalls() against possible
misinterpretation of a range as pertaining to the wrong file
---
clang-tools-extra/clangd/XRefs.cpp | 21 +++++++++++---
.../clangd/unittests/CallHierarchyTests.cpp | 29 +++++++++++++++++++
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 4fd11307857ff8..61fa66180376cd 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -63,6 +63,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include <optional>
@@ -2275,7 +2276,7 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
// Initially store the ranges in a map keyed by SymbolID of the caller.
// This allows us to group different calls with the same caller
// into the same CallHierarchyIncomingCall.
- llvm::DenseMap<SymbolID, std::vector<Range>> CallsIn;
+ llvm::DenseMap<SymbolID, std::vector<Location>> CallsIn;
// We can populate the ranges based on a refs request only. As we do so, we
// also accumulate the container IDs into a lookup request.
LookupRequest ContainerLookup;
@@ -2285,7 +2286,7 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
elog("incomingCalls failed to convert location: {0}", Loc.takeError());
return;
}
- CallsIn[R.Container].push_back(Loc->range);
+ CallsIn[R.Container].push_back(*Loc);
ContainerLookup.IDs.insert(R.Container);
});
@@ -2294,9 +2295,21 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
auto It = CallsIn.find(Caller.ID);
assert(It != CallsIn.end());
- if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
+ if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+ std::vector<Range> FromRanges;
+ for (const Location &L : It->second) {
+ if (L.uri != CHI->uri) {
+ // Call location not in same file as caller.
+ // This can happen in some edge cases. There's not much we can do,
+ // since the protocol only allows returning ranges interpreted as
+ // being in the caller's file.
+ continue;
+ }
+ FromRanges.push_back(L.range);
+ }
Results.push_back(
- CallHierarchyIncomingCall{std::move(*CHI), std::move(It->second)});
+ CallHierarchyIncomingCall{std::move(*CHI), std::move(FromRanges)});
+ }
});
// Sort results by name of container.
llvm::sort(Results, [](const CallHierarchyIncomingCall &A,
diff --git a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
index b2278ff12735dc..8821d3aad9c784 100644
--- a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -491,6 +491,35 @@ TEST(CallHierarchy, HierarchyOnVar) {
fromRanges(Source.range("Callee")))));
}
+TEST(CallHierarchy, CallInDifferentFileThanCaller) {
+ Annotations Header(R"cpp(
+ #define WALDO void caller() {
+ )cpp");
+ Annotations Source(R"cpp(
+ void call^ee();
+ WALDO
+ callee();
+ }
+ )cpp");
+ auto TU = TestTU::withCode(Source.code());
+ TU.HeaderCode = Header.code();
+ auto AST = TU.build();
+ auto Index = TU.index();
+
+ std::vector<CallHierarchyItem> Items =
+ prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
+ ASSERT_THAT(Items, ElementsAre(withName("callee")));
+
+ auto Incoming = incomingCalls(Items[0], Index.get());
+
+ // The only call site is in the source file, which is a different file from
+ // the declaration of the function containing the call, which is in the
+ // header. The protocol does not allow us to represent such calls, so we drop
+ // them. (The call hierarchy item itself is kept.)
+ EXPECT_THAT(Incoming,
+ ElementsAre(AllOf(from(withName("caller")), fromRanges())));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list