[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D47623#1118810, @sammccall wrote:

> - friend decls that are not definitions should be ignored for indexing purposes


This is not generally true IIUC. A friend declaration can be a reference, a declaration or a definition.

  int foo(int);
  int bar(int, int);
  class ExistingFwdCls;
  
  class X {
    friend class ExistingFwdCls; // a reference and a declaration.
    friend class NewClass; // a new declaration.
    friend int foo(int); // a reference and a declaration.
    friend int baz(int, int, int); // a new declaration.
  };
  
  class Y {
    friend class ::ExistingFwdCls; // qualified => just a reference.
    friend int ::bar(int a, int b); // qualified => just a reference.
    friend int foo(int) { // a reference  and a definition
      return 100;
    }
  };

Note that friend functions with bodies are probably ok as canonical declaration, as they are often the only declaration, e.g.

  class Foo {
    friend bool operator < (Foo const& lhs, Foo const&lhs) {
     return false;
    }
  };



================
Comment at: clangd/index/SymbolCollector.cpp:293
   assert(CompletionAllocator && CompletionTUInfo);
+  // A declaration created for a friend declaration should not be used as the
+  // canonical declaration in the index.
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Maybe move this closer to `shouldFilterDecl()`? We have similar filters there.
> > That would also mean we properly add the reference counts for friend declarations that get a normal declaration after their usage later.
> I didn't put this filter there because I think it's a bit more special than those filters in `shouldFilterDecl`. We check the `OrigD` and we could potentially replace `D` with `OrigD`. We could change `shouldFilterDecl` to handle that, but I'm not sure if it's worth it.
> 
> Reference counting for friend declarations is actually a bit tricky as USRs of the generated declarations might be ambiguous.
> 
>  Consider the following exmaple:
> ```
> namespace a {                
> class A {};                  
> namespace b { class B { friend class A; };  }  // b                          
> } // a                            
> ```
> 
> I would expect the generated friend decl to be `a::A`, but it's actually `a::b::A`! So getting USR right is a bit tricky, and I think it's probably ok to ignore references in friend decls.
> 
> For reference, `[namespace.memdef]p3`:
> "If the name in a friend declaration is neither qualified nor a template-id and the declaration is a function or an elaborated-type-specifier, the lookup to determine whether the entity has been previously declared shall not consider any scopes outside the innermost enclosing namespace.
> Reference counting for friend declarations is actually a bit tricky as USRs of the generated declarations might be ambiguous.
This seems like an obvious bug in the USRs that we should fix. WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47623