[clang] [clang][ASTImporter] New fix for default template parameter values. (PR #101836)

Balázs Kéri via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 30 01:35:00 PDT 2024


https://github.com/balazske updated https://github.com/llvm/llvm-project/pull/101836

>From 2e98fc222566c5e746ade4ccaba23de3b59e0a5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Sat, 3 Aug 2024 18:10:34 +0200
Subject: [PATCH 1/3] [clang][ASTImporter] New fix for default template
 parameter values.

Commit e4440b8 added a change that introduced new crash in an
incorrectly handled case. This is fixed here.
---
 clang/lib/AST/ASTImporter.cpp           | 12 ++-
 clang/unittests/AST/ASTImporterTest.cpp | 97 +++++++++++++++++++++++++
 2 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 103235547f482e..7e4a92ccbe40f7 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -5972,7 +5972,11 @@ ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
+    // The import process can trigger import of the parent template which can
+    // set the default argument value (to "inherited").
+    // In this case do nothing here.
+    if (!ToD->hasDefaultArgument())
+      ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
   }
 
   return ToD;
@@ -6004,7 +6008,8 @@ ASTNodeImporter::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
+    if (!ToD->hasDefaultArgument())
+      ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
   }
 
   return ToD;
@@ -6041,7 +6046,8 @@ ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
+    if (!ToD->hasDefaultArgument())
+      ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
   }
 
   return ToD;
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 57242ff49fe3b8..4c41171deec46a 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9919,6 +9919,103 @@ TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingVarTemplate) {
   testImport(FromLastD);
 }
 
+TEST_P(ImportTemplateParmDeclDefaultValue,
+       ImportParentTemplateDuringNonTypeTemplateParmDecl) {
+  // This wants to provoke that during import of 'Y' in "typename T = Y"
+  // (before this import returns) the later definition of 'X' is imported fully.
+  const char *Code =
+      R"(
+      struct Z;
+
+      struct Y {
+        Z *z;
+        static const int x = 1;
+      };
+
+      template <int P = Y::x>
+      struct X;
+
+      template <int P>
+      struct X {
+        static const int A = 1;
+      };
+
+      struct Z {
+        template<int P>
+        void f(int A = X<P>::A);
+      };
+      )";
+
+  Decl *FromTU = getTuDecl(Code, Lang_CXX14);
+  auto *FromD = FirstDeclMatcher<NonTypeTemplateParmDecl>().match(
+      FromTU, nonTypeTemplateParmDecl(hasName("P")));
+  auto *ToD = Import(FromD, Lang_CXX14);
+  EXPECT_TRUE(ToD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue,
+       ImportParentTemplateDuringTemplateTypeParmDecl) {
+  const char *Code =
+      R"(
+      struct Z;
+
+      struct Y {
+        Z *z;
+      };
+
+      template <typename T = Y>
+      struct X;
+
+      template <typename T>
+      struct X {
+        static const int A = 1;
+      };
+
+      struct Z {
+        template<typename T>
+        void f(int A = X<T>::A);
+      };
+      )";
+
+  Decl *FromTU = getTuDecl(Code, Lang_CXX14);
+  auto *FromD = FirstDeclMatcher<TemplateTypeParmDecl>().match(
+      FromTU, templateTypeParmDecl(hasName("T")));
+  auto *ToD = Import(FromD, Lang_CXX14);
+  EXPECT_TRUE(ToD);
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue,
+       ImportParentTemplateDuringTemplateTemplateParmDecl) {
+  const char *Code =
+      R"(
+      struct Z;
+
+      template <int>
+      struct Y {
+        Z *z;
+      };
+
+      template <template <int> class T = Y>
+      struct X;
+
+      template <template <int> class T>
+      struct X {
+        static const int A = 1;
+      };
+
+      struct Z {
+        template <template <int> class T>
+        void f(int A = X<T>::A);
+      };
+      )";
+
+  Decl *FromTU = getTuDecl(Code, Lang_CXX14);
+  auto *FromD = FirstDeclMatcher<TemplateTemplateParmDecl>().match(
+      FromTU, templateTemplateParmDecl(hasName("T")));
+  auto *ToD = Import(FromD, Lang_CXX14);
+  EXPECT_TRUE(ToD);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 

>From 70ef4f2579bbb91f7babdf62feef0e5c9173fa31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Fri, 16 Aug 2024 09:58:49 +0200
Subject: [PATCH 2/3] added more tests, updated comments

