[Clang] Tighten restrictions on enum out of range diagnostic

[shafik]

Summary

In D131528 using Info.EvalMode == EvalInfo::EM_ConstantExpression is not strict enough to restrict the diagnostic to only constant expression contexts. It is sometimes set in cases where we are still determining if we are in a constant expression context.

Using InConstantContext will tighten the restriction.

[aaron.ballman]

LGTM presuming precommit CI comes back clean.

[shafik]

Ok it looks like this is a bit more subtle, I broke the llvm-test-suite file paq8p.cpp again. We need both conditions to be true Info.EvalMode == EvalInfo::EM_ConstantExpression && Info.InConstantContext. We need to be in a context that requires a constant value but also in a constant expression context.

We are doing a little bit of an odd thing here and so this is a bit unique. I am doing a quick check-clang and will patch this up as long as that passes.

[akhuang]

We're seeing this warning in code with global constants, e.g.

const Enum x = static_cast<Enum>(-1);

is this intended?

[shafik]

In D131704#3717774, @akhuang wrote:

We're seeing this warning in code with global constants, e.g.

const Enum x = static_cast<Enum>(-1);

is this intended?

This is constant initialization, so this is indeed expected.

Depending on your situation you can fix this by using a fixed underlying type e.g. : enum A : int or by making it a scoped enum enum class A or by extending the enumerators themselves to include the value you are using.

If this not your code you can also turn the error into a warning using -Wno-error=enum-constexpr-conversion

[thakis]

This is constant initialization, so this is indeed expected.

Do we have a test for this in clang? It seems that the diag appeared for that case after the change that turned this diag into a disable-able warning. Given it's intentional, we should make sure we test that this diag is emitted, to make sure it stays around once the diag goes back to always-an-error.

[thakis]

In D131704#3717950, @shafik wrote:

In D131704#3717774, @akhuang wrote:

We're seeing this warning in code with global constants, e.g.

const Enum x = static_cast<Enum>(-1);

is this intended?

This is constant initialization, so this is indeed expected.

Depending on your situation you can fix this by using a fixed underlying type e.g. : enum A : int or by making it a scoped enum enum class A or by extending the enumerators themselves to include the value you are using.

If this not your code you can also turn the error into a warning using -Wno-error=enum-constexpr-conversion

Another question about this. The diag name and all the wording so far was about "constexpr". The expression Amy pasted is constant initialization, but _not_ constexpr.

Is this really intentional? This is quite an increase in scope for the diag, if so. we did a full cleanup for the original diag, but after the recent tweaks it fires in many many more places.

Can we put this under a different warning flag for non-constexpr cases if this is really intentional? I'm assuming the non-constexpr case won't become an unconditional error later? It probably also shouldn't be mapped to an error?

[shafik]

In D131704#3719954, @thakis wrote:

This is constant initialization, so this is indeed expected.

Do we have a test for this in clang? It seems that the diag appeared for that case after the change that turned this diag into a disable-able warning. Given it's intentional, we should make sure we test that this diag is emitted, to make sure it stays around once the diag goes back to always-an-error.

After discussing this with @erichkeane we decided that a diagnostic for the constant initialization of a global was not desirable.

It took me a while to find a solution for this case. One might think that CCEDiag machinery would have the information we need to determine whether we are initializing a constexpr variable or not but actually the determination of whether to discard the diagnostic is done by the consumer further away. An example can be seen in Sema::CheckCompleteVariableDeclaration when it gets the diagnostic notes from var->checkForConstantInitialization(Notes). If the initialization is not constant then it must do various checks for example var->isConstexpr() to determine whether to discard the diagnostics or not.

Fortunately, we have access to the VarDecl through Info.EvaluatingDecl and we do similar checks in other instances. I am testing some approaches.

clang/lib/AST/ExprConstant.cpp

if (!Result.isInt()) {
       // to detect here.  We let it through on the assumption the user knows
       // what they are doing.)
       if (Result.isAddrLabelDiff())
         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.EvalMode == EvalInfo::EM_ConstantExpression &&
+    if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
         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.
       //
       // 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

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

void testValueInRangeOfEnumerationValues() {
 
   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}}
 }
+
+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;
+}
 }