Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions clang-tools-extra/clangd/XRefs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,15 @@ void enhanceLocatedSymbolsFromIndex(llvm::MutableArrayRef<LocatedSymbol> Result,
});
}

bool objcMethodIsTouched(const SourceManager &SM, const ObjCMethodDecl *OMD,
SourceLocation Loc) {
unsigned NumSels = OMD->getNumSelectorLocs();
for (unsigned I = 0; I < NumSels; ++I)
if (SM.getSpellingLoc(OMD->getSelectorLoc(I)) == Loc)
return true;
return false;
}

// Decls are more complicated.
// The AST contains at least a declaration, maybe a definition.
// These are up-to-date, and so generally preferred over index results.
Expand Down Expand Up @@ -430,6 +439,22 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
continue;
}
}
// Special case: - (void)^method; should jump to all overrides. Note that an
// Objective-C method can override a parent class or protocol.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's worthwhile to also add a test to check we don't return overridden methods when calling xrefs on usage of the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (const auto *OMD = llvm::dyn_cast<ObjCMethodDecl>(D)) {
if (TouchedIdentifier &&
objcMethodIsTouched(SM, OMD, TouchedIdentifier->location())) {
llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
OMD->getOverriddenMethods(Overrides);
if (!Overrides.empty()) {
for (const auto *Override : Overrides)
AddResultDecl(Override);
LocateASTReferentMetric.record(1, "objc-overriden-method");
}
AddResultDecl(OMD);
continue;
}
}

// Special case: the cursor is on an alias, prefer other results.
// This targets "using ns::^Foo", where the target is more interesting.
Expand Down Expand Up @@ -1283,6 +1308,12 @@ std::vector<LocatedSymbol> findImplementations(ParsedAST &AST, Position Pos,
} else if (const auto *RD = dyn_cast<CXXRecordDecl>(ND)) {
IDs.insert(getSymbolID(RD));
QueryKind = RelationKind::BaseOf;
} else if (const auto *OMD = dyn_cast<ObjCMethodDecl>(ND)) {
IDs.insert(getSymbolID(OMD));
QueryKind = RelationKind::OverriddenBy;
} else if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(ND)) {
IDs.insert(getSymbolID(ID));
QueryKind = RelationKind::BaseOf;
}
}
return findImplementors(std::move(IDs), QueryKind, Index, AST.tuPath());
Expand Down Expand Up @@ -1438,6 +1469,15 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
getOverriddenMethods(CMD, OverriddenMethods);
}
}
// Special case: Objective-C methods can override a parent class or
// protocol, we should be sure to report references to those.
if (const auto *OMD = llvm::dyn_cast<ObjCMethodDecl>(ND)) {
OverriddenBy.Subjects.insert(getSymbolID(OMD));
llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
OMD->getOverriddenMethods(Overrides);
for (const auto *Override : Overrides)
OverriddenMethods.insert(getSymbolID(Override));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for C++ we recursively traverse up the whole virtual-method hierarchy. any reason for not doing the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this already does - getOverriddenMethods will return methods for the hierarchy since it itself recurses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure if it does, e.g. https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclObjC.cpp#L1268-L1273 stops at the first overridden method.

maybe you can add some unittests for this as well ?

}
}
}

Expand Down
36 changes: 36 additions & 0 deletions clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1335,6 +1335,42 @@ TEST_F(SymbolCollectorTest, OverrideRelationsMultipleInheritance) {
OverriddenBy(CBar, DBar), OverriddenBy(CBaz, DBaz)));
}

TEST_F(SymbolCollectorTest, ObjCOverrideRelationsSimpleInheritance) {
std::string Header = R"cpp(
@interface A
- (void)foo;
@end
@interface B : A
- (void)foo; // A::foo
- (void)bar;
@end
@interface C : B
- (void)bar; // B::bar
@end
@interface D : C
- (void)foo; // B::foo
- (void)bar; // C::bar
@end
)cpp";
runSymbolCollector(Header, /*Main=*/"",
{"-xobjective-c++", "-Wno-objc-root-class"});
const Symbol &AFoo = findSymbol(Symbols, "A::foo");
const Symbol &BFoo = findSymbol(Symbols, "B::foo");
const Symbol &DFoo = findSymbol(Symbols, "D::foo");