---
 clang/lib/AST/ASTImporter.cpp           | 30 +++++-----
 clang/unittests/AST/ASTImporterTest.cpp | 76 ++++++++++++++++++-------
 2 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 7e4a92ccbe40f7..f8865baa80ae8c 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -384,16 +384,10 @@ namespace clang {
     // following declarations have a reference to the original default value
     // through the "inherited" value. This value should be set for all imported
     // template parameters that have a previous declaration (also a previous
-    // template declaration).
-    //
-    // In the `Visit*ParmDecl` functions the default value of these template
-    // arguments is always imported. At that location the previous declaration
-    // is not easily accessible, it is not possible to call
-    // `setInheritedDefaultArgument` at that place.
-    // `updateTemplateParametersInheritedFrom` is called later when the already
-    // imported default value is erased and changed to "inherited".
-    // It is important to change the mode to "inherited" otherwise false
-    // structural in-equivalences could be detected.
+    // template declaration). The chain of parameter default value inheritances
+    // will have the same order as the chain of previous declarations, in the
+    // "To" AST. The "From" AST may have a different order because import does
+    // not happen sequentially.
     void updateTemplateParametersInheritedFrom(
         const TemplateParameterList &RecentParams,
         TemplateParameterList &NewParams) {
@@ -5934,8 +5928,8 @@ ASTNodeImporter::VisitObjCPropertyImplDecl(ObjCPropertyImplDecl *D) {
 ExpectedDecl
 ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   // For template arguments, we adopt the translation unit as our declaration
-  // context. This context will be fixed when the actual template declaration
-  // is created.
+  // context. This context will be fixed when (during) the actual template
+  // declaration is created.
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
@@ -5968,13 +5962,19 @@ ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   }
 
   if (D->hasDefaultArgument()) {
+    // Default argument can be "inherited" when it has a reference to the
+    // previous declaration (of the default argument) which is stored only once.
+    // Here we import the default argument in any case, and the inherited state
+    // is updated later after the parent template was created. If the
+    // inherited-from object would be imported here it causes more difficulties
+    // (parent template may not be created yet and import loops can occur).
     Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    // The import process can trigger import of the parent template which can
-    // set the default argument value (to "inherited").
-    // In this case do nothing here.
+    // The just called import process can trigger import of the parent template
+    // which can update the default argument value to "inherited". This should
+    // not be changed.
     if (!ToD->hasDefaultArgument())
       ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
   }
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 4c41171deec46a..73a5cb541e4d29 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9837,6 +9837,37 @@ struct ImportTemplateParmDeclDefaultValue
     checkTemplateParams(ToD);
   }
 
+  // In these tests the ASTImporter visit function for second template parameter
+  // (called 'T2') is called recursively from visit function of the first
+  // parameter (called 'T1') (look at the codes). The default value is specified
+  // at 'T1' and inherited at 'T2'. But because during the import the second
+  // template is created first, it is added to the "To" AST first. This way the
+  // declaration chain is reversed with the import. The import ensures currently
+  // that the first occurrence will have the default template argument in place
+  // and the second will have it inherited from the first. This is reversed
+  // compared to the original.
+  // FIXME: This reversal is probably an unexpected property of the new "To"
+  // AST. Source locations can become wrong, for example default argument that
+  // was specified at 'T2' appears at 'T1' after the import but with the
+  // original source location. This can affect source ranges of parent
+  // declarations.
+  template <class TemplateParmDeclT>
+  void testImportTemplateParmDeclReversed(TemplateParmDeclT *FromD,
+                                          TemplateParmDeclT *FromDInherited) {
+    ASSERT_FALSE(FromD->getDefaultArgStorage().isInherited());
+    ASSERT_TRUE(FromDInherited->getDefaultArgStorage().isInherited());
+
+    auto *ToD = Import(FromD, Lang_CXX14);
+    EXPECT_TRUE(ToD);
+
+    auto *ToDInherited = Import(FromDInherited, Lang_CXX14);
+    EXPECT_TRUE(ToDInherited);
+
+    EXPECT_TRUE(ToD->getDefaultArgStorage().isInherited());
+    EXPECT_FALSE(ToDInherited->getDefaultArgStorage().isInherited());
+    EXPECT_EQ(ToD->getDefaultArgStorage().getInheritedFrom(), ToDInherited);
+  }
+
   const char *CodeFunction =
       R"(
       template <class> struct X;
@@ -9920,9 +9951,7 @@ TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingVarTemplate) {
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue,
-       ImportParentTemplateDuringNonTypeTemplateParmDecl) {
-  // This wants to provoke that during import of 'Y' in "typename T = Y"
-  // (before this import returns) the later definition of 'X' is imported fully.
+       ImportNonTypeTemplateParmDeclReversed) {
   const char *Code =
       R"(
       struct Z;
@@ -9932,10 +9961,10 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
         static const int x = 1;
       };
 
