[clang] Prevent duplicated instantiation of enumerators of unscoped member enumerations

[thebrandre]

Fixes issue #124405.

[llvmbot]

@llvm/pr-subscribers-clang

Author: André Brand (thebrandre)

Changes

Fixes issue #124405.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+9-4)
• (added) clang/test/SemaCXX/member-enum-declarations.cpp (+112)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 6a2331e59477a2..fe412c2291fc3b 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1627,12 +1627,17 @@ Decl *TemplateDeclInstantiator::VisitEnumDecl(EnumDecl *D) {
   // specialization causes the implicit instantiation of the declarations, but
   // not the definitions of scoped member enumerations.
   //
-  // DR1484 clarifies that enumeration definitions inside of a template
+  // DR1484 clarifies that enumeration definitions inside a template
   // declaration aren't considered entities that can be separately instantiated
-  // from the rest of the entity they are declared inside of.
+  // from the rest of the entity they are declared inside.
   if (isDeclWithinFunction(D) ? D == Def : Def && !Enum->isScoped()) {
-    SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Enum);
-    InstantiateEnumDefinition(Enum, Def);
+    // Prevent redundant instantiation of the enumerator-definition if the
+    // definition has already been instantiated due to a prior
+    // opaque-enum-declaration.
+    if (PrevDecl == nullptr) {
+      SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Enum);
+      InstantiateEnumDefinition(Enum, Def);
+    }
   }
 
   return Enum;
diff --git a/clang/test/SemaCXX/member-enum-declarations.cpp b/clang/test/SemaCXX/member-enum-declarations.cpp
new file mode 100644
index 00000000000000..e08f6e7a3fcd6d
--- /dev/null
+++ b/clang/test/SemaCXX/member-enum-declarations.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only %s -verify
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only %s -verify
+
+
+namespace ScopedEnumerations {
+
+template <typename T>
+struct S1 {
+  enum class E : T;
+};
+
+template <typename T>
+enum class S1<T>::E : T {
+  S1_X = 0x123
+};
+
+static_assert(static_cast<int>(S1<int>::E::S1_X) == 0x123, "");
+
+template <typename T>
+struct S2 {
+  static constexpr T f(int) { return 0; };
+  enum class E : T;
+  static constexpr T f(char) { return 1; };
+  enum class E : T { X = f(T{}) };
+};
+
+static_assert(static_cast<int>(S2<char>::E::X) == 1, "");
+
+template <typename T>
+struct S3 {
+  enum class E : T;
+  enum class E : T { X = 0x7FFFFF00 }; // expected-error {{cannot be narrowed to type 'char'}} expected-warning {{implicit conversion from 'int' to 'char'}}
+};
+template struct S3<char>; // expected-note {{in instantiation}}
+
+template <typename T>
+struct S4 {
+  enum class E : T;
+  enum class E : T { S4_X = 5 };
+};
+
+auto x4 = S4<int>::E::S4_X;
+
+template <typename T>
+T f1() {
+  enum class E : T { X_F1, Y_F1, Z_F1 };
+  return X_F1;  // expected-error {{use of undeclared identifier 'X_F1'}}
+}
+
+const int resf1 = f1<int>();
+
+}
+
+
+namespace UnscopedEnumerations {
+
+template <typename T>
+struct S1 {
+  enum E : T;
+};
+
+template <typename T>
+enum S1<T>::E : T {
+  S1_X = 0x123
+};
+
+static_assert(static_cast<int>(S1<int>::S1_X) == 0x123, "");
+
+template <typename T>
+struct S2 {
+  static constexpr T f(int) { return 0; };
+  enum E : T;
+  static constexpr T f(char) { return 1; };
+  enum E : T { S2_X = f(T{}) };
+};
+
+static_assert(static_cast<int>(S2<char>::E::S2_X) == 1, "");
+
+template <typename T>
+struct S3 {
+  enum E : T;
+  enum E : T { S3_X = 0x7FFFFF00 }; // expected-error {{cannot be narrowed to type 'char'}} expected-warning {{implicit conversion from 'int' to 'char'}}
+};
+template struct S3<char>; // expected-note {{in instantiation of template class}}
+
+template <typename T>
+struct S4 {
+  enum E : T;
+  enum E : T { S4_X = 5 };
+};
+
+auto x4 = S4<int>::S4_X;
+
+template <typename T>
+struct S5 {
+  enum E : T;
+  T S5_X = 5; // expected-note {{previous definition is here}}
+  enum E : T { S5_X = 5 }; // expected-error {{redefinition of 'S5_X'}}
+};
+
+
+template <typename T>
+T f1() {
+  enum E : T { X_F2, Y_F2, Z_F2 };
+  return X_F2;
+}
+
+const int resf1 = f1<int>();
+
+}
+

[thebrandre]

Here are some details why I think this is a valid fix:

If you take a look at the generated AST you can see that the EnumConstantDecl for X 'S<int>::E' appears twice in the instantiation in the AST for the following example.

template <typename T>
struct S {
    enum E : T;
    enum E : T { X = 5 };
};

auto x = S<char>::X;

`-ClassTemplateSpecializationDecl <line:1:1, line:5:1> line:2:8 struct S definition implicit_instantiation
|   |-DefinitionData [...]
|   |-TemplateArgument type 'int'
|   | `-BuiltinType 'int'
|   |-CXXRecordDecl <col:1, col:8> col:8 implicit struct S
|   |-EnumDecl <line:3:5, col:14> line:4:10 E 'int'
|   | `-EnumConstantDecl <col:18, col:22> col:18 X 'S<int>::E'
|   |   `-ConstantExpr <col:22> 'int'
|   |     |-value: Int 5
|   |     `-IntegerLiteral <col:22> 'int' 5
|   `-EnumDecl prev 0x1b3d5d90 <col:5, col:14> col:10 E 'int'
|     `-EnumConstantDecl <col:18, col:22> col:18 X 'S<int>::E'
|       `-ConstantExpr <col:22> 'int'
|         |-value: Int 5
|         `-IntegerLiteral <col:22> 'int' 5

If you compare it with analog cases with member classes and scoped member enumerations such as the following ...

template <typename T>
struct S {
    struct C;
    struct C {
        static constexpr T X = 5;
    };
};

auto x = S<int>::C::X;

template <typename T>
struct S {
    enum class E : T;
    enum class E : T { X = 5 };
};

auto x = S<int>::E::X;

... you can see here on Compiler Explorer that the second declarations in the instantiation simply refer to the previous and do not generate another definition.

Note that this issue does also not occur for function-local enumerations:

template <typename T>
T f() {
    enum E : T;
    enum E : T { X = 5 };
    return (T)X;
}

auto x = f<int>();

See here on Compiler Explorer.

I also added tests to make sure that out-of-class definitions are still supported.

[thebrandre]

@cor3ntin I came across this while refactoring, which is still in progress because I'm coming across odd things like this that make me a bit uncertain. Sometimes it's hard to tell if its a bug or a feature.

[thebrandre]

I almost forgot to mention that this is actually valid code.
C++23 [class.mem] p6

A member shall not be declared twice in the member-specification except that

• a nested class or member class template can be declared and then later defined, and
• an enumeration can be introduced with an opaque-enum-declaration and later redeclared with an enum-specifier.