[Possible regression?] Canonical decl of friend template decls in dependent contexts

Consider the following:

template<typename T>
struct C; // d1

template<typename T>
class D {
    static void f() {}

    template<typename>
    friend struct C; // d2
};

template<typename T>
struct C { }; // d3

int main() { }

Here there are 3 declarations corresponding to class template C (marked
as d1, d2 and d3).

In clang 3.9 getCanonicalDecl() for all 3 of them returns d1. Starting
from clang 4.0, d2->getCanonicalDecl() == d2. As far as I can tell, this
is caused by commit 291753 [1].

Is this intended? It looks to me, that, for example,
clang::declaresSameEntity(d1, d2) [2] will return a wrong result.

[1] https://llvm.org/viewvc/llvm-project?view=revision&revision=291753
[2]
https://clang.llvm.org/doxygen/namespaceclang.html#ad9d926b16adbdbc93705737b69d47cae

Consider the following:

template<typename T>
struct C; // d1

template<typename T>
class D {
     static void f() {}

     template<typename>
     friend struct C; // d2
};

template<typename T>
struct C { }; // d3

int main() { }

Here there are 3 declarations corresponding to class template C (marked
as d1, d2 and d3).

In clang 3.9 getCanonicalDecl() for all 3 of them returns d1. Starting
from clang 4.0, d2->getCanonicalDecl() == d2. As far as I can tell, this
is caused by commit 291753 [1].

That was intentional because keeping friend declarations in the redecl chain confuses the template instantiator. That's visible especially with modules.

Is this intended? It looks to me, that, for example,
clang::declaresSameEntity(d1, d2) [2] will return a wrong result.

Richard, would it be feasible to not add the fiend decl to the redecl chain but to still rewire it to the 'right' canonical decl. That way getMostRecentDecl() won't return d2 (in some cases) and declaresSameEntity(d1,d2) will return true?

Consider the following:

template
struct C; // d1

template
class D {
static void f() {}

template
friend struct C; // d2
};

template
struct C { }; // d3

int main() { }

Here there are 3 declarations corresponding to class template C (marked
as d1, d2 and d3).

In clang 3.9 getCanonicalDecl() for all 3 of them returns d1. Starting
from clang 4.0, d2->getCanonicalDecl() == d2. As far as I can tell, this
is caused by commit 291753 [1].

That was intentional because keeping friend declarations in the redecl chain confuses the template instantiator. That’s visible especially with modules.

Is this intended? It looks to me, that, for example,
clang::declaresSameEntity(d1, d2) [2] will return a wrong result.

Richard, would it be feasible to not add the fiend decl to the redecl chain but to still rewire it to the ‘right’ canonical decl. That way getMostRecentDecl() won’t return d2 (in some cases) and declaresSameEntity(d1,d2) will return true?

This would’ve been useful when I was implementing access control originally. There’s some pretty low-level complexity forced by the attempt to maintain a single chain of redeclarations. I’m thinking particularly of how friend declarations have to inherit the right IDNS from the original declaration because they replace it as the most recent declaration.

And in practice it’s possible for lookup to find an older declaration anyway (because of things like UsingShadowDecls).

John.

This would pretty badly break some of our invariants (eg, that walking over
PreviousDecl edges and then from the first decl to the MostRecentDecl will
eventually return you to the declaration where you started, all
declarations of an entity have the same semantic DeclContext, all
declarations have equivalent template parameter lists, ...). This would
bring back some of the problems we fixed when we removed these declarations
from the redecl chain (though, granted, not all of the, because walking the
redeclaration chain starting from somewhere other than the depedent-context
friend would never take you to the dependent-context friend).

And I don't see much benefit. Any correct code looking at a dependent
friend already needs to cope with the possibility that it has not been
wired into the redeclaration chain (because some portion of the friend
declaration itself might be dependent). Given that, it seems much simpler
to say that *no* friends in dependent contexts are wired into a
redeclaration chain.

That said, I can imagine that some (particularly, tooling) users of Clang
might want a best-effort "what is this friend declaration redeclaring, if
you happen to know?" query. And as it happens, we do work that out while
we're initially parsing the friend declaration, so I don't see a problem
with storing it and later making it available to such uses, but not as the
canonical / previous declaration (or anywhere that suggests it's reliable
and not merely best-effort).

Hmm, I think you're talking about a change to *all* friends, to not make
them reachable from other declarations through the redeclaration chain,
right? Perhaps I misunderstood, but I thought Vassil was referring to the
case of friends in dependent contexts (which we currently don't put into
the redeclaration chain at all), which I think is a somewhat distinct
issue, because from a semantic point of view, they're not supposed to exist
*at all* outside their template until instantiation. Eg:

  template<typename T> struct A { friend int f(); };
  template<typename T> struct B { friend float f(); };

... is presumably valid unless both templates are instantiated in the same
program.

But now you mention it... I'm imagining an approach where a FriendDecl
would hold a pointer to the befriended entity (outside the class), and also
contain an (isolated, never part of a redecl chain) declaration that
lexically appeared within it, with the befriended entity being lazily
computed by the access checking code or similar. Is that something like
what you had in mind, or were you really thinking of making the redecl
chain not be a single circular list?

I don't think the same approach would work for local extern declarations,
which use an analogous IDNS mechanism to be redeclarable in the enclosing
namespace scope without being visible to ordinary lookup there, since we
really do need them to be in the same redeclaration chain.

Right. There’s a lot of silliness like this with friends in templates.

I was thinking that the redecl chain doesn’t really need to be a circular list, yeah. But I haven’t thought very deeply about it; it might be completely unreasonable.

I know that we can’t necessarily completely remove friends from the redeclaration chain because they might be the first declaration of something that’s later explicitly declared, or even the definition.

John.