-      template <int P = Y::x>
+      template <int P1 = Y::x>
       struct X;
 
-      template <int P>
+      template <int P2>
       struct X {
         static const int A = 1;
       };
@@ -9948,13 +9977,14 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
   auto *FromD = FirstDeclMatcher<NonTypeTemplateParmDecl>().match(
-      FromTU, nonTypeTemplateParmDecl(hasName("P")));
-  auto *ToD = Import(FromD, Lang_CXX14);
-  EXPECT_TRUE(ToD);
+      FromTU, nonTypeTemplateParmDecl(hasName("P1")));
+  auto *FromDInherited = FirstDeclMatcher<NonTypeTemplateParmDecl>().match(
+      FromTU, nonTypeTemplateParmDecl(hasName("P2")));
+
+  testImportTemplateParmDeclReversed(FromD, FromDInherited);
 }
 
-TEST_P(ImportTemplateParmDeclDefaultValue,
-       ImportParentTemplateDuringTemplateTypeParmDecl) {
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportTemplateTypeParmDeclReversed) {
   const char *Code =
       R"(
       struct Z;
@@ -9963,10 +9993,10 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
         Z *z;
       };
 
-      template <typename T = Y>
+      template <typename T1 = Y>
       struct X;
 
-      template <typename T>
+      template <typename T2>
       struct X {
         static const int A = 1;
       };
@@ -9979,13 +10009,15 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
   auto *FromD = FirstDeclMatcher<TemplateTypeParmDecl>().match(
-      FromTU, templateTypeParmDecl(hasName("T")));
-  auto *ToD = Import(FromD, Lang_CXX14);
-  EXPECT_TRUE(ToD);
+      FromTU, templateTypeParmDecl(hasName("T1")));
+  auto *FromDInherited = FirstDeclMatcher<TemplateTypeParmDecl>().match(
+      FromTU, templateTypeParmDecl(hasName("T2")));
+
+  testImportTemplateParmDeclReversed(FromD, FromDInherited);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue,
-       ImportParentTemplateDuringTemplateTemplateParmDecl) {
+       ImportTemplateTemplateParmDeclReversed) {
   const char *Code =
       R"(
       struct Z;
@@ -9995,10 +10027,10 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
         Z *z;
       };
 
-      template <template <int> class T = Y>
+      template <template <int> class T1 = Y>
       struct X;
 
-      template <template <int> class T>
+      template <template <int> class T2>
       struct X {
         static const int A = 1;
       };
@@ -10011,9 +10043,11 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
   auto *FromD = FirstDeclMatcher<TemplateTemplateParmDecl>().match(
-      FromTU, templateTemplateParmDecl(hasName("T")));
-  auto *ToD = Import(FromD, Lang_CXX14);
-  EXPECT_TRUE(ToD);
+      FromTU, templateTemplateParmDecl(hasName("T1")));
+  auto *FromDInherited = FirstDeclMatcher<TemplateTemplateParmDecl>().match(
+      FromTU, templateTemplateParmDecl(hasName("T2")));
+
+  testImportTemplateParmDeclReversed(FromD, FromDInherited);
 }
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,

>From fcb886c3a367882bd317cc7bf7038d981713bccb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Fri, 30 Aug 2024 10:34:04 +0200
Subject: [PATCH 3/3] import inherited default value in original order

---
 clang/lib/AST/ASTImporter.cpp           | 120 ++++------
 clang/unittests/AST/ASTImporterTest.cpp | 281 +++++++++++++++---------
 2 files changed, 215 insertions(+), 186 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index f8865baa80ae8c..40319a4d99dde1 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -360,45 +360,42 @@ namespace clang {
     }
 
     template <typename TemplateParmDeclT>
