[Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[shafik]

Summary

The restrictions added in D131704 were not sufficient to avoid all non-constant expression contexts. In particular constant initialization cases.

We need to check EvaluatingDecl to detect if the variable we are initializing is constexpr or not.

At this point it looks like this is the remaining case affecting various projects with this diagnostic.

[mstorsjo]

Thanks! With this, I think all my cases (that built before D131307) now build successfully again! (Can't say much about the code change itself, but I think it looks reasonable.)

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

13547–13551

This seems to be equivalent unless I'm misunderstanding something about the member dyn_cast.

13571–13578

Might as well test the easy condition before doing more work to get values and compare them.

I think we should rename NotConstexprVar to ConstexprVar so that we don't need the double negation here. WDYT?

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

2424–2427

Do you think it's worth it to add this as a test as well?

consteval void testDefaultArgForParam2(E2 e2Param = (E2)-1) {
}

void test() {
  testDefaultArgForParam2(); // Make sure we get the error
  testDefaultArgForParam2((E2)0); // Make sure we don't get the error
}

[shafik]

• Changed NotConstexprVar to ConstExprVar
• Moved checking of ConstexprVar up so that we do the less expensive check first

clang/lib/AST/ExprConstant.cpp

13547–13551

I think the problem is that PointerUnion requires that it be one of the static types it was defined with and in this case that is const ValueDecl *, const Expr * but maybe I am missing something.

13571–13578

Makes sense.

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

2424–2427

It is a good idea but the diagnostic does not point to the line in test() which is unfortunate.

[shafik]

clang/lib/AST/ExprConstant.cpp

13547–13551

It looks like the big difference is that in doCastIfPossible(...) we end up calling Self::isPossible(f) which ends up in PointerUnion::isPossible...) which uses FirstIndexOfType and if the template arguments don't contain the type then it fail.

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

13547–13551

Whelp, TIL!

Then I guess I'd go with:

if (const auto *VD = dyn_cast_or_null<VarDecl>(Info.EvaluatingDecl.dyn_cast<const ValueDecl *>))

[shafik]

• Addressing comments on casting of EvaluatingDecl

[smeenai]

I confirmed that this fixes all remaining issues on our end. Thank you!

[aaron.ballman]

Precommit CI failures are unrelated to this patch.

clang/lib/AST/ExprConstant.cpp

13547–13550

You already know that VD is nonnull, so no need for that check, and we can remove the predicate entirely because you're setting ConstexprVar based on isConstexpr() anyway.

[shafik]

• Simplify condition to set ConstexprVar even more

[shafik]

• Added consteval test with default parameters

[aaron.ballman]

LGTM!

[mstorsjo]

Thanks! It seems like this now finally works as it should again, with the code I regularly build.

clang/lib/AST/ExprConstant.cpp

if (!Result.isInt()) {
         return Info.Ctx.getTypeSize(DestType) <= Info.Ctx.getTypeSize(SrcType);
       // Only allow casts of lvalues if they are lossless.
       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 *>())) {
+        if (VD && !VD->isConstexpr())
+          ConstexprVar = false;
+      }
+
       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.
       //
       // This corressponds to [expr.static.cast]p10 which says:
       // A value of integral or enumeration type can be explicitly converted
       // to a complete enumeration type ... If the enumeration type does not
       // have a fixed underlying type, the value is unchanged if the original
       // value is within the range of the enumeration values ([dcl.enum]), and
       // otherwise, the behavior is undefined.
       //
       // This was resolved as part of DR2338 which has CD5 status.
       if (!ED->isFixed()) {
         llvm::APInt Min;
         llvm::APInt Max;
 
         ED->getValueRange(Max, Min);
         --Max;
 
-        if (ED->getNumNegativeBits() &&
+        if (ED->getNumNegativeBits() && ConstexprVar &&
             (Max.slt(Result.getInt().getSExtValue()) ||
              Min.sgt(Result.getInt().getSExtValue())))
-          Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-                                       diag::warn_constexpr_unscoped_enum_out_of_range)
-         << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << Max.getSExtValue();
-        else if (!ED->getNumNegativeBits() &&
+          Info.Ctx.getDiagnostics().Report(
+              E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+              << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+              << Max.getSExtValue();
+        else if (!ED->getNumNegativeBits() && ConstexprVar &&
                  Max.ult(Result.getInt().getZExtValue()))
           Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                        diag::warn_constexpr_unscoped_enum_out_of_range)
       << llvm::toString(Result.getInt(),10) << Min.getZExtValue() << Max.getZExtValue();
       }
     }
 
     return Success(HandleIntToIntCast(Info, E, DestType, SrcType,

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

 
 // Enum with fixed underlying type because the underlying type is explicitly specified
 enum EFixed : int {efixed1=-4, efixed2=4};
 // Enum with fixed underlying type because it is scoped
 enum class EScoped {escoped1=-4, escoped2=4};
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast<E1>(-8);
   constexpr E1 x2 = static_cast<E1>(8);
   // expected-error@-1 {{integer value 8 is outside the valid range of values [-8, 7] for this enumeration type}}
   E1 x2b = static_cast<E1>(8); // ok, not a constant expression context
 
   constexpr E2 x3 = static_cast<E2>(-8);
   // expected-error@-1 {{integer value -8 is outside the valid range of values [0, 7] for this enumeration type}}
void testValueInRangeOfEnumerationValues() {
   // expected-error@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
 
   constexpr EFixed x17 = static_cast<EFixed>(100);
   constexpr EScoped x18 = static_cast<EScoped>(100);
 
   constexpr EMaxInt x19 = static_cast<EMaxInt>(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast<EMaxInt>((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); // ok, not a constant expression context
 }
 
 enum SortOrder {
   AscendingOrder,
   DescendingOrder
 };
 
 class A {
   static void f(SortOrder order);
 };
 
 void A::f(SortOrder order) {
   if (order == SortOrder(-1)) // ok, not a constant expression context
     return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values [0, 7] for this enumeration type}}