[ASTImporter] Remove extranous FunctionTemplateDecl introduced by templated friend

[danix800]

Summary

An extranous FunctionTemplateDecl is introduced in the following testcase:

template <typename T> struct A {
  template <typename U> friend void f();
};

From:

ClassTemplateDecl 0x55dae7001500 <input.cc:1:1, col:73> col:30 A
|-TemplateTypeParmDecl 0x55dae70013b0 <col:11, col:20> col:20 typename depth 0 index 0 T
`-CXXRecordDecl 0x55dae7001470 <col:23, col:73> col:30 struct A definition
  |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl 0x55dae7001750 <col:23, col:30> col:30 implicit struct A
  `-FriendDecl 0x55dae7001a68 <col:35, col:71> col:69
    `-FunctionTemplateDecl 0x55dae70019a0 parent 0x55dae6f97e28 <col:35, col:71> col:69 f
      |-TemplateTypeParmDecl 0x55dae70017e0 <col:45, col:54> col:54 typename depth 1 index 0 U
      `-FunctionDecl 0x55dae70018e8 parent 0x55dae6f97e28 <col:57, col:71> col:69 f 'void ()'

To:

ClassTemplateDecl 0x55dae7116618 <input.cc:1:1, col:73> col:30 A
|-TemplateTypeParmDecl 0x55dae7116490 <col:11, col:20> col:20 typename depth 0 index 0 T
`-CXXRecordDecl 0x55dae7116550 <col:23, col:73> col:30 struct A definition
  |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-FunctionTemplateDecl 0x55dae7116a38 parent 0x55dae6fa2b68 <col:35, col:71> col:69 f                            // extranous node
  | |-TemplateTypeParmDecl 0x55dae7116860 <col:45, col:54> col:54 typename depth 1 index 0 U
  | `-FunctionDecl 0x55dae7116968 parent 0x55dae6fa2b68 <col:57, col:71> col:69 f 'void ()'
  |-FriendDecl 0x55dae7116aa0 <col:35, col:71> col:69
  | `-FunctionTemplateDecl 0x55dae7116a38 parent 0x55dae6fa2b68 <col:35, col:71> col:69 f
  |   |-TemplateTypeParmDecl 0x55dae7116860 <col:45, col:54> col:54 typename depth 1 index 0 U
  |   `-FunctionDecl 0x55dae7116968 parent 0x55dae6fa2b68 <col:57, col:71> col:69 f 'void ()'
  `-CXXRecordDecl 0x55dae7116ae0 <col:23, col:30> col:30 implicit struct A

clang-import-test would crash on this case:

clang-import-test: /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ExternalASTMerger.cpp:533: auto clang::ExternalASTMerger::FindExternalLexicalDecls(const clang::DeclContext *, llvm::function_ref<bool (Decl::Kind)>, SmallVectorImpl<clang::Decl *> &)::(anonymous class)::operator()(clang::ASTImporter &, clang::ASTImporter &, Source<const clang::DeclContext *>) const: Assertion `!(*ImportedDeclOrErr) || IsSameDC((*ImportedDeclOrErr)->getDeclContext(), DC)' failed.
f #0 0x00007fec39df812a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:602:11
 #1 0x00007fec39df82db PrintStackTraceSignalHandler(void*) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:675:1
 #2 0x00007fec39df6846 llvm::sys::RunSignalHandlers() /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00007fec39df8af5 SignalHandler(int) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:413:1
 #4 0x00007fec3985afd0 (/lib/x86_64-linux-gnu/libc.so.6+0x3bfd0)
 #5 0x00007fec398a9d3c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #6 0x00007fec3985af32 raise ./signal/../sysdeps/posix/raise.c:27:6
 #7 0x00007fec39845472 abort ./stdlib/abort.c:81:7
 #8 0x00007fec39845395 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #9 0x00007fec39853e32 (/lib/x86_64-linux-gnu/libc.so.6+0x34e32)
