[ASTImporter] Make the Import() return value consistent with the map of imported decls when merging ClassTemplateSpecializationDecls

[teemperor]

Summary

When importing a ClassTemplateSpecializationDecl definition into a TU with a matching
ClassTemplateSpecializationDecl definition and a more recent forward decl, the ASTImporter
currently will call MapImported() for the definitions, but will return the forward declaration
from the ASTImporter::Import() call.

This is triggering some assertions in LLDB when we try to fully import some DeclContexts
before we delete the 'From' AST. The returned 'To' Decl before this patch is just the most recent
forward decl but that's not the Decl with the definition to which the ASTImporter will import
the child declarations.

This patch just changes that the ASTImporter returns the definition that the imported Decl was
merged with instead of the found forward declaration.

[teemperor]

Can be triggered in LLDB by running something like this twice in LLDB expr --top-level -- template<typename T> struct TL; template<> struct TL<int>; template<typename T> struct TL { int m; }; template<> struct TL<int> { int m; };

[martong]

LGTM! And thanks for the pro unittest!

[shafik]

Great catch!

clang/lib/AST/ASTImporter.cpp

if (PartialSpec) {
     PrevDecl = ClassTemplate->findPartialSpecialization(TemplateArgs,
                                                         *ToTPListOrErr,
                                                         InsertPos);
   } else
     PrevDecl = ClassTemplate->findSpecialization(TemplateArgs, InsertPos);
 
   if (PrevDecl) {
     if (IsStructuralMatch(D, PrevDecl)) {
-      if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
-        Importer.MapImported(D, PrevDecl->getDefinition());
+      CXXRecordDecl *PrevDefinition = PrevDecl->getDefinition();
+      if (D->isThisDeclarationADefinition() && PrevDefinition) {
+        Importer.MapImported(D, PrevDefinition);
         // Import those default field initializers which have been
         // instantiated in the "From" context, but not in the "To" context.
         for (auto *FromField : D->fields()) {
           auto ToOrErr = import(FromField);
           if (!ToOrErr)
             return ToOrErr.takeError();
         }
 
         // Import those methods which have been instantiated in the
         // "From" context, but not in the "To" context.
         for (CXXMethodDecl *FromM : D->methods()) {
           auto ToOrErr = import(FromM);
           if (!ToOrErr)
             return ToOrErr.takeError();
         }
 
         // TODO Import instantiated default arguments.
         // TODO Import instantiated exception specifications.
         //
         // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
         // what else could be fused during an AST merge.
-        return PrevDecl;
+        return PrevDefinition;
       }
     } else { // ODR violation.
       // FIXME HandleNameConflict
       return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
   // Import the location of this declaration.

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase,
   auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
   ASSERT_TRUE(ImportedSpec);
   auto *ToSpec = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
       ToTU, classTemplateSpecializationDecl(hasName("X")));
   EXPECT_EQ(ImportedSpec, ToSpec);
   EXPECT_TRUE(ToFun->hasBody());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, MergeTemplateSpecWithForwardDecl) {
+  std::string ClassTemplate =
+      R"(
+      template<typename T>
+      struct X { int m; };
+      template<>
+      struct X<int> { int m; };
+      )";
+  // Append a forward decl for our template specialization.
+  getToTuDecl(ClassTemplate + "template<> struct X<int>;", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate, Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
+      FromTU, classTemplateSpecializationDecl(hasName("X"), isDefinition()));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  // Check that our definition got merged with the existing definition.
+  EXPECT_TRUE(FromSpec->isThisDeclarationADefinition());
+  EXPECT_TRUE(ImportedSpec->isThisDeclarationADefinition());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
        ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
   std::string ClassTemplate =
       R"(
       template <typename T>
       struct X {};
       )";
   Decl *ToTU = getToTuDecl(ClassTemplate +