[ASTImporter] Improved import of friend templates.

[balazske]

Summary

When importing a friend class template declaration,
this declaration should not be merged with any other existing declaration
for the same type. Otherwise the getFriendDecl of the FriendDecl can point
to an other already referenced declaration, this case causes problems.
Additionally the previous decl of class templates is set at import.

[a_sidorin]

unittests/AST/ASTImporterTest.cpp

2721 ↗

(On Diff #159935)

Should we use EXPECT_EQ for "To" checks?

[balazske]

• replaced ASSERT in test

cfe/trunk/lib/AST/ASTImporter.cpp

Decl *ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
   // Import the definition
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))
     return nullptr;
 
   return D2;
 }
 
 Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
+  bool IsFriendTemplate = false;
+  if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
+    IsFriendTemplate =
+        DCXX->getDescribedClassTemplate() &&
+        DCXX->getDescribedClassTemplate()->getFriendObjectKind() !=
+            Decl::FOK_None;
+  }
+
   // If this record has a definition in the translation unit we're coming from,
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
   if (Definition && Definition != D &&
+      // Friend template declaration must be imported on its own.
+      !IsFriendTemplate &&
       // In contrast to a normal CXXRecordDecl, the implicit
       // CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration.
       // The definition of the implicit CXXRecordDecl in this case is the
       // ClassTemplateSpecializationDecl itself. Thus, we start with an extra
       // condition in order to be able to import the implict Decl.
       !D->isImplicit()) {
     Decl *ImportedDef = Importer.Import(Definition);
     if (!ImportedDef)
for (auto *FoundDecl : FoundDecls) {
         if (!SearchName) {
           if (!IsStructuralMatch(D, FoundRecord, false))
             continue;
         }
 
         PrevDecl = FoundRecord;
 
         if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-          if ((SearchName && !D->isCompleteDefinition())
+          if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
               || (D->isCompleteDefinition() &&
                   D->isAnonymousStructOrUnion()
                     == FoundDef->isAnonymousStructOrUnion() &&
                   IsStructuralMatch(D, FoundDef))) {
             // The record types structurally match, or the "from" translation
             // unit only had a forward declaration anyway; call it the same
             // function.
             // FIXME: Structural equivalence check should check for same
for (auto *FoundDecl : FoundDecls) {
           if (D->hasExternalLexicalStorage())
             D->getASTContext().getExternalSource()->CompleteType(D);
 
           if (FoundRecord->isCompleteDefinition() &&
               D->isCompleteDefinition() &&
               !IsStructuralMatch(D, FoundRecord))
             continue;
 
+          if (IsFriendTemplate)
+            continue;
+
           AdoptDecl = FoundRecord;
           continue;
         } else if (!SearchName) {
           continue;
         }
       }
 
       ConflictingDecls.push_back(FoundDecl);
if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
 
       if (ClassTemplateDecl *FromDescribed =
           DCXX->getDescribedClassTemplate()) {
         auto *ToDescribed = cast_or_null<ClassTemplateDecl>(
             Importer.Import(FromDescribed));
         if (!ToDescribed)
           return nullptr;
         D2CXX->setDescribedClassTemplate(ToDescribed);
-        if (!DCXX->isInjectedClassName()) {
+        if (!DCXX->isInjectedClassName() && !IsFriendTemplate) {
           // In a record describing a template the type should be an
           // InjectedClassNameType (see Sema::CheckClassTemplate). Update the
           // previously set type to the correct value here (ToDescribed is not
           // available at record create).
           // FIXME: The previous type is cleared but not removed from
           // ASTContext's internal storage.
           CXXRecordDecl *Injected = nullptr;
           for (NamedDecl *Found : D2CXX->noload_lookup(Name)) {
static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
   if (!ToTemplatedDef)
     return nullptr;
   ClassTemplateDecl *TemplateWithDef =
       ToTemplatedDef->getDescribedClassTemplate();
   return TemplateWithDef;
 }
 
 Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
+  bool IsFriend = D->getFriendObjectKind() != Decl::FOK_None;
+
   // If this record has a definition in the translation unit we're coming from,
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   auto *Definition =
       cast_or_null<CXXRecordDecl>(D->getTemplatedDecl()->getDefinition());
-  if (Definition && Definition != D->getTemplatedDecl()) {
+  if (Definition && Definition != D->getTemplatedDecl() && !IsFriend) {
     Decl *ImportedDef
       = Importer.Import(Definition->getDescribedClassTemplate());
     if (!ImportedDef)
       return nullptr;
 
     return Importer.MapImported(D, ImportedDef);
   }
 
for (auto *FoundDecl : FoundDecls) {
       if (auto *FoundTemplate = dyn_cast<ClassTemplateDecl>(Found)) {
 
         // The class to be imported is a definition.
         if (D->isThisDeclarationADefinition()) {
           // Lookup will find the fwd decl only if that is more recent than the
           // definition. So, try to get the definition if that is available in
           // the redecl chain.
           ClassTemplateDecl *TemplateWithDef = getDefinition(FoundTemplate);
-          if (!TemplateWithDef)
+          if (TemplateWithDef)
+            FoundTemplate = TemplateWithDef;
+          else
             continue;
-          FoundTemplate = TemplateWithDef; // Continue with the definition.
         }
 
         if (IsStructuralMatch(D, FoundTemplate)) {
-          // The class templates structurally match; call it the same template.
-
+          if (!IsFriend) {
             Importer.MapImported(D->getTemplatedDecl(),
                                  FoundTemplate->getTemplatedDecl());
             return Importer.MapImported(D, FoundTemplate);
           }
+
+          continue;
+        }
       }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
                                          ConflictingDecls.data(),
Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
 
   ClassTemplateDecl *D2;
   if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(), DC, Loc, Name,
                               TemplateParams, ToTemplated))
     return D2;
 
   ToTemplated->setDescribedClassTemplate(D2);
 
+  if (ToTemplated->getPreviousDecl()) {
+    assert(
+        ToTemplated->getPreviousDecl()->getDescribedClassTemplate() &&
+        "Missing described template");
+    D2->setPreviousDecl(
+        ToTemplated->getPreviousDecl()->getDescribedClassTemplate());
+  }
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
+  if (!IsFriend)
     LexicalDC->addDeclInternal(D2);
 
   if (FromTemplated->isCompleteDefinition() &&
       !ToTemplated->isCompleteDefinition()) {
     // FIXME: Import definition!
   }
 
   return D2;
 }

cfe/trunk/unittests/AST/ASTImporterTest.cpp

for (auto *FromField : FromLambda->fields()) {
     unsigned ToIndex = ASTImporter::getFieldIndex(ToField);
     EXPECT_EQ(ToIndex, FromIndex + 1);
     ++FromIndex;
   }
 
   EXPECT_EQ(FromIndex, 3u);
 }
 
+TEST_P(
+    ASTImporterTestBase,
+    ImportOfFriendRecordDoesNotMergeDefinition) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      class A {
+        template <int I> class F {};
+        class X {
+          template <int I> friend class F;
+        };
+      };
+      )",
+      Lang_CXX, "input0.cc");
+
+  auto *FromClass = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("F"), isDefinition()));
+  auto *FromFriendClass = LastDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("F")));
+
+  ASSERT_TRUE(FromClass);
+  ASSERT_TRUE(FromFriendClass);
+  ASSERT_NE(FromClass, FromFriendClass);
+  ASSERT_EQ(FromFriendClass->getDefinition(), FromClass);
+  ASSERT_EQ(FromFriendClass->getPreviousDecl(), FromClass);
+  ASSERT_EQ(
+      FromFriendClass->getDescribedClassTemplate()->getPreviousDecl(),
+      FromClass->getDescribedClassTemplate());
+
+  auto *ToClass = cast<CXXRecordDecl>(Import(FromClass, Lang_CXX));
+  auto *ToFriendClass = cast<CXXRecordDecl>(Import(FromFriendClass, Lang_CXX));
+
+  EXPECT_TRUE(ToClass);
+  EXPECT_TRUE(ToFriendClass);
+  EXPECT_NE(ToClass, ToFriendClass);
+  EXPECT_EQ(ToFriendClass->getDefinition(), ToClass);
+  EXPECT_EQ(ToFriendClass->getPreviousDecl(), ToClass);
+  EXPECT_EQ(
+      ToFriendClass->getDescribedClassTemplate()->getPreviousDecl(),
+      ToClass->getDescribedClassTemplate());
+}
+
+TEST_P(
+    ASTImporterTestBase,
+    ImportOfRecursiveFriendClass) {
+  Decl *FromTu = getTuDecl(
+      R"(
+      class declToImport {
+        friend class declToImport;
+      };
+      )",
+      Lang_CXX, "input.cc");
+
+  auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTu, cxxRecordDecl(hasName("declToImport")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  auto Pattern = cxxRecordDecl(hasName("declToImport"), has(friendDecl()));
+  ASSERT_TRUE(MatchVerifier<Decl>{}.match(FromD, Pattern));
+  EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
+}
+
+TEST_P(
+    ASTImporterTestBase,
+    ImportOfRecursiveFriendClassTemplate) {
+  Decl *FromTu = getTuDecl(
+      R"(
+      template <class A> class declToImport {
+        template <class A1> friend class declToImport;
+      };
+      )",
+      Lang_CXX, "input.cc");
+
+  auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+      FromTu, classTemplateDecl(hasName("declToImport")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  
+  auto Pattern = classTemplateDecl(
+      has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()))))));
+  ASSERT_TRUE(MatchVerifier<Decl>{}.match(FromD, Pattern));
+  EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
+  
+  auto *Class =
+      FirstDeclMatcher<ClassTemplateDecl>().match(ToD, classTemplateDecl());
+  auto *Friend = FirstDeclMatcher<FriendDecl>().match(ToD, friendDecl());
+  EXPECT_NE(Friend->getFriendDecl(), Class);
+  EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
   Decl *TU = getTuDecl(
       R"(
       namespace NS {
 
       template <typename T>