#10 0x00007fec3c90c203 clang::ExternalASTMerger::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&)::$_5::operator()(clang::ASTImporter&, clang::ASTImporter&, (anonymous namespace)::Source<clang::DeclContext const*>) const /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ExternalASTMerger.cpp:532:11
#11 0x00007fec3c9094a7 void clang::ExternalASTMerger::ForEachMatchingDC<clang::ExternalASTMerger::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&)::$_5>(clang::DeclContext const*, clang::ExternalASTMerger::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&)::$_5) /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ExternalASTMerger.cpp:283:3
#12 0x00007fec3c9093e1 clang::ExternalASTMerger::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ExternalASTMerger.cpp:540:1
#13 0x00007fec3c6e6262 clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::SmallVectorImpl<clang::Decl*>&) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ExternalASTSource.h:186:3
#14 0x00007fec3c6e1d14 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/DeclBase.cpp:1446:7
#15 0x00007fec3c6e218f clang::DeclContext::decls_begin() const /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/DeclBase.cpp:0:5
#16 0x00007fec3275b7c9 clang::DeclContext::decls() const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/DeclBase.h:2206:48
#17 0x00007fec33d4c04e clang::Sema::InstantiateClass(clang::SourceLocation, clang::CXXRecordDecl*, clang::CXXRecordDecl*, clang::MultiLevelTemplateArgumentList const&, clang::TemplateSpecializationKind, bool) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaTemplateInstantiate.cpp:3239:32
#18 0x00007fec33d4d7b9 clang::Sema::InstantiateClassTemplateSpecialization(clang::SourceLocation, clang::ClassTemplateSpecializationDecl*, clang::TemplateSpecializationKind, bool) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaTemplateInstantiate.cpp:3740:3
#19 0x00007fec33f0a038 clang::Sema::RequireCompleteTypeImpl(clang::SourceLocation, clang::QualType, clang::Sema::CompleteTypeKind, clang::Sema::TypeDiagnoser*)::$_3::operator()() const /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaType.cpp:9268:23
#20 0x00007fec33f09fe5 void llvm::function_ref<void ()>::callback_fn<clang::Sema::RequireCompleteTypeImpl(clang::SourceLocation, clang::QualType, clang::Sema::CompleteTypeKind, clang::Sema::TypeDiagnoser*)::$_3>(long) /home/xxxxx/Sources/llvm-project-main/llvm/include/llvm/ADT/STLFunctionalExtras.h:45:5
#21 0x00007fec329104a9 llvm::function_ref<void ()>::operator()() const /home/xxxxx/Sources/llvm-project-main/llvm/include/llvm/ADT/STLFunctionalExtras.h:68:5
#22 0x00007fec328f61dd clang::runWithSufficientStackSpace(llvm::function_ref<void ()>, llvm::function_ref<void ()>) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/Basic/Stack.h:52:3
#23 0x00007fec328e2230 clang::Sema::runWithSufficientStackSpace(clang::SourceLocation, llvm::function_ref<void ()>) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/Sema.cpp:513:1
#24 0x00007fec33eeb714 clang::Sema::RequireCompleteTypeImpl(clang::SourceLocation, clang::QualType, clang::Sema::CompleteTypeKind, clang::Sema::TypeDiagnoser*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaType.cpp:9272:22
#25 0x00007fec33eeaff3 clang::Sema::RequireCompleteType(clang::SourceLocation, clang::QualType, clang::Sema::CompleteTypeKind, clang::Sema::TypeDiagnoser&) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaType.cpp:8992:7
#26 0x00007fec33eec333 clang::Sema::RequireCompleteType(clang::SourceLocation, clang::QualType, clang::Sema::CompleteTypeKind, unsigned int) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaType.cpp:9337:10
#27 0x00007fec328f884d clang::Sema::RequireCompleteType(clang::SourceLocation, clang::QualType, unsigned int) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/Sema/Sema.h:2532:5
#28 0x00007fec32dd1518 clang::Sema::ActOnUninitializedDecl(clang::Decl*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Sema/SemaDecl.cpp:13962:11
#29 0x00007fec3dd0f188 clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseDecl.cpp:0:13
#30 0x00007fec3dd0d083 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseDecl.cpp:2264:9
#31 0x00007fec3dd0c0ad clang::Parser::ParseSimpleDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, bool, clang::Parser::ForRangeInit*, clang::SourceLocation*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseDecl.cpp:1957:10
#32 0x00007fec3dd0bc69 clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, clang::SourceLocation*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseDecl.cpp:1881:12
#33 0x00007fec3de16da9 clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&, clang::ParsedAttributes&) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseStmt.cpp:249:16
#34 0x00007fec3de1670b clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseStmt.cpp:117:20
#35 0x00007fec3de1f51e clang::Parser::ParseCompoundStatementBody(bool) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseStmt.cpp:1205:11
#36 0x00007fec3de20c24 clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseStmt.cpp:2468:21
#37 0x00007fec3de4b2df clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:1475:3
#38 0x00007fec3dd0cc5b clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseDecl.cpp:2199:27
#39 0x00007fec3de4a151 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:1214:10
#40 0x00007fec3de4963f clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:1229:12
#41 0x00007fec3de48f04 clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:1040:14
#42 0x00007fec3de46dcc clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:742:12
#43 0x00007fec3de464c0 clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/Parser.cpp:594:8
#44 0x00007fec3dcea898 clang::ParseAST(clang::Sema&, bool, bool) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseAST.cpp:162:15
#45 0x00007fec3dcea6b8 clang::ParseAST(clang::Preprocessor&, clang::ASTConsumer*, clang::ASTContext&, bool, clang::TranslationUnitKind, clang::CodeCompleteConsumer*, bool) /home/xxxxx/Sources/llvm-project-main/clang/lib/Parse/ParseAST.cpp:113:1
#46 0x000055c2677c497e (anonymous namespace)::ParseSource(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, clang::CompilerInstance&, clang::ASTConsumer&) /home/xxxxx/Sources/llvm-project-main/clang/tools/clang-import-test/clang-import-test.cpp:302:10
#47 0x000055c2677c3d07 (anonymous namespace)::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, llvm::MutableArrayRef<(anonymous namespace)::CIAndOrigins>, bool, bool) /home/xxxxx/Sources/llvm-project-main/clang/tools/clang-import-test/clang-import-test.cpp:335:19
#48 0x000055c2677c382b main /home/xxxxx/Sources/llvm-project-main/clang/tools/clang-import-test/clang-import-test.cpp:384:29
#49 0x00007fec398461ca __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#50 0x00007fec39846285 call_init ./csu/../csu/libc-start.c:128:20
#51 0x00007fec39846285 __libc_start_main ./csu/../csu/libc-start.c:347:5
#52 0x000055c2677c29f1 _start (/home/xxxxx/Sources/llvm-project-main//build/bin/clang-import-test+0x279f1)

