[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

Mon Mar 4 09:57:27 PST 2019

nathawes added a comment.
Herald added a subscriber: ormris.

In D58814#1415607 <https://reviews.llvm.org/D58814#1415607>, @gribozavr wrote:

> > These references were added to support using the index data for symbol rename.
>
> Understood -- thank you for providing the context (the original commit that added this code didn't have the explanation why this reference was added).

That was my patch (via Argyrios), so sorry about that!

> However, my suggestion would be to still take this patch.  For symbol rename, my suggestion would be one of the following.
> 
> Option (1): Symbol rename is a complex operation that requires knowing many places where the class name appears.  So I think it is fair that it would need to navigate to all such declarations using the semantic index -- find the class, then find its constructors, destructor, out-of-line functions, find derived classes, then find `using` declarations in derived classes, find calls to constructors, destructor etc.  I don't think it is not the job of the core indexer API to hardcode all of these places as a "reference to the class".  Different clients require different navigation to related symbols, and hardcoding all such navigation patterns in the indexer would create a messy index that is difficult to work with, since you'd have to filter out lots of stuff.  Not to even mention the index size.

I feel like the complexity you mention here is what makes it worthwhile to expose all these locations as occurrences of the type, and I'm fairly sure we're actually already doing that in all these locations (unless I've missed other patches removing them). Pushing this work onto all clients that want to provide automatic global rename functionality or to support users manually renaming by including these in their find-references results seems like a poorer outcome from a code sharing/maintenance point of view than continuing to provide it and requiring clients that don't want it to check the SymbolRole bits inside their IndexDataConsumer's handleDeclOccurrence (`if (Roles & SymbolRole::NameReference) return true;`). I don't think the index size is concerning; clients don't have to store these occurrences if they don't care about them.

> Option (2): A client that wants to hardcode all navigation patterns in the index can still do this -- in the `IndexDataConsumer`, it is possible to handle the `ConstructorDecl` as a reference to the class, if desired.  The flow of the indexing information becomes more clear in my opinion: when there is a constructor decl in the source, the `IndexDataConsumer` gets one `ConstructorDecl`. Then the specific implementation of `IndexDataConsumer` decides to treat it also as a class reference, because it wants to support a specific approach to performing symbol renaming.

I think the downside here is again the maintenance burden. This code would probably live downstream somewhere so whenever changes happen upstream that affect the code deriving these extra occurrences in clients that want it, or introduce new types of locations that those clients don't handle and miss, there's more duplicated effort updating/hunting down bugs. I also personally see these as legitimate occurrences of the type rather than hardcoded navigation patterns (see below), even though rename was the motivation here.

> As is, the indexing information is misleading and surprising.  When we see a constructor decl or a constructor call, there is no reference to the class at that point -- there is a reference to a constructor.  I think index is supposed to expose semantic information, not just that there's a token in the source code that has the same spelling as the class name.
> 
>> could we instead distinguish these cases with a more specific SymbolRole, e.g. NameReference as opposed to Reference, and filter them based on that instead?
> 
> It feels like a workaround to me, that also pushes the fix to the clients of the indexing API. The core of the problem still exists: the indexing information is surprising, and requires extra work on the client to make it not surprising (filtering out NameReferences).

I agree that most people think of it as purely being the constructor name, but I don't think the semantic connection to the class is really as loose as 'a token with the same spelling' and I think it's reasonable to report it. To resolve a constructor call, the compiler looks for a *type* with that spelling. It's not as if the user chooses to name their constructor with the same name as the type – the name lookup finds the type. One interesting consequence of this is that the compiler seems to complain if you try to reference the constructor as opposed to the type when you call it, e.g.

  class A {
  public:
    A(int x) {}
  };

  int main() {
    auto x = A(2); // ok
    auto y = A::A(2); // error: qualified reference to 'A' is a constructor name rather than a type in this context
    return 0;
  }

If we report these occurrences with a SymbolRole of NameReference, is it really that misleading as to why they are reported? The SymbolRole bit set tells you the kind of occurrence you’re dealing with (Decl, Definition, Call, etc) and already has an Implicit role for occurrences that don’t actually appear in the source code for cases like the implicitly generated default constructors as mentioned above.  Some clients may find that surprising at first or undesirable for their particular use case, but it's still useful overall and ultimately understandable IMO.

Also taking a step back a bit, what were the specific concerns / original motivation to change the existing behavior? Is it just that these references don't match what you expected the index to model, or their contribution to idex size, or something else? I kind of just made my own assumptions and went with it, sorry.

Repository:
  rC Clang

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

https://reviews.llvm.org/D58814