[clang][ASTImporter] Fix import of anonymous enums if multiple are present

[balazske]

After changes in PR #87144 and #93923 regressions appeared in some cases. The problem was that if multiple anonymous enums are present in a class and are imported as new the import of the second enum can fail because it is detected as different from the first and causes ODR error.

Now in case of enums without name an existing similar enum is searched, if not found the enum is imported. ODR error is not detected. This may be incorrect if non-matching structures are imported, but this is the less important case (import of matching classes is more important to work).

[llvmbot]

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

After the last change in PR #87144 regressions appeared in some cases. The problem was that if multiple anonymous enums are present in a class and are imported as new the import of the second enum can fail because it is detected as different from the first and causes ODR error.

Now in case of enums without name an existing similar enum is searched, if not found the enum is imported. ODR error is not detected. This may be incorrect if non-matching structures are imported, but this is the less important case (import of matching classes is more important to work).

---

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

2 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+7-2)
• (modified) clang/unittests/AST/ASTImporterTest.cpp (+82-13)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 4e1b3a5a94de7..43891d29ad4ea 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -2949,7 +2949,7 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
       if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
         if (!hasSameVisibilityContextAndLinkage(FoundEnum, D))
           continue;
-        if (IsStructuralMatch(D, FoundEnum)) {
+        if (IsStructuralMatch(D, FoundEnum, !SearchName.isEmpty())) {
           EnumDecl *FoundDef = FoundEnum->getDefinition();
           if (D->isThisDeclarationADefinition() && FoundDef)
             return Importer.MapImported(D, FoundDef);
@@ -2960,7 +2960,12 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
       }
     }
 
-    if (!ConflictingDecls.empty()) {
+    // In case of unnamed enums, we try to find an existing similar one, if none
+    // was found, perform the import always.
+    // Structural in-equivalence is not detected in this way here, but it may
+    // be found when the parent decl is imported (if the enum is part of a
+    // class). To make this totally exact a more difficult solution is needed.
+    if (SearchName && !ConflictingDecls.empty()) {
       ExpectedName NameOrErr = Importer.HandleNameConflict(
           SearchName, DC, IDNS, ConflictingDecls.data(),
           ConflictingDecls.size());
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 92f9bae6cb064..6d987cc7e9ec6 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9681,37 +9681,106 @@ AST_MATCHER_P(EnumDecl, hasEnumConstName, StringRef, ConstName) {
   return false;
 }
 
-TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousEnum) {
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousEnums) {
+  const char *Code =
+      R"(
+      struct A {
+        enum { E1, E2 } x;
+        enum { E3, E4 } y;
+      };
+      )";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+  auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
+      FromTU, enumDecl(hasEnumConstName("E1")));
+  auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
+  EXPECT_TRUE(ImportedEnumE1);
+  auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
+      FromTU, enumDecl(hasEnumConstName("E3")));
+  auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
+  EXPECT_TRUE(ImportedEnumE3);
+  EXPECT_NE(ImportedEnumE1, ImportedEnumE3);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportFreeStandingAnonymousEnums) {
+  const char *Code =
+      R"(
+      struct A {
+        enum { E1, E2 };
+        enum { E3, E4 };
+      };
+      )";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+  auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
+      FromTU, enumDecl(hasEnumConstName("E1")));
+  auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
+  EXPECT_TRUE(ImportedEnumE1);
+  auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
+      FromTU, enumDecl(hasEnumConstName("E3")));
+  auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
+  EXPECT_TRUE(ImportedEnumE3);
+  EXPECT_NE(ImportedEnumE1, ImportedEnumE3);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingAnonymousEnums) {
   const char *ToCode =
       R"(
       struct A {
-        enum { E1, E2} x;
-        enum { E3, E4} y;
+        enum { E1, E2 } x;
+        enum { E3, E4 } y;
       };
       )";
   Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
-  auto *ToE1 = FirstDeclMatcher<EnumDecl>().match(
+  auto *ToEnumE1 = FirstDeclMatcher<EnumDecl>().match(
       ToTU, enumDecl(hasEnumConstName("E1")));
-  auto *ToE3 = FirstDeclMatcher<EnumDecl>().match(
+  auto *ToEnumE3 = FirstDeclMatcher<EnumDecl>().match(
       ToTU, enumDecl(hasEnumConstName("E3")));
   const char *Code =
       R"(
       struct A {
-        enum { E1, E2} x;
-        enum { E3, E4} y;
+        enum { E1, E2 } x;
+        enum { E3, E4 } y;
       };
       )";
   Decl *FromTU = getTuDecl(Code, Lang_CXX11);
-  auto *FromE1 = FirstDeclMatcher<EnumDecl>().match(
+  auto *FromEnumE1 = FirstDeclMatcher<EnumDecl>().match(
       FromTU, enumDecl(hasEnumConstName("E1")));
+  auto *ImportedEnumE1 = Import(FromEnumE1, Lang_CXX11);
+  ASSERT_TRUE(ImportedEnumE1);
+  EXPECT_EQ(ImportedEnumE1, ToEnumE1);
+  auto *FromEnumE3 = FirstDeclMatcher<EnumDecl>().match(
+      FromTU, enumDecl(hasEnumConstName("E3")));
+  auto *ImportedEnumE3 = Import(FromEnumE3, Lang_CXX11);
+  ASSERT_TRUE(ImportedEnumE3);
+  EXPECT_EQ(ImportedEnumE3, ToEnumE3);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportExistingEmptyAnonymousEnums) {
+  const char *ToCode =
+      R"(
+      struct A {
+        enum {};
+      };
+      )";
+  Decl *ToTU = getToTuDecl(ToCode, Lang_CXX11);
+  auto *ToE1 = FirstDeclMatcher<EnumDecl>().match(ToTU, enumDecl());
+  const char *Code =
+      R"(
+      struct A {
+        enum {};
+        enum {};
+      };
+      )";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+  auto *FromE1 = FirstDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
   auto *ImportedE1 = Import(FromE1, Lang_CXX11);
   ASSERT_TRUE(ImportedE1);
   EXPECT_EQ(ImportedE1, ToE1);
-  auto *FromE3 = FirstDeclMatcher<EnumDecl>().match(
-      FromTU, enumDecl(hasEnumConstName("E3")));
-  auto *ImportedE3 = Import(FromE3, Lang_CXX11);
-  ASSERT_TRUE(ImportedE3);
-  EXPECT_EQ(ImportedE3, ToE3);
+  auto *FromE2 = LastDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
+  ASSERT_NE(FromE1, FromE2);
+  auto *ImportedE2 = Import(FromE2, Lang_CXX11);
+  ASSERT_TRUE(ImportedE2);
+  // FIXME: These should not be equal, or the import should fail.
+  EXPECT_EQ(ImportedE2, ToE1);
 }
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,

[vabridgers]

LGTM, pending integration tests, but someone else must approve. Thanks for following up on this!

[NagyDonat]

LGTM as far as I can judge this tricky situation.