-    void tryUpdateTemplateParmDeclInheritedFrom(NamedDecl *RecentParm,
-                                                NamedDecl *NewParm) {
-      if (auto *ParmT = dyn_cast<TemplateParmDeclT>(RecentParm)) {
-        if (ParmT->hasDefaultArgument()) {
-          auto *P = cast<TemplateParmDeclT>(NewParm);
-          P->removeDefaultArgument();
-          P->setInheritedDefaultArgument(Importer.ToContext, ParmT);
+    Error importTemplateParameterDefaultArgument(const TemplateParmDeclT *D,
+                                                 TemplateParmDeclT *ToD) {
+      Error Err = Error::success();
+      if (D->hasDefaultArgument()) {
+        if (D->defaultArgumentWasInherited()) {
+          auto *ToInheritedFrom = const_cast<TemplateParmDeclT *>(
+              importChecked(Err, D->getDefaultArgStorage().getInheritedFrom()));
+          if (Err)
+            return std::move(Err);
+          if (!ToInheritedFrom->hasDefaultArgument()) {
+            // Resolve possible circular dependency between default value of the
+            // template argument and the template declaration.
+            const auto ToInheritedDefaultArg =
+                importChecked(Err, D->getDefaultArgStorage()
+                                       .getInheritedFrom()
+                                       ->getDefaultArgument());
+            if (Err)
+              return std::move(Err);
+            ToInheritedFrom->setDefaultArgument(Importer.getToContext(),
+                                                ToInheritedDefaultArg);
+          }
+          ToD->setInheritedDefaultArgument(ToD->getASTContext(),
+                                           ToInheritedFrom);
+        } else {
+          Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
+              import(D->getDefaultArgument());
+          if (!ToDefaultArgOrErr)
+            return ToDefaultArgOrErr.takeError();
+          // Default argument could have been set in the
+          // '!ToInheritedFrom->hasDefaultArgument()' branch above.
+          if (!ToD->hasDefaultArgument())
+            ToD->setDefaultArgument(Importer.getToContext(),
+                                    *ToDefaultArgOrErr);
         }
       }
-    }
-
-    // Update the parameter list `NewParams` of a template declaration
-    // by "inheriting" default argument values from `RecentParams`,
-    // which is the parameter list of an earlier declaration of the
-    // same template. (Note that "inheriting" default argument values
-    // is not related to object-oriented inheritance.)
-    //
-    // In the clang AST template parameters (NonTypeTemplateParmDec,
-    // TemplateTypeParmDecl, TemplateTemplateParmDecl) have a reference to the
-    // default value, if one is specified at the first declaration. The default
-    // value can be specified only once. The template parameters of the
-    // following declarations have a reference to the original default value
-    // through the "inherited" value. This value should be set for all imported
-    // template parameters that have a previous declaration (also a previous
-    // template declaration). The chain of parameter default value inheritances
-    // will have the same order as the chain of previous declarations, in the
-    // "To" AST. The "From" AST may have a different order because import does
-    // not happen sequentially.
-    void updateTemplateParametersInheritedFrom(
-        const TemplateParameterList &RecentParams,
-        TemplateParameterList &NewParams) {
-      for (auto [Idx, Param] : enumerate(RecentParams)) {
-        tryUpdateTemplateParmDeclInheritedFrom<NonTypeTemplateParmDecl>(
-            Param, NewParams.getParam(Idx));
-        tryUpdateTemplateParmDeclInheritedFrom<TemplateTypeParmDecl>(
-            Param, NewParams.getParam(Idx));
-        tryUpdateTemplateParmDeclInheritedFrom<TemplateTemplateParmDecl>(
-            Param, NewParams.getParam(Idx));
-      }
+      return Err;
     }
 
   public:
@@ -5961,23 +5958,8 @@ ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
     ToD->setTypeConstraint(ToConceptRef, ToIDC);
   }
 
-  if (D->hasDefaultArgument()) {
-    // Default argument can be "inherited" when it has a reference to the
-    // previous declaration (of the default argument) which is stored only once.
-    // Here we import the default argument in any case, and the inherited state
-    // is updated later after the parent template was created. If the
-    // inherited-from object would be imported here it causes more difficulties
-    // (parent template may not be created yet and import loops can occur).
-    Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
-        import(D->getDefaultArgument());
-    if (!ToDefaultArgOrErr)
-      return ToDefaultArgOrErr.takeError();
-    // The just called import process can trigger import of the parent template
-    // which can update the default argument value to "inherited". This should
-    // not be changed.
-    if (!ToD->hasDefaultArgument())
-      ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
-  }
+  if (Error Err = importTemplateParameterDefaultArgument(D, ToD))
+    return Err;
 
   return ToD;
 }
@@ -6003,14 +5985,9 @@ ASTNodeImporter::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
                               D->isParameterPack(), ToTypeSourceInfo))
     return ToD;
 
-  if (D->hasDefaultArgument()) {
-    Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
-        import(D->getDefaultArgument());
-    if (!ToDefaultArgOrErr)
-      return ToDefaultArgOrErr.takeError();
-    if (!ToD->hasDefaultArgument())
-      ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
-  }
+  Err = importTemplateParameterDefaultArgument(D, ToD);
+  if (Err)
+    return Err;
 
   return ToD;
 }
@@ -6041,14 +6018,8 @@ ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
           *TemplateParamsOrErr))
     return ToD;
 
