PR31469: Don't add friend template class decls to redecl chain in dependent contexts.

[v.g.vassilev]

Summary

Fixes a crash in modules where the template class decl becomes the most recent decl in the redeclaration chain and forcing the template instantiator try to instantiate the friend declaration, rather than the template definition.

In practice, A::list<int> produces a TemplateSpecializationType A::__1::list<int, allocator<type-parameter-0-0> >' failing to replace to subsitute the default argument to allocator<int>.

[kimgr]

I think we're seeing similar new behavior in IWYU, I've been meaning to bring it up on cfe-dev. But since it might be relevant here, I'll add our observations.

To us, it looks like friends in templates form their own redecl chain. An example:

// template.h
void foo();

template<class T>
struct Template {
  friend void foo();
};

// foo.h
inline void foo() {
}

// main.cpp
#include "template.h"
#include "foo.h"

Template<int> i;

int main() {
  foo();  // attributed to Template<T>::foo
}

This behavior changed in r283207.

We never got to the bottom of whether the redecl shuffling is correct or not, so we decided to just work around it, our code here:
https://github.com/include-what-you-use/include-what-you-use/blob/master/iwyu_ast_util.cc#L842

Relevant to this patch, I wonder if you're also about to work around a more central problem?

[yaron.keren]

While at it, http://llvm.org/pr30994

[v.g.vassilev]

@kimgr, here is the discussion about the change in r283207 https://reviews.llvm.org/D16989

[rsmith]

lib/Sema/SemaTemplate.cpp

1237–1238

You should keep the class out of its redeclaration chain too.

[v.g.vassilev]

Address comments.

[v.g.vassilev]

@yaron.keren, it seems that http://llvm.org/pr30994 concerns friend function declarations. My current patch focuses on friend class templates. Perhaps we should open another review item for a fix of http://llvm.org/pr30994.

[yaron.keren]

In D28399#642458, @v.g.vassilev wrote:

@yaron.keren, it seems that http://llvm.org/pr30994 concerns friend function declarations. My current patch focuses on friend class templates. Perhaps we should open another review item for a fix of http://llvm.org/pr30994.

yes, of course.

[v.g.vassilev]

Landed in r291753.

include/clang/AST/DeclTemplate.h

bool isThisDeclarationADefinition() const {
     return getTemplatedDecl()->isThisDeclarationADefinition();
   }
 
   /// \brief Create a class template node.
   static ClassTemplateDecl *Create(ASTContext &C, DeclContext *DC,
                                    SourceLocation L,
                                    DeclarationName Name,
                                    TemplateParameterList *Params,
-                                   NamedDecl *Decl,
-                                   ClassTemplateDecl *PrevDecl);
+                                   NamedDecl *Decl);
 
   /// \brief Create an empty class template node.
   static ClassTemplateDecl *CreateDeserialized(ASTContext &C, unsigned ID);
 
   /// \brief Return the specialization with the provided arguments if it exists,
   /// otherwise return the insertion point.
   ClassTemplateSpecializationDecl *
   findSpecialization(ArrayRef<TemplateArgument> Args, void *&InsertPos);

lib/AST/ASTImporter.cpp

Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
   // Create the class template declaration itself.
   TemplateParameterList *TemplateParams
     = ImportTemplateParameterList(D->getTemplateParameters());
   if (!TemplateParams)
     return nullptr;
 
   ClassTemplateDecl *D2 = ClassTemplateDecl::Create(Importer.getToContext(), DC, 
                                                     Loc, Name, TemplateParams, 
-                                                    D2Templated, 
-                                                    /*PrevDecl=*/nullptr);
+                                                    D2Templated);
   D2Templated->setDescribedClassTemplate(D2);    
   
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(D2);
   
   // Note the relationship between the class templates.
   Importer.Imported(D, D2);

lib/AST/DeclTemplate.cpp

void ClassTemplateDecl::DeallocateCommon(void *Ptr) {
   static_cast<Common *>(Ptr)->~Common();
 }
 
 ClassTemplateDecl *ClassTemplateDecl::Create(ASTContext &C,
                                              DeclContext *DC,
                                              SourceLocation L,
                                              DeclarationName Name,
                                              TemplateParameterList *Params,
-                                             NamedDecl *Decl,
-                                             ClassTemplateDecl *PrevDecl) {
+                                             NamedDecl *Decl) {
   AdoptTemplateParameterList(Params, cast<DeclContext>(Decl));
   ClassTemplateDecl *New = new (C, DC) ClassTemplateDecl(C, DC, L, Name,
                                                          Params, Decl);
-  New->setPreviousDecl(PrevDecl);
   return New;
 }
 
 ClassTemplateDecl *ClassTemplateDecl::CreateDeserialized(ASTContext &C,
                                                          unsigned ID) {
   return new (C, ID) ClassTemplateDecl(C, nullptr, SourceLocation(),
                                        DeclarationName(), nullptr, nullptr);
 }

lib/Sema/SemaTemplate.cpp

if (SS.isSet()) {
     if (!SS.isInvalid() && !Invalid && !PrevClassTemplate) {
       Diag(NameLoc, TUK == TUK_Friend ? diag::err_friend_decl_does_not_match
                                       : diag::err_member_decl_does_not_match)
         << Name << SemanticContext << /*IsDefinition*/true << SS.getRange();
       Invalid = true;
     }
   }
 
