[clangd][ObjC] Improve xrefs for protocols and classes

[dgoldman]

Summary

Previously clangd would jump to forward declarations for protocols
and classes instead of their definition/implementation.

[dgoldman]

clang-tools-extra/clangd/XRefs.cpp

88–92

Let me know if there's a better way to handle this multi-"definition" support

clang-tools-extra/clangd/unittests/XRefsTests.cpp

752

Figured this would be easier than copy + paste, LMK

[dgoldman]

clang-tools-extra/clangd/XRefs.cpp

321

Think it would make sense to special case ObjCInterfaceDecl here to get at both the interface definition + implementation if available?

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

83

This is a really useful comment, thanks!

88–92

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.

321

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)

clang-tools-extra/clangd/unittests/XRefsTests.cpp

753

Hm, I quite like this. You can probably also just do TU.ExtraArgs.push_back("-xobjective-c++") though, it probably won't break anything.

[dgoldman]

clang-tools-extra/clangd/XRefs.cpp

88–92

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).

[dgoldman]

Fixes for getDefinition/getCanonicalDecl for ObjC

[sammccall]

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)

clang-tools-extra/clangd/XRefs.cpp

88–92

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.

263

nit: real definition -> primary declaration

265

I'd consider getPreferredDecl here.
We may consider moving this to AST.h someday.

[dgoldman]

Add support for categories + fix tests

[dgoldman]

In D83501#2144144, @sammccall wrote:

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)

I'll look into the index changes as well in this change, just wanted to get the base functionality down for xrefs first. If it seems large I'll probably send a follow up diff.

Looking further into the indexing support, I've questions:

• How targetDecl should behave for these objc container types
• Similarly for SymbolCollector (guessing in SymbolCollector::handleDeclOccurrence ?)

clang-tools-extra/clangd/XRefs.cpp

321

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)

[dgoldman]

Swap to getPreferredDecl and improve targetDecl

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

356

I think this is what you had in mind for findTarget, right? I guess though to be safe we should check D for nullptr before reporting below?

[dgoldman]

Find target fixes + symbol collector test

[dgoldman]

Move up null check

[dgoldman]

rebase, phabricator keeps getting confused

[sammccall]

In 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+@interface are another.
i.e. getCanonicalDecl() and USR generation both split them into equivalence classes {@implementation} and {@class, @interface}. Is that right?

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).

• 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...

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

83

... and it's gone.
I think some comment is useful here, as this line is doing something subtly different than all the other lines - returning a decl that isn't equivalent to its input.

321

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.

[dgoldman]

clang-tools-extra/clangd/XRefs.cpp

83

I had moved it to getPreferredDecl, will add it back here as well with more info (need to special case one more special category: a class extension)

321

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)

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

321

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?

[dgoldman]

Fixes for ObjC in SymbolCollector as well as XRefs

• Make sure ObjC indexing doesn't use the forward decls and doesn't treat the impl as the canonical decl
• Don't treat ObjC protocol forward declarations as true declarations
• Also AddDecl for ObjC interfaces when the interface name in a category is selected

[dgoldman]

In D83501#2146892, @sammccall wrote:

In 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+@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?

clang-tools-extra/clangd/XRefs.cpp

321

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@file:///clangd-test/TestTU.cpp def=1:30-1:38@file:///clangd-test/TestTU.cpp, callback: 2:29-2:37@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;

[dgoldman]

Rebase

[dgoldman]

Minor lint fixes

[sammccall]

In 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...@end @implementation Foo...@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.

[sammccall]

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

clang-tools-extra/clangd/XRefs.cpp

321

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?

clang-tools-extra/clangd/index/SymbolCollector.cpp

159–160

"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.

162

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).

179

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"

369

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

374

you're now clobbering BasicSymbol every time, even if it was set and this isn't a preferred declaration

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

750

can we add a case here and in symbolcollector testing the "implicit interface decl" case? (ObjCInterfaceDecl::isImplicitInterfaceDecl())

[dgoldman]

In D83501#2153605, @sammccall wrote:

In 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...@end @implementation Foo...@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.

clang-tools-extra/clangd/XRefs.cpp

321

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?

clang-tools-extra/clangd/index/SymbolCollector.cpp

369

Ok, I've done something like this but it gets odd since we need to keep track that it is canonical. LMK if I should swap this to be later to replace OriginalDecl or isPreferredDeclaration.

