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

[David Goldman via Phabricator via cfe-commits]

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


================
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();
+  }
----------------
sammccall wrote:
> 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.
Thanks, that makes sense - I agree that B should be the actual canonical declaration for our case (even though Clang picks A) and C is the definition. I think that part was confusing me, since Clang seems to use the forward declaration as the canonical one when it exists, but it seems like that's intentional/OK and shouldn't change (e.g. for `ReferenceFinder` in the same file). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501