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

[David Goldman via Phabricator via cfe-commits]

dgoldman marked 4 inline comments as done.
dgoldman added a comment.

In D83501#2146892 <https://reviews.llvm.org/D83501#2146892>, @sammccall wrote:

> In D83501#2144283 <https://reviews.llvm.org/D83501#2144283>, @dgoldman wrote:
>
> > Looking further into the indexing support, I've questions:
> >
> > - How `targetDecl` should behave for these objc container types
>
>
> (Hmm, targetDecl doesn't seem to canonicalize decls anywhere, which might be a bug, but it's unrelated to objc)
>
> The main thing is that AIUI @implementation is one thing, as far as clang's concerned, while @class+ at interface are another.
>  i.e. getCanonicalDecl() and USR generation both split them into equivalence classes {@implementation} and {@class, @interface}. Is that right?


I believe they share the same USR but besides that, yes that's correct.

> In that case the thing targetDecl needs to do is "jump" from the implementation to the interface, because we're pretending they're the same decl.
>  It doesn't need to jump from the @class to the @interface, semantic canonicalization doesn't belong there. (In fact it may need to do the opposite, since targetDecl might be relying on implicitly canonicalizing with clang's semantics).

SGTM, done.

>> - Similarly for SymbolCollector (guessing in `SymbolCollector::handleDeclOccurrence` ?)
> 
> Right - this is where most of the real canonicalization happens.
>  You need to ensure that:
> 
> - isPreferred is true for @interface, so that when we see both @class and @interface we'll take the latter as our preferred declaration
> - addDefinition gets called for @implementation, so that the definition location is set correctly. Also the SymbolID used in this case must be based on the USR of the interface, so we're considering them a single symbol. (Either addDeclaration should not be called in this case, or it should be called with the @implementation again somehow). I think that should be enough...

Gone ahead and implemented this. It seems OK. I think the only thing that might be missing here is class extensions:

  @interface Foo
  @end
  @interface Foo ()
  - (void)bar;
  @end
  @implementation Foo
  - (void)bar {}
  @end

I implemented goto-definition from Foo() --> Foo impl in Xrefs, on the symbolcollector side I don't think there's anything to do?



================
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)) {
----------------
sammccall wrote:
> dgoldman wrote:
> > sammccall wrote:
> > > dgoldman wrote:
> > > > sammccall wrote:
> > > > > 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)
> > > > Whoops, meant to comment here but it was lost. I'm not sure what you meant here. Should this be done here inside the for loop or in getDeclAtPosition?
> > > > 
> > > > Are you saying that given:
> > > > 
> > > > ```
> > > > @interface Foo // A
> > > > @end
> > > > @implementation Foo // B
> > > > @end
> > > > ```
> > > > B --> A here
> > > > 
> > > > and similarly
> > > > 
> > > > ```
> > > > @interface Foo // A
> > > > @end
> > > > @interface Foo (Ext) // B
> > > > @end
> > > > @implementation Foo (Ext) // C
> > > > @end
> > > > ```
> > > > B --> A
> > > > C --> B (and A? it's unclear how this should map over, e.g. maybe Foo loc --> A, Ext --> B)
> > > In the first example, selecting either A or B should yield one LocatedSymbol with {Decl=A, Def=B}. This shouldn't require any special-casing.
> > > 
> > > The second example is very similar to template specialization, with exceptions:
> > >  - there's always a Decl/Def pair you may want to navigate between, whereas in templates there rarely is, so we have ambiguity
> > >  - there's no AST like there is for template names and args, just a bag of tokens
> > > 
> > > I'd suggest, given `@interface Foo (Ext)`:
> > >  - we produce a LocatedSymbol with {Decl=@interface Foo(Ext), Def=@implementation  Foo(Ext)} - this is the default behavior
> > >  - if the cursor is exactly on the token `Foo`, we also produce a LocatedSymbol with {Decl=@interface Foo, Def=@implementation Foo} - this is similar to the template special case
> > >  - if the cursor is exactly on the token Ext... are categories explicitly/separately declared anywhere? I guess not. If they are, we could special case this too.
> > > And `@implementation Foo(Ext)` should behave in exactly the same way.
> > Trying this out now, two problems:
> > 
> > - getDeclAtPosition will call findTarget. Due to the changes above we map `ObjCCategoryImplDecl` to its interface. This is OK but then when we check for the loc it's obviously != to the impl's loc. Should I modify this to check the contents of the loc for equality?
> > 
> > - We call `getDeclAtPosition` only looking for DeclRelation::TemplatePattern | DeclRelation::Alias, but this is actually a DeclRelation::Underlying. If I don't add that we filter out the ObjCCategoryImplDecl. If I add it we get a failure in LocateSymbol.TemplateTypedef (we now find another symbol, not sure what is intended here)
> > 
> > 
> > getDeclAtPosition will call findTarget. Due to the changes above we map ObjCCategoryImplDecl to its interface. This is OK but then when we check for the loc it's obviously != to the impl's loc. Should I modify this to check the contents of the loc for equality?
> 
> Oh right... yes, this seems fine to me (for the ObjCContainerDecl subclasses only, and with a comment) 
> 
> > We call getDeclAtPosition only looking for DeclRelation::TemplatePattern | DeclRelation::Alias, but this is actually a DeclRelation::Underlying. If I don't add that we filter out the ObjCCategoryImplDecl. If I add it we get a failure in LocateSymbol.TemplateTypedef (we now find another symbol, not sure what is intended here)
> 
> I'm not sure what "this" in "this is actually a DeclRelation::Underlying" refers to. Do you have a failing testcase?
> if the cursor is exactly on the token Ext... are categories explicitly/separately declared anywhere? I guess not. If they are, we could special case this too.

Not besides what was mentioned above, so I think it's OK at the moment.

> I'm not sure what "this" in "this is actually a DeclRelation::Underlying" refers to. Do you have a failing testcase?

I meant that for ObjC, I used `DeclRelation::Underlying` so I have to include it to get the `ObjCCategoryImplDecl`. I guess I can later filter out the `DeclRelation::Underlying` unless it's for the ObjC case.

Failing test, I think only returning callback is intentional?

```
 LocateSymbol.TemplateTypedefs

Value of: locateSymbolAt(AST, T.point())
Expected: has 1 element that sym "callback"
  Actual: { function: 1:30-1:38 at file:///clangd-test/TestTU.cpp def=1:30-1:38 at file:///clangd-test/TestTU.cpp, callback: 2:29-2:37 at file:///clangd-test/TestTU.cpp }, which has 2 elements

Code:
    template <class T> struct function {};
    template <class T> using callback = function<T()>;

    c^allback<int> foo;
```






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501