[balazske]

It is better to add a test to ASTImporterTests.cpp with the same code, and check the properties of DeclContext and LexicalDeclContext in the From and To AST (should be the same in "From" and "To" side).

clang/lib/AST/ASTImporter.cpp

6451

addDeclToContexts(D, ToFunction) should be better

[danix800]

1. Add unit testcase
2. Use better API

[balazske]

When committing this patch, the commit message should not contain the whole AST and crash dump (phabricator takes the "summary" text), this looks too much in a commit message. One AST dump from the wrong To AST is enough.

clang/test/Import/templated-friend/test.cpp

5 ↗

(On Diff #550160)

These test files are really not needed. The same AST import is performed in the unit test.

clang/unittests/AST/ASTImporterTest.cpp

5644

This formatting looks not correct.

[danix800]

In D157691#4587490, @balazske wrote:

When committing this patch, the commit message should not contain the whole AST and crash dump (phabricator takes the "summary" text), this looks too much in a commit message. One AST dump from the wrong To AST is enough.

👍

[danix800]

1. Remove unnecessary testcase.
2. Format unittest code.

[balazske]

Additionally, use the "[clang][ASTImporter]" tags at commit message.

clang/lib/AST/ASTImporter.cpp

ASTNodeImporter::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
   // "adoption" happens multiple times with different parent, even recursively
   // for TemplateTemplateParmDecl. The same happens at import when the
   // FunctionTemplateDecl objects are created, but in different order.
   // In this way the DeclContext of these template parameters is not necessarily
   // the same as in the "from" context.
   SmallVector<DeclContext *, 2> OldParamDC;
   OldParamDC.reserve(Params->size());
   llvm::transform(*Params, std::back_inserter(OldParamDC),
                   [](NamedDecl *ND) { return ND->getDeclContext(); });
 
   FunctionTemplateDecl *ToFunc;
   if (GetImportedOrCreateDecl(ToFunc, D, Importer.getToContext(), DC, Loc, Name,
                               Params, TemplatedFD))
     return ToFunc;
 
   TemplatedFD->setDescribedFunctionTemplate(ToFunc);
 
   ToFunc->setAccess(D->getAccess());
   ToFunc->setLexicalDeclContext(LexicalDC);
-  LexicalDC->addDeclInternal(ToFunc);
+  addDeclToContexts(D, ToFunc);
 
   ASTImporterLookupTable *LT = Importer.SharedState->getLookupTable();
   if (LT && !OldParamDC.empty()) {
     for (unsigned int I = 0; I < OldParamDC.size(); ++I)
       LT->updateForced(Params->getParam(I), OldParamDC[I]);
   }
 
   if (FoundByLookup) {

clang/unittests/AST/ASTImporterTest.cpp

Decl *FromTU = getTuDecl(
       Lang_CXX03);
   auto *FromFoo = FirstDeclMatcher<FunctionTemplateDecl>().match(
       FromTU, functionTemplateDecl(hasName("foo")));
   auto *Imported = Import(FromFoo, Lang_CXX03);
 
   EXPECT_EQ(Imported->getPreviousDecl(), Friend);
 }
 
+TEST_P(ImportFriendFunctionTemplates, ImportFriendFunctionInsideClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"(
+      template <typename T> struct X {
+        template <typename U> friend void f();
+      };
+      )",
+      Lang_CXX03, "", Lang_CXX03, "X");
+
+  auto *FromFriend = FirstDeclMatcher<FriendDecl>().match(From, friendDecl());
+  auto *ToFriend = FirstDeclMatcher<FriendDecl>().match(To, friendDecl());
+
+  EXPECT_TRUE(FromFriend ==
+              LastDeclMatcher<FriendDecl>().match(From, friendDecl()));
+  EXPECT_TRUE(ToFriend ==
+              LastDeclMatcher<FriendDecl>().match(To, friendDecl()));
+
+  auto *FromDecl = FromFriend->getFriendDecl();
+  auto *FromDC = FromFriend->getDeclContext();
+  auto *FromLexicalDC = FromFriend->getLexicalDeclContext();
+
+  EXPECT_TRUE(FromDC->containsDecl(FromFriend));
+  EXPECT_FALSE(FromDC->containsDecl(FromDecl));
+  EXPECT_TRUE(FromLexicalDC->containsDecl(FromFriend));
+  EXPECT_FALSE(FromLexicalDC->containsDecl(FromDecl));
+
+  auto *ToDecl = ToFriend->getFriendDecl();
+  auto *ToDC = ToFriend->getDeclContext();
+  auto *ToLexicalDC = ToFriend->getLexicalDeclContext();
+
+  EXPECT_TRUE(ToDC->containsDecl(ToFriend));
+  EXPECT_FALSE(ToDC->containsDecl(ToDecl));
+  EXPECT_TRUE(ToLexicalDC->containsDecl(ToFriend));
+  EXPECT_FALSE(ToLexicalDC->containsDecl(ToDecl));
+}
+
 struct ASTImporterWithFakeErrors : ASTImporter {
   using ASTImporter::ASTImporter;
   bool returnWithErrorInTest() override { return true; }
 };
 
 struct ErrorHandlingTest : ASTImporterOptionSpecificTestBase {
   ErrorHandlingTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,