[clang] Check constexpr int->enum conversions consistently. #143034

efriedma-quic · 2025-06-05T21:00:38Z

In 8de5137 and related patches, we added some code to avoid triggering -Wenum-constexpr-conversion in some cases. This isn't necessary anymore because -Wenum-constexpr-conversion doesn't exist anymore. And the checks are subtly wrong: they exclude cases where we actually do need to check the conversion. This patch gets rid of the unnecessary checks.

llvmbot · 2025-06-05T21:01:17Z

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

In 8de5137 and related patches, we added some code to avoid triggering -Wenum-constexpr-conversion in some cases. This isn't necessary anymore because -Wenum-constexpr-conversion doesn't exist anymore. And the checks are subtly wrong: they exclude cases where we actually do need to check the conversion. This patch gets rid of the unnecessary checks.

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

2 Files Affected:

(modified) clang/lib/AST/ExprConstant.cpp (+3-17)
(modified) clang/test/SemaCXX/constant-expression-cxx11.cpp (+7)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index ab964e592de80..3417e47e33db6 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15223,21 +15223,7 @@ bool IntExprEvaluator::VisitCastExpr(const CastExpr *E) {
       return Info.Ctx.getTypeSize(DestType) == Info.Ctx.getTypeSize(SrcType);
     }
 
