[clang][ASTImporter] Fix import of variable template redeclarations. #72841

balazske · 2023-11-20T09:31:05Z

In some cases variable templates (specially if static member of record) were not correctly imported and an assertion "Missing call to MapImported?" could happen.

llvmbot · 2023-11-20T09:31:33Z

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

In some cases variable templates (specially if static member of record) were not correctly imported and an assertion "Missing call to MapImported?" could happen.

Full diff: https://github.com/llvm/llvm-project/pull/72841.diff

2 Files Affected:

(modified) clang/lib/AST/ASTImporter.cpp (+17-10)
(modified) clang/unittests/AST/ASTImporterTest.cpp (+105)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c4e931e220f69b5..7a5e3d665328532 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -6245,17 +6245,21 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
                                               D->getTemplatedDecl()))
         continue;
       if (IsStructuralMatch(D, FoundTemplate)) {
-        // The Decl in the "From" context has a definition, but in the
-        // "To" context we already have a definition.
+        // FIXME Check for ODR error if the two definitions have
+        // different initializers?
         VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-        if (D->isThisDeclarationADefinition() && FoundDef)
-          // FIXME Check for ODR error if the two definitions have
-          // different initializers?
-          return Importer.MapImported(D, FoundDef);
-        if (FoundTemplate->getDeclContext()->isRecord() &&
-            D->getDeclContext()->isRecord())
-          return Importer.MapImported(D, FoundTemplate);
-
+        if (D->getDeclContext()->isRecord()) {
+          assert(FoundTemplate->getDeclContext()->isRecord() &&
+                 "Member variable template imported as non-member, "
+                 "inconsistent imported AST?");
+          if (FoundDef)
+            return Importer.MapImported(D, FoundDef);
+          if (!D->isThisDeclarationADefinition())
+            return Importer.MapImported(D, FoundTemplate);
+        } else {
+          if (FoundDef && D->isThisDeclarationADefinition())
+            return Importer.MapImported(D, FoundDef);
+        }
         FoundByLookup = FoundTemplate;
         break;
       }
@@ -6374,7 +6378,10 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateSpecializationDecl(
         // variable.
         return Importer.MapImported(D, FoundDef);
       }
+      // FIXME HandleNameConflict
+      return make_error<ASTImportError>(ASTImportError::NameConflict);
     }