[dgoldman]

SymbolCollector fixes + more tests

[dgoldman]

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

614

Here's the ClassExtension that I was talking about.

Ideally we can map each

Cat () --> @implementation Cat like I did in XRefs

But as you said the Cat () could be in a different file and I think it has a different USR.

See also https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html#//apple_ref/doc/uid/TP40011210-CH6-SW3

[sammccall]

In D83501#2154300, @dgoldman wrote:

In D83501#2153605, @sammccall wrote:

In 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...@end @implementation Foo...@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).

clang-tools-extra/clangd/XRefs.cpp

321

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).

[dgoldman]

In D83501#2157957, @sammccall wrote:

In D83501#2154300, @dgoldman wrote:

In D83501#2153605, @sammccall wrote:

In 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...@end @implementation Foo...@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?

clang-tools-extra/clangd/XRefs.cpp

321

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 =)

[dgoldman]

• Minor test and formatting fixes

[dgoldman]

Rebase

[sammccall]

clang-tools-extra/clangd/FindTarget.cpp

360

This just describes what the code is doing, say why instead?
// We treat ObjC{Interface,Implementation}Decl as if they were a decl/def pair.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

754

Hmm, do we want to use the @interface or @implementation for this case? The interface is implicit but probably still has a valid location.
Currently symbolcollector and findtarget do different things...

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

614

Ideally we can map each
Cat () --> @implementation Cat like I did in XRefs

I'm not sure there's anything that would ideally be done differently here.
The logic in xrefs is a special "go-to-definition" action - there's some ambiguity about what's being *targeted* by the user. But here there's no targeting going on, and there's no ambiguity about what's being *declared*.

The thing to test would be that we're emitting *refs* from @interface [[Cat]] () to catdecl.

clang-tools-extra/clangd/unittests/XRefsTests.cpp

720

maybe add a comment like // prefer interface definition over forward declaration

813

this seems to be copy/pasted from the test above.
Is there a reason this can't be part of the test above?

848

and again here

Desire to split these tables up into named tests is something we want to address somehow, but we don't have a good answer right now and it's important for maintenance that the logic/annotation conventions don't diverge across different tests that could be the same.

[sammccall]

