Skip to content

[clang] Consistently handle consteval constructors for variables. #144970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

efriedma-quic
Copy link
Collaborator

@efriedma-quic efriedma-quic commented Jun 20, 2025

443377a handled simple variable definitions, but it didn't handle uninitialized variables with a consteval constructor, and it didn't handle template instantiation.

Fixes #135281 .

@efriedma-quic efriedma-quic requested a review from cor3ntin June 20, 2025 02:24
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 20, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 20, 2025

@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

This is a simple extension of 443377a to also handle the implicit expressions created by default initialization. This usually doesn't matter, but it's relevant if the constructor stores "this" as a member.

Fixes #135281 .


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaDecl.cpp (+6)
  • (modified) clang/test/SemaCXX/cxx2b-consteval-propagate.cpp (+18)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index bbd63372c168b..e10dc65897b8a 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -14423,6 +14423,9 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
         Var->getType().getAddressSpace() == LangAS::hlsl_input)
       return;
 
+    if (getLangOpts().CPlusPlus)
+      ActOnCXXEnterDeclInitializer(nullptr, Var);
+
     // C++03 [dcl.init]p9:
     //   If no initializer is specified for an object, and the
     //   object is of (possibly cv-qualified) non-POD class type (or
@@ -14458,6 +14461,9 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
     }
 
     CheckCompleteVariableDeclaration(Var);
+
+    if (getLangOpts().CPlusPlus)
+      ActOnCXXExitDeclInitializer(nullptr, Var);
   }
 }
 
diff --git a/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp b/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
index dd5063cb29c5b..aa5d79af589ca 100644
--- a/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ b/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -576,3 +576,21 @@ int f() {
   //expected-note@-2 {{read of non-const variable 'a' is not allowed in a constant expression}}
 }
 }
+
+#if __cplusplus >= 202302L
+namespace GH135281 {
+  struct B {
+    const void* p;
+    consteval B() : p{this} {}
+  };
+  B b;
+  B b2{};
+  B &&b3{};
+  void f() {
+    static B b4;
+    B b5; // expected-error {{call to consteval function 'GH135281::B::B' is not a constant expression}} \
+          // expected-note {{pointer to temporary is not a constant expression}} \
+          // expected-note {{temporary created here}}
+  }
+}
+#endif

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little suspicious of this change because right now we are only calling ActOnCXXExitDeclInitializer in ~InitializerScopeRAII() and now it seems like we need to sprinkle it in other places.

@efriedma-quic
Copy link
Collaborator Author

ActOnCXXExitDeclInitializer does three things:

  • Some scope handling which isn't relevant in this context.
  • Does some special handling for variables, to compute whether the variable's initializer is manifestly constant-evaluated.
  • Calls PopExpressionEvaluationContext

The special handling for variables is specifically relevant for variables... I don't see why we'd need it anywhere else? Except, hmm... I guess we need the same handling if we're instantiating a template, like:

struct B {
  const void* p;
  consteval B() : p{this} {}
};
template<typename T> T x{};
B* y = &x<B>;

But I can't think of anything else.

Any suggestions for how to refactor?

@efriedma-quic efriedma-quic force-pushed the consteval-uninit-var branch from b508252 to 75ae080 Compare July 1, 2025 23:56
@llvmbot llvmbot added the coroutines C++20 coroutines label Jul 1, 2025
@efriedma-quic efriedma-quic force-pushed the consteval-uninit-var branch from 75ae080 to b423545 Compare July 1, 2025 23:59
@efriedma-quic
Copy link
Collaborator Author

Came up with a different approach: I moved the check to PopExpressionEvaluationContext(). This required annotating the ExpressionEvaluationContextRecord to indicate we're handling a variable initializer.

443377a handled simple variables
definitions, but it didn't handle uninitialized variables with a
constexpr constructor, and it didn't handle template instantiation.

Fixes llvm#135281 .
@efriedma-quic efriedma-quic force-pushed the consteval-uninit-var branch from b423545 to b1f7402 Compare July 2, 2025 00:10
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a changelog entry? LGTM otherwiswe

@efriedma-quic efriedma-quic changed the title [clang] Handle consteval constructors with default initialization. [clang] Consistently handle consteval constructors for variables. Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category coroutines C++20 coroutines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consteval constructor cannot store this inside object
5 participants