Skip to content

[clangd] Update XRefs to support overriden ObjC methods #127109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 19, 2025
Merged

[clangd] Update XRefs to support overriden ObjC methods #127109

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d335931
[clangd] Update XRefs to support overriden ObjC methods
DavidGoldman Feb 13, 2025
65f19b7
Run clang-format
DavidGoldman Feb 13, 2025
68fd059
Only jump to overrides when selecting the method; fix weird findImpl …
DavidGoldman Feb 14, 2025
e962112
Add Objective-C inheritance test to SymbolCollector
DavidGoldman Feb 14, 2025
4f88a70
Recursively find all ObjC overrides + add test
DavidGoldman Feb 18, 2025
478cd5a
Only give overrides on ObjC method defs
DavidGoldman Feb 19, 2025
13c7011
Check for definition first
DavidGoldman Feb 19, 2025
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
32 changes: 32 additions & 0 deletions clang-tools-extra/clangd/XRefs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,18 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
continue;
}
}
// Special case: 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.

what about the interaction in the other direction? i.e. we're on the protocol declaration and try to do a go-to-definition.

moreover this is going to make go-to-defn on references to a symbol quite chatty (now you'll have both the definition in implementation and definition in protocol as an alternative). I think we want to limit this to only when user triggered the interaction on the definition of the method and not references to it (similar to c++ code path above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking for that users can use go to implementation to find them.

And that's a good point - didn't think about that, updated.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking for that users can use go to implementation to find them.

i think using go-to-definition for such interactions is a more common workflow (also go-to-impl is not necessarily implemented by all editors). so I think there's merit in making sure that also works (though I won't block this patch on it, as it's not a regression. but I think it warrants at least a FIXME)

if (const auto *OMD = llvm::dyn_cast<ObjCMethodDecl>(D)) {
llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
OMD->getOverriddenMethods(Overrides);
for (const auto *Override : Overrides)
AddResultDecl(Override);
if (!Overrides.empty())
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 +1295,17 @@ 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;

llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
OMD->getOverriddenMethods(Overrides);
for (const auto *Override : Overrides)
IDs.insert(getSymbolID(Override));
} 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 +1461,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
76 changes: 76 additions & 0 deletions clang-tools-extra/clangd/unittests/XRefsTests.cpp
View file
Open in desktop
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