(Sorry this has been pending a while - I think it's basically there. Only things we really need to address to land this is have a consistent view of what the canonical decl is for the no-@interface case, and avoid too much duplication of mechanisms in the tests)

[dgoldman]

• More fixes for review

[dgoldman]

In D83501#2173534, @sammccall wrote:

(Sorry this has been pending a while - I think it's basically there. Only things we really need to address to land this is have a consistent view of what the canonical decl is for the no-@interface case, and avoid too much duplication of mechanisms in the tests)

No problem, thanks for the review

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

754

Good catch, don't think this is a common case but yeah I think the impl makes more sense then. swapped over

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

614

Hmm, it looks like at the moment it either shares the same QName or doesn't have one. This might be good to look into a follow up patch?

clang-tools-extra/clangd/unittests/XRefsTests.cpp

813

I could merge them but I figured it would be better to separate tests with multi def/decls from those with just one. WDYT?

848

This one is split because you can't annotate one symbol with multiple annotations. I can instead make this a regular non generic test like the following, WDYT?

@interface $interfacedecl[[Cat]]
@end
@interface $classextensiondecl[[Ca^t]] ()
- (void)meow;
@end
@implementation $implementationdecl[[Cat]]
- (void)meow {}
@end

[dgoldman]

Rebase + lint fix

[sammccall]

Ugh, I forgot to hit submit and went on vacation :-\ Really sorry again.

As much as we can simplify/unify the tests that helps, but let's not block on this anymore, up to you.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

614

Sorry, I don't know what you mean about QName. USR?

Yeah, fine to defer out of this patch. This is just "does find references on an @interface find extensions of that interface".

clang-tools-extra/clangd/unittests/XRefsTests.cpp

813

I think it would be better to split each decl/def pair into its own testcase, and combine with the above (even if it means a bit of duplication between test cases)

848

This one is split because you can't annotate one symbol with multiple annotations

Not sure I quite see what you mean here, but $foo[[$bar[[symbol]]]] should work.

Anyway, fine if you want to leave this one separate. I'd avoid the tests array+loop if there's just one.

[dgoldman]

• Final touches

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

614

QualifiedName, will follow up later

clang-tools-extra/clangd/unittests/XRefsTests.cpp

813

These tests are odd since one point (^) can trigger multiple decls, so I think it's worth keeping separate vs. the other tests which assume 1 decl/def pair.

848

🤦 Ah thanks, that works!

clang-tools-extra/clangd/FindTarget.cpp

void add(const Decl *Dcl, RelSet Flags) {
     } else if (const UsingShadowDecl *USD = dyn_cast<UsingShadowDecl>(D)) {
       // Include the using decl, but don't traverse it. This may end up
       // including *all* shadows, which we don't want.
       report(USD->getUsingDecl(), Flags | Rel::Alias);
       // Shadow decls are synthetic and not themselves interesting.
       // Record the underlying decl instead, if allowed.
       D = USD->getTargetDecl();
       Flags |= Rel::Underlying; // continue with the underlying decl.
     } else if (const auto *DG = dyn_cast<CXXDeductionGuideDecl>(D)) {
       D = DG->getDeducedTemplate();
+    } else if (const ObjCImplementationDecl *IID =
+                   dyn_cast<ObjCImplementationDecl>(D)) {
+      // Treat ObjC{Interface,Implementation}Decl as if they were a decl/def
+      // pair as long as the interface isn't implicit.
+      if (const auto *CID = IID->getClassInterface())
+        if (const auto *DD = CID->getDefinition())
+          if (!DD->isImplicitInterfaceDecl())
+            D = DD;
+    } else if (const ObjCCategoryImplDecl *CID =
+                   dyn_cast<ObjCCategoryImplDecl>(D)) {
+      // Treat ObjC{Category,CategoryImpl}Decl as if they were a decl/def pair.
+      D = CID->getCategoryDecl();
     }
+    if (!D)
+      return;
 
     if (const Decl *Pat = getTemplatePattern(D)) {
       assert(Pat != D);
       add(Pat, Flags | Rel::TemplatePattern);
       // Now continue with the instantiation.
       Flags |= Rel::TemplateInstantiation;
     }
 

clang-tools-extra/clangd/XRefs.cpp

const NamedDecl *getDefinition(const NamedDecl *D) {
   assert(D);
   // Decl has one definition that we can find.
   if (const auto *TD = dyn_cast<TagDecl>(D))
     return TD->getDefinition();
   if (const auto *VD = dyn_cast<VarDecl>(D))
     return VD->getDefinition();
   if (const auto *FD = dyn_cast<FunctionDecl>(D))
     return FD->getDefinition();
+  // Objective-C classes can have three types of declarations:
+  //
+  // - forward declaration: @class MyClass;
+  // - true declaration (interface definition): @interface MyClass ... @end
+  // - true definition (implementation): @implementation MyClass ... @end
+  //
+  // Objective-C categories are extensions are on classes:
+  //
+  // - declaration: @interface MyClass (Ext) ... @end
+  // - definition: @implementation MyClass (Ext) ... @end
+  //
+  // With one special case, a class extension, which is normally used to keep
+  // some declarations internal to a file without exposing them in a header.
+  //
+  // - class extension declaration: @interface MyClass () ... @end
+  // - which really links to class definition: @implementation MyClass ... @end
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+    return ID->getImplementation();
+  if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D)) {
+    if (CD->IsClassExtension()) {
+      if (const auto *ID = CD->getClassInterface())
+        return ID->getImplementation();
+      return nullptr;
+    }
+    return CD->getImplementation();
+  }
   // Only a single declaration is allowed.
   if (isa<ValueDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
       isa<TemplateTemplateParmDecl>(D)) // except cases above
     return D;
   // Multiple definitions are allowed.
   return nullptr; // except cases above
 }
 
if (auto Loc =
       Macro.PreferredDeclaration = *Loc;
       Macro.Definition = Loc;
       return Macro;
     }
   }
   return llvm::None;
 }
 
