[Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

[shafik]

Summary

In D131307 we allowed the diagnostic to be turned into a warning for a transition period.

This had the side effect of triggering the warning in contexts not required to be constant expression. This change will restrict the diagnostic to constant expression contexts. This should reduce the fallout of this diagnostic.

[smeenai]

Should there be a test case to verify that the warning isn't triggered in a non-constexpr context anymore?

[shafik]

• Adding test to confirm the warning does not trigger outside of a constant expression context.

[thakis]

Lg

[erichkeane]

Yep, looks fine to me. We might consider adding an additional warning-as-warning for the alternative case (could you put a comment on this 'if' statement to that effect?).

[zatrazz]

It does help on test-suite, thanks.

[thakis]

Can this land? Our bots have been red since the default-error-mapped-warning patch landed...

[mstorsjo]

There are still some cases that were broken by D131307, that aren't fixed by this patch. Building https://martin.st/temp/qt-enum.cpp with clang -target i686-w64-mingw32 -c -std=c++17 qt-enum.cpp -Wno-ignored-attributes -Wno-user-defined-literals succeeded before the change to make those errors downgradable, and those are still an error now.

[thakis]

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

[erichkeane]

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Are you sure that is because of THIS patch? That appears to be the bitfield conversion diagnostic that was recently mucked with, and no longer this one.

[erichkeane]

In D131528#3715217, @mstorsjo wrote:

There are still some cases that were broken by D131307, that aren't fixed by this patch. Building https://martin.st/temp/qt-enum.cpp with clang -target i686-w64-mingw32 -c -std=c++17 qt-enum.cpp -Wno-ignored-attributes -Wno-user-defined-literals succeeded before the change to make those errors downgradable, and those are still an error now.

Diagnostic I'm getting is:

<source>:135567:18: error: integer value -1 is outside the valid range of values [0, 1] for this enumeration type [-Wenum-constexpr-conversion]
    if (order == Qt::SortOrder(-1))

SortOrder is:

 enum SortOrder {
    AscendingOrder,
    DescendingOrder
};

So looks like at least the range diagnosed is correct. Latest godbolt shows the issue:
https://godbolt.org/z/vKn57PbGf

BUT I believe this is exactly the case that this patch should have made no longer a problem. Hopefully @shafik can look into this to confirm, and see why this wasn't suppressed.

[shafik]

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Apologies, my condition was not strict enough, I have put up D131704

I plan on landing it soon.

[gulfem]

Can somebody please clarify this? Is the following code results in undefined behavior?

enum E1 {e11=-4, e12=4};
E1 x2b = static_cast<E1>(8);

constexpr E1 x2 = static_cast<E1>(8) seems like undefined, so -Wenum-constexpr-conversion is triggered.
We started seeing -Wenum-constexpr-conversion in our codebase after https://reviews.llvm.org/D131307, but we stopped seeing them after this review.
Is the first example that I show above still undefined, but -Wenum-constexpr-conversion is not going to warn in such cases?

[shafik]

In D131528#3717141, @gulfem wrote:

Can somebody please clarify this? Is the following code results in undefined behavior?

enum E1 {e11=-4, e12=4};
E1 x2b = static_cast<E1>(8);

constexpr E1 x2 = static_cast<E1>(8) seems like undefined, so -Wenum-constexpr-conversion is triggered.
We started seeing -Wenum-constexpr-conversion in our codebase after https://reviews.llvm.org/D131307, but we stopped seeing them after this review.
Is the first example that I show above still undefined, but -Wenum-constexpr-conversion is not going to warn in such cases?

Yes, this is indeed undefined behavior. The initial patch effected a lot of users and several third party packages and so I am trying to narrow the impact by restricting the error to those cases that are constant expression contexts only.

So you may indeed less diagnostics now.

[gulfem]

In D131528#3717509, @shafik wrote:

In D131528#3717141, @gulfem wrote:

Can somebody please clarify this? Is the following code results in undefined behavior?

enum E1 {e11=-4, e12=4};
E1 x2b = static_cast<E1>(8);

constexpr E1 x2 = static_cast<E1>(8) seems like undefined, so -Wenum-constexpr-conversion is triggered.
We started seeing -Wenum-constexpr-conversion in our codebase after https://reviews.llvm.org/D131307, but we stopped seeing them after this review.
Is the first example that I show above still undefined, but -Wenum-constexpr-conversion is not going to warn in such cases?

Yes, this is indeed undefined behavior. The initial patch effected a lot of users and several third party packages and so I am trying to narrow the impact by restricting the error to those cases that are constant expression contexts only.

So you may indeed less diagnostics now.

Thanks for your response @shafik. This is something temporary to ease the transition, but we will enable -Wenum-constexpr-conversion for non-const expressions at some point, right?

[mstorsjo]

In D131528#3716876, @shafik wrote:

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Apologies, my condition was not strict enough, I have put up D131704

Unfortunately, even after D131704 (with both commits), I'm still hitting a couple cases that weren't an error before this. With https://martin.st/temp/qt-jit-enum.cpp and https://martin.st/temp/protobuf-wire-format.cpp, built with clang -target i686-w64-mingw32 -c qt-jit-enum.cpp -std=c++17 -Wno-user-defined-literals -Wno-ignored-attributes (and same for the other file, although -std=c++17 can be omitted for the protobuf source) I'm still hitting errors for things that weren't even warned about before.

[shafik]

In D131528#3718405, @mstorsjo wrote:

In D131528#3716876, @shafik wrote:

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Apologies, my condition was not strict enough, I have put up D131704

Unfortunately, even after D131704 (with both commits), I'm still hitting a couple cases that weren't an error before this. With https://martin.st/temp/qt-jit-enum.cpp and https://martin.st/temp/protobuf-wire-format.cpp, built with clang -target i686-w64-mingw32 -c qt-jit-enum.cpp -std=c++17 -Wno-user-defined-literals -Wno-ignored-attributes (and same for the other file, although -std=c++17 can be omitted for the protobuf source) I'm still hitting errors for things that weren't even warned about before.

Thank you for those examples, it is super helpful to receive this feedback especially with simple reproducers.

This is expected, it is constant initialization and I verified that they can be turned into warning using -Wno-error=enum-constexpr-conversion

Both of these cases are undefined behavior and they can fixed a number of ways. One approach would be to give them a fixed underlying type either like enum WireType : int for example of making them a scoped enum like enum class WireType. Another solution would be to add the value you want to use an enumerator explicitly.

[ayermolo]

Was this and previous change intended to capture this case?
const enum NumberType neg_one = (enum NumberType) ((enum NumberType) 0 - (enum NumberType) 1);

[mstorsjo]

In D131528#3719414, @shafik wrote:

In D131528#3718405, @mstorsjo wrote:

In D131528#3716876, @shafik wrote:

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Apologies, my condition was not strict enough, I have put up D131704

Unfortunately, even after D131704 (with both commits), I'm still hitting a couple cases that weren't an error before this. With https://martin.st/temp/qt-jit-enum.cpp and https://martin.st/temp/protobuf-wire-format.cpp, built with clang -target i686-w64-mingw32 -c qt-jit-enum.cpp -std=c++17 -Wno-user-defined-literals -Wno-ignored-attributes (and same for the other file, although -std=c++17 can be omitted for the protobuf source) I'm still hitting errors for things that weren't even warned about before.

Thank you for those examples, it is super helpful to receive this feedback especially with simple reproducers.

This is expected, it is constant initialization and I verified that they can be turned into warning using -Wno-error=enum-constexpr-conversion

Ok, but however, before D131307 which allowed to downgrade the error to a warning, these two source examples didn't produce any single warning or error (relating to the enums) at all - and AFAIK the intent of D131307 was to lower the impact of the enum-conversion error, not widen the scope of what it covers - right?

[shafik]

In D131528#3719430, @ayermolo wrote:

Was this and previous change intended to capture this case?
const enum NumberType neg_one = (enum NumberType) ((enum NumberType) 0 - (enum NumberType) 1);

That was not intended and I plan on fixing that.

[shafik]

In D131528#3719561, @mstorsjo wrote:

In D131528#3719414, @shafik wrote:

In D131528#3718405, @mstorsjo wrote:

In D131528#3716876, @shafik wrote:

In D131528#3715841, @thakis wrote:

We're also still seeing the diag fire after this: https://ci.chromium.org/p/chromium/builders/ci/ToTLinux

(And we cleaned up our codebase back when it was still an error.)

Our bots have been red since the change to turn this into a warning landed.

Apologies, my condition was not strict enough, I have put up D131704

Unfortunately, even after D131704 (with both commits), I'm still hitting a couple cases that weren't an error before this. With https://martin.st/temp/qt-jit-enum.cpp and https://martin.st/temp/protobuf-wire-format.cpp, built with clang -target i686-w64-mingw32 -c qt-jit-enum.cpp -std=c++17 -Wno-user-defined-literals -Wno-ignored-attributes (and same for the other file, although -std=c++17 can be omitted for the protobuf source) I'm still hitting errors for things that weren't even warned about before.

Thank you for those examples, it is super helpful to receive this feedback especially with simple reproducers.

This is expected, it is constant initialization and I verified that they can be turned into warning using -Wno-error=enum-constexpr-conversion

Ok, but however, before D131307 which allowed to downgrade the error to a warning, these two source examples didn't produce any single warning or error (relating to the enums) at all - and AFAIK the intent of D131307 was to lower the impact of the enum-conversion error, not widen the scope of what it covers - right?

Correct.

[ayermolo]

In D131528#3722395, @shafik wrote:

In D131528#3719430, @ayermolo wrote:

Was this and previous change intended to capture this case?
const enum NumberType neg_one = (enum NumberType) ((enum NumberType) 0 - (enum NumberType) 1);

That was not intended and I plan on fixing that.

Ah I see. Thanks for elaborating. Can you tag me in the fix diff please.

[smeenai]

In D131528#3723231, @ayermolo wrote:

In D131528#3722395, @shafik wrote:

In D131528#3719430, @ayermolo wrote:

Was this and previous change intended to capture this case?
const enum NumberType neg_one = (enum NumberType) ((enum NumberType) 0 - (enum NumberType) 1);

That was not intended and I plan on fixing that.

Ah I see. Thanks for elaborating. Can you tag me in the fix diff please.

D131874 seems like the fix for that.

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 && DestType->isEnumeralType()) {
+    if (Info.Ctx.getLangOpts().CPlusPlus &&
+        Info.EvalMode == EvalInfo::EM_ConstantExpression &&
+        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
       // have a fixed underlying type, the value is unchanged if the original