[ASTImporter] Compare the DC of the underlying type of a FriendDecl

[martong]

Summary

Currently, we do not check the scope of friend classes. This can result in some
cases with missing nodes in the imported AST. E.g.:

class Container {
  friend class X; // A friend class ::X
  class X{ };
  friend class X; // Friend class Container::X
};

... here either ::X or Container::X is missing after importing Container.
This patch fixes the problem.

[shafik]

I believe that your main example violates basic.scope.class p2:

A name N used in a class S shall refer to the same declaration in its context and when re-evaluated in the completed scope of S.
No diagnostic is required for a violation of this rule.

Although it is ill-formed no diagnostic required.

CC @rsmith

clang/lib/AST/ASTImporter.cpp

3637

It looks like we use FriendDecl *D in other places, we should try to be consistent across the file.

I would not be against changing it but perhaps in a different PR?

3656

return {};

[balazske]

clang/lib/AST/ASTImporter.cpp

3679

Computing FriendDC can be moved out of the loop.

clang/unittests/AST/ASTImporterTest.cpp

4035

Use ToTU and FromTU as is in other tests.

[martong]

In D75922#1915918, @shafik wrote:

I believe that your main example violates basic.scope.class p2:

A name N used in a class S shall refer to the same declaration in its context and when re-evaluated in the completed scope of S.
No diagnostic is required for a violation of this rule.

Although it is ill-formed no diagnostic required.

Thanks Shafik for pointing this out! I thought that this kind of scoping problems could happen only with FriendDecls.

Unfortunately, similar code constructs to the main example are there in live projects, the example is a reduced case from Google's Protobuf. Also, I understand that giving a diagnostic would require an additional round of analysis (in Sema) of the scope after that is completed and that could cause performance issues. This is a good target for a clang-tidy check though.

So, I think this leaves us with 3 possible directions:

1. Do not handle this case, i.e. leaving one node missing from the imported AST of Container (this means abandoning this patch). This would be OK since we are not required to handle ill-formed code. The problem with this option is that when we want to import Container again then we will find that structurally nonequivalent to the previously imported Container, because it has one less FriendDecl.
2. Do handle the case. This option is justified from the view of the task of the ASTImporter that is to copy ASTs (maybe even if they result from ill-formed code). But perhaps it is not the best idea to blindly copy all kind of malformed ASTs. E.g. ASTs built from debug info could be malformed and then it is better to have diagnostics from the Importer (next option).
3. Emit a diagnostic here from ASTImporter and return with an Error because the code is ill-formed and we can diagnose that. Still, we would be able to diagnose these kind of errors only in case of FriendDecls. But perhaps the task of reporting these kind of errors should be in clang-tidy rather.

This is a hard decision to make, but I vote for 2) then 3).

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;
 }
 
+// Returns the DeclContext of the underlying friend declaration/type.  If that
+// is a dependent type then the returned optional does not have a value.
+static Optional<DeclContext *> getDCOfUnderlyingDecl(FriendDecl *FrD) {
+  if (NamedDecl *ND = FrD->getFriendDecl())
+    return ND->getDeclContext();
+  if (FrD->getFriendType()) {
+    QualType Ty = FrD->getFriendType()->getType();
+    if (isa<ElaboratedType>(Ty))
+      Ty = cast<ElaboratedType>(Ty)->getNamedType();
+    if (!Ty->isDependentType()) {
+      if (const auto *RTy = dyn_cast<RecordType>(Ty))
+        return RTy->getAsCXXRecordDecl()->getDeclContext();
+      else if (const auto *SpecTy = dyn_cast<TemplateSpecializationType>(Ty))
+        return SpecTy->getAsCXXRecordDecl()->getDeclContext();
+      else if (const auto TypedefTy = dyn_cast<TypedefType>(Ty))
+        return TypedefTy->getDecl()->getDeclContext();
+      else
+        llvm_unreachable("Unhandled type of friend");
+    }
+  }
+  // DependentType
+  return Optional<DeclContext *>();
+}
+
 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);
