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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I think without index changes this will still give the wrong answer for go-to-definition if the @implementation is in a different file.
Do you want to include those too or will that work ok in a separate patch?
(I'm not 100% sure of the interactions here, possible it'll seem a bit glitchy)



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:241
+// because it is first, but we actually want the class definition if it is
+// available since that is what a programmer would consider the real definition
+// to be.
----------------
nit: real definition -> primary declaration


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:243
+// to be.
+const NamedDecl *getNonStandardCanonicalDecl(const NamedDecl *D) {
+  D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
----------------
I'd consider getPreferredDecl here.
We may consider moving this to AST.h someday.


================
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:
> 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). 
> Clang seems to use the forward declaration as the canonical one when it exists

This is **super** confusing.

Clang has a concept of canonical declaration that's basically arbitrary: you pick one so that e.g. if you have canonical decls you can compare their pointers. It picks the first one seen, which is nice because it's stable over the life of the parse.

Clangd has a concept of canonical declaration that's not arbitrary: it's meant to be the declaration the user wants to see.

And yet, clangd's canonical is initialized to clang's canonical by default, because we've got to pick something.
There is some more logic (e.g. a class's canonical decl is its def, if that's in a header file) but it mostly exists in the indexer, so it's not really obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501