From 4becf68c6f17fe143539ceac954b21175914e1c1 Mon Sep 17 00:00:00 2001 From: Gabor Marton Date: Wed, 4 Dec 2019 17:12:08 +0100 Subject: [PATCH] [ASTImporter] Friend class decl should not be visible in its context 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. Reviewers: balazske, a_sidorin, a.sidorin, shafik Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, teemperor, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D71020 --- clang/lib/AST/ASTImporter.cpp | 79 ++++++++++++++----------- clang/unittests/AST/ASTImporterTest.cpp | 50 ++++++++++++++-- 2 files changed, 88 insertions(+), 41 deletions(-) diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 414092f33c472..336ee88f994a5 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -298,6 +298,45 @@ namespace clang { 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(FromD)) { + auto *ToNamed = cast(ToD); + DeclContextLookupResult FromLookup = + FromDC->lookup(FromNamed->getDeclName()); + if (llvm::is_contained(FromLookup, FromNamed)) + ToDC->makeDeclVisibleInContext(ToNamed); + } + } + } + public: explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {} @@ -2737,11 +2776,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { 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()) { @@ -2807,7 +2842,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { Name.getAsIdentifierInfo(), PrevDecl)) return D2; D2->setLexicalDeclContext(LexicalDC); - LexicalDC->addDeclInternal(D2); + addDeclToContexts(D, D2); } if (auto BraceRangeOrErr = import(D->getBraceRange())) @@ -3386,23 +3421,7 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) { 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(D)) if (Error Err = ImportOverriddenMethods(cast(ToFunction), @@ -3850,10 +3869,7 @@ ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) { 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) { @@ -5133,7 +5149,6 @@ template static auto getTemplateDefinition(T *D) -> T * { } ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { - bool IsFriend = D->getFriendObjectKind() != Decl::FOK_None; // Import the major distinguishing characteristics of this class template. DeclContext *DC, *LexicalDC; @@ -5210,10 +5225,7 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { 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 = @@ -5239,9 +5251,6 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { D2->setPreviousDecl(Recent); } - if (LexicalDC != DC && IsFriend) - DC->makeDeclVisibleInContext(D2); - if (FromTemplated->isCompleteDefinition() && !ToTemplated->isCompleteDefinition()) { // FIXME: Import definition! diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index 6652111fd48e1..3e8f804374f45 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -247,6 +247,11 @@ template RecordDecl *getRecordDecl(T *D) { return cast(ET->getNamedType().getTypePtr())->getDecl(); } +static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) { + QualType Ty = FD->getFriendType()->getType().getCanonicalType(); + return cast(Ty)->getDecl(); +} + struct ImportExpr : TestImportBase {}; struct ImportType : TestImportBase {}; struct ImportDecl : TestImportBase {}; @@ -2707,7 +2712,7 @@ TEST_P(ImportFriendFunctions, Lookup) { 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")); @@ -3778,6 +3783,44 @@ TEST_P(ImportFriendClasses, ImportOfRecursiveFriendClass) { EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern)); } +TEST_P(ImportFriendClasses, UndeclaredFriendClassShouldNotBeVisible) { + Decl *FromTu = getTuDecl("class X { friend class Y; };", Lang_CXX, "from.cc"); + auto *FromX = FirstDeclMatcher().match( + FromTu, cxxRecordDecl(hasName("X"))); + auto *FromFriend = FirstDeclMatcher().match(FromTu, friendDecl()); + RecordDecl *FromRecordOfFriend = + const_cast(getRecordDeclOfFriend(FromFriend)); + + ASSERT_EQ(FromRecordOfFriend->getDeclContext(), cast(FromTu)); + ASSERT_EQ(FromRecordOfFriend->getLexicalDeclContext(), + cast(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().match(ToTu, friendDecl()); + RecordDecl *ToRecordOfFriend = + const_cast(getRecordDeclOfFriend(ToFriend)); + + ASSERT_EQ(ToRecordOfFriend->getDeclContext(), cast(ToTu)); + ASSERT_EQ(ToRecordOfFriend->getLexicalDeclContext(), cast(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"( @@ -4477,11 +4520,6 @@ TEST_P(ASTImporterLookupTableTest, LookupDeclNamesFromDifferentTUs) { ASSERT_EQ(Res.size(), 0u); } -static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) { - QualType Ty = FD->getFriendType()->getType().getCanonicalType(); - return cast(Ty)->getDecl(); -} - TEST_P(ASTImporterLookupTableTest, LookupFindsFwdFriendClassDeclWithElaboratedType) { TranslationUnitDecl *ToTU = getToTuDecl(