-  if (D->hasDefaultArgument()) {
-    Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
-        import(D->getDefaultArgument());
-    if (!ToDefaultArgOrErr)
-      return ToDefaultArgOrErr.takeError();
-    if (!ToD->hasDefaultArgument())
-      ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
-  }
+  if (Error Err = importTemplateParameterDefaultArgument(D, ToD))
+    return Err;
 
   return ToD;
 }
@@ -6186,9 +6157,6 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
     }
 
     D2->setPreviousDecl(Recent);
-
-    updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
-                                          **TemplateParamsOrErr);
   }
 
   return D2;
@@ -6503,9 +6471,6 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
         ToTemplated->setPreviousDecl(PrevTemplated);
     }
     ToVarTD->setPreviousDecl(Recent);
-
-    updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
-                                          **TemplateParamsOrErr);
   }
 
   return ToVarTD;
@@ -6778,9 +6743,6 @@ ASTNodeImporter::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
         TemplatedFD->setPreviousDecl(PrevTemplated);
     }
     ToFunc->setPreviousDecl(Recent);
-
-    updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
-                                          *Params);
   }
 
   return ToFunc;
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 73a5cb541e4d29..ad3193f03cf522 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9800,62 +9800,59 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportMultipleAnonymousEnumDecls) {
 struct ImportTemplateParmDeclDefaultValue
     : public ASTImporterOptionSpecificTestBase {
 protected:
-  void checkTemplateParams(RedeclarableTemplateDecl *D) {
-    auto *CanD = cast<RedeclarableTemplateDecl>(D->getCanonicalDecl());
-    auto *CanNonTypeP = cast<NonTypeTemplateParmDecl>(
-        CanD->getTemplateParameters()->getParam(0));
-    auto *CanTypeP =
-        cast<TemplateTypeParmDecl>(CanD->getTemplateParameters()->getParam(1));
-    auto *CanTemplateP = cast<TemplateTemplateParmDecl>(
-        CanD->getTemplateParameters()->getParam(2));
-    EXPECT_FALSE(CanNonTypeP->getDefaultArgStorage().isInherited());
-    EXPECT_FALSE(CanTypeP->getDefaultArgStorage().isInherited());
-    EXPECT_FALSE(CanTemplateP->getDefaultArgStorage().isInherited());
-    for (Decl *Redecl : D->redecls()) {
-      auto *ReD = cast<RedeclarableTemplateDecl>(Redecl);
-      if (ReD != CanD) {
-        auto *NonTypeP = cast<NonTypeTemplateParmDecl>(
-            ReD->getTemplateParameters()->getParam(0));
-        auto *TypeP = cast<TemplateTypeParmDecl>(
-            ReD->getTemplateParameters()->getParam(1));
-        auto *TemplateP = cast<TemplateTemplateParmDecl>(
-            ReD->getTemplateParameters()->getParam(2));
-        EXPECT_TRUE(NonTypeP->getDefaultArgStorage().isInherited());
-        EXPECT_TRUE(TypeP->getDefaultArgStorage().isInherited());
-        EXPECT_TRUE(TemplateP->getDefaultArgStorage().isInherited());
-        EXPECT_EQ(NonTypeP->getDefaultArgStorage().getInheritedFrom(),
-                  CanNonTypeP);
-        EXPECT_EQ(TypeP->getDefaultArgStorage().getInheritedFrom(), CanTypeP);
-        EXPECT_EQ(TemplateP->getDefaultArgStorage().getInheritedFrom(),
-                  CanTemplateP);
-      }
+  void checkTemplateParams(RedeclarableTemplateDecl *D,
+                           RedeclarableTemplateDecl *InheritedFromD) {
+    auto *NonTypeP =
+        cast<NonTypeTemplateParmDecl>(D->getTemplateParameters()->getParam(0));
+    auto *TypeP =
+        cast<TemplateTypeParmDecl>(D->getTemplateParameters()->getParam(1));
+    auto *TemplateP =
+        cast<TemplateTemplateParmDecl>(D->getTemplateParameters()->getParam(2));
+    if (InheritedFromD) {
+      EXPECT_TRUE(NonTypeP->getDefaultArgStorage().isInherited());
+      EXPECT_TRUE(TypeP->getDefaultArgStorage().isInherited());
+      EXPECT_TRUE(TemplateP->getDefaultArgStorage().isInherited());
+      EXPECT_EQ(NonTypeP->getDefaultArgStorage().getInheritedFrom(),
+                InheritedFromD->getTemplateParameters()->getParam(0));
+      EXPECT_EQ(TypeP->getDefaultArgStorage().getInheritedFrom(),
+                InheritedFromD->getTemplateParameters()->getParam(1));
+      EXPECT_EQ(TemplateP->getDefaultArgStorage().getInheritedFrom(),
+                InheritedFromD->getTemplateParameters()->getParam(2));
+    } else {
+      EXPECT_FALSE(NonTypeP->getDefaultArgStorage().isInherited());
+      EXPECT_FALSE(TypeP->getDefaultArgStorage().isInherited());
+      EXPECT_FALSE(TemplateP->getDefaultArgStorage().isInherited());
     }
   }
 
-  void testImport(RedeclarableTemplateDecl *FromD) {
-    RedeclarableTemplateDecl *ToD = Import(FromD, Lang_CXX14);
-    checkTemplateParams(ToD);
+  void testImport(RedeclarableTemplateDecl *FromD1,
+                  RedeclarableTemplateDecl *FromD2,
+                  RedeclarableTemplateDecl *FromD3,
+                  RedeclarableTemplateDecl *ToExistingD1) {
+    auto *ToD1 = Import(FromD1, Lang_CXX14);
+    auto *ToD2 = Import(FromD2, Lang_CXX14);
+    auto *ToD3 = Import(FromD3, Lang_CXX14);
+    checkTemplateParams(ToD1, nullptr);
+    checkTemplateParams(ToD2, ToD1);
+    checkTemplateParams(ToD3, ToExistingD1 ? ToExistingD1 : ToD1);
   }
 
-  // In these tests the ASTImporter visit function for second template parameter
-  // (called 'T2') is called recursively from visit function of the first
-  // parameter (called 'T1') (look at the codes). The default value is specified
-  // at 'T1' and inherited at 'T2'. But because during the import the second
-  // template is created first, it is added to the "To" AST first. This way the
-  // declaration chain is reversed with the import. The import ensures currently
-  // that the first occurrence will have the default template argument in place
-  // and the second will have it inherited from the first. This is reversed
-  // compared to the original.
-  // FIXME: This reversal is probably an unexpected property of the new "To"
-  // AST. Source locations can become wrong, for example default argument that
-  // was specified at 'T2' appears at 'T1' after the import but with the
-  // original source location. This can affect source ranges of parent
-  // declarations.
+  // In these tests a circular dependency is created between the template
+  // parameter default value and the template declaration (with the same
+  // template parameter).
   template <class TemplateParmDeclT>
-  void testImportTemplateParmDeclReversed(TemplateParmDeclT *FromD,
-                                          TemplateParmDeclT *FromDInherited) {
-    ASSERT_FALSE(FromD->getDefaultArgStorage().isInherited());
-    ASSERT_TRUE(FromDInherited->getDefaultArgStorage().isInherited());
+  void
+  testTemplateParmDeclCircularDependency(ClassTemplateDecl *FromD,
+                                         ClassTemplateDecl *FromDInherited) {
+    auto GetTemplateParm =
+        [](ClassTemplateDecl *D) -> const TemplateParmDeclT * {
+      return dyn_cast<TemplateParmDeclT>(
+          D->getTemplateParameters()->getParam(0));
+    };
+
+    ASSERT_FALSE(GetTemplateParm(FromD)->getDefaultArgStorage().isInherited());
+    ASSERT_TRUE(
+        GetTemplateParm(FromDInherited)->getDefaultArgStorage().isInherited());
 
     auto *ToD = Import(FromD, Lang_CXX14);
     EXPECT_TRUE(ToD);
@@ -9863,9 +9860,15 @@ struct ImportTemplateParmDeclDefaultValue
     auto *ToDInherited = Import(FromDInherited, Lang_CXX14);
     EXPECT_TRUE(ToDInherited);
 
-    EXPECT_TRUE(ToD->getDefaultArgStorage().isInherited());
-    EXPECT_FALSE(ToDInherited->getDefaultArgStorage().isInherited());
-    EXPECT_EQ(ToD->getDefaultArgStorage().getInheritedFrom(), ToDInherited);
+    EXPECT_FALSE(GetTemplateParm(ToD)->getDefaultArgStorage().isInherited());
+    EXPECT_TRUE(
+        GetTemplateParm(ToDInherited)->getDefaultArgStorage().isInherited());
+    EXPECT_EQ(GetTemplateParm(ToDInherited)
+                  ->getDefaultArgStorage()
+                  .getInheritedFrom(),
+              GetTemplateParm(ToD));
+
+    EXPECT_EQ(ToD->getPreviousDecl(), ToDInherited);
   }
 
   const char *CodeFunction =
@@ -9873,85 +9876,145 @@ struct ImportTemplateParmDeclDefaultValue
       template <class> struct X;
 
       template <int A = 2, typename B = int, template<class> class C = X>
-      void f();
+      void test();
       template <int A, typename B, template<class> class C>
-      void f();
+      void test();
       template <int A, typename B, template<class> class C>
-      void f() {}
+      void test() {}
       )";
 
   const char *CodeClass =
       R"(
+      namespace N {
       template <class> struct X;
 
       template <int A = 2, typename B = int, template<class> class C = X>
-      struct S;
+      struct test;
       template <int A, typename B, template<class> class C>
-      struct S;
+      struct test;
       template <int A, typename B, template<class> class C>
-      struct S {};
+      struct test {};
+      }
       )";
 
   const char *CodeVar =
       R"(
+      namespace N {
       template <class> struct X;
 
       template <int A = 2, typename B = int, template<class> class C = X>
-      extern int V;
+      extern int test;
       template <int A, typename B, template<class> class C>
-      extern int V;
+      extern int test;
       template <int A, typename B, template<class> class C>
-      int V = A;
+      int test = A;
+      }
       )";
 };
 
-TEST_P(ImportTemplateParmDeclDefaultValue, ImportFunctionTemplate) {
-  Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
+TEST_P(ImportTemplateParmDeclDefaultValue, InvisibleInheritedFrom) {
+  const char *ToCode =
+      R"(
+      template <int P = 1>
+      void f() {}
+      )";
+  TranslationUnitDecl *ToTU = getToTuDecl(ToCode, Lang_CXX14);
+  auto *ToFDef = FirstDeclMatcher<FunctionTemplateDecl>().match(
+      ToTU, functionTemplateDecl(hasName("f")));
+
+  const char *FromCode =
+      R"(
+      template <int P = 1>
+      void f() {}
+      template <int P>
+      void f();
+      )";
+  TranslationUnitDecl *FromTU = getTuDecl(FromCode, Lang_CXX14);
+  auto *FromFDef = FirstDeclMatcher<FunctionTemplateDecl>().match(
+      FromTU, functionTemplateDecl(hasName("f")));
+  auto *FromF = LastDeclMatcher<FunctionTemplateDecl>().match(
       FromTU, functionTemplateDecl(hasName("f")));
-  testImport(FromLastD);
+
+  auto *ToFDefImported = Import(FromFDef, Lang_CXX14);
+  EXPECT_EQ(ToFDefImported, ToFDef);
+  auto *ToF = Import(FromF, Lang_CXX14);
+  EXPECT_NE(ToF, ToFDef);
+  const auto *Parm = dyn_cast<NonTypeTemplateParmDecl>(
+      ToF->getTemplateParameters()->getParam(0));
+  EXPECT_TRUE(Parm->defaultArgumentWasInherited());
+  // FIXME: This behavior may be confusing:
+  // Default value is not inherited from the existing declaration, instead a new
+  // is created at import that is similar to the existing but not reachable from
+  // the AST.
+  EXPECT_NE(Parm->getDefaultArgStorage().getInheritedFrom(),
+            ToFDef->getTemplateParameters()->getParam(0));
+}
+
+TEST_P(ImportTemplateParmDeclDefaultValue, ImportFunctionTemplate) {
+  TranslationUnitDecl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<FunctionTemplateDecl>().match(
+      FromTU, functionTemplateDecl(hasName("test") /*, hasBody(stmt())*/));
+  auto *D2 = dyn_cast<FunctionTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<FunctionTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, nullptr);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingFunctionTemplate) {
-  getToTuDecl(CodeFunction, Lang_CXX14);
-  Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
-      FromTU, functionTemplateDecl(hasName("f")));
-  testImport(FromLastD);
+  TranslationUnitDecl *ToTU = getToTuDecl(CodeFunction, Lang_CXX14);
+  auto *ToD1 = FirstDeclMatcher<FunctionTemplateDecl>().match(
+      ToTU, functionTemplateDecl(hasName("test")));
+  TranslationUnitDecl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<FunctionTemplateDecl>().match(
+      FromTU, functionTemplateDecl(hasName("test")));
+  auto *D2 = dyn_cast<FunctionTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<FunctionTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, ToD1);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue, ImportClassTemplate) {
-  Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
-      FromTU, classTemplateDecl(hasName("S")));
-  testImport(FromLastD);
+  TranslationUnitDecl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("test")));
+  auto *D2 = dyn_cast<ClassTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<ClassTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, nullptr);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingClassTemplate) {
-  getToTuDecl(CodeClass, Lang_CXX14);
-  Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
-      FromTU, classTemplateDecl(hasName("S")));
-  testImport(FromLastD);
+  TranslationUnitDecl *ToTU = getToTuDecl(CodeClass, Lang_CXX14);
+  auto *ToD1 = FirstDeclMatcher<ClassTemplateDecl>().match(
+      ToTU, classTemplateDecl(hasName("test")));
+  TranslationUnitDecl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("test")));
+  auto *D2 = dyn_cast<ClassTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<ClassTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, ToD1);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue, ImportVarTemplate) {
-  Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
-      FromTU, varTemplateDecl(hasName("V")));
-  testImport(FromLastD);
+  TranslationUnitDecl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("test")));
+  auto *D2 = dyn_cast<VarTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<VarTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, nullptr);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingVarTemplate) {
-  getToTuDecl(CodeVar, Lang_CXX14);
-  Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
-  auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
-      FromTU, varTemplateDecl(hasName("V")));
-  testImport(FromLastD);
+  TranslationUnitDecl *ToTU = getToTuDecl(CodeVar, Lang_CXX14);
+  auto *ToD1 = FirstDeclMatcher<VarTemplateDecl>().match(
+      ToTU, varTemplateDecl(hasName("test")));
+  TranslationUnitDecl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
+  auto *D3 = LastDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("test")));
+  auto *D2 = dyn_cast<VarTemplateDecl>(D3->getPreviousDecl());
+  auto *D1 = dyn_cast<VarTemplateDecl>(D2->getPreviousDecl());
+  testImport(D1, D2, D3, ToD1);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue,
-       ImportNonTypeTemplateParmDeclReversed) {
+       NonTypeTemplateParmDeclCircularDependency) {
   const char *Code =
       R"(
       struct Z;
@@ -9976,15 +10039,17 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
       )";
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
-  auto *FromD = FirstDeclMatcher<NonTypeTemplateParmDecl>().match(
-      FromTU, nonTypeTemplateParmDecl(hasName("P1")));
-  auto *FromDInherited = FirstDeclMatcher<NonTypeTemplateParmDecl>().match(
-      FromTU, nonTypeTemplateParmDecl(hasName("P2")));
+  auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
+  auto *FromDInherited = LastDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
 