+// A wrapper around `Decl::getCanonicalDecl` to support cases where Clang's
+// definition of a canonical declaration doesn't match up to what a programmer
+// would expect. For example, Objective-C classes can have three types of
+// declarations:
+//
+// - forward declaration(s): @class MyClass;
+// - true declaration (interface definition): @interface MyClass ... @end
+// - true definition (implementation): @implementation MyClass ... @end
+//
+// Clang will consider the forward declaration to be the canonical declaration
+// because it is first. We actually want the class definition if it is
+// available since that is what a programmer would consider the primary
+// declaration to be.
+const NamedDecl *getPreferredDecl(const NamedDecl *D) {
+  // FIXME: Canonical declarations of some symbols might refer to built-in
+  // decls with possibly-invalid source locations (e.g. global new operator).
+  // In such cases we should pick up a redecl with valid source location
+  // instead of failing.
+  D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+
+  // Prefer Objective-C class/protocol definitions over the forward declaration.
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+    if (const auto *DefinitionID = ID->getDefinition())
+      return DefinitionID;
+  if (const auto *PD = dyn_cast<ObjCProtocolDecl>(D))
+    if (const auto *DefinitionID = PD->getDefinition())
+      return DefinitionID;
+
+  return D;
+}
+
 // Decls are more complicated.
 // The AST contains at least a declaration, maybe a definition.
 // These are up-to-date, and so generally preferred over index results.
 // We perform a single batch index lookup to find additional definitions.
 std::vector<LocatedSymbol>
 locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
                   ParsedAST &AST, llvm::StringRef MainFilePath,
                   const SymbolIndex *Index, ASTNodeKind *NodeKind) {
   const SourceManager &SM = AST.getSourceManager();
   // Results follow the order of Symbols.Decls.
   std::vector<LocatedSymbol> Result;
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   auto AddResultDecl = [&](const NamedDecl *D) {
-    // FIXME: Canonical declarations of some symbols might refer to built-in
-    // decls with possibly-invalid source locations (e.g. global new operator).
-    // In such cases we should pick up a redecl with valid source location
-    // instead of failing.
-    D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+    D = getPreferredDecl(D);
     auto Loc =
         makeLocation(AST.getASTContext(), nameLocation(*D, SM), MainFilePath);
     if (!Loc)
       return;
 
     Result.emplace_back();
     Result.back().Name = printName(AST.getASTContext(), *D);
     Result.back().PreferredDeclaration = *Loc;
     if (const NamedDecl *Def = getDefinition(D))
       Result.back().Definition = makeLocation(
           AST.getASTContext(), nameLocation(*Def, SM), MainFilePath);
 
     // Record SymbolID for index lookup later.
     if (auto ID = getSymbolID(D))
       ResultIndex[*ID] = Result.size() - 1;
   };
 
   // Emit all symbol locations (declaration or definition) from AST.
   DeclRelationSet Relations =
       DeclRelation::TemplatePattern | DeclRelation::Alias;
   for (const NamedDecl *D :
        getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
     // Special case: void foo() ^override: jump to the overridden method.
     if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
       const InheritableAttr *Attr = D->getAttr<OverrideAttr>();
       if (!Attr)
         Attr = D->getAttr<FinalAttr>();
       if (Attr && TouchedIdentifier &&
           SM.getSpellingLoc(Attr->getLocation()) ==
               TouchedIdentifier->location()) {
         // We may be overridding multiple methods - offer them all.
if (llvm::isa<UsingDecl>(D)) {
       // all overload candidates, we only want the targeted one if the cursor is
       // on an using-alias usage, workround it with getDeclAtPosition.
       llvm::for_each(
           getDeclAtPosition(AST, CurLoc, DeclRelation::Underlying, NodeKind),
           [&](const NamedDecl *UD) { AddResultDecl(UD); });
       continue;
     }
 
+    // Special case: if the class name is selected, also map Objective-C
+    // categories and category implementations back to their class interface.
+    //
+    // Since `TouchedIdentifier` might refer to the `ObjCCategoryImplDecl`
+    // instead of the `ObjCCategoryDecl` we intentionally check the contents
+    // of the locs when checking for class name equivalence.
+    if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D))
+      if (const auto *ID = CD->getClassInterface())
+        if (TouchedIdentifier &&
+            (CD->getLocation() == TouchedIdentifier->location() ||
+             ID->getName() == TouchedIdentifier->text(SM)))
+          AddResultDecl(ID);
+
     // Otherwise the target declaration is the right one.
     AddResultDecl(D);
   }
 
   // Now query the index for all Symbol IDs we found in the AST.
   if (Index && !ResultIndex.empty()) {
     LookupRequest QueryRequest;
     for (auto It : ResultIndex)

clang-tools-extra/clangd/index/SymbolCollector.cpp

 #include "SourceCode.h"
 #include "SymbolLocation.h"
 #include "URI.h"
 #include "index/SymbolID.h"
 #include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
getTokenLocation(SourceLocation TokLoc, const SourceManager &SM,
   SymbolLocation Result;
   Result.FileURI = FileURIStorage.c_str();
   auto Range = getTokenRange(TokLoc, SM, LangOpts);
   Result.Start = Range.first;
   Result.End = Range.second;
 
   return Result;
 }
 
-// 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.
-// FIXME: handle symbol types that are not TagDecl (e.g. functions), if using
-// the first seen declaration as canonical declaration is not a good enough
-// heuristic.
+// 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.
+//
+// Example: preferring a class declaration over its forward declaration.
 bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
   const auto &SM = ND.getASTContext().getSourceManager();
+  if (isa<TagDecl>(ND))
     return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
-         isa<TagDecl>(&ND) && !isInsideMainFile(ND.getLocation(), SM);
+           !isInsideMainFile(ND.getLocation(), SM);
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(&ND))
+    return ID->isThisDeclarationADefinition();
+  if (const auto *PD = dyn_cast<ObjCProtocolDecl>(&ND))
+    return PD->isThisDeclarationADefinition();
+  return false;
 }
 
 RefKind toRefKind(index::SymbolRoleSet Roles, bool Spelled = false) {
   RefKind Result = RefKind::Unknown;
   if (Roles & static_cast<unsigned>(index::SymbolRole::Declaration))
     Result |= RefKind::Declaration;
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
     Result |= RefKind::Definition;
   if (Roles & static_cast<unsigned>(index::SymbolRole::Reference))
     Result |= RefKind::Reference;
   if (Spelled)
     Result |= RefKind::Spelled;
   return Result;
if ((ASTNode.OrigD->getFriendObjectKind() !=
        Decl::FriendObjectKind::FOK_None) &&
       !(Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
   // A declaration created for a friend declaration should not be used as the
   // canonical declaration in the index. Use OrigD instead, unless we've already
   // picked a replacement for D
   if (D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None)
     D = CanonicalDecls.try_emplace(D, ASTNode.OrigD).first->second;
+  // Flag to mark that D should be considered canonical meaning its declaration
+  // will override any previous declaration for the Symbol.
+  bool DeclIsCanonical = false;
+  // Avoid treating ObjCImplementationDecl as a canonical declaration if it has
+  // a corresponding non-implicit and non-forward declared ObjcInterfaceDecl.
+  if (const auto *IID = dyn_cast<ObjCImplementationDecl>(D)) {
+    DeclIsCanonical = true;
+    if (const auto *CID = IID->getClassInterface())
+      if (const auto *DD = CID->getDefinition())
+        if (!DD->isImplicitInterfaceDecl())
+          D = DD;
+  }
+  // Avoid treating ObjCCategoryImplDecl as a canonical declaration in favor of
+  // its ObjCCategoryDecl if it has one.
+  if (const auto *CID = dyn_cast<ObjCCategoryImplDecl>(D)) {
+    DeclIsCanonical = true;
+    if (const auto *CD = CID->getCategoryDecl())
+      D = CD;
+  }
   const NamedDecl *ND = dyn_cast<NamedDecl>(D);
   if (!ND)
     return true;
 
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto &SM = ASTCtx->getSourceManager();
   if (Opts.CountReferences &&
bool SymbolCollector::handleDeclOccurrence(
   // FIXME: ObjCPropertyDecl are not properly indexed here:
   // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
   // not a NamedDecl.
   auto *OriginalDecl = dyn_cast<NamedDecl>(ASTNode.OrigD);
   if (!OriginalDecl)
     return true;
 
   const Symbol *BasicSymbol = Symbols.find(*ID);
-  if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-    BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
-  else if (isPreferredDeclaration(*OriginalDecl, Roles))
-    // If OriginalDecl is preferred, replace the existing canonical
+  if (isPreferredDeclaration(*OriginalDecl, Roles))
+    // If OriginalDecl is preferred, replace/create the existing canonical
     // declaration (e.g. a class forward declaration). There should be at most
     // one duplicate as we expect to see only one preferred declaration per
     // TU, because in practice they are definitions.
     BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
+  else if (!BasicSymbol || DeclIsCanonical)
+    BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
 
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
     addDefinition(*OriginalDecl, *BasicSymbol);
 
   return true;
 }
 
 void SymbolCollector::handleMacros(const MainFileMacros &MacroRefsToIndex) {
   assert(PP.get());
   const auto &SM = PP->getSourceManager();
   const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
   assert(MainFileEntry);
 
   const auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts);
   // Add macro references.
   for (const auto &IDToRefs : MacroRefsToIndex.MacroRefs) {

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

TEST_F(TargetDeclTest, ObjC) {
 
   Code = R"cpp(
     @interface Foo
     @end
     void test([[Foo]] *p);
   )cpp";
   EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface Foo");
 
+  Code = R"cpp(// Don't consider implicit interface as the target.
+    @implementation [[Implicit]]
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCImplementationDecl", "@implementation Implicit");
+
+  Code = R"cpp(
+    @interface Foo
+    @end
+    @implementation [[Foo]]
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCImplementationDecl", "@interface Foo");
+
+  Code = R"cpp(
+    @interface Foo
+    @end
+    @interface Foo (Ext)
+    @end
+    @implementation [[Foo]] (Ext)
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)");
+
   Code = R"cpp(
     @protocol Foo
     @end
     void test([[id<Foo>]] p);
   )cpp";
   EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo");
 
   Code = R"cpp(

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

TEST_F(SymbolCollectorTest, ObjCPropertyImpl) {
   TestFileName = testPath("test.m");
   runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
   EXPECT_THAT(Symbols, Contains(QName("Container")));
   EXPECT_THAT(Symbols, Contains(QName("Container::magic")));
   // FIXME: Results also contain Container::_magic on some platforms.
   //        Figure out why it's platform-dependent.
 }
 
+TEST_F(SymbolCollectorTest, ObjCLocations) {
+  Annotations Header(R"(
+    // Declared in header, defined in main.
+    @interface $dogdecl[[Dog]]
+    @end
+    @interface $fluffydecl[[Dog]] (Fluffy)
+    @end
+  )");
+  Annotations Main(R"(
+    @interface Dog ()
+    @end
+    @implementation $dogdef[[Dog]]
+    @end
+    @implementation $fluffydef[[Dog]] (Fluffy)
+    @end
+    // Category with no declaration (only implementation).
+    @implementation $ruff[[Dog]] (Ruff)
+    @end
+    // Implicitly defined interface.
+    @implementation $catdog[[CatDog]]
+    @end
+  )");
+  runSymbolCollector(Header.code(), Main.code(),
+                     {"-xobjective-c++", "-Wno-objc-root-class"});
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("Dog"), DeclRange(Header.range("dogdecl")),
+                        DefRange(Main.range("dogdef"))),
+                  AllOf(QName("Fluffy"), DeclRange(Header.range("fluffydecl")),
+                        DefRange(Main.range("fluffydef"))),
+                  AllOf(QName("CatDog"), DeclRange(Main.range("catdog")),
+                        DefRange(Main.range("catdog"))),
+                  AllOf(QName("Ruff"), DeclRange(Main.range("ruff")),
+                        DefRange(Main.range("ruff")))));
+}
+
+TEST_F(SymbolCollectorTest, ObjCForwardDecls) {
+  Annotations Header(R"(
+    // Forward declared in header, declared and defined in main.
+    @protocol Barker;
+    @class Dog;
+    // Never fully declared so Clang latches onto this decl.
+    @class $catdogdecl[[CatDog]];
+  )");
+  Annotations Main(R"(
+    @protocol $barkerdecl[[Barker]]
+    - (void)woof;
+    @end
+    @interface $dogdecl[[Dog]]<Barker>
+    - (void)woof;
+    @end
+    @implementation $dogdef[[Dog]]
+    - (void)woof {}
+    @end
+    @implementation $catdogdef[[CatDog]]
+    @end
+  )");
+  runSymbolCollector(Header.code(), Main.code(),
+                     {"-xobjective-c++", "-Wno-objc-root-class"});
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("CatDog"), DeclRange(Header.range("catdogdecl")),
+                        DefRange(Main.range("catdogdef"))),
+                  AllOf(QName("Dog"), DeclRange(Main.range("dogdecl")),
+                        DefRange(Main.range("dogdef"))),
+                  AllOf(QName("Barker"), DeclRange(Main.range("barkerdecl"))),
+                  QName("Barker::woof"), QName("Dog::woof")));
+}
+
+TEST_F(SymbolCollectorTest, ObjCClassExtensions) {
+  Annotations Header(R"(
+    @interface $catdecl[[Cat]]
+    @end
+  )");
+  Annotations Main(R"(
+    @interface Cat ()
+    - (void)meow;
+    @end
+    @interface Cat ()
+    - (void)pur;
+    @end
+  )");
+  runSymbolCollector(Header.code(), Main.code(),
+                     {"-xobjective-c++", "-Wno-objc-root-class"});
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("Cat"), DeclRange(Header.range("catdecl"))),
+                  QName("Cat::meow"), QName("Cat::pur")));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
     // Declared in header, defined in main.
     extern int $xdecl[[X]];
     class $clsdecl[[Cls]];
     void $printdecl[[print]]();
 
     // Declared in header, defined nowhere.