const Symbol &BBar = findSymbol(Symbols, "B::bar");
const Symbol &CBar = findSymbol(Symbols, "C::bar");
const Symbol &DBar = findSymbol(Symbols, "D::bar");

std::vector<Relation> Result;
for (const Relation &R : Relations)
if (R.Predicate == RelationKind::OverriddenBy)
Result.push_back(R);
EXPECT_THAT(Result, UnorderedElementsAre(
OverriddenBy(AFoo, BFoo), OverriddenBy(BBar, CBar),
OverriddenBy(BFoo, DFoo), OverriddenBy(CBar, DBar)));
}

TEST_F(SymbolCollectorTest, CountReferences) {
const std::string Header = R"(
class W;
Expand Down
76 changes: 76 additions & 0 deletions clang-tools-extra/clangd/unittests/XRefsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,27 @@ TEST(LocateSymbol, FindOverrides) {
sym("foo", Code.range("2"), std::nullopt)));
}

TEST(LocateSymbol, FindOverridesObjC) {
auto Code = Annotations(R"objc(
@interface Foo
- (void)$1[[foo]];
@end

@interface Bar : Foo
@end
@implementation Bar
- (void)$2[[fo^o]] {}
@end
)objc");
TestTU TU = TestTU::withCode(Code.code());
TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
EXPECT_THAT(
locateSymbolAt(AST, Code.point(), TU.index().get()),
UnorderedElementsAre(sym("foo", Code.range("1"), std::nullopt),
sym("foo", Code.range("2"), Code.range("2"))));
}

TEST(LocateSymbol, WithIndexPreferredLocation) {
Annotations SymbolHeader(R"cpp(
class $p[[Proto]] {};
Expand Down Expand Up @@ -1834,6 +1855,41 @@ TEST(FindImplementations, Inheritance) {
}
}

TEST(FindImplementations, InheritanceObjC) {
llvm::StringRef Test = R"objc(
@interface $base^Base
- (void)fo$foo^o;
@end
@protocol Protocol
- (void)$protocol^protocol;
@end
@interface $ChildDecl[[Child]] : Base <Protocol>
- (void)concrete;
- (void)$fooDecl[[foo]];
@end
@implementation $ChildDef[[Child]]
- (void)concrete {}
- (void)$fooDef[[foo]] {}
- (void)$protocolDef[[protocol]] {}
@end
)objc";

Annotations Code(Test);
auto TU = TestTU::withCode(Code.code());
TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
auto Index = TU.index();
EXPECT_THAT(findImplementations(AST, Code.point("base"), Index.get()),
UnorderedElementsAre(sym("Child", Code.range("ChildDecl"),
Code.range("ChildDef"))));
EXPECT_THAT(findImplementations(AST, Code.point("foo"), Index.get()),
UnorderedElementsAre(
sym("foo", Code.range("fooDecl"), Code.range("fooDef"))));
EXPECT_THAT(findImplementations(AST, Code.point("protocol"), Index.get()),
UnorderedElementsAre(sym("protocol", Code.range("protocolDef"),
Code.range("protocolDef"))));
}

TEST(FindImplementations, CaptureDefinition) {
llvm::StringRef Test = R"cpp(
struct Base {
Expand Down Expand Up @@ -1963,6 +2019,7 @@ void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
Annotations T(Test);
auto TU = TestTU::withCode(T.code());
TU.ExtraArgs.push_back("-std=c++20");
TU.ExtraArgs.push_back("-xobjective-c++");

auto AST = TU.build();
std::vector<Matcher<ReferencesResult::Reference>> ExpectedLocations;
Expand Down Expand Up @@ -2260,6 +2317,25 @@ TEST(FindReferences, IncludeOverrides) {
checkFindRefs(Test, /*UseIndex=*/true);
}

TEST(FindReferences, IncludeOverridesObjC) {
llvm::StringRef Test =
R"objc(
@interface Base
- (void)$decl(Base)[[f^unc]];
@end
@interface Derived : Base
- (void)$overridedecl(Derived::func)[[func]];
@end
@implementation Derived
- (void)$overridedef[[func]] {}
@end
void test(Derived *derived, Base *base) {
[derived func]; // No references to the overrides.
[base $(test)[[func]]];
})objc";
checkFindRefs(Test, /*UseIndex=*/true);
}

TEST(FindReferences, RefsToBaseMethod) {
llvm::StringRef Test =
R"cpp(
Expand Down