[clang][ASTImporter] Fix friend class template import within dependent context

[danix800]

Summary

For friend class template within dependent context:

1. Should not do structure matching checking. This fixes importing failure of template with non-type parm;
2. Should not be added into redecls chain.

See Sema::CheckClassTemplate().

Fixes https://github.com/llvm/llvm-project/issues/64169

[danix800]

Apply git-clang-format.

[shafik]

This mostly makes sense to me, but I want another review.

[aaron.ballman]

The changes seem correct to me, but it'd be great if one of the more regular AST importer maintainers could verify just in case I missed something. This should probably have a release note, though.

[steakhal]

@balazske Could you please have a look?

[danix800]

Update ReleaseNotes

[balazske]

clang/lib/AST/ASTImporter.cpp

2862–2866

The code seems to work but I was confused by the different conditions used (is it possible that IsFriendTemplate and ShouldAddRedecl is true at the same time?). I had the observation that if ShouldAddRedecl is false we do not need the whole search for previous decl. At a friend template we shall not find a definition, the loop would just find the last declaration (this value is not used). So I have the idea of this code (could not find the "suggest edit" command):

bool DependentFriend = IsFriendTemplate && IsDependentContext;

// We may already have a record of the same name; try to find and match it.
RecordDecl *PrevDecl = nullptr;
if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {

2904

This change is not needed if the code above is used.

2976

This change is not needed if the code above is used.

5805–5812

Similar change here:

bool DependentFriend = IsFriendTemplate && IsDependentContext;

ClassTemplateDecl *FoundByLookup = nullptr;

// We may already have a template of the same name; try to find and match it.
if (!DependentFriend && !DC->isFunctionOrMethod()) {

IsFriendTemplate and ShouldAddRedecl is not needed (no changes in the later lines).

[danix800]

Cleanup as @balazske suggested.

[danix800]

clang/lib/AST/ASTImporter.cpp

2862–2866

This is more clear. Thanks.

[danix800]

Update ReleaseNotes.rst

[balazske]

The fix looks OK, but the test could be improved and cleaned up (for example FromClass is the same as FromD in the test, and DeclContext is not checked, can be done like in the test UndeclaredFriendClassShouldNotBeVisible but the AST is different). Probably there are other similar cases, and there is a related problem shown in D156693 (the fix in that patch is not correct, the solution here is not good for that case, it is possible that the same code as here needs to be changed again or a better fix is found). I am accepting this code but probably will create a new patch to improve and add tests for similar cases (if not done before by somebody else).

[danix800]

In D155661#4550340, @balazske wrote:

The fix looks OK, but the test could be improved and cleaned up (for example FromClass is the same as FromD in the test, and DeclContext is not checked, can be done like in the test UndeclaredFriendClassShouldNotBeVisible but the AST is different).

Testcase will be cleaned up in the final commit.

Probably there are other similar cases, and there is a related problem shown in D156693 (the fix in that patch is not correct, the solution here is not good for that case, it is possible that the same code as here needs to be changed again or a better fix is found). I am accepting this code but probably will create a new patch to improve and add tests for similar cases (if not done before by somebody else).

Confirmed, but I'll not touch this issue in this commit.

clang/docs/ReleaseNotes.rst

 - Fix cases where we ignore ambiguous name lookup when looking up memebers.
   (`#22413 <https://github.com/llvm/llvm-project/issues/22413>_`),
   (`#29942 <https://github.com/llvm/llvm-project/issues/29942>_`),
   (`#35574 <https://github.com/llvm/llvm-project/issues/35574>_`) and
   (`#27224 <https://github.com/llvm/llvm-project/issues/27224>_`).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
+- Fixed an import failure of recursive friend class template.
+  `Issue 64169 <https://github.com/llvm/llvm-project/issues/64169>`_
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Target Specific Changes

clang/lib/AST/ASTImporter.cpp

ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
   if (!SearchName && D->getTypedefNameForAnonDecl()) {
     if (Error Err = importInto(
         SearchName, D->getTypedefNameForAnonDecl()->getDeclName()))
       return std::move(Err);
     IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
     IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+                                            : DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod() && !D->isLambda()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     auto FoundDecls =
         Importer.findDeclsInToCtx(DC, SearchName);
     if (!FoundDecls.empty()) {
       // We're going to have to compare D against potentially conflicting Decls,
       // so complete it.
       if (D->hasExternalLexicalStorage() && !D->isCompleteDefinition())
         D->getASTContext().getExternalSource()->CompleteType(D);
for (auto *FoundDecl : FoundDecls) {
         //  struct X { struct { int a; }; struct { int b; }; }; // anon structs
         if (!SearchName)
           if (!IsStructuralMatch(D, FoundRecord, false))
             continue;
 
         if (!hasSameVisibilityContextAndLinkage(FoundRecord, D))
           continue;
 
         if (IsStructuralMatch(D, FoundRecord)) {
           RecordDecl *FoundDef = FoundRecord->getDefinition();
           if (D->isThisDeclarationADefinition() && FoundDef) {
             // FIXME: Structural equivalence check should check for same
             // user-defined methods.
             Importer.MapImported(D, FoundDef);
             if (const auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
               auto *FoundCXX = dyn_cast<CXXRecordDecl>(FoundDef);
               assert(FoundCXX && "Record type mismatch");
if (DCXX->isLambda()) {
               *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
               cast_or_null<CXXRecordDecl>(PrevDecl), DelayTypeCreation))
         return D2CXX;
       Importer.getToContext().getTypeDeclType(
           D2CXX, dyn_cast<CXXRecordDecl>(DC));
     } else {
       if (GetImportedOrCreateDecl(D2CXX, D, Importer.getToContext(),
                                   D->getTagKind(), DC, *BeginLocOrErr, Loc,
                                   Name.getAsIdentifierInfo(),
                                   cast_or_null<CXXRecordDecl>(PrevDecl)))
         return D2CXX;
     }
 
     D2 = D2CXX;
     D2->setAccess(D->getAccess());
     D2->setLexicalDeclContext(LexicalDC);
     addDeclToContexts(D, D2);
ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return std::move(Err);
   if (ToD)
     return ToD;
 
+  bool IsFriendTemplate = D->getFriendObjectKind() != Decl::FOK_None;
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+                                            : DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   ClassTemplateDecl *FoundByLookup = nullptr;
 
   // We may already have a template of the same name; try to find and match it.
-  if (!DC->isFunctionOrMethod()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(Decl::IDNS_Ordinary |
                                               Decl::IDNS_TagFriend))
         continue;
 
       Decl *Found = FoundDecl;
       auto *FoundTemplate = dyn_cast<ClassTemplateDecl>(Found);

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportClasses, ImportNestedPrototypeThenDefinition) {
   auto ToDef = LastDeclMatcher<RecordDecl>().match(ToTU, Pattern);
   EXPECT_TRUE(ImportedDef == ToDef);
   EXPECT_TRUE(ImportedProto == ToProto);
   EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
   EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
   EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
 }
 
+struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {
+  void testRecursiveFriendClassTemplate(Decl *FromTu) {
+    auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+        FromTu, classTemplateDecl());
+
+    auto Pattern = classTemplateDecl(
+        has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()))))));
+    ASSERT_TRUE(MatchVerifier<Decl>{}.match(FromD, Pattern));
 
-struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {};
+    auto *FromFriend =
+        FirstDeclMatcher<FriendDecl>().match(FromD, friendDecl());
+    auto *FromRecordOfFriend =
+        cast<ClassTemplateDecl>(FromFriend->getFriendDecl())
+            ->getTemplatedDecl();
+    EXPECT_NE(FromRecordOfFriend, FromD->getTemplatedDecl());
+    EXPECT_TRUE(FromRecordOfFriend->getPreviousDecl() == nullptr);
+
+    auto *FromDC = FromRecordOfFriend->getDeclContext();
+    auto *FromLexicalDC = FromRecordOfFriend->getLexicalDeclContext();
+    ASSERT_EQ(FromDC, cast<DeclContext>(FromTu));
+    ASSERT_EQ(FromLexicalDC, cast<DeclContext>(FromD->getTemplatedDecl()));
+
+    ASSERT_FALSE(FromDC->containsDecl(FromRecordOfFriend));
+    ASSERT_FALSE(FromLexicalDC->containsDecl(FromRecordOfFriend));
+    ASSERT_FALSE(cast<RecordDecl>(FromRecordOfFriend)
+                     ->getLookupParent()
+                     ->lookup(FromRecordOfFriend->getDeclName())
+                     .empty());
+
+    auto *ToD = Import(FromD, Lang_CXX03);
+    EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
+
+    auto *ToFriend = FirstDeclMatcher<FriendDecl>().match(ToD, friendDecl());
+    auto *ToRecordOfFriend =
+        cast<ClassTemplateDecl>(ToFriend->getFriendDecl())->getTemplatedDecl();
+
+    EXPECT_NE(ToRecordOfFriend, ToD->getTemplatedDecl());
+    EXPECT_TRUE(ToRecordOfFriend->getPreviousDecl() == nullptr);
+
+    auto *ToDC = ToRecordOfFriend->getDeclContext();
+    auto *ToLexicalDC = ToRecordOfFriend->getLexicalDeclContext();
+    ASSERT_EQ(ToDC, cast<DeclContext>(ToD->getTranslationUnitDecl()));
+    ASSERT_EQ(ToLexicalDC, cast<DeclContext>(ToD->getTemplatedDecl()));
+
+    ASSERT_FALSE(ToDC->containsDecl(ToRecordOfFriend));
+    ASSERT_FALSE(ToLexicalDC->containsDecl(ToRecordOfFriend));
+    ASSERT_FALSE(cast<RecordDecl>(ToRecordOfFriend)
+                     ->getLookupParent()
+                     ->lookup(ToRecordOfFriend->getDeclName())
+                     .empty());
+  }
+};
 
 TEST_P(ImportFriendClasses, ImportOfFriendRecordDoesNotMergeDefinition) {
   Decl *FromTU = getTuDecl(
       R"(
       class A {
         template <int I> class F {};
         class X {
           template <int I> friend class F;
TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClassTemplate) {
   Decl *FromTu = getTuDecl(
       R"(
       template<class A> class declToImport {
         template<class A1> friend class declToImport;
       };
       )",
       Lang_CXX03, "input.cc");
 
-  auto *FromD =
-      FirstDeclMatcher<ClassTemplateDecl>().match(FromTu, classTemplateDecl());
-  auto *ToD = Import(FromD, Lang_CXX03);
-
-  auto Pattern = classTemplateDecl(
-      has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()))))));
-  ASSERT_TRUE(MatchVerifier<Decl>{}.match(FromD, Pattern));
-  EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
+  testRecursiveFriendClassTemplate(FromTu);
+}
 
-  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);
+TEST_P(ImportFriendClasses,
+       ImportOfRecursiveFriendClassTemplateWithNonTypeParm) {
+  Decl *FromTu = getTuDecl(
+      R"(
+      template<class A1, A1 A> class declToImport {
+        template<class B1, B1> friend class declToImport;
+      };
+      )",
+      Lang_CXX03, "input.cc");
+  testRecursiveFriendClassTemplate(FromTu);
 }
 
 TEST_P(ImportFriendClasses, ProperPrevDeclForClassTemplateDecls) {
   auto Pattern = classTemplateSpecializationDecl(hasName("X"));
 
   ClassTemplateSpecializationDecl *Imported1;
   {
     Decl *FromTU = getTuDecl("template<class T> class X;"