This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dgoldman on Jul 9 2020, 12:01 PM.

Details

Summary

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

Diff Detail

Event Timeline

dgoldman created this revision.Jul 9 2020, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 12:01 PM
dgoldman added inline comments.Jul 9 2020, 12:04 PM
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 added inline comments.Jul 9 2020, 12:31 PM
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 marked an inline comment as done.Jul 9 2020, 2:06 PM
sammccall added inline comments.
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 marked an inline comment as done.Jul 10 2020, 6:43 AM
dgoldman added inline comments.
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 updated this revision to Diff 277026.Jul 10 2020, 7:06 AM

Fixes for getDefinition/getCanonicalDecl for ObjC

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 updated this revision to Diff 277054.Jul 10 2020, 8:27 AM
dgoldman marked 2 inline comments as done.

Add support for categories + fix tests

dgoldman marked an inline comment as done.EditedJul 10 2020, 8:43 AM

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 updated this revision to Diff 277084.Jul 10 2020, 10:09 AM

Swap to getPreferredDecl and improve targetDecl

dgoldman marked 5 inline comments as done and an inline comment as not done.Jul 10 2020, 10:12 AM
dgoldman added inline comments.
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 updated this revision to Diff 277100.Jul 10 2020, 10:47 AM

Find target fixes + symbol collector test

dgoldman updated this revision to Diff 277103.Jul 10 2020, 10:58 AM

Move up null check

dgoldman updated this revision to Diff 277105.Jul 10 2020, 11:02 AM

rebase, phabricator keeps getting confused

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 added inline comments.Jul 13 2020, 3:35 AM
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 marked 2 inline comments as done.Jul 13 2020, 9:29 AM
dgoldman added inline comments.
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 added inline comments.Jul 13 2020, 9:51 AM
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 updated this revision to Diff 277582.Jul 13 2020, 3:09 PM
dgoldman marked an inline comment as done.

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 marked 4 inline comments as done.Jul 13 2020, 3:56 PM

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 updated this revision to Diff 277814.Jul 14 2020, 6:51 AM

Minor lint fixes

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.

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 marked 7 inline comments as done.Jul 15 2020, 2:00 PM

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 updated this revision to Diff 278310.Jul 15 2020, 2:32 PM
dgoldman marked an inline comment as done.

SymbolCollector fixes + more tests

dgoldman marked an inline comment as done.Jul 15 2020, 2:34 PM
dgoldman added inline comments.
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

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 marked 7 inline comments as done.Jul 17 2020, 6:21 AM

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 updated this revision to Diff 278752.Jul 17 2020, 6:22 AM
dgoldman marked an inline comment as done.
  • Minor test and formatting fixes
sammccall added inline comments.Jul 24 2020, 4:01 PM
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.

(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 updated this revision to Diff 280930.Jul 27 2020, 8:33 AM
dgoldman marked 7 inline comments as done.
  • More fixes for review

(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 updated this revision to Diff 284425.Aug 10 2020, 10:13 AM

Rebase + lint fix

sammccall accepted this revision.Aug 11 2020, 12:59 AM

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.

This revision is now accepted and ready to land.Aug 11 2020, 12:59 AM
dgoldman updated this revision to Diff 284788.Aug 11 2020, 9:34 AM
dgoldman marked 3 inline comments as done.
  • 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!

This revision was landed with ongoing or failed builds.Aug 11 2020, 9:37 AM
This revision was automatically updated to reflect the committed changes.