[ASTImporter] Corrected import of repeated friend declarations.

[balazske]

Summary

Import declarations in correct order if a class contains
multiple redundant friend (type or decl) declarations.
If the order is incorrect this could cause false structural
equivalences and wrong declaration chains after import.

[a_sidorin]

Hello Balazs,
This look almost good to me except some comments inline.

clang/lib/AST/ASTImporter.cpp

3635

It's better to turn the tuple into a named struct (FriendCountAndPosition?) to make the code more readable.

3636

const FriendDecl *FD?

3639

const?

3640

if (const TypeSourceInfo *FriendType = FD->getFriendType()) {
    QualType TypeOfFriend = FriendType->getType().getCanonicalType();
...

?

3706

Why not strictly equal?

clang/unittests/AST/ASTImporterTest.cpp

4009

clang-tidy wants this to be const auto* Code.

4034

clang-tidy wants this to be const auto* Code.

clang/unittests/AST/StructuralEquivalenceTest.cpp

801

We need to place spaces after foo.

811

Could you please add a positive test with two struct foo{ friend class X; friend class Y; };", structures as well?

[balazske]

Fixes according to comments.

[balazske]

clang/lib/AST/ASTImporter.cpp

3640

In the current way the branches (after if and else) look relatively similar, I like that better.

3706

D id the Decl to be imported, ImportedEquivalentFriends contains the similar friends already imported. This may be less than the total (0 at first time) if not all friends are imported yet (should be a temporary state). The next line checks for equality.

[shafik]

LGTM WDYT @teemperor

clang/lib/AST/ASTImporter.cpp

3638

uint32_t

Is there a reason to not prefer fixed width integer types?

3640

uint32_t

3644

uint32_t

3660

Can we refactor to not repeat code? They look identical. Duplicate code is a source for errors when once piece of code is fixed both the other is not etc

clang/unittests/AST/ASTImporterTest.cpp

4009

Does auto really buy us anything here? Why not use use const char*?

4034

const char*?

[balazske]

Removed code repetition.
Using const char * instead of auto.

[balazske]

clang/lib/AST/ASTImporter.cpp

3638

I do not know about any reason, but in ASTImporter.cpp uint32_t does not appear but unsigned and unsigned int yes. Why can be fixed width type better that the default-sized int?

[martong]

Ping :)

[shafik]

LGTM

[balazske]

Rebase

clang/lib/AST/ASTImporter.cpp

if (GetImportedOrCreateDecl(ToIndirectField, D, Importer.getToContext(), DC,
     return ToIndirectField;
 
   ToIndirectField->setAccess(D->getAccess());
   ToIndirectField->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(ToIndirectField);
   return ToIndirectField;
 }
 
+/// Used as return type of getFriendCountAndPosition.
+struct FriendCountAndIndex {
+  /// Number of similar looking friends.
+  unsigned int TotalCount;
+  /// Index of the specific FriendDecl.
+  unsigned int IndexOfDecl;
+};
+
+FriendCountAndIndex getFriendCountAndPosition(const FriendDecl *FD) {
+  unsigned int FriendCount = 0;
+  llvm::Optional<unsigned int> FriendPosition;
+  const auto *RD = cast<CXXRecordDecl>(FD->getLexicalDeclContext());
+  if (FD->getFriendType()) {
+    QualType TypeOfFriend = FD->getFriendType()->getType().getCanonicalType();
+    for (const FriendDecl *FoundFriend : RD->friends()) {
+      if (FoundFriend == FD) {
+        FriendPosition = FriendCount;
+        ++FriendCount;
+      } else if (FoundFriend->getFriendType() &&
+                 FoundFriend->getFriendType()->getType().getCanonicalType() ==
+                     TypeOfFriend)
+        ++FriendCount;
+    }
+  } else {
+    const Decl *CanDeclOfFriend = FD->getFriendDecl()->getCanonicalDecl();
+    for (const FriendDecl *FoundFriend : RD->friends()) {
+      if (FoundFriend == FD) {
+        FriendPosition = FriendCount;
+        ++FriendCount;
+      } else if (FoundFriend->getFriendDecl() &&
+                 FoundFriend->getFriendDecl()->getCanonicalDecl() ==
+                     CanDeclOfFriend)
+        ++FriendCount;
+    }
+  }
+  assert(FriendPosition && "Friend decl not found in own parent.");
+  return {FriendCount, *FriendPosition};
+}
+
 ExpectedDecl ASTNodeImporter::VisitFriendDecl(FriendDecl *D) {
   // Import the major distinguishing characteristics of a declaration.
   DeclContext *DC, *LexicalDC;
   if (Error Err = ImportDeclContext(D, DC, LexicalDC))
     return std::move(Err);
 
   // Determine whether we've already imported this decl.
   // FriendDecl is not a NamedDecl so we cannot use lookup.
-  auto *RD = cast<CXXRecordDecl>(DC);
+  // We try to maintain order and count of redundant friend declarations.
+  const auto *RD = cast<CXXRecordDecl>(DC);
   FriendDecl *ImportedFriend = RD->getFirstFriend();
+  SmallVector<FriendDecl *, 2> ImportedEquivalentFriends;
 
   while (ImportedFriend) {
+    bool Match = false;
     if (D->getFriendDecl() && ImportedFriend->getFriendDecl()) {
-      if (IsStructuralMatch(D->getFriendDecl(), ImportedFriend->getFriendDecl(),
-                            /*Complain=*/false))
-        return Importer.MapImported(D, ImportedFriend);
-
+      Match =
+          IsStructuralMatch(D->getFriendDecl(), ImportedFriend->getFriendDecl(),
+                            /*Complain=*/false);
     } else if (D->getFriendType() && ImportedFriend->getFriendType()) {
-      if (Importer.IsStructurallyEquivalent(
-            D->getFriendType()->getType(),
-            ImportedFriend->getFriendType()->getType(), true))
-        return Importer.MapImported(D, ImportedFriend);
+      Match = Importer.IsStructurallyEquivalent(
+          ImportedFriend->getFriendType()->getType(), /*Complain=*/false);
     }
+    if (Match)
+      ImportedEquivalentFriends.push_back(ImportedFriend);
+
     ImportedFriend = ImportedFriend->getNextFriend();
   }
+  FriendCountAndIndex CountAndPosition = getFriendCountAndPosition(D);
+
+  assert(ImportedEquivalentFriends.size() <= CountAndPosition.TotalCount &&
+         "Class with non-matching friends is imported, ODR check wrong?");
+  if (ImportedEquivalentFriends.size() == CountAndPosition.TotalCount)
+    return Importer.MapImported(
+        D, ImportedEquivalentFriends[CountAndPosition.IndexOfDecl]);
 
   // Not found. Create it.
+  // The declarations will be put into order later by ImportDeclContext.
   FriendDecl::FriendUnion ToFU;
   if (NamedDecl *FriendD = D->getFriendDecl()) {
     NamedDecl *ToFriendD;
     if (Error Err = importInto(ToFriendD, FriendD))
       return std::move(Err);
 
     if (FriendD->getFriendObjectKind() != Decl::FOK_None &&
         !(FriendD->isInIdentifierNamespace(Decl::IDNS_NonMemberOperator)))

clang/unittests/AST/ASTImporterTest.cpp

Decl *FromTU1 = getTuDecl(
       Lang_CXX, "input1.cc");
   auto *Definition = FirstDeclMatcher<CXXRecordDecl>().match(
       FromTU1, cxxRecordDecl(hasName("F")));
   auto *ImportedDef = Import(Definition, Lang_CXX);
   EXPECT_TRUE(ImportedDef->getPreviousDecl());
   EXPECT_EQ(ImportedFwd, ImportedDef->getPreviousDecl());
 }
 
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendType) {
+  const auto *Code =
+      R"(
+      class Container {
+        friend class X;
+        friend class X;
+      };
+      )";
+  Decl *ToTu = getToTuDecl(Code, Lang_CXX);
+  Decl *FromTu = getTuDecl(Code, Lang_CXX, "from.cc");
+
+  auto *ToFriend1 = FirstDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *ToFriend2 = LastDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *FromFriend1 =
+      FirstDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+  auto *FromFriend2 = LastDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+
+  FriendDecl *ToImportedFriend1 = Import(FromFriend1, Lang_CXX);
+  FriendDecl *ToImportedFriend2 = Import(FromFriend2, Lang_CXX);
+
+  EXPECT_NE(ToImportedFriend1, ToImportedFriend2);
+  EXPECT_EQ(ToFriend1, ToImportedFriend1);
+  EXPECT_EQ(ToFriend2, ToImportedFriend2);
+}
+
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendDecl) {
+  const auto *Code =
+      R"(
+      class Container {
+        friend void f();
+        friend void f();
+      };
+      )";
+  Decl *ToTu = getToTuDecl(Code, Lang_CXX);
+  Decl *FromTu = getTuDecl(Code, Lang_CXX, "from.cc");
+
+  auto *ToFriend1 = FirstDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *ToFriend2 = LastDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *FromFriend1 =
+      FirstDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+  auto *FromFriend2 = LastDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+
+  FriendDecl *ToImportedFriend1 = Import(FromFriend1, Lang_CXX);
+  FriendDecl *ToImportedFriend2 = Import(FromFriend2, Lang_CXX);
+
+  EXPECT_NE(ToImportedFriend1, ToImportedFriend2);
+  EXPECT_EQ(ToFriend1, ToImportedFriend1);
+  EXPECT_EQ(ToFriend2, ToImportedFriend2);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, FriendFunInClassTemplate) {
   auto *Code = R"(
   template <class T>
   struct X {
     friend void foo(){}
   };
       )";
   TranslationUnitDecl *ToTU = getToTuDecl(Code, Lang_CXX);

clang/unittests/AST/StructuralEquivalenceTest.cpp

 }
 
 TEST_F(StructuralEquivalenceRecordTest, RecordsWithDifferentBody) {
   auto t = makeNamedDecls("class foo { int B; };", "class foo { int A; };",
                           Lang_CXX11);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, SameFriendMultipleTimes) {
+  auto t = makeNamedDecls("struct foo { friend class X; };",
+                          "struct foo { friend class X; friend class X; };",
+                          Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest, SameFriendsDifferentOrder) {
+  auto t = makeNamedDecls("struct foo { friend class X; friend class Y; };",
+                          "struct foo { friend class Y; friend class X; };",
+                          Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest, SameFriendsSameOrder) {
+  auto t = makeNamedDecls("struct foo { friend class X; friend class Y; };",
+                          "struct foo { friend class X; friend class Y; };",
+                          Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
 
 TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
   // Get the LambdaExprs, unfortunately we can't match directly the underlying
   // implicit CXXRecordDecl of the Lambda classes.
   auto t = makeDecls<LambdaExpr>(
       "void f() { auto L0 = [](int){}; }",
       "void f() { auto L1 = [](){}; }",