-  FriendDecl *ImportedFriend = RD->getFirstFriend();
 
-  while (ImportedFriend) {
+  for (FriendDecl *ImportedFriend = RD->getFirstFriend(); ImportedFriend;
+       ImportedFriend = ImportedFriend->getNextFriend()) {
+
+    // Compare the semantic DeclContext of the underlying declarations of the
+    // existing and the to be imported friend.
+    // Normally, lookup ensures this, but with friends we cannot use the lookup.
+    Optional<DeclContext *> ImportedFriendDC =
+        getDCOfUnderlyingDecl(ImportedFriend);
+    Optional<DeclContext *> FromFriendDC = getDCOfUnderlyingDecl(D);
+    if (FromFriendDC) { // The underlying friend type is not dependent.
+      ExpectedDecl FriendDCDeclOrErr = import(cast<Decl>(*FromFriendDC));
+      if (!FriendDCDeclOrErr)
+        return FriendDCDeclOrErr.takeError();
+      DeclContext *FriendDC = cast<DeclContext>(*FriendDCDeclOrErr);
+      if (ImportedFriendDC != FriendDC)
+        continue;
+    }
+
     if (D->getFriendDecl() && ImportedFriend->getFriendDecl()) {
       if (IsStructuralMatch(D->getFriendDecl(), ImportedFriend->getFriendDecl(),
                             /*Complain=*/false))
         return Importer.MapImported(D, ImportedFriend);
 
     } else if (D->getFriendType() && ImportedFriend->getFriendType()) {
       if (Importer.IsStructurallyEquivalent(
             D->getFriendType()->getType(),
             ImportedFriend->getFriendType()->getType(), true))
         return Importer.MapImported(D, ImportedFriend);
     }
-    ImportedFriend = ImportedFriend->getNextFriend();
   }
 
   // Not found. Create it.
   FriendDecl::FriendUnion ToFU;
   if (NamedDecl *FriendD = D->getFriendDecl()) {
     NamedDecl *ToFriendD;
     if (Error Err = importInto(ToFriendD, FriendD))
       return std::move(Err);

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, FriendFunInClassTemplate) {
 
   TranslationUnitDecl *FromTU = getTuDecl(Code, Lang_CXX, "input.cc");
   auto *FromFoo = FirstDeclMatcher<FunctionDecl>().match(
       FromTU, functionDecl(hasName("foo")));
   auto *ImportedFoo = Import(FromFoo, Lang_CXX);
   EXPECT_EQ(ImportedFoo, ToFoo);
 }
 
+TEST_P(ImportFriendClasses, ImportOfInlineFriendClassAfterFwdWithSameName) {
+  auto Code =
+      R"(
+      class Container {
+        friend class X; // A friend class ::X
+        class X{ };
+        friend class X; // Friend class Container::X
+      };
+      )";
+  Decl *FromTu = getTuDecl(Code, Lang_CXX, "from.cc");
+
+  auto *To = Import(FirstDeclMatcher<CXXRecordDecl>().match(
+                        FromTu, cxxRecordDecl(hasName("Container"))),
+                    Lang_CXX);
+  ASSERT_TRUE(To);
+
+  Decl *ToTu = To->getTranslationUnitDecl();
+  auto *ToFriend1 = FirstDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *ToFriend2 = LastDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  auto *ToX = FirstDeclMatcher<RecordDecl>().match(
+      ToTu, cxxRecordDecl(hasName("X"), unless(isImplicit())));
+
+  EXPECT_NE(ToFriend1, ToFriend2);
+  const RecordDecl *RecordOfFriend1 = getRecordDeclOfFriend(ToFriend1);
+  const RecordDecl *RecordOfFriend2 = getRecordDeclOfFriend(ToFriend2);
+  EXPECT_NE(RecordOfFriend1, ToX);
+  EXPECT_EQ(RecordOfFriend2, ToX);
+}
+
 struct DeclContextTest : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
   Decl *TU = getTuDecl(
       R"(
       namespace NS {
 
       template <typename T>