[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

3649

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

3650

const FriendDecl *FD?

3653

const?

3654

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

?

3729

Why not strictly equal?

clang/unittests/AST/ASTImporterTest.cpp

3978

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

4003

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

clang/unittests/AST/StructuralEquivalenceTest.cpp

763

We need to place spaces after foo.

773

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

3654

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

3729

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

3652

uint32_t

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

3654

uint32_t

3658

uint32_t

3674

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

3978

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

4003

const char*?

[balazske]

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

[balazske]

clang/lib/AST/ASTImporter.cpp

3652

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 FriendCountAndPosition {
+  /// Number of similar looking friends.
+  unsigned int TotalCount;
+  /// Index of the specific FriendDecl.
+  unsigned int IndexOfDecl;
+};
+
+template <class T>
+FriendCountAndPosition getFriendCountAndPosition(
+    const FriendDecl *FD,
+    std::function<T(const FriendDecl *)> GetCanTypeOrDecl) {
+  unsigned int FriendCount = 0;
+  llvm::Optional<unsigned int> FriendPosition;
+  const auto *RD = cast<CXXRecordDecl>(FD->getLexicalDeclContext());
+
+  T TypeOrDecl = GetCanTypeOrDecl(FD);
+
+  for (const FriendDecl *FoundFriend : RD->friends()) {
+    if (FoundFriend == FD) {
+      FriendPosition = FriendCount;
+      ++FriendCount;
+    } else if (!FoundFriend->getFriendDecl() == !FD->getFriendDecl() &&
+               GetCanTypeOrDecl(FoundFriend) == TypeOrDecl) {
+      ++FriendCount;
+    }
+  }
+
+  assert(FriendPosition && "Friend decl not found in own parent.");
+
+  return {FriendCount, *FriendPosition};
+}
+
+FriendCountAndPosition getFriendCountAndPosition(const FriendDecl *FD) {
+  if (FD->getFriendType())
+    return getFriendCountAndPosition<QualType>(FD, [](const FriendDecl *F) {
+      if (TypeSourceInfo *TSI = F->getFriendType())
+        return TSI->getType().getCanonicalType();
+      llvm_unreachable("Wrong friend object type.");
+    });
+  else
+    return getFriendCountAndPosition<Decl *>(FD, [](const FriendDecl *F) {
+      if (Decl *D = F->getFriendDecl())
+        return D->getCanonicalDecl();
+      llvm_unreachable("Wrong friend object type.");
+    });
+}
+
 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();
   }
+  FriendCountAndPosition 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_CXX03, "input1.cc");
   auto *Definition = FirstDeclMatcher<CXXRecordDecl>().match(
       FromTU1, cxxRecordDecl(hasName("F")));
   auto *ImportedDef = Import(Definition, Lang_CXX03);
   EXPECT_TRUE(ImportedDef->getPreviousDecl());
   EXPECT_EQ(ImportedFwd, ImportedDef->getPreviousDecl());
 }
 
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendType) {
+  const char *Code =
+      R"(
+      class Container {
+        friend class X;
+        friend class X;
+      };
+      )";
+  Decl *ToTu = getToTuDecl(Code, Lang_CXX03);
+  Decl *FromTu = getTuDecl(Code, Lang_CXX03, "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_CXX03);
+  FriendDecl *ToImportedFriend2 = Import(FromFriend2, Lang_CXX03);
+
+  EXPECT_NE(ToImportedFriend1, ToImportedFriend2);
+  EXPECT_EQ(ToFriend1, ToImportedFriend1);
+  EXPECT_EQ(ToFriend2, ToImportedFriend2);
+}
+
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendDecl) {
+  const char *Code =
+      R"(
+      class Container {
+        friend void f();
+        friend void f();
+      };
+      )";
+  Decl *ToTu = getToTuDecl(Code, Lang_CXX03);
+  Decl *FromTu = getTuDecl(Code, Lang_CXX03, "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_CXX03);
+  FriendDecl *ToImportedFriend2 = Import(FromFriend2, Lang_CXX03);
+
+  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_CXX03);

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_CXX11);
+  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_CXX11);
+  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_CXX11);
+  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 = [](){}; }",