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

DavidGoldman · 2025-02-13T18:54:26Z

Support finding implementors of a protocol and discovering subclasses for ObjC interfaces via the implementations call
Add tests

- Also support finding implementators of a protocol - Also support discovering subclasses for ObjC interfaces via the implementations call - Add tests

llvmbot · 2025-02-13T18:54:59Z

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: David Goldman (DavidGoldman)

Changes

Support finding implementors of a protocol and discovering subclasses for ObjC interfaces via the implementations call
Add tests

Full diff: https://github.com/llvm/llvm-project/pull/127109.diff

2 Files Affected:

(modified) clang-tools-extra/clangd/XRefs.cpp (+32)
(modified) clang-tools-extra/clangd/unittests/XRefsTests.cpp (+75)

diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 1a23f6cca7756..095664a2391c6 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -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.
+    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.
@@ -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());
@@ -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));
+        }
       }
     }
 
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index 7d824d659ad2c..a310a51cf4cfb 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -411,6 +411,26 @@ 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]] {};
@@ -1834,6 +1854,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 {
@@ -1963,6 +2018,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;
@@ -2260,6 +2316,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(

github-actions · 2025-02-13T18:57:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kadircet

can you also add some tests to SymbolCollectorTests.cpp to make sure we capture overriddenby relationships relevant to your use cases?

index tests in XRefsTests.cpp uses rather dynamic-index hence can have different characteristics then you'd get for the static/global index

clang-tools-extra/clangd/XRefs.cpp

kadircet · 2025-02-14T09:50:36Z

clang-tools-extra/clangd/XRefs.cpp

+          llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
+          OMD->getOverriddenMethods(Overrides);
+          for (const auto *Override : Overrides)
+            OverriddenMethods.insert(getSymbolID(Override));


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

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

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 ?

kadircet · 2025-02-14T10:14:54Z

clang-tools-extra/clangd/XRefs.cpp

+    // Special case: an Objective-C method can override a parent class or
+    // protocol.


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).

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.

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)

…behavior

DavidGoldman · 2025-02-14T18:48:49Z

can you also add some tests to SymbolCollectorTests.cpp to make sure we capture overriddenby relationships relevant to your use cases?

index tests in XRefsTests.cpp uses rather dynamic-index hence can have different characteristics then you'd get for the static/global index

Done, added a simple inheritance test.

kadircet · 2025-02-17T09:22:23Z

clang-tools-extra/clangd/XRefs.cpp

+          llvm::SmallVector<const ObjCMethodDecl *, 4> Overrides;
+          OMD->getOverriddenMethods(Overrides);
+          for (const auto *Override : Overrides)
+            OverriddenMethods.insert(getSymbolID(Override));


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 ?

kadircet · 2025-02-17T09:26:41Z

clang-tools-extra/clangd/XRefs.cpp

+    // Special case: - (void)^method; should jump to all overrides. Note that an
+    // Objective-C method can override a parent class or protocol.


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.

kadircet · 2025-02-17T09:30:03Z

clang-tools-extra/clangd/XRefs.cpp

+    // Special case: an Objective-C method can override a parent class or
+    // protocol.


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)

DavidGoldman · 2025-02-18T19:55:45Z

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 ?

Done, you're right, so fixed that as well.

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)

Added a FIXME

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

kadircet

thanks!

clang-tools-extra/clangd/XRefs.cpp

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

[clangd] Update XRefs to support overriden ObjC methods

d335931

- Also support finding implementators of a protocol - Also support discovering subclasses for ObjC interfaces via the implementations call - Add tests

DavidGoldman requested a review from kadircet February 13, 2025 18:54

llvmbot added clang-tools-extra clangd labels Feb 13, 2025

DavidGoldman requested a review from HighCommander4 February 13, 2025 18:54

Run clang-format

65f19b7

kadircet requested changes Feb 14, 2025

View reviewed changes

DavidGoldman added 2 commits February 14, 2025 13:33

Only jump to overrides when selecting the method; fix weird findImpl …

68fd059

…behavior

Add Objective-C inheritance test to SymbolCollector

e962112

DavidGoldman requested a review from kadircet February 14, 2025 18:48

kadircet requested changes Feb 17, 2025

View reviewed changes

Recursively find all ObjC overrides + add test

4f88a70

kadircet reviewed Feb 19, 2025

View reviewed changes

clang-tools-extra/clangd/unittests/XRefsTests.cpp Show resolved Hide resolved

Only give overrides on ObjC method defs

478cd5a

DavidGoldman requested a review from kadircet February 19, 2025 15:43

kadircet approved these changes Feb 19, 2025

View reviewed changes

clang-tools-extra/clangd/XRefs.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clangd/unittests/XRefsTests.cpp Show resolved Hide resolved

Check for definition first

13c7011

DavidGoldman merged commit 1841bcd into llvm:main Feb 19, 2025
8 checks passed

joaosaffran mentioned this pull request Feb 26, 2025

[libc++] Set feature-test macro __cpp_lib_atomic_float (#127559) joaosaffran/llvm-project#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DavidGoldman commented Feb 13, 2025

llvmbot commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading

kadircet left a comment

kadircet Feb 14, 2025

DavidGoldman Feb 14, 2025

kadircet Feb 17, 2025

kadircet Feb 14, 2025

DavidGoldman Feb 14, 2025

kadircet Feb 17, 2025

DavidGoldman commented Feb 14, 2025

kadircet Feb 17, 2025

kadircet Feb 17, 2025

DavidGoldman Feb 18, 2025

kadircet Feb 17, 2025

DavidGoldman commented Feb 18, 2025

kadircet left a comment

		// Special case: an Objective-C method can override a parent class or
		// protocol.

		// Special case: - (void)^method; should jump to all overrides. Note that an
		// Objective-C method can override a parent class or protocol.

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

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

Conversation

DavidGoldman commented Feb 13, 2025

llvmbot commented Feb 13, 2025 • edited Loading

github-actions bot commented Feb 13, 2025 • edited Loading

kadircet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGoldman commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGoldman commented Feb 18, 2025

kadircet left a comment

Choose a reason for hiding this comment

llvmbot commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading