[ASTImporter] Friend class decl should not be visible in its context

[martong]

Summary

In the past we had to use DeclContext::makeDeclVisibleInContext to make
friend declarations available for subsequent lookup calls and this way
we could chain (redecl) the structurally equivalent decls.
By doing this we created an AST that improperly made declarations
visible in some contexts, so the AST was malformed.
Since we use the importer specific lookup this is no longer necessary,
because with that we can find every previous nodes.

[a_sidorin]

Hi Gabor,
This patch looks mostly good to me. Thanks!

clang/lib/AST/ASTImporter.cpp

334

llvm::is_contained?

clang/unittests/AST/ASTImporterTest.cpp

260

Why don't we use getCanonicalType() anymore?

[martong]

Addressing Alexeis comments.

clang/lib/AST/ASTImporter.cpp

334

Thanks! Good catch.

clang/unittests/AST/ASTImporterTest.cpp

260

Thanks, that is a very good catch.

[a_sidorin]

Hi Gabor,
I have an inline comment for the patch. Otherwise, looks good.

clang/unittests/AST/ASTImporterTest.cpp

252

I wonder if ElaboratedType can be a canonical type because it is only a sugar type itself. Do we need to handle this case?

[martong]

Simplify getRecordDeclOfFriend() further.

[martong]

clang/unittests/AST/ASTImporterTest.cpp

252

You are right, I removed this case. Thanks!

[a_sidorin]

LGTM, thanks!

[stella.stamenova]

This broke the windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/11751

[thakis]

Looks like this breaks building on Windows in general: http://45.33.8.238/win/4247/step_4.txt

[thakis]

Reverted in 55c55f8eb86ba3e77fe for now.

[martong]

Recommited in bc5b7e21e32 . I changed llvm:is_contained to a simple for loop over the lookup result. This way the copy assignment of the iterator is avoided even if windows STL is used.

clang/lib/AST/ASTImporter.cpp

// Wrapper for an overload set.
     Decl *FindAndMapDefinition(FunctionDecl *D, FunctionDecl *FoundFunction) {
       const FunctionDecl *Definition = nullptr;
       if (D->doesThisDeclarationHaveABody() &&
           FoundFunction->hasBody(Definition))
         return Importer.MapImported(D, const_cast<FunctionDecl *>(Definition));
       return nullptr;
     }
 
+    void addDeclToContexts(Decl *FromD, Decl *ToD) {
+      if (Importer.isMinimalImport()) {
+        // In minimal import case the decl must be added even if it is not
+        // contained in original context, for LLDB compatibility.
+        // FIXME: Check if a better solution is possible.
+        if (!FromD->getDescribedTemplate() &&
+            FromD->getFriendObjectKind() == Decl::FOK_None)
+          ToD->getLexicalDeclContext()->addDeclInternal(ToD);
+        return;
+      }
+
+      DeclContext *FromDC = FromD->getDeclContext();
+      DeclContext *FromLexicalDC = FromD->getLexicalDeclContext();
+      DeclContext *ToDC = ToD->getDeclContext();
+      DeclContext *ToLexicalDC = ToD->getLexicalDeclContext();
+
+      bool Visible = false;
+      if (FromDC->containsDeclAndLoad(FromD)) {
+        ToDC->addDeclInternal(ToD);
+        Visible = true;
+      }
+      if (ToDC != ToLexicalDC && FromLexicalDC->containsDeclAndLoad(FromD)) {
+        ToLexicalDC->addDeclInternal(ToD);
+        Visible = true;
+      }
+
+      // If the Decl was added to any context, it was made already visible.
+      // Otherwise it is still possible that it should be visible.
+      if (!Visible) {
+        if (auto *FromNamed = dyn_cast<NamedDecl>(FromD)) {
+          auto *ToNamed = cast<NamedDecl>(ToD);
+          DeclContextLookupResult FromLookup =
+              FromDC->lookup(FromNamed->getDeclName());
+          if (llvm::is_contained(FromLookup, FromNamed))
+            ToDC->makeDeclVisibleInContext(ToNamed);
+        }
+      }
+    }
+
   public:
     explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {}
 
     using TypeVisitor<ASTNodeImporter, ExpectedType>::Visit;
     using DeclVisitor<ASTNodeImporter, ExpectedDecl>::Visit;
     using StmtVisitor<ASTNodeImporter, ExpectedStmt>::Visit;
 
     // Importing types
if (DCXX->isLambda()) {
                                   Name.getAsIdentifierInfo(),
                                   cast_or_null<CXXRecordDecl>(PrevDecl)))
         return D2CXX;
     }
 
     D2 = D2CXX;
     D2->setAccess(D->getAccess());
     D2->setLexicalDeclContext(LexicalDC);
-    if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
-      LexicalDC->addDeclInternal(D2);
-
-    if (LexicalDC != DC && D->isInIdentifierNamespace(Decl::IDNS_TagFriend))
-      DC->makeDeclVisibleInContext(D2);
+    addDeclToContexts(D, D2);
 
     if (ClassTemplateDecl *FromDescribed =
         DCXX->getDescribedClassTemplate()) {
       ClassTemplateDecl *ToDescribed;
       if (Error Err = importInto(ToDescribed, FromDescribed))
         return std::move(Err);
       D2CXX->setDescribedClassTemplate(ToDescribed);
       if (!DCXX->isInjectedClassName() && !IsFriendTemplate) {
if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
     }
 
   } else {
     if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(),
                                 D->getTagKind(), DC, *BeginLocOrErr, Loc,
                                 Name.getAsIdentifierInfo(), PrevDecl))
       return D2;
     D2->setLexicalDeclContext(LexicalDC);
-    LexicalDC->addDeclInternal(D2);
+    addDeclToContexts(D, D2);
   }
 
   if (auto BraceRangeOrErr = import(D->getBraceRange()))
     D2->setBraceRange(*BraceRangeOrErr);
   else
     return BraceRangeOrErr.takeError();
   if (auto QualifierLocOrErr = import(D->getQualifierLoc()))
     D2->setQualifierInfo(*QualifierLocOrErr);
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   }
 
   // FIXME: Other bits to merge?
 
   // If it is a template, import all related things.
   if (Error Err = ImportTemplateInformation(D, ToFunction))
     return std::move(Err);
 
-  bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend);
-
-  // TODO Can we generalize this approach to other AST nodes as well?
-  if (D->getDeclContext()->containsDeclAndLoad(D))
-    DC->addDeclInternal(ToFunction);
-  if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
-    LexicalDC->addDeclInternal(ToFunction);
-
-  // Friend declaration's lexical context is the befriending class, but the
-  // semantic context is the enclosing scope of the befriending class.
-  // We want the friend functions to be found in the semantic context by lookup.
-  // FIXME should we handle this generically in VisitFriendDecl?
-  // In Other cases when LexicalDC != DC we don't want it to be added,
-  // e.g out-of-class definitions like void B::f() {} .
-  if (LexicalDC != DC && IsFriend) {
-    DC->makeDeclVisibleInContext(ToFunction);
-  }
+  addDeclToContexts(D, ToFunction);
 
   if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
     if (Error Err = ImportOverriddenMethods(cast<CXXMethodDecl>(ToFunction),
                                             FromCXXMethod))
       return std::move(Err);
 
   // Import the rest of the chain. I.e. import all subsequent declarations.
   for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) {
ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) {
   }
 
   if (Error Err = ImportInitializer(D, ToVar))
     return std::move(Err);
 
   if (D->isConstexpr())
     ToVar->setConstexpr(true);
 
-  if (D->getDeclContext()->containsDeclAndLoad(D))
-    DC->addDeclInternal(ToVar);
-  if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
-    LexicalDC->addDeclInternal(ToVar);
+  addDeclToContexts(D, ToVar);
 
   // Import the rest of the chain. I.e. import all subsequent declarations.
   for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt) {
     ExpectedDecl RedeclOrErr = import(*RedeclIt);
     if (!RedeclOrErr)
       return RedeclOrErr.takeError();
   }
 
template <typename T> static auto getTemplateDefinition(T *D) -> T * {
   auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)
     return nullptr;
   auto *TemplateWithDef = ToTemplatedDef->getDescribedTemplate();
   return cast_or_null<T>(TemplateWithDef);
 }
 
 ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
-  bool IsFriend = D->getFriendObjectKind() != Decl::FOK_None;
 
   // Import the major distinguishing characteristics of this class template.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return std::move(Err);
if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(), DC, Loc, Name,
                               *TemplateParamsOrErr, ToTemplated))
     return D2;
 
   ToTemplated->setDescribedClassTemplate(D2);
 
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
 
-  if (D->getDeclContext()->containsDeclAndLoad(D))
-    DC->addDeclInternal(D2);
-  if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
-    LexicalDC->addDeclInternal(D2);
+  addDeclToContexts(D, D2);
 
   if (FoundByLookup) {
     auto *Recent =
         const_cast<ClassTemplateDecl *>(FoundByLookup->getMostRecentDecl());
 
     // It is possible that during the import of the class template definition
     // we start the import of a fwd friend decl of the very same class template
     // and we add the fwd friend decl to the lookup table. But the ToTemplated
if (!ToTemplated->getPreviousDecl()) {
           FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
       if (ToTemplated != PrevTemplated)
         ToTemplated->setPreviousDecl(PrevTemplated);
     }
 
     D2->setPreviousDecl(Recent);
   }
 
-  if (LexicalDC != DC && IsFriend)
-    DC->makeDeclVisibleInContext(D2);
-
   if (FromTemplated->isCompleteDefinition() &&
       !ToTemplated->isCompleteDefinition()) {
     // FIXME: Import definition!
   }
 
   return D2;
 }
 

clang/unittests/AST/ASTImporterTest.cpp

public:
   }
 };
 
 template <typename T> RecordDecl *getRecordDecl(T *D) {
   auto *ET = cast<ElaboratedType>(D->getType().getTypePtr());
   return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
 }
 
