[clang][ASTImporter] Import TemplateName correctly

[balazske]

Summary

This is a fix for a problem when multiple template
specializations are created by ASTImporter for the
same specialization. The problem happens if a
TemplateName is imported that points to a template
delcaration (for a template template argument)
(specialization) that has multiple instances in the
declaration chain. If two TemplateName objects contain
different pointers to a template specialization,
these TemplateName objects will have different checksum
even if they point into the same declaration chain.
The problem is fixed if the canonical declaration is used.

[vabridgers]

This patch needs a unit test (as @balazske mentioned). So far, the case we have is too large to be a suitable unittest or lit case - so requires reduction. @balazske , will you be adding this as a unittest or lit case?

Also, I think this patch needs to be integrated with D144273 IMHO, since they address similar issues.

I verified the patch stack D144273, D140562, and D144622 (this one) addresses the problems we've seen as a result of D133468. When the patches are ready, a brief history should be included in the commit header.

[balazske]

I think these patches are fix for separate problems and can be applied independently. It is not better if these are moved into one change. The other patch D144273 is not finished (it can get bigger), and a test for this change is needed. I try still to discover what the exact problem is here, then I can add an unit test.

[balazske]

Added a test case.

[donat.nagy]

Basically LGTM (assuming that the TC passes), I added two minor suggestions, but I'm not opposed to merging this in its current state.

clang/unittests/AST/ASTImporterTest.cpp

8142–8150

Nitpick: consider saving this string in a constant instead of repeating it twice.

8201–8202

For clarity, perhaps also assert that Spec1 != Spec2.

[balazske]

Updated the test.

[vabridgers]

LGTM

[donat.nagy]

LGTM as well.

clang/lib/AST/ASTImporter.cpp

ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
 
   return Builder.getWithLocInContext(getToContext());
 }
 
 Expected<TemplateName> ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
     if (ExpectedDecl ToTemplateOrErr = Import(From.getAsTemplateDecl()))
-      return TemplateName(cast<TemplateDecl>(*ToTemplateOrErr));
+      return TemplateName(cast<TemplateDecl>((*ToTemplateOrErr)->getCanonicalDecl()));
     else
       return ToTemplateOrErr.takeError();
 
   case TemplateName::OverloadedTemplate: {
     OverloadedTemplateStorage *FromStorage = From.getAsOverloadedTemplate();
     UnresolvedSet<2> ToTemplates;
     for (auto *I : *FromStorage) {
       if (auto ToOrErr = Import(I))

clang/unittests/AST/ASTImporterTest.cpp

auto *ToBar = FirstDeclMatcher<FunctionDecl>().match(
       ToTU, functionDecl(hasName("bar")));
 
   EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
 struct ImportInjectedClassNameType : public ASTImporterOptionSpecificTestBase {
 protected:
   const CXXRecordDecl *findInjected(const CXXRecordDecl *Parent) {
     for (Decl *Found : Parent->decls()) {
       const auto *Record = dyn_cast<CXXRecordDecl>(Found);
       if (Record && Record->isInjectedClassName())
         return Record;
     }
     return nullptr;
   }
 
   void checkInjType(const CXXRecordDecl *D) {
     // The whole redecl chain should have the same InjectedClassNameType
     // instance. The injected record declaration is a separate chain, this
     // should contain the same type too.
     const Type *Ty = nullptr;
     for (const Decl *ReD : D->redecls()) {
       const auto *ReRD = cast<CXXRecordDecl>(ReD);
       EXPECT_TRUE(ReRD->getTypeForDecl());
const char *ToCodeC =
       template <class T>
       struct C;
       )";
   const char *ToCodeCDef =
       R"(
       template <class T>
       struct A {
         typedef A T1;
       };
 
       template <class T1, class T2>
       struct B {};
 
       template<class T>
       struct C {
         typedef typename A<T>::T1 T1;
         typedef B<T1, T> T2;
         typedef B<T1, C> T3;
QualType ToInjParmVar =
       ToF->parameters()[0]->getType().getDesugaredType(ToCtx);
   ToInjParmVar =
       ToInjParmVar->getAs<PointerType>()->getPointeeType().getCanonicalType();
   EXPECT_TRUE(isa<InjectedClassNameType>(ToInjTypedef));
   EXPECT_TRUE(isa<InjectedClassNameType>(ToInjParmVar));
   EXPECT_TRUE(ToCtx.hasSameType(ToInjTypedef, ToInjParmVar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportCorrectTemplateName) {
+  constexpr auto TestCode = R"(
+      template <class T>
+      struct A;
+      template <class T>
+      struct A {};
+      template <template<class> class T = A>
+      struct B {};
+      using C = B<>;
+      )";
+  Decl *ToTU = getToTuDecl(TestCode, Lang_CXX11);
+  Decl *FromTU = getTuDecl(TestCode, Lang_CXX11);
+
+  auto *ToUsingFirst = FirstDeclMatcher<TypeAliasDecl>().match(
+      ToTU, typeAliasDecl(hasName("C")));
+
+  auto *FromUsing = FirstDeclMatcher<TypeAliasDecl>().match(
+      FromTU, typeAliasDecl(hasName("C")));
+  auto *ToUsing = Import(FromUsing, Lang_CXX11);
+  EXPECT_TRUE(ToUsing);
+
+  auto *ToB = FirstDeclMatcher<ClassTemplateDecl>().match(
+      ToTU, classTemplateDecl(hasName("B")));
+  auto *ToB1 = LastDeclMatcher<ClassTemplateDecl>().match(
+      ToTU, classTemplateDecl(hasName("B")));
+  // One template definition of 'B' should exist.
+  EXPECT_EQ(ToB, ToB1);
+
+  // These declarations are imported separately.
+  EXPECT_NE(ToUsingFirst, ToUsing);
+
+  auto SpB = ToB->spec_begin();
+  auto SpE = ToB->spec_end();
+  EXPECT_TRUE(SpB != SpE);
+  ClassTemplateSpecializationDecl *Spec1 = *SpB;
+  ++SpB;
+  // The template 'B' should have one specialization (with default argument).
+  EXPECT_TRUE(SpB == SpE);
+
+  // Even if 'B' has one specialization with the default arguments, the AST
+  // contains after the import two specializations that are linked in the
+  // declaration chain. The 'spec_begin' iteration does not find these because
+  // the template arguments are the same. But the imported type alias has the
+  // link to the second specialization. The template name object in these
+  // specializations must point to the same (and one) instance of definition of
+  // 'B'.
+  auto *Spec2 = cast<ClassTemplateSpecializationDecl>(
+      ToUsing->getUnderlyingType()
+          ->getAs<TemplateSpecializationType>()
+          ->getAsRecordDecl());
+  EXPECT_NE(Spec1, Spec2);
+  EXPECT_TRUE(Spec1->getPreviousDecl() == Spec2 ||
+              Spec2->getPreviousDecl() == Spec1);
+  TemplateDecl *Templ1 =
+      Spec1->getTemplateArgs()[0].getAsTemplate().getAsTemplateDecl();
+  TemplateDecl *Templ2 =
+      Spec2->getTemplateArgs()[0].getAsTemplate().getAsTemplateDecl();
+  EXPECT_EQ(Templ1, Templ2);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);