-  testImportTemplateParmDeclReversed(FromD, FromDInherited);
+  testTemplateParmDeclCircularDependency<NonTypeTemplateParmDecl>(
+      FromD, FromDInherited);
 }
 
-TEST_P(ImportTemplateParmDeclDefaultValue, ImportTemplateTypeParmDeclReversed) {
+TEST_P(ImportTemplateParmDeclDefaultValue,
+       TemplateTypeParmDeclCircularDependency) {
   const char *Code =
       R"(
       struct Z;
@@ -10008,16 +10073,17 @@ TEST_P(ImportTemplateParmDeclDefaultValue, ImportTemplateTypeParmDeclReversed) {
       )";
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
-  auto *FromD = FirstDeclMatcher<TemplateTypeParmDecl>().match(
-      FromTU, templateTypeParmDecl(hasName("T1")));
-  auto *FromDInherited = FirstDeclMatcher<TemplateTypeParmDecl>().match(
-      FromTU, templateTypeParmDecl(hasName("T2")));
+  auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
+  auto *FromDInherited = LastDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
 
-  testImportTemplateParmDeclReversed(FromD, FromDInherited);
+  testTemplateParmDeclCircularDependency<TemplateTypeParmDecl>(FromD,
+                                                               FromDInherited);
 }
 
 TEST_P(ImportTemplateParmDeclDefaultValue,
-       ImportTemplateTemplateParmDeclReversed) {
+       TemplateTemplateParmDeclCircularDependency) {
   const char *Code =
       R"(
       struct Z;
@@ -10042,12 +10108,13 @@ TEST_P(ImportTemplateParmDeclDefaultValue,
       )";
 
   Decl *FromTU = getTuDecl(Code, Lang_CXX14);
-  auto *FromD = FirstDeclMatcher<TemplateTemplateParmDecl>().match(
-      FromTU, templateTemplateParmDecl(hasName("T1")));
-  auto *FromDInherited = FirstDeclMatcher<TemplateTemplateParmDecl>().match(
-      FromTU, templateTemplateParmDecl(hasName("T2")));
+  auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
+  auto *FromDInherited = LastDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("X")));
 
-  testImportTemplateParmDeclReversed(FromD, FromDInherited);
+  testTemplateParmDeclCircularDependency<TemplateTemplateParmDecl>(
+      FromD, FromDInherited);
 }
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,



More information about the cfe-commits mailing list