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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks pretty good, I think there are a couple of adjustments...



================
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:
> 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?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:160
 
-// Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
-// in a header file, in which case clangd would prefer to use ND as a canonical
-// declaration.
+// Checks whether \p ND is a preferred declaration to clangd. Typically this
+// means preferring true declarations over forward declarations.
----------------
"preferred" declaration is just the method name, and "true" declaration doesn't seem to clarify much.

For this overview paragraph I'd suggest: Checks whether \p ND is a good candidate to be the *canonical* declaration of its symbol (e.g. a go-to-declaration target). This overrides the default of using Clang's canonical declaration, which is the first in the TU.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:163
+//
+// For C++:
+// If \p ND is a definition of a TagDecl (class/struct/enum/union) in a header,
----------------
I'm not sure the for C++/for ObjC split in the doc makes sense - these special cases are more to do with the node types than the underlying languages (which can overlap).


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:186
+//
+// For example, Objective-C classes can have three types of
+// declarations, all with the same USR:
----------------
this isn't really an example, it's the only case. I think it can be moved inside the function, above the case itself.

"We treat ObjCImplDecl as a definition but not a declaration - the corresponding ObjcInterfaceDecl is the declaration"


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:371
+  // TU, because in practice they are definitions.
+  if (shouldRecordDeclaration(*ND)) {
+    if (isPreferredDeclaration(*OriginalDecl, Roles))
----------------
Hmm, not sure about this. What if the @implementation is in a different file we're not indexing? Then we'll end up with no declaration.

Probably instead we should treat this as if it were a non-preferred non-canonical declaration whose canonical declaration is the @interface.
i.e. around where FOK is handled:
```
if (auto *OCID = llvm::dyn_cast<ObjCImplDecl>(D))
  D = OCID->getClassInterface(); // hmm, but maybe not if it's an implicit @interface
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:376
+    else
+      BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
+  }
----------------
you're now clobbering BasicSymbol every time, even if it was set and this isn't a preferred declaration


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:611
 
+  Code = R"cpp(
+    @interface Foo
----------------
can we add a case here and in symbolcollector testing the "implicit interface decl" case? (ObjCInterfaceDecl::isImplicitInterfaceDecl())


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501