[PATCH] D83501: [clangd][ObjC] Improve xrefs for protocols and classes

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:83
+    return PD->getDefinition();
+  // Objective-C classes can have three types of declarations:
+  //
----------------
This is a really useful comment, thanks!


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:88-92
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D)) {
+    if (const auto *IMD = ID->getImplementation())
+      return IMD;
+    return ID->getDefinition();
+  }
----------------
dgoldman wrote:
> Let me know if there's a better way to handle this multi-"definition" support
I think there might be (what you've done seems reasonable in isolation but I'm not sure it'll yield the best behavior).

Consider this code:
```
int foo(); // A
int foo(); // B
int foo(){} // C
```
We have 3 declarations. A will be chosen as the canonical declaration (because it's first, though things get more complicated when the index is involved). C is the definition. So go-to-definition will land on C, and then triggering it again will take you to A, and then back to C. go-to-declaration is the opposite. B is basically just a reference for our purposes, we won't navigate you there (except by find-refs).

Now let's look at your example again:
```
@class MyClass; // A
@interface MyClass ... @end // B
@implementation MyClass ... @end // C
```
Thinking about where you might want to navigate to, A is certainly the least important of these, right?
It seems clear we want B to be considered the canonical declaration and C the definition.

So I think:
 - we should only return the implementation here if it exists, and otherwise nullptr rather than the inferface.
 - getCanonicalDecl in AddResultDecl should special case ObjCInterfaceDecl to get the definition (i.e. @interface) if possible
 - we need to convince other parts of clangd that these are the same symbol:
   - targetDecl should resolve any reference to an ObjCImplDecl to the corresponding ObjCInterfaceDecl instead
   - the indexer (`SymbolCollector`) also needs to handle this (the ObjCImplDecl should end up being the stored Definition under the ObjCInterfaceDecl's SymbolID)

Some code will only see the forward declaration and that's OK. The index's merge rules are that a candidate "canonical declaration" which has an associated definition is preferred over one that doesn't. Since the implementation can always see the interface, the interface will end up being the canonical declaration after merge.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:276
        getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
     // Special case: void foo() ^override: jump to the overridden method.
     if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
----------------
dgoldman wrote:
> Think it would make sense to special case ObjCInterfaceDecl here to get at both the interface definition + implementation if available?
Rather than returning both results, I think it's more consistent to return them as a declaration/definition pair.

(This means special-casing ObjCImplDecl in namedDecl or at least getDeclAsPosition, so you always end up with the ObjCInterfaceDecl instead)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:723
+    std::string ObjcPrefix = "//objc";
+    if (strncmp(Test, ObjcPrefix.c_str(), ObjcPrefix.size()) == 0) {
+      TU.Filename = "TestTU.m";
----------------
Hm, I quite like this. You can probably also just do TU.ExtraArgs.push_back("-xobjective-c++") though, it probably won't break anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83501/new/

https://reviews.llvm.org/D83501