+  // If this is a templated friend in a dependent context we should not put it
+  // on the redecl chain. In some cases, the templated friend can be the most
+  // recent declaration tricking the template instantiator to make substitutions
+  // there.
+  // FIXME: Figure out how to combine with shouldLinkDependentDeclWithPrevious
+  bool ShouldAddRedecl
+    = !(TUK == TUK_Friend && CurContext->isDependentContext());
+
   CXXRecordDecl *NewClass =
     CXXRecordDecl::Create(Context, Kind, SemanticContext, KWLoc, NameLoc, Name,
-                          PrevClassTemplate?
+                          PrevClassTemplate && ShouldAddRedecl ?
                             PrevClassTemplate->getTemplatedDecl() : nullptr,
                           /*DelayTypeCreation=*/true);
   SetNestedNameSpecifier(NewClass, SS);
   if (NumOuterTemplateParamLists > 0)
     NewClass->setTemplateParameterListsInfo(
         Context, llvm::makeArrayRef(OuterTemplateParamLists,
                                     NumOuterTemplateParamLists));
 
   // Add alignment attributes if necessary; these attributes are checked when
   // the ASTContext lays out the structure.
   if (TUK == TUK_Definition) {
     AddAlignmentAttributesForRecord(NewClass);
     AddMsStructLayoutForRecord(NewClass);
   }
 
   ClassTemplateDecl *NewTemplate
     = ClassTemplateDecl::Create(Context, SemanticContext, NameLoc,
                                 DeclarationName(Name), TemplateParams,
-                                NewClass, PrevClassTemplate);
+                                NewClass);
+
+  if (ShouldAddRedecl)
+    NewTemplate->setPreviousDecl(PrevClassTemplate);
+
   NewClass->setDescribedClassTemplate(NewTemplate);
 
   if (ModulePrivateLoc.isValid())
     NewTemplate->setModulePrivate();
 
   // Build the type for the class template declaration now.
   QualType T = NewTemplate->getInjectedClassNameSpecialization();
   T = Context.getInjectedClassNameType(NewClass, T);

lib/Sema/SemaTemplateInstantiateDecl.cpp

= CXXRecordDecl::Create(SemaRef.Context, Pattern->getTagKind(), DC,
                             Pattern->getIdentifier(), PrevDecl,
                             /*DelayTypeCreation=*/true);
 
   if (QualifierLoc)
     RecordInst->setQualifierInfo(QualifierLoc);
 
   ClassTemplateDecl *Inst
     = ClassTemplateDecl::Create(SemaRef.Context, DC, D->getLocation(),
-                                D->getIdentifier(), InstParams, RecordInst,
-                                PrevClassTemplate);
+                                D->getIdentifier(), InstParams, RecordInst);
+  assert(!(isFriend && Owner->isDependentContext()));
+  Inst->setPreviousDecl(PrevClassTemplate);
+
   RecordInst->setDescribedClassTemplate(Inst);
 
   if (isFriend) {
     if (PrevClassTemplate)
       Inst->setAccess(PrevClassTemplate->getAccess());
     else
       Inst->setAccess(D->getAccess());
 

test/Misc/ast-dump-decl.cpp

namespace testCanonicalTemplate {
   // CHECK:      ClassTemplateDecl{{.*}} TestClassTemplate
   // CHECK-NEXT:   TemplateTypeParmDecl
   // CHECK-NEXT:   CXXRecordDecl{{.*}} class TestClassTemplate
   // CHECK-NEXT:     CXXRecordDecl{{.*}} class TestClassTemplate
   // CHECK-NEXT:     FriendDecl
   // CHECK-NEXT:       ClassTemplateDecl{{.*}} TestClassTemplate
   // CHECK-NEXT:         TemplateTypeParmDecl
   // CHECK-NEXT:         CXXRecordDecl{{.*}} class TestClassTemplate
-  // CHECK-NEXT:         ClassTemplateSpecialization{{.*}} 'TestClassTemplate'
   // CHECK-NEXT:   ClassTemplateSpecializationDecl{{.*}} class TestClassTemplate
   // CHECK-NEXT:     TemplateArgument{{.*}}A
   // CHECK-NEXT:     CXXRecordDecl{{.*}} class TestClassTemplate
 }
 
 template <class T>
 class TestClassScopeFunctionSpecialization {
   template<class U> void foo(U a) { }

test/Modules/Inputs/PR31469/empty.h

+// This file is triggers loading of module M.

test/Modules/Inputs/PR31469/module.modulemap

+module M {
+  module "textual_shadow" { header "textual_file_shadow.h" export *}
+  module "trigger" { header "empty.h" export * }
+  export *
+}

test/Modules/Inputs/PR31469/textual.h

+namespace A {
+inline
+namespace __1 {
+  template <class _Tp> class allocator;
+  template <class _Tp, class _Alloc = allocator<_Tp>> class list;
+  template <class _VoidPtr> class __list_iterator {
+    //template <class> friend class list; // causes another crash in ASTDeclReader::attachPreviousDecl
+    template <class, class> friend class list;
+  };
+  template <class _Tp, class _Alloc> class __list_imp {};
+  template <class _Tp, class _Alloc> class list : __list_imp<_Tp, _Alloc> {
+  public:
+    list() {}
+  };
+  template <class _Tp> void f(list<_Tp>);
+}
+}

test/Modules/Inputs/PR31469/textual_file_shadow.h

+#include "textual.h"
+

test/Modules/pr31469.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR31469 -verify %s
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR31469 -fmodules -fmodules-local-submodule-visibility \
+// RUN:    -fimplicit-module-maps -fmodules-cache-path=%t  -verify %s
+
+#include "textual.h"
+#include "empty.h"
+
+namespace A {
+  template <class _Tp> void f();
+}
+
+A::list<int> use;
+
+// expected-no-diagnostics