+static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
+  QualType Ty = FD->getFriendType()->getType().getCanonicalType();
+  if (isa<ElaboratedType>(Ty))
+    Ty = cast<ElaboratedType>(Ty)->getNamedType();
+  return cast<RecordType>(Ty)->getDecl();
+}
+
 struct ImportExpr : TestImportBase {};
 struct ImportType : TestImportBase {};
 struct ImportDecl : TestImportBase {};
 
 struct CanonicalRedeclChain : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(CanonicalRedeclChain, ShouldBeConsequentWithMatchers) {
   Decl *FromTU = getTuDecl("void f();", Lang_CXX);
   auto Pattern = functionDecl(hasName("f"));
   auto *D0 = FirstDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
 
   auto Redecls = getCanonicalForwardRedeclChain(D0);
TEST_P(ImportFriendFunctions, Lookup) {
   EXPECT_EQ(LookupRes.size(), 1u);
 
   EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, FunctionPattern), 1u);
   auto *To0 = FirstDeclMatcher<FunctionDecl>().match(ToTU, FunctionPattern);
   EXPECT_TRUE(To0->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   EXPECT_FALSE(To0->isInIdentifierNamespace(Decl::IDNS_Ordinary));
 }
 
-TEST_P(ImportFriendFunctions, DISABLED_LookupWithProtoAfter) {
+TEST_P(ImportFriendFunctions, LookupWithProtoAfter) {
   auto FunctionPattern = functionDecl(hasName("f"));
   auto ClassPattern = cxxRecordDecl(hasName("X"));
 
   TranslationUnitDecl *FromTU = getTuDecl(
       "struct X { friend void f(); };"
       // This proto decl makes f available to normal
       // lookup, otherwise it is hidden.
       // Normal C++ lookup (implemented in
TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClass) {
   auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
       FromTu, cxxRecordDecl(hasName("declToImport")));
   auto *ToD = Import(FromD, Lang_CXX);
   auto Pattern = cxxRecordDecl(has(friendDecl()));
   ASSERT_TRUE(MatchVerifier<Decl>{}.match(FromD, Pattern));
   EXPECT_TRUE(MatchVerifier<Decl>{}.match(ToD, Pattern));
 }
 
+TEST_P(ImportFriendClasses, UndeclaredFriendClassShouldNotBeVisible) {
+  Decl *FromTu = getTuDecl("class X { friend class Y; };", Lang_CXX, "from.cc");
+  auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTu, cxxRecordDecl(hasName("X")));
+  auto *FromFriend = FirstDeclMatcher<FriendDecl>().match(FromTu, friendDecl());
+  RecordDecl *FromRecordOfFriend =
+      const_cast<RecordDecl *>(getRecordDeclOfFriend(FromFriend));
+
+  ASSERT_EQ(FromRecordOfFriend->getDeclContext(), cast<DeclContext>(FromTu));
+  ASSERT_EQ(FromRecordOfFriend->getLexicalDeclContext(),
+            cast<DeclContext>(FromX));
+  ASSERT_FALSE(
+      FromRecordOfFriend->getDeclContext()->containsDecl(FromRecordOfFriend));
+  ASSERT_FALSE(FromRecordOfFriend->getLexicalDeclContext()->containsDecl(
+      FromRecordOfFriend));
+  ASSERT_FALSE(FromRecordOfFriend->getLookupParent()
+                   ->lookup(FromRecordOfFriend->getDeclName())
+                   .empty());
+
+  auto *ToX = Import(FromX, Lang_CXX);
+  ASSERT_TRUE(ToX);
+
+  Decl *ToTu = ToX->getTranslationUnitDecl();
+  auto *ToFriend = FirstDeclMatcher<FriendDecl>().match(ToTu, friendDecl());
+  RecordDecl *ToRecordOfFriend =
+      const_cast<RecordDecl *>(getRecordDeclOfFriend(ToFriend));
+
+  ASSERT_EQ(ToRecordOfFriend->getDeclContext(), cast<DeclContext>(ToTu));
+  ASSERT_EQ(ToRecordOfFriend->getLexicalDeclContext(), cast<DeclContext>(ToX));
+  EXPECT_FALSE(
+      ToRecordOfFriend->getDeclContext()->containsDecl(ToRecordOfFriend));
+  EXPECT_FALSE(ToRecordOfFriend->getLexicalDeclContext()->containsDecl(
+      ToRecordOfFriend));
+  EXPECT_FALSE(ToRecordOfFriend->getLookupParent()
+                   ->lookup(ToRecordOfFriend->getDeclName())
+                   .empty());
+}
+
 TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClassTemplate) {
   Decl *FromTu = getTuDecl(
       R"(
       template<class A> class declToImport {
         template<class A1> friend class declToImport;
       };
       )",
       Lang_CXX, "input.cc");
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().getCanonicalType();
-  return cast<RecordType>(Ty)->getDecl();
-}
-
 TEST_P(ASTImporterLookupTableTest,
        LookupFindsFwdFriendClassDeclWithElaboratedType) {
   TranslationUnitDecl *ToTU = getToTuDecl(
       R"(
       class Y { friend class F; };
       )",
       Lang_CXX);