No longer add enumeration constants to the wrong scope #134998

AaronBallman · 2025-04-09T12:38:42Z

Previously, the enumerators were being added both to the class context and to the namespace scope. e.g., we accepted this invalid code:

  struct A {
    enum E : int;
  };

  enum A::E : int  { e1 = 100, e2 };

  int func() {
    return e1; // Was accepted, now correctly rejected
  }

Fixes #23317

Previously, the enumerators were being added both to the class context and to the namespace scope. e.g., we accepted this invalid code: struct A { enum E : int; }; enum A::E : int { e1 = 100, e2 }; int func() { return e1; // Was accepted, now correctly rejected } Fixes llvm#23317

llvmbot · 2025-04-09T12:39:08Z

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

Previously, the enumerators were being added both to the class context and to the namespace scope. e.g., we accepted this invalid code:

  struct A {
    enum E : int;
  };

  enum A::E : int  { e1 = 100, e2 };

  int func() {
    return e1; // Was accepted, now correctly rejected
  }

Fixes #23317

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+5)
(modified) clang/lib/Sema/SemaDecl.cpp (+4-1)
(modified) clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp (+4-4)
(modified) clang/test/SemaCXX/enum.cpp (+18)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e671183522565..555fa1955ba45 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -421,6 +421,11 @@ Bug Fixes to C++ Support
 - Clang now issues an error when placement new is used to modify a const-qualified variable
   in a ``constexpr`` function. (#GH131432)
 - Clang now emits a warning when class template argument deduction for alias templates is used in C++17. (#GH133806)
+- No longer add enumerators to the scope chain when the enumeration is declared
+  within a class context but is defined out of line. Previously, the
+  enumerators were being added both to the class context and to the namespace
+  scope. (#GH23317)
+
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 540f5f23fe89a..fd853f7be74f1 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -1523,7 +1523,10 @@ void Sema::PushOnScopeChains(NamedDecl *D, Scope *S, bool AddToContext) {
 
   // Out-of-line definitions shouldn't be pushed into scope in C++, unless they
   // are function-local declarations.
-  if (getLangOpts().CPlusPlus && D->isOutOfLine() && !S->getFnParent())
+  bool OutOfLine = D->isOutOfLine();
+  if (const auto *ECD = dyn_cast<EnumConstantDecl>(D))
+    OutOfLine = OutOfLine || cast<Decl>(ECD->getDeclContext())->isOutOfLine();
+  if (getLangOpts().CPlusPlus && OutOfLine && !S->getFnParent())
     return;
 
   // Template instantiations should also not be pushed into scope.
diff --git a/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp b/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
index e5807993a7a18..f41f3f39f2784 100644
--- a/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
@@ -12,7 +12,7 @@ A<int> a;
 A<int>::E a0 = A<int>().v;
 int n = A<int>::E::e1; // expected-error {{implicit instantiation of undefined member}}
 
-template<typename T> enum A<T>::E : T { e1, e2 }; // expected-note 2 {{declared here}}
+template<typename T> enum A<T>::E : T { e1, e2 };
 
 // FIXME: Now that A<T>::E is defined, we are supposed to inject its enumerators
 // into the already-instantiated class A<T>. This seems like a really bad idea,
@@ -20,7 +20,7 @@ template<typename T> enum A<T>::E : T { e1, e2 }; // expected-note 2 {{declared
 //
 // Either do as the standard says, or only include enumerators lexically defined
 // within the class in its scope.
-A<int>::E a1 = A<int>::e1; // expected-error {{no member named 'e1' in 'A<int>'; did you mean simply 'e1'?}}
+A<int>::E a1 = A<int>::e1; // expected-error {{no member named 'e1' in 'A<int>'}}
 
 A<char>::E a2 = A<char>::e2;
 
@@ -43,7 +43,7 @@ B<int>::E b1 = B<int>::E::e1;
 
 B<char>::E b2 = B<char>::E::e2;
 
-template<typename T> typename B<T>::E B<T>::g() { return e2; }
+template<typename T> typename B<T>::E B<T>::g() { return e2; } // expected-error {{use of undeclared identifier 'e2'}}
 B<short>::E b3 = B<short>().g();
 
 
@@ -94,7 +94,7 @@ D<int>::E d1 = D<int>::E::e1; // expected-error {{incomplete type 'D<int>::E'}}
 template<> enum class D<int>::E { e2 };
 D<int>::E d2 = D<int>::E::e2;
 D<char>::E d3 = D<char>::E::e1; // expected-note {{first required here}}
-D<char>::E d4 = D<char>::E::e2; // expected-error {{no member named 'e2' in 'D<char>::E'; did you mean simply 'e2'?}}
+D<char>::E d4 = D<char>::E::e2; // expected-error {{no member named 'e2' in 'D<char>::E'}}
 template<> enum class D<char>::E { e3 }; // expected-error {{explicit specialization of 'E' after instantiation}}
 
 template<> enum class D<short>::E;
diff --git a/clang/test/SemaCXX/enum.cpp b/clang/test/SemaCXX/enum.cpp
index 44042d8bf5cfc..e2cda23269d76 100644
--- a/clang/test/SemaCXX/enum.cpp
+++ b/clang/test/SemaCXX/enum.cpp
@@ -151,3 +151,21 @@ class C {
                         // expected-error {{unexpected ';' before ')'}}
 };
 }
+
+#if __cplusplus >= 201103L
+namespace GH23317 {
+struct A {
+  enum E : int;
+  constexpr int foo() const;
+};
+
+enum A::E : int  { ae1 = 100, ae2 }; // expected-note {{'A::ae1' declared here}}
+
+constexpr int A::foo() const { return ae1; } // This is fine
+static_assert(A{}.foo() == 100, "oh no");
+
+int foo() {
+  return ae1; // expected-error {{use of undeclared identifier 'ae1'; did you mean 'A::ae1'?}}
+}
+} // namespace GH23317
+#endif // __cplusplus >= 201103L

clang/lib/Sema/SemaDecl.cpp

zygoloid · 2025-04-09T15:54:49Z

I thought CWG's preferred direction on this was to reject entirely?

erichkeane · 2025-04-09T15:56:47Z

I thought CWG's preferred direction on this was to reject entirely?

What do they wish to reject? Defining an enum in a different declaration context than its primary context? That seems odd.

Do you have an idea of the CWG DR? We looked but couldn't find anything that looked like it.

erichkeane · 2025-04-09T15:58:55Z

clang/test/SemaCXX/enum.cpp

+
+#if __cplusplus >= 201103L
+namespace GH23317 {
+struct A {


Is this still going to work for:

struct A { enum E : int; enum E : int {ae1, ae2}; constexpr int foo() const; };

Yup, that still compiles without diagnosing.

erichkeane · 2025-04-09T16:00:17Z

clang/lib/AST/Decl.cpp

+
+  // In C++, if the enumeration is out of line, the enumeration constants are
+  // also out of line.
+  if (getLangOpts().CPlusPlus)


Not sure I get the point of the C++ check? I would presume that C just has no way to SPELL out-of-line enums, so this is a 'can't happen'. But I wonder if we should just depend on that and do this for each language, in case C ever figures out how to spell namespacing :)

Agreed that C has no way to spell this and from my looking, it seems like every place which calls isOutOfLine() is a C++-specific code path, but many times it seems to be accidental. For example, we have several uses that are guarded on whether something is a static data member, which is a C++-only concept, rather than guarding on C++ itself. As C integrates more C++ features, I worry that we'll end up calling this in C code as well, so that's a defensive measure. It technically can be removed, but I'm not certain we want to remove it, WDYT?

I can kinda see both ways here actually... there is a seemingly equal risk of "C won't standardize something enough like C++" and "C will standardize what C++ does". And the part I'd be concerned about is, "Implementer didn't realize the implications of everything, so accidentally partially implemented"

In each case we get similar bug reports, though inverted based on case. I could go either way.

clang/lib/AST/Decl.cpp

zygoloid · 2025-04-09T16:18:59Z

I thought CWG's preferred direction on this was to reject entirely?

What do they wish to reject? Defining an enum in a different declaration context than its primary context? That seems odd.

Do you have an idea of the CWG DR? We looked but couldn't find anything that looked like it.

CWG1485: "CWG agreed that an unscoped opaque enumeration in class scope should be forbidden."

erichkeane · 2025-04-09T16:21:51Z

I thought CWG's preferred direction on this was to reject entirely?

What do they wish to reject? Defining an enum in a different declaration context than its primary context? That seems odd.
Do you have an idea of the CWG DR? We looked but couldn't find anything that looked like it.

CWG1485: "CWG agreed that an unscoped opaque enumeration in class scope should be forbidden."

Interesting! WE missed that one looking through. That definitely solves the problem!

AaronBallman · 2025-04-09T16:38:54Z

I thought CWG's preferred direction on this was to reject entirely?

What do they wish to reject? Defining an enum in a different declaration context than its primary context? That seems odd.
Do you have an idea of the CWG DR? We looked but couldn't find anything that looked like it.

CWG1485: "CWG agreed that an unscoped opaque enumeration in class scope should be forbidden."

Thank you for this! What are the chances that CWG changes their stance given that zero implementations implement that direction 13 years later? https://godbolt.org/z/h5Ys9shcn

I'd be a bit concerned with how much code we might break at this point if we turned that into an error.

zygoloid · 2025-04-09T17:15:45Z

This came up on the core reflector again just yesterday; would be worth asking there.

erichkeane · 2025-04-09T17:22:36Z

This came up on the core reflector again just yesterday; would be worth asking there.

That was us :D

Endilll · 2025-04-09T21:35:58Z

It's not clear to me why the example in the PR description should be rejected, given https://eel.is/c++draft/dcl.enum#12.sentence-1. I also don't see a clear consensus on CWG reflector to determine whether this change moves us in the right direction. Do we have to change status quo before CWG revisits CWG1485?

AaronBallman · 2025-04-10T11:23:40Z

It's not clear to me why the example in the PR description should be rejected, given https://eel.is/c++draft/dcl.enum#12.sentence-1. I also don't see a clear consensus on CWG reflector to determine whether this change moves us in the right direction. Do we have to change status quo before CWG revisits CWG1485?

I asked on the Core reflectors whether that direction is still one the committee thinks makes sense, the feedback so far as been roughly "maybe something close to it". So I'm waiting for the discussion to die down there to see if there's some agreement on a path forward.

AaronBallman added clang Clang issues not falling into any other category c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" accepts-invalid labels Apr 9, 2025

AaronBallman requested review from zygoloid, cor3ntin, erichkeane and Endilll April 9, 2025 12:38

erichkeane reviewed Apr 9, 2025

View reviewed changes

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

Update based on review feedback

e76dd60

erichkeane reviewed Apr 9, 2025

View reviewed changes

cor3ntin added the waiting-for-wg21 Blocked on C++ Standards Committee label Apr 16, 2025

No longer add enumeration constants to the wrong scope #134998

Are you sure you want to change the base?

No longer add enumeration constants to the wrong scope #134998

Uh oh!

Conversation

AaronBallman commented Apr 9, 2025

Uh oh!

llvmbot commented Apr 9, 2025

Uh oh!

Uh oh!

zygoloid commented Apr 9, 2025

Uh oh!

erichkeane commented Apr 9, 2025

Uh oh!

erichkeane Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zygoloid commented Apr 9, 2025

Uh oh!

erichkeane commented Apr 9, 2025

Uh oh!

AaronBallman commented Apr 9, 2025

Uh oh!

zygoloid commented Apr 9, 2025

Uh oh!

erichkeane commented Apr 9, 2025

Uh oh!

Endilll commented Apr 9, 2025

Uh oh!

AaronBallman commented Apr 10, 2025

Uh oh!

Uh oh!