-    if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
-        Info.EvalMode == EvalInfo::EM_ConstantExpression &&
-        DestType->isEnumeralType()) {
-
-      bool ConstexprVar = true;
-
-      // We know if we are here that we are in a context that we might require
-      // a constant expression or a context that requires a constant
-      // value. But if we are initializing a value we don't know if it is a
-      // constexpr variable or not. We can check the EvaluatingDecl to determine
-      // if it constexpr or not. If not then we don't want to emit a diagnostic.
-      if (const auto *VD = dyn_cast_or_null<VarDecl>(
-              Info.EvaluatingDecl.dyn_cast<const ValueDecl *>()))
-        ConstexprVar = VD->isConstexpr();
-
+    if (Info.Ctx.getLangOpts().CPlusPlus && DestType->isEnumeralType()) {
       const EnumType *ET = dyn_cast<EnumType>(DestType.getCanonicalType());
       const EnumDecl *ED = ET->getDecl();
       // Check that the value is within the range of the enumeration values.
@@ -15257,13 +15243,13 @@ bool IntExprEvaluator::VisitCastExpr(const CastExpr *E) {
         ED->getValueRange(Max, Min);
         --Max;
 
-        if (ED->getNumNegativeBits() && ConstexprVar &&
+        if (ED->getNumNegativeBits() &&
             (Max.slt(Result.getInt().getSExtValue()) ||
              Min.sgt(Result.getInt().getSExtValue())))
           Info.CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
               << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
               << Max.getSExtValue() << ED;
-        else if (!ED->getNumNegativeBits() && ConstexprVar &&
+        else if (!ED->getNumNegativeBits() &&
                  Max.ult(Result.getInt().getZExtValue()))
           Info.CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
               << llvm::toString(Result.getInt(), 10) << Min.getZExtValue()
diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 0a135654fab18..10c514be5025f 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2513,6 +2513,9 @@ void testValueInRangeOfEnumerationValues() {
   // expected-error@-1 {{constexpr variable 'x2' must be initialized by a constant expression}}
   // expected-note@-2 {{integer value 8 is outside the valid range of values [-8, 7] for the enumeration type 'E1'}}
   E1 x2b = static_cast<E1>(8); // ok, not a constant expression context
+  static_assert(static_cast<E1>(8));
+  // expected-error@-1 {{static assertion expression is not an integral constant expression}}
+  // expected-note@-2 {{integer value 8 is outside the valid range of values [-8, 7] for the enumeration type 'E1'}}
 
   constexpr E2 x3 = static_cast<E2>(-8);
   // expected-error@-1 {{constexpr variable 'x3' must be initialized by a constant expression}}
@@ -2551,6 +2554,10 @@ void testValueInRangeOfEnumerationValues() {
   // expected-note@-2 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for the enumeration type 'EMaxInt'}}
 
   const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); // ok, not a constant expression context
+  constexpr NumberType neg_one_constexpr = neg_one;
+  // expected-error@-1 {{constexpr variable 'neg_one_constexpr' must be initialized by a constant expression}}
+  // expected-note@-2 {{initializer of 'neg_one' is not a constant expression}}
+  // expected-note@-4 {{declared here}}
 
   CONSTEXPR_CAST_TO_SYSTEM_ENUM_OUTSIDE_OF_RANGE;
   // expected-error@-1 {{constexpr variable 'system_enum' must be initialized by a constant expression}}

In 8de5137 and related patches, we added some code to avoid triggering -Wenum-constexpr-conversion in some cases. This isn't necessary anymore because -Wenum-constexpr-conversion doesn't exist anymore. And the checks are subtly wrong: they exclude cases where we actually do need to check the conversion. This patch gets rid of the unnecessary checks.

shafik · 2025-06-09T21:58:47Z

clang/test/SemaCXX/constant-expression-cxx11.cpp

@@ -2513,6 +2513,9 @@ void testValueInRangeOfEnumerationValues() {
  // expected-error@-1 {{constexpr variable 'x2' must be initialized by a constant expression}}
  // expected-note@-2 {{integer value 8 is outside the valid range of values [-8, 7] for the enumeration type 'E1'}}
  E1 x2b = static_cast<E1>(8); // ok, not a constant expression context
+  static_assert(static_cast<E1>(8), "");


I guess these are the cases my previous checks missed?

Yes. The static_assert case wasn't emitting an error because of the Info.EvalMode == EvalInfo::EM_ConstantExpression check. The "neg_one_constexpr" case wasn't emitting an error because of the ConstexprVar check.

(Looking again, I guess I could have also come up with a separate testcase for the InConstantContext check, but I didn't really think too deeply about it.)

) In 8de5137 and related patches, we added some code to avoid triggering -Wenum-constexpr-conversion in some cases. This isn't necessary anymore because -Wenum-constexpr-conversion doesn't exist anymore. And the checks are subtly wrong: they exclude cases where we actually do need to check the conversion. This patch gets rid of the unnecessary checks.

rnk · 2025-06-13T23:15:33Z

Internally, we discovered that this patch breaks Boost as used by MySQL. I don't have a full external reproducer at the moment and I don't know whether the code is valid or not, but I wanted to give a heads up that we may need to revisit this. This seems like the kind of incompatibility that would get reported during release testing later on.

wlei-llvm · 2025-06-13T23:27:47Z

Internally, we discovered that this patch breaks Boost as used by MySQL. I don't have a full external reproducer at the moment and I don't know whether the code is valid or not, but I wanted to give a heads up that we may need to revisit this. This seems like the kind of incompatibility that would get reported during release testing later on.

+1, this also breaks our internal Boost which is on 1.77.0. I'm checking out the latest Boost to verify if there is already a fix.

@rnk Do you know yet if it also breaks the latest Boost?

I found some related fixes in Boost that may help:
boostorg/numeric_conversion@50a1eae

percona/percona-server-mongodb@8e1167e

) In 8de5137 and related patches, we added some code to avoid triggering -Wenum-constexpr-conversion in some cases. This isn't necessary anymore because -Wenum-constexpr-conversion doesn't exist anymore. And the checks are subtly wrong: they exclude cases where we actually do need to check the conversion. This patch gets rid of the unnecessary checks.

efriedma-quic · 2025-06-16T06:46:43Z

Most likely, the user code is invalid. I was hoping we would get away without hitting any bugs in major frameworks, but I'm not surprised we hit something.

If this is widespread, we can look into mitigations, but it's difficult to downgrade constant evaluation errors to warnings. (Particularly if SFINAE is involved.)

eaeltsin · 2025-06-16T07:39:51Z

I think the pattern is something like

enum { const_value = 1 };
template<size_t N> class Foo {...};
Foo<const_value + 1> foo;

efriedma-quic · 2025-06-16T07:55:11Z

That example doesn't have any int->enum conversions.

eaeltsin · 2025-06-16T13:14:00Z

Sorry, I over-generalized :)

Here is the proper sample - https://godbolt.org/z/E9xscz1os:

template< typename T, T N >
struct IC
{
  static const T next_value = static_cast<T>(N + 1);
  typedef IC< T, next_value > next;
};

enum E
{
  integral_to_integral,
  integral_to_float,
  float_to_integral,
  float_to_float
};

IC<E, float_to_float> foo;

wlei-llvm · 2025-06-16T16:26:16Z

Internally, we discovered that this patch breaks Boost as used by MySQL. I don't have a full external reproducer at the moment and I don't know whether the code is valid or not, but I wanted to give a heads up that we may need to revisit this. This seems like the kind of incompatibility that would get reported during release testing later on.

+1, this also breaks our internal Boost which is on 1.77.0. I'm checking out the latest Boost to verify if there is already a fix.

@rnk Do you know yet if it also breaks the latest Boost?

I found some related fixes in Boost that may help: boostorg/numeric_conversion@50a1eae

percona/percona-server-mongodb@8e1167e

JFYI, backporting boostorg/numeric_conversion@50a1eae?fbclid=IwY2xjawK9JA1leHRuA2FlbQIxMQBicmlkETFwWjFtb3JJaWRrNWNSYlZXAR5AdfRqNL1nDOUfpxRcrFyszDejF-qEjXb_H0f82unnsTPFqOYsRiDlItWLRw_aem_dNRSalHTNq6FUW_hRNhJrQ
fixes our issue.

we got the same repro:

enum int_float_mixture_enum {
    integral_to_integral,
  };
  template <typename T, T N> struct integral_c {
    static const T value = N;
    static const T prior_value = T((N - 1));
    typedef integral_c<T, prior_value> prior;
  };
  integral_c<int_float_mixture_enum, integral_to_integral> a;

jyknight · 2025-06-16T17:02:16Z

This same code in Boost was already implicated by the 2022 changes, and I reported a bug for it back then. It was fixed a couple years later in Boost 1.86. boostorg/mpl#69 (comment) mentions two commits that can be backported to older versions of Boost.

rnk · 2025-06-16T17:21:44Z

If we've had fixes available for years, I think backporting them would be the right call, but this seems like it might be a release-note-worthy change.

efriedma-quic · 2025-06-16T18:08:34Z

That pattern look like basically the same thing as the neg_one_constexpr testcase I added. The diagnostic is expected.

Posted #144407 to add a relnote.

eaeltsin · 2025-06-16T18:14:48Z

I'm not questioning the correctness of the change, just sharing the now-breaking pattern that is quite wide-spread.

We also hope to handle this internally by patching boost.

erichkeane · 2025-06-24T15:20:42Z

@efriedma-quic : If you haven't already, please add a release note that is pretty descriptive of the error/issue here, so that people can find it in the future. Additionally, a discourse post to the 'potentially-breaking' tag would be a VERY useful place to socialize this.

efriedma-quic · 2025-06-24T16:03:44Z

@erichkeane already added release note in #144407; please take a look to see if it makes sense.

efriedma-quic · 2025-06-24T21:30:40Z

This might not actually be fixed in the latest boost::mpl. See https://godbolt.org/z/jaxqTK59a . Maybe someone else can confirm I'm not missing anything?

If that's the case, we might need to downgrade the error to a warning somehow. Probably we can avoid any interaction with SFINAE by adding a hack to VarDecl::checkForConstantInitialization.

erichkeane · 2025-06-24T21:53:13Z

@erichkeane already added release note in #144407; please take a look to see if it makes sense.

Yeah, sorry, going backwards through my reviews after my week at WG21 :) I commented there, but generally 'good enough'. The discourse post would be appreciated if you haven't already.

jyknight · 2025-06-25T04:01:15Z

This might not actually be fixed in the latest boost::mpl.

Hm, indeed. It seems that the "trick" they used to solve it in mpl indeed was fixed/obsoleted by this change -- the trick no longer avoids the error. Sorry, I didn't realize that their fix was depending on a bug in Clang!

However -- I haven't actually seen any problems caused by this commit, when using an up-to-date version of Boost! I believe that's because just about nothing actually uses boost::mpl::integral_c. The boost fix which actually matters to real-world code was that boost numeric_conversion was changed to stop using boost::mpl::integral_c. And that fix is still operational!

So...I wonder if it maybe does not really matter that MPL's integral_c is broken again, if it's not really impacting anything. It'd be great if some expert could help them craft a correct fix to the type, of course...

Include more specifics from recent discussion on llvm#143034.

efriedma-quic · 2025-06-25T18:10:58Z

Pushed #145755 to revise the release note with information about numeric_conversion specifically. If the underlying issue with boost::mpl::integral_c isn't actually impacting anyone, I won't worry about it.

efriedma-quic requested a review from shafik June 5, 2025 21:00

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 5, 2025

efriedma-quic added 2 commits June 5, 2025 17:54

Fix test failure

189674e

efriedma-quic force-pushed the enum-conversion-nonvar branch from 416a167 to 189674e Compare June 6, 2025 00:59

cor3ntin requested review from erichkeane and tbaederr June 6, 2025 07:46

tbaederr approved these changes Jun 6, 2025

View reviewed changes

erichkeane approved these changes Jun 6, 2025

View reviewed changes

efriedma-quic merged commit 6090232 into llvm:main Jun 6, 2025
7 checks passed

shafik reviewed Jun 9, 2025

View reviewed changes

efriedma-quic added a commit to efriedma-quic/llvm-project that referenced this pull request Jun 25, 2025

[clang] Revise relnote for int->enum conversion.

60891ee

Include more specifics from recent discussion on llvm#143034.

efriedma-quic mentioned this pull request Jun 25, 2025

[clang] Revise relnote for int->enum conversion. #145755

Open

[clang] Check constexpr int->enum conversions consistently. #143034

[clang] Check constexpr int->enum conversions consistently. #143034

Uh oh!

Conversation

efriedma-quic commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025

Uh oh!

Uh oh!

shafik Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

rnk commented Jun 13, 2025

Uh oh!

wlei-llvm commented Jun 13, 2025

Uh oh!

efriedma-quic commented Jun 16, 2025

Uh oh!

eaeltsin commented Jun 16, 2025

Uh oh!

efriedma-quic commented Jun 16, 2025

Uh oh!

eaeltsin commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wlei-llvm commented Jun 16, 2025

Uh oh!

jyknight commented Jun 16, 2025

Uh oh!

rnk commented Jun 16, 2025

Uh oh!

efriedma-quic commented Jun 16, 2025

Uh oh!

eaeltsin commented Jun 16, 2025

Uh oh!

erichkeane commented Jun 24, 2025

Uh oh!

efriedma-quic commented Jun 24, 2025

Uh oh!

efriedma-quic commented Jun 24, 2025

Uh oh!

erichkeane commented Jun 24, 2025

Uh oh!

jyknight commented Jun 25, 2025

Uh oh!

efriedma-quic commented Jun 25, 2025

Uh oh!

Uh oh!

eaeltsin commented Jun 16, 2025 •

edited

Loading