[ASTImporter] Fix unhandled cases in ASTImporterLookupTable

[martong]

Summary

In most cases the FriendDecl contains the declaration of the befriended
class as a child node, so it is discovered during the recursive
visitation. However, there are cases when the befriended class is not a
child, thus it must be fetched explicitly from the FriendDecl, and only
then can we add it to the lookup table.
(Note, this does affect only CTU and does not affect LLDB, because we
cannot and do not use the ASTImporterLookupTable in LLDB.)

[a_sidorin]

Hi Gabor,
This looks fine, but I have a question inline.

clang/unittests/AST/ASTImporterTest.cpp

4259 ↗

(On Diff #200043)

Will getCanonicalType() do the job?

[martong]

clang/unittests/AST/ASTImporterTest.cpp

4259 ↗

(On Diff #200043)

That's a good catch! Thank you for pointing out.

[martong]

• Remove getUnderlyingType

[a_sidorin]

clang/unittests/AST/ASTImporterTest.cpp

4249 ↗

(On Diff #200435)

Redundant space after '*'.

4259 ↗

(On Diff #200043)

Can we replace the whole function with:

QualType Ty = FD->getFriendType()->getType().getCanonicalType();
return cast<RecordType>(Ty)->getDecl();

?

[martong]

clang/unittests/AST/ASTImporterTest.cpp

4259 ↗

(On Diff #200043)

Thank you, this is indeed simpler, changed that.

[martong]

• Simplify getRecordDeclOfFriend

cfe/trunk/lib/AST/ASTImporter.cpp

for (auto *FoundDecl : FoundDecls) {
         QualType FromUT = D->getUnderlyingType();
         QualType FoundUT = FoundTypedef->getUnderlyingType();
         if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
           // If the "From" context has a complete underlying type but we
           // already have a complete underlying type then return with that.
           if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
         }
-        // FIXME Handle redecl chain.
+        // FIXME Handle redecl chain. When you do that make consistent changes
+        // in ASTImporterLookupTable too.
         break;
       }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, IDNS,

cfe/trunk/lib/AST/ASTImporterLookupTable.cpp

 
 struct Builder : RecursiveASTVisitor<Builder> {
   ASTImporterLookupTable &LT;
   Builder(ASTImporterLookupTable &LT) : LT(LT) {}
   bool VisitNamedDecl(NamedDecl *D) {
     LT.add(D);
     return true;
   }
+  // In most cases the FriendDecl contains the declaration of the befriended
+  // class as a child node, so it is discovered during the recursive
+  // visitation. However, there are cases when the befriended class is not a
+  // child, thus it must be fetched explicitly from the FriendDecl, and only
+  // then can we add it to the lookup table.
   bool VisitFriendDecl(FriendDecl *D) {
     if (D->getFriendType()) {
       QualType Ty = D->getFriendType()->getType();
-      // FIXME Can this be other than elaborated?
-      QualType NamedTy = cast<ElaboratedType>(Ty)->getNamedType();
-      if (!NamedTy->isDependentType()) {
-        if (const auto *RTy = dyn_cast<RecordType>(NamedTy))
+      if (isa<ElaboratedType>(Ty))
+        Ty = cast<ElaboratedType>(Ty)->getNamedType();
+      // A FriendDecl with a dependent type (e.g. ClassTemplateSpecialization)
+      // always has that decl as child node.
+      // However, there are non-dependent cases which does not have the
+      // type as a child node. We have to dig up that type now.
+      if (!Ty->isDependentType()) {
+        if (const auto *RTy = dyn_cast<RecordType>(Ty))
           LT.add(RTy->getAsCXXRecordDecl());
-        else if (const auto *SpecTy =
-                     dyn_cast<TemplateSpecializationType>(NamedTy)) {
+        else if (const auto *SpecTy = dyn_cast<TemplateSpecializationType>(Ty))
           LT.add(SpecTy->getAsCXXRecordDecl());
+        else if (isa<TypedefType>(Ty)) {
+          // We do not put friend typedefs to the lookup table because
+          // ASTImporter does not organize typedefs into redecl chains.
+        } else {
+          llvm_unreachable("Unhandled type of friend class");
         }
       }
     }
     return true;
   }
 
   // Override default settings of base.
   bool shouldVisitTemplateInstantiations() const { return true; }

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterLookupTableTest, LookupDeclNamesFromDifferentTUs) {
   ASSERT_EQ(Res.size(), 1u);
   EXPECT_EQ(*Res.begin(), ToPlus);
 
   // FromPlus have a different TU, thus its DeclarationName is different too.
   Res = LT.lookup(ToTU, FromPlus->getDeclName());
   ASSERT_EQ(Res.size(), 0u);
 }
 
-static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) {
-  QualType Ty = FD->getFriendType()->getType();
-  QualType NamedTy = cast<ElaboratedType>(Ty)->getNamedType();
-  return cast<RecordType>(NamedTy)->getDecl();
+static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
+  QualType Ty = FD->getFriendType()->getType().getCanonicalType();
+  return cast<RecordType>(Ty)->getDecl();
 }
 
-TEST_P(ASTImporterLookupTableTest, LookupFindsFwdFriendClassDecl) {
+TEST_P(ASTImporterLookupTableTest,
+       LookupFindsFwdFriendClassDeclWithElaboratedType) {
   TranslationUnitDecl *ToTU = getToTuDecl(
       R"(
       class Y { friend class F; };
       )",
       Lang_CXX);
 
   // In this case, the CXXRecordDecl is hidden, the FriendDecl is not a parent.
   // So we must dig up the underlying CXXRecordDecl.
   ASTImporterLookupTable LT(*ToTU);
   auto *FriendD = FirstDeclMatcher<FriendDecl>().match(ToTU, friendDecl());
   const RecordDecl *RD = getRecordDeclOfFriend(FriendD);
   auto *Y = FirstDeclMatcher<CXXRecordDecl>().match(
       ToTU, cxxRecordDecl(hasName("Y")));
 
   DeclarationName Name = RD->getDeclName();
   auto Res = LT.lookup(ToTU, Name);
   EXPECT_EQ(Res.size(), 1u);
   EXPECT_EQ(*Res.begin(), RD);
 
   Res = LT.lookup(Y, Name);
   EXPECT_EQ(Res.size(), 0u);
 }
 
+TEST_P(ASTImporterLookupTableTest,
+       LookupFindsFwdFriendClassDeclWithUnelaboratedType) {
+  TranslationUnitDecl *ToTU = getToTuDecl(
+      R"(
+      class F;
+      class Y { friend F; };
+      )",
+      Lang_CXX11);
+
+  // In this case, the CXXRecordDecl is hidden, the FriendDecl is not a parent.
+  // So we must dig up the underlying CXXRecordDecl.
+  ASTImporterLookupTable LT(*ToTU);
+  auto *FriendD = FirstDeclMatcher<FriendDecl>().match(ToTU, friendDecl());
+  const RecordDecl *RD = getRecordDeclOfFriend(FriendD);
+  auto *Y = FirstDeclMatcher<CXXRecordDecl>().match(ToTU, cxxRecordDecl(hasName("Y")));
+
+  DeclarationName Name = RD->getDeclName();
+  auto Res = LT.lookup(ToTU, Name);
+  EXPECT_EQ(Res.size(), 1u);
+  EXPECT_EQ(*Res.begin(), RD);
+
+  Res = LT.lookup(Y, Name);
+  EXPECT_EQ(Res.size(), 0u);
+}
+
+TEST_P(ASTImporterLookupTableTest,
+       LookupFindsFriendClassDeclWithTypeAliasDoesNotAssert) {
+  TranslationUnitDecl *ToTU = getToTuDecl(
+      R"(
+      class F;
+      using alias_of_f = F;
+      class Y { friend alias_of_f; };
+      )",
+      Lang_CXX11);
+
+  // ASTImporterLookupTable constructor handles using declarations correctly,
+  // no assert is expected.
+  ASTImporterLookupTable LT(*ToTU);
+
+  auto *Alias = FirstDeclMatcher<TypeAliasDecl>().match(
+      ToTU, typeAliasDecl(hasName("alias_of_f")));
+  DeclarationName Name = Alias->getDeclName();
+  auto Res = LT.lookup(ToTU, Name);
+  EXPECT_EQ(Res.count(Alias), 1u);
+}
+
 TEST_P(ASTImporterLookupTableTest, LookupFindsFwdFriendClassTemplateDecl) {
   TranslationUnitDecl *ToTU = getToTuDecl(
       R"(
       class Y { template <class T> friend class F; };
       )",
       Lang_CXX);
 
   ASTImporterLookupTable LT(*ToTU);