+    return Importer.MapImported(D, D2);
   } else {
     TemplateArgumentListInfo ToTAInfo;
     if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 5f4d8d040772cb1..d439a14b7b9985f 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5050,6 +5050,111 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = 1;
+      )",
+      Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = 2;
+      )",
+      Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      struct A {
+        template <class U, class V>
+        static int X;
+      };
+      )",
+      Lang_CXX14);
+  auto *ToX = FirstDeclMatcher<VarTemplateDecl>().match(
+      ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct A {
+        template <class U>
+        static int X;
+      };
+      template <class U>
+      int A::X = 2;
+      )",
+      Lang_CXX14, "input1.cc");
+  auto *FromXDef = LastDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("X")));
+  ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
+  auto *ToXDef = Import(FromXDef, Lang_CXX14);
+  EXPECT_TRUE(ToXDef);
+  EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
+  EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateSpecializationDeclValue) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = U::Value;
+      struct A { static constexpr int Value = 1; };
+      constexpr int Y = X<A>;
+      )",
+      Lang_CXX14);
+
+  auto *ToTUX = FirstDeclMatcher<VarTemplateSpecializationDecl>().match(
+      ToTU, varTemplateSpecializationDecl(hasName("X")));
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = U::Value;
+      struct A { static constexpr int Value = 1; };
+      constexpr int Y = X<A>;
+      )",
+      Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher<VarTemplateSpecializationDecl>().match(
+      FromTU, varTemplateSpecializationDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX14);
+  EXPECT_TRUE(ToX);
+  EXPECT_EQ(ToTUX, ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       VarTemplateSpecializationDeclValueConflict) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = U::Value;
+      struct A { static constexpr int Value = 1; };
+      constexpr int Y = X<A>;
+      )",
+      Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <class U>
+      constexpr int X = U::Value;
+      struct A { static constexpr int Value = 2; };
+      constexpr int Y = X<A>;
+      )",
+      Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher<VarTemplateSpecializationDecl>().match(
+      FromTU, varTemplateSpecializationDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX14);
+  EXPECT_FALSE(ToX);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateParameterDeclContext) {
   constexpr auto Code =
       R"(

balazske · 2023-11-23T10:52:13Z

I plan to fix import of VarTemplateSpecializationDecl in a different PR. The indicated assertion "Missing call to MapImported?" is likely to related to this part.

shafik · 2023-12-02T05:42:07Z

clang/unittests/AST/ASTImporterTest.cpp

+  Decl *FromTU = getTuDecl(
+      R"(
+      struct A {
+        template <class U>


Did you mean to drop the class V here?

Yes, this code was incorrect. It is still a bug that the test did not fail, my next planned fixes should improve this situation.

jcsxky · 2023-12-14T07:22:34Z

clang/unittests/AST/ASTImporterTest.cpp

+      FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);


If import X should fail, What about this case in ASTImporterGenericRedeclTest.cpp

struct VariableTemplate { using DeclTy = VarTemplateDecl; static constexpr auto *Prototype = "template <class T> extern T X;"; static constexpr auto *Definition = R"( template <class T> T X; template <> int X<int>; )"; // There is no matcher for varTemplateDecl so use a work-around. BindableMatcher<Decl> getPattern() { return namedDecl(hasName("X"), unless(isImplicit()), has(templateTypeParmDecl())); } };

Storage of X in Prototype and Definition is different, it should fail when imported.
I added structural equivalence of VarTemplateDecl and makes this import failed in VarTemplateDeclConflict. is it also need to fix the testcase in ASTImporterGenericRedeclTest.cpp?

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context, VarTemplateDecl *D1, VarTemplateDecl *D2) { if (!IsStructurallyEquivalent(Context, D1->getDeclName(), D2->getDeclName())) return false; // Check the templated declaration. if (!IsStructurallyEquivalent(Context, D1->getTemplatedDecl(), D2->getTemplatedDecl())) return false; // Check template parameters. return IsStructurallyEquivalent(Context, D1->getTemplateParameters(), D2->getTemplateParameters()); }

This import should not fail (it is no error to compile code with a variable template that is declared as extern and has a non-extern definition, and the "extern" part could be in an include file), and the same does not fail with non-template variables. This case should be checked and the structural equivalence of variable templates is missing now, but that change could go into a new pull request.

NagyDonat · 2023-11-24T12:46:56Z

clang/lib/AST/ASTImporter.cpp

+          assert(FoundTemplate->getDeclContext()->isRecord() &&
+                 "Member variable template imported as non-member, "
+                 "inconsistent imported AST?");


Why is this assert always valid? I thought about this for some time and I couldn't rule out that this would crash on some tricky code.

The existing "To" AST can be somehow invalid or declarations can be in wrong scope, because previous wrong AST imports and structural equivalence problems, or here a wrong declaration may be found. This assertion looks to check for such problems (check FoundTemplate->getDeclContext()->isRecord() if D->getDeclContext()->isRecord()).

NagyDonat

Thanks for the clarifications, LGTM. As others had lots of opportunities to comment on this change, I think it's safe to merge this now (assuming that the tests are OK).

…lvm#72841) In some cases variable templates (specially if static member of record) were not correctly imported and an assertion "Missing call to MapImported?" could happen.

[clang][ASTImporter] Fix import of variable template redeclarations.

99d6169

In some cases variable templates (specially if static member of record) were not correctly imported and an assertion "Missing call to MapImported?" could happen.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 20, 2023

removed changes related to VarTemplateSpecializationDecl

87b62c7

balazske requested review from shafik and NagyDonat November 23, 2023 10:49

balazske mentioned this pull request Nov 27, 2023

[clang][ASTImporter] Improve structural equivalence of overloadable operators. #72242

Merged

shafik reviewed Dec 2, 2023

View reviewed changes

fixed wrong test code

1edb738

jcsxky reviewed Dec 14, 2023

View reviewed changes

NagyDonat reviewed Jan 9, 2024

View reviewed changes

NagyDonat approved these changes Jan 15, 2024

View reviewed changes

balazske merged commit 5e35594 into llvm:main Jan 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

balazske commented Nov 20, 2023

llvmbot commented Nov 20, 2023

balazske commented Nov 23, 2023

shafik Dec 2, 2023

balazske Dec 5, 2023

jcsxky Dec 14, 2023

balazske Dec 19, 2023

NagyDonat Nov 24, 2023

balazske Jan 12, 2024

NagyDonat left a comment

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

Conversation

balazske commented Nov 20, 2023

llvmbot commented Nov 20, 2023

balazske commented Nov 23, 2023

shafik Dec 2, 2023

Choose a reason for hiding this comment

balazske Dec 5, 2023

Choose a reason for hiding this comment

jcsxky Dec 14, 2023

Choose a reason for hiding this comment

balazske Dec 19, 2023

Choose a reason for hiding this comment

NagyDonat Nov 24, 2023

Choose a reason for hiding this comment

balazske Jan 12, 2024

Choose a reason for hiding this comment

NagyDonat left a comment

Choose a reason for hiding this comment