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

Fri Jul 17 06:21:35 PDT 2020

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

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

> In D83501#2154300 <https://reviews.llvm.org/D83501#2154300>, @dgoldman wrote:
>
> > In D83501#2153605 <https://reviews.llvm.org/D83501#2153605>, @sammccall wrote:
> >
> > > In D83501#2148671 <https://reviews.llvm.org/D83501#2148671>, @dgoldman wrote:
> > >
> > > > I implemented goto-definition from Foo() --> Foo impl in Xrefs, on the symbolcollector side I don't think there's anything to do?
> > >
> > >
> > > This can't be done purely in xrefs as the AST may not be available.
> > >
> > > A.m: `@class Foo; ^Foo x;` <-- go-to-definition at ^
> > >  B.m: `@interface Foo... at end @implementation Foo... at end`
> > >
> > > The index needs to know that for the USR associated with the @class (found by targetDecl), the canonical decl is the @interface in B and the definition is the @implementation in B.
> > >  So SymbolCollector needs to see it as a definition. **The tests seem to show it does already**, but it's not obvious why from the code. Do you know? Maybe it's the fact that they share a USR and thus a symbol ID. This is worth making explicit somewhere.
> >
> >
> > Think we're talking about different things. See `ObjCClassExtensions` in SymbolCollectorTests which I added, the idea is that the `Cat ()` can link to the implementation like I added to XRefs, but I'm not sure how this should be represented. As for @class -> @interface/@implementation those should have the same USR, yes.
>
>
> Oh, sorry I indeed missed the parens.
>  Yes, this is a go-to-def special case, SymbolCollector shouldn't need anything. (I'm assuming that a reference to the class is reported already - could add a test that the ref exists if you like).

if I understand this correctly I think `MultipleDeclsWithSameDefinition` already tests that, although it relies upon `locateASTReferent` 's `TouchedIdentifier` being the interface. Should it always be added instead?

================
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:
> > > > > > 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;
> > > > ```
> > > > 
> > > > 
> > > > 
> > > > 
> > > > I meant that for ObjC, I used DeclRelation::Underlying
> > > 
> > > `Underlying` isn't appropriate here - it's used for situations like typedefs where there's clearly two symbols (two names) and an asymmetrical synonym relation (e.g. int32 is a synonym for int, but not the other way around).
> > > Here we have a declaration/definition pair of a (what we're treating as) a single decl.
> > > 
> > > Can you just stop setting `Underlying` in FindTargets?
> > If I do that it'll be filtered out since it's not a DeclRelation::TemplatePattern | DeclRelation::Alias. Should I add a new DeclRelation for this?
> I think one of us is confused about what these bitmasks do :-)
>  - bits set on a particular decl *restrict* when that decl will be considered a target
>  - bits passed to targetDecl() *allow* decls with those bits to be returned
> 
> Not all targets found have a relation set, e.g. a DeclRefExpr to a variable doesn't have any of these bits. This means it's universal - it'll always be considered the target. 
> Bits are basically set when we have to make a choice of whether to return some decl (if we're looking at the std::string type, then the std::string typedef is a target if we want aliases).
Haha yeah, should have just tried it. Didn't realize the masking style in `targetDecl` which filters from `allTargetDecls` always includes 0 Relations sets (since it's a not a normal filter, it's inverting). It works =)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501