clang-tools-extra/clangd/unittests/XRefsTests.cpp

const char *Tests[] = {
       )cpp",
 
       R"cpp(// Heuristic resolution of dependent enumerator
         template <typename T>
         struct Foo {
           enum class E { [[A]], B };
           E e = E::A^;
         };
-      )cpp"};
+      )cpp",
+
+      R"objc(
+        @protocol Dog;
+        @protocol $decl[[Dog]]
+        - (void)bark;
+        @end
+        id<Do^g> getDoggo() {
+          return 0;
+        }
+      )objc",
+
+      R"objc(
+        @interface Cat
+        @end
+        @implementation Cat
+        @end
+        @interface $decl[[Cat]] (Exte^nsion)
+        - (void)meow;
+        @end
+        @implementation $def[[Cat]] (Extension)
+        - (void)meow {}
+        @end
+      )objc",
+
+      R"objc(
+        @class $decl[[Foo]];
+        Fo^o * getFoo() {
+          return 0;
+        }
+      )objc",
+
+      R"objc(// Prefer interface definition over forward declaration
+        @class Foo;
+        @interface $decl[[Foo]]
+        @end
+        Fo^o * getFoo() {
+          return 0;
+        }
+      )objc",
+
+      R"objc(
+        @class Foo;
+        @interface $decl[[Foo]]
+        @end
+        @implementation $def[[Foo]]
+        @end
+        Fo^o * getFoo() {
+          return 0;
+        }
+      )objc"};
   for (const char *Test : Tests) {
     Annotations T(Test);
     llvm::Optional<Range> WantDecl;
     llvm::Optional<Range> WantDef;
     if (!T.ranges().empty())
       WantDecl = WantDef = T.range();
     if (!T.ranges("decl").empty())
       WantDecl = T.range("decl");
     if (!T.ranges("def").empty())
       WantDef = T.range("def");
 
     TestTU TU;
     TU.Code = std::string(T.code());
 
     // FIXME: Auto-completion in a template requires disabling delayed template
     // parsing.
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
+    TU.ExtraArgs.push_back("-xobjective-c++");
 
     auto AST = TU.build();
     auto Results = locateSymbolAt(AST, T.point());
 
     if (!WantDecl) {
       EXPECT_THAT(Results, IsEmpty()) << Test;
     } else {
       ASSERT_THAT(Results, ::testing::SizeIs(1)) << Test;
       EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
       llvm::Optional<Range> GotDef;
       if (Results[0].Definition)
         GotDef = Results[0].Definition->range;
       EXPECT_EQ(WantDef, GotDef) << Test;
     }
   }
 }
 
+TEST(LocateSymbol, AllMulti) {
+  // Ranges in tests:
+  //   $declN is the declaration location
+  //   $defN is the definition location (if absent, symbol has no definition)
+  //
+  // NOTE:
+  //   N starts at 0.
+  //   Due to limitations in clang's Annotations, you can't annotate the same
+  //   text with multiple ranges, e.g. `$def0$def1[[Foo]]` is invalid.
+  struct ExpectedRanges {
+    Range WantDecl;
+    llvm::Optional<Range> WantDef;
+  };
+  const char *Tests[] = {
+      R"objc(
+      @interface $decl0[[Cat]]
+      @end
+      @implementation $def0[[Cat]]
+      @end
+      @interface $decl1[[Ca^t]] (Extension)
+      - (void)meow;
+      @end
+      @implementation $def1[[Cat]] (Extension)
+      - (void)meow {}
+      @end
+    )objc",
+
+      R"objc(
+      @interface $decl0[[Cat]]
+      @end
+      @implementation $def0[[Cat]]
+      @end
+      @interface $decl1[[Cat]] (Extension)
+      - (void)meow;
+      @end
+      @implementation $def1[[Ca^t]] (Extension)
+      - (void)meow {}
+      @end
+    )objc",
+  };
+  for (const char *Test : Tests) {
+    Annotations T(Test);
+    std::vector<ExpectedRanges> Ranges;
+    for (int Idx = 0; true; Idx++) {
+      bool HasDecl = !T.ranges("decl" + std::to_string(Idx)).empty();
+      bool HasDef = !T.ranges("def" + std::to_string(Idx)).empty();
+      if (!HasDecl && !HasDef)
+        break;
+      ExpectedRanges Range;
+      if (HasDecl)
+        Range.WantDecl = T.range("decl" + std::to_string(Idx));
+      if (HasDef)
+        Range.WantDef = T.range("def" + std::to_string(Idx));
+      Ranges.push_back(Range);
+    }
+
+    TestTU TU;
+    TU.Code = std::string(T.code());
+    TU.ExtraArgs.push_back("-xobjective-c++");
+
+    auto AST = TU.build();
+    auto Results = locateSymbolAt(AST, T.point());
+
+    ASSERT_THAT(Results, ::testing::SizeIs(Ranges.size())) << Test;
+    for (size_t Idx = 0; Idx < Ranges.size(); Idx++) {
+      EXPECT_EQ(Results[Idx].PreferredDeclaration.range, Ranges[Idx].WantDecl)
+          << "($decl" << Idx << ")" << Test;
+      llvm::Optional<Range> GotDef;
+      if (Results[Idx].Definition)
+        GotDef = Results[Idx].Definition->range;
+      EXPECT_EQ(GotDef, Ranges[Idx].WantDef) << "($def" << Idx << ")" << Test;
+    }
+  }
+}
+
+TEST(LocateSymbol, MultipleDeclsWithSameDefinition) {
+  // Ranges in tests:
+  //   $def is the shared definition location
+  //   $declN is one of the declaration locations
+  //
+  // NOTE:
+  //   N starts at 0.
+  const char *Tests[] = {
+      R"objc(
+      @interface $decl0[[Cat]]
+      @end
+      @interface $decl1[[Ca^t]] ()
+      - (void)meow;
+      @end
+      @implementation $def[[Cat]]
+      - (void)meow {}
+      @end
+    )objc",
+  };
+  for (const char *Test : Tests) {
+    Annotations T(Test);
+    std::vector<Range> DeclRanges;
+    Range WantDef;
+    for (int Idx = 0; true; Idx++) {
+      bool HasDecl = !T.ranges("decl" + std::to_string(Idx)).empty();
+      if (!HasDecl)
+        break;
+      DeclRanges.push_back(T.range("decl" + std::to_string(Idx)));
+    }
+    WantDef = T.range("def");
+
+    TestTU TU;
+    TU.Code = std::string(T.code());
+    TU.ExtraArgs.push_back("-xobjective-c++");
+
+    auto AST = TU.build();
+    auto Results = locateSymbolAt(AST, T.point());
+
+    ASSERT_THAT(Results, ::testing::SizeIs(DeclRanges.size())) << Test;
+    for (size_t Idx = 0; Idx < DeclRanges.size(); Idx++) {
+      EXPECT_EQ(Results[Idx].PreferredDeclaration.range, DeclRanges[Idx])
+          << "($decl" << Idx << ")" << Test;
+      llvm::Optional<Range> GotDef;
+      if (Results[Idx].Definition)
+        GotDef = Results[Idx].Definition->range;
+      EXPECT_EQ(GotDef, WantDef) << "(def for $decl" << Idx << ")" << Test;
+    }
+  }
+}
+
 // LocateSymbol test cases that produce warnings.
 // These are separated out from All so that in All we can assert
 // that there are no diagnostics.
 TEST(LocateSymbol, Warnings) {
   const char *Tests[] = {
       R"cpp(// Field, GNU old-style field designator
         struct Foo { int [[x]]; };
         int main() {