[Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[shafik]

Summary

DR2338 clarified that it was undefined behavior to set the value outside the range of the enumerations values for an enum without a fixed underlying type.

We should diagnose this with a constant expression context. This PR adds this diagnosis for C++20 since the DR status was CD5 and a test to verify the behavior.

[erichkeane]

clang/include/clang/Basic/DiagnosticASTKinds.td

370

This is a bit awkwardly phrased to me... though I don't have a better idea. Perhaps take another run, or get Aaron to look at it?

clang/lib/AST/ExprConstant.cpp

13512

Why the check for C++20? This is UB as a Defect Report, which applies to all versions of C++.

13533

So my reading of 'within the range of the enumeration values' is different here. Given:

enum F { -4, 4};, the valid values for assignment I would say are -4, -3, -2, -1, 0, 1, 2, 3, 4. This error seems to be a bit more permissive here by making sure it is represent-able by the number of bits required to represent the enum.

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

1477

Theres likely quite a few more tests that need to be done. We should have one where it validates against a fixed underlying type, AND probably a pair of ones to check against scoped enums.

I wouldn't expect a scoped enum to diagnose, but a test to make sure we're checking the right thing is likely valuable.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

13533

Aaron pointed out off-line that I'm incorrect in what 'range of enumeration values' means, and that the values comes from https://eel.is/c++draft/dcl.enum#8.sentence-2.

So the logic here needs to be to find the smallest integer (regardless of the power-of-2-ness of its bit-size) that can represent all of the values (including a 1 bit value for empty I think?), and diagnose based on that.

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

1477

Another good test is the empty-enum case.

[shafik]

Note I took my range generation code from CGExpr.cpp function getRangeForType(...) I was considering perhaps making the code common, maybe this could be a helper in EnumDecl? Does that makes sense?

clang/lib/AST/ExprConstant.cpp

13512

I did not realized we apply defects back, I can remove the C++20 check.

13533

So I used ubsan to validate my checks e.g. https://godbolt.org/z/1j43465K7 but perhaps ubsan is getting it wrong as well?

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

1477

Good point on the tests, yes we should not diagnose on a scoped enum.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

13527

Why "End" instead of max?

13533

Playing with that link, I THINK this is right? I couldn't get it to be the 'wrong' message there. It DID take a while to figure out what is going on here.

I DO like the idea of abstracting this into some sort of InRangeOfValues thing on EnumDecl so that UBSan and this can share it.

[shafik]

• Added more tests
• Added the calculation of the value range to EnumDecl so the code can be shared
• Modified tests that were failing due to undefined behavior that this change catches

[shafik]

clang/test/Sema/aarch64-sve-intrinsics/acle_sve_imm.cpp

209 ↗

(On Diff #446045)

This test was invoking undefined behavior. So I modified the diagnostic it was looking for. I believe this is still faithful to the original test.

clang/test/SemaTemplate/temp_arg_enum_printing.cpp

20 ↗

(On Diff #446045)

This line invoked undefined behavior if the underlying type is not fixed, so I modified the enum to be scoped. This meant the checks had to be updated but the test still test what it meant to.

[shafik]

clang/include/clang/AST/Decl.h

3834 ↗

(On Diff #446045)

I should add a documenting comment.

clang/include/clang/Basic/DiagnosticASTKinds.td

370

@aaron.ballman I would appreciate help with better wording on this.

[erichkeane]

clang/lib/AST/Decl.cpp

4609 ↗

(On Diff #446045)

How does this work if both NumNegativeBits and NumPositiveBits are 0? Based on my reading of the standard, an empty enum should have the valid values 0 and 1, so a 'bitwidth' of 1.

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

2409 ↗

(On Diff #446045)

Would also like to see enum E4 {e41=0};, which should have the values 0 and 1.

2429 ↗

(On Diff #446045)

Ok, so I think '1' should be valid here based on my reading.

An empty should be the exact same case as the proposed E4 above, and I believe we can never allow 'zero' bits.

[shafik]

• Added documentation for getValueRange(...)
• Added new tests to cover fix introduced by D130301

[aaron.ballman]

clang/include/clang/Basic/DiagnosticASTKinds.td

370

How about: integer value %0 is outside the valid range of values [%1, %2) for this enumeration type

This also would nicely handle the case where the value comes as a result of a complex constant expression or constexpr function call, etc and isn't immediately obvious to the reader.

[shafik]

• Changed wording for diagnostic and updated the places where it was used and checked in tests

[aaron.ballman]

LGTM, with some minor nits. Also, please be sure to add a release note.

@erichkeane -- should we update the cxx_dr_status.html page for this? We currently claim Clang 12 for this DR, but it seems like Clang 16 (or 15 if we cherry-pick this) will be the actual release we finished the DR in.

clang/include/clang/Basic/DiagnosticASTKinds.td

370

You should make sure this is flowed to the 80-col limit.

clang/lib/AST/ExprConstant.cpp

13516–13521

[erichkeane]

Mostly Ok with this. The diagnostic seems a little subtle with the mathematical notation... just looking at the diagnostics I was like, "wait, why is 8 NOT included in the range -8, 8.... oh wait". This will be a little shocking to others perhaps?

Also, misisng any tests for the E3, I'd like to see the range for that too.

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

2420 ↗

(On Diff #447825)

Are we ok with how subtle the [N, M) syntax is here?

2427 ↗

(On Diff #447825)

I see no tests for E3?

[erichkeane]

In D130058#3682192, @aaron.ballman wrote:

LGTM, with some minor nits. Also, please be sure to add a release note.

@erichkeane -- should we update the cxx_dr_status.html page for this? We currently claim Clang 12 for this DR, but it seems like Clang 16 (or 15 if we cherry-pick this) will be the actual release we finished the DR in.

Yeah, I suspect we should update the dr-status.

[aaron.ballman]

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

2420 ↗

(On Diff #447825)

FWIW, I pulled this from diagnostics like: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L9904 and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L11541

[erichkeane]

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

2420 ↗

(On Diff #447825)

Those aren't particularly high quality diagnostics, the first is for builtin ranges (and builtins have notoriously bad diagnostics), the 2nd is for the matrix type, which is only slightly better.

That said, if you are ok with it, I'm ok, just somewhat afraid it'll be a touch confusing.

[aaron.ballman]

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

2420 ↗

(On Diff #447825)

Yeah, it's not the best diagnostic, to be sure. The trouble is that spelling it out makes it worse IMO: integer value %0 is outside the valid range of values %1 (inclusive) and %2 (exclusive) for this enumeration type

[erichkeane]

Still want a test for E3, a release note, and updating the cxx_status.html. But those don't need review IMO.

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

2420 ↗

(On Diff #447825)

Ok then, I can't think of anything better really (PERHAPS something that says, integer value %0 is outside of the valid range of values (%1 - %2 inclusive) for this enumeration type, so I'm ok living with it until someone proposes better in a followup patch.

[tahonermann]

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

2420 ↗

(On Diff #447825)

I've never cared for the [ vs ( notation to indicate inclusivity vs exclusivity. All I see are unbalanced tokens and I can never remember which brace means what; I have to look it up every time and it isn't an easy search, especially for people that aren't already somewhat familiar with the notation; you have to know to search for something like "range inclusive exclusive notation". I urge use of the more elaborate diagnostic.

[aaron.ballman]

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

2420 ↗

(On Diff #447825)

I'm fine with being more verbose in the diagnostic so long as it doesn't go overboard. I don't really like the wording Erich suggested because it can be misinterpreted as both values being inclusive. I can hold my nose at what we have above. We're inconsistent in how we report this kind of information and it seems like someday we should improve this whole class of diagnostics (ones with ranges) to have a consistent display to the user. (CC @cjdb for awareness for his project, nothing actionable though.)

[erichkeane]

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

2420 ↗

(On Diff #447825)

My intent WAS for both values to be inclusive! That is, we'd say integer value -8 is outside the valid range of values(0 - 7 inclusive) for this enumeration type), but the additional logic there is likely a PITA for minor improvement.

I'm ALSO ok with holding my nose here, but would welcome a patch to improve this diagnostic (and, as Aaron said, ALL range diagnostics!). I, however, am not clever enough to come up with it.

[royjacobson]

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

2420 ↗

(On Diff #447825)

Maybe [%1 <= x < %2]? Feels a bit clumsy, but it disambiguates

[cjdb]

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

2420 ↗

(On Diff #447825)

While I like [%1, %2) (because I nerd out over maths), I think %1 <= x < %2 will be more accessible to folks who haven't taken university calculus or discrete maths.

For @tahonermann specifically: a potential mnemonic is that closed intervals use a straight line, which intersects an axis, whereas open intervals are curved, which represents them being asymptotic.

[aaron.ballman]

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

2420 ↗

(On Diff #447825)

As far as wording goes, I think %1 <= x < %2 is reasonable (I really don't like that x in there though -- the chances of that being the user's variable are very slim right up until x happens to be something contextually baffling like the name of a template type parameter. However, I don't see any diagnostics using that kind of wording either, so this would be adding another variant of expressing a range of values (not a huge issue, but a bit unfortunate for users).

Here's an idea that may be worse than anything anyone else has come up with. Split the diagnostic into two parts:

integer value %0 is %select{less than the smallest|greater than the largest}1 possible value %2 for this enumeration type

[shafik]

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

2420 ↗

(On Diff #447825)

I agree having the x in the diagnostic could be confusing based on the context.

I could make sure both value of the range are inclusive and go with wording like:

integer value %0 is outside the valid range of values (%1 through %2) for this enumeration type

2427 ↗

(On Diff #447825)

Apologies, not intentional. I will add a test with my next update, hopefully with new diagnostic wording.

[erichkeane]

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

2420 ↗

(On Diff #447825)

IMO, we've rat-holed this more than enough, and I see nothing that is 'better enough' than the version in the patch we have to warrant diverging from other diagnostic precedence.

There _IS_ an opportunity for someone to come along with a future patch to fix all of our range-diagnostics in a BETTER way, but I don't think this is the patch to do so, nor do we have the way of doing so identified here.

[aaron.ballman]

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

2420 ↗

(On Diff #447825)

+1, I think we should leave the version that's in this patch. It's not ideal, but I like it better than the alternatives.

[shafik]

• Add release note
• Reflow diagnostic
• Update defect report status
• Add test for E3

[shafik]

• dyn_cast was not seeing through type sugar switch to isEnumeralType()
• APInt requires the bit widths to be the same when comparing and so switched to obtaining the value from Result from just getInt() to getInt().getSExtValue() and getInt().getZExtValue() which uses the int64_t and uint64_t overloads.
• Updated some more tests that were failing after fixing the other issues.

[shafik]

clang/test/Analysis/cfg.cpp

227 ↗

(On Diff #448390)

This is undefined behavior unless the enum has a fixed type, using : int above does that.

clang/test/SemaCXX/enum-scoped.cpp

330 ↗

(On Diff #448390)

@erichkeane not sure if this fix for the test maintains the original intent of the test or not.

cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp

28 ↗

(On Diff #448390)

These enums are used with undefined behavior later on by casting values outside of the range and using them as template arguments. Given them a fixed underling type removes the UB.

[erichkeane]

clang/test/SemaCXX/enum-scoped.cpp

330 ↗

(On Diff #448390)

From the looks of it, the problem (https://github.com/llvm/llvm-project/commit/bf5fad86db497775a4f0279b8a806d150c0fa201) was that the RHS of this expression (F = (enum C) -1) was not an ICE, it was an eum. So I don't think this changes the AST at all, which should have hit the same crash.

[thakis]

Hello, this breaks a bunch of existing code over here (and I imagine elsewhere).

Can we make this a warning that's mapped to an error by default, so that people can turn it off for a while while they fix their code?

[tahonermann]

clang/docs/ReleaseNotes.rst

55–57 ↗

(On Diff #448390)

Nit: "of of" and need for possessive quote; "enumeration's values".

[aaron.ballman]

In D130058#3687680, @thakis wrote:

Hello, this breaks a bunch of existing code over here (and I imagine elsewhere).

Do you have an idea on how much a bunch is and whether the fixes are particularly involved or not? Did it break a system header?

Can we make this a warning that's mapped to an error by default, so that people can turn it off for a while while they fix their code?

It might be possible to do that, but I'd like to better understand the situation before we make a decision. This was fixing an accepts-invalid bug in Clang and we usually don't turn that into a warning-as-error unless there's a significant amount of code to be fixed in the ecosystem (like implicit int being accepted for a decade+ even when it was "wrong" for compilers to do so). However, Clang is the first compiler to actually get this particular behavior right, so I am sympathetic to turning it into a warning that defaults to an error.

[thakis]

Here's a reduced repro of one case:

% cat test.cc
typedef enum VkResult {
    VK_ERROR_INVALID_OPAQUE_CAPTURE_ADDRESS = -1000257000,
    VK_RESULT_MAX_ENUM = 0x7FFFFFFF
} VkResult;

constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);

% out/gn/bin/clang -c test.cc -std=c++17
test.cc:6:20: error: constexpr variable 'VK_FAKE_DEVICE_OOM_FOR_TESTING' must be initialized by a constant expression
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                   ^                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cc:6:53: note: integer value 2147483646 is outside the valid range of values [-2147483648, -2147483648) for this enumeration type
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                                                    ^
1 error generated.

Isn't valid range of values [-2147483648, -2147483648) just wrong here?

[erichkeane]

In D130058#3688049, @thakis wrote:

Here's a reduced repro of one case:

% cat test.cc
typedef enum VkResult {
    VK_ERROR_INVALID_OPAQUE_CAPTURE_ADDRESS = -1000257000,
    VK_RESULT_MAX_ENUM = 0x7FFFFFFF
} VkResult;

constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);

% out/gn/bin/clang -c test.cc -std=c++17
test.cc:6:20: error: constexpr variable 'VK_FAKE_DEVICE_OOM_FOR_TESTING' must be initialized by a constant expression
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                   ^                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cc:6:53: note: integer value 2147483646 is outside the valid range of values [-2147483648, -2147483648) for this enumeration type
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                                                    ^
1 error generated.

Isn't valid range of values [-2147483648, -2147483648) just wrong here?

That range with the leading negative looks wrong to me, but perhaps that is a diagnostic problem? Either way, this test case SHOULD work, it is within the correct range.

[shafik]

In D130058#3688049, @thakis wrote:

Here's a reduced repro of one case:

% cat test.cc
typedef enum VkResult {
    VK_ERROR_INVALID_OPAQUE_CAPTURE_ADDRESS = -1000257000,
    VK_RESULT_MAX_ENUM = 0x7FFFFFFF
} VkResult;

constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);

% out/gn/bin/clang -c test.cc -std=c++17
test.cc:6:20: error: constexpr variable 'VK_FAKE_DEVICE_OOM_FOR_TESTING' must be initialized by a constant expression
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                   ^                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cc:6:53: note: integer value 2147483646 is outside the valid range of values [-2147483648, -2147483648) for this enumeration type
constexpr VkResult VK_FAKE_DEVICE_OOM_FOR_TESTING = static_cast<VkResult>(VK_RESULT_MAX_ENUM - 1);
                                                    ^
1 error generated.

Isn't valid range of values [-2147483648, -2147483648) just wrong here?

That is a bug, I am looking at possible fixes right now and if I can't figure out something good in the next day otherwise I will revert.

[thakis]

In D130058#3687868, @aaron.ballman wrote:

In D130058#3687680, @thakis wrote:

Hello, this breaks a bunch of existing code over here (and I imagine elsewhere).

Do you have an idea on how much a bunch is and whether the fixes are particularly involved or not? Did it break a system header?

Sorry for the slow reply, this was fairly involved to count. (Since this is an error, not a warning, I had to do local builds on all our supported platforms, and fix all instances before I could know I found everything.)

With D130811, the most annoying instance is resolved. I'd say it's now few enough instances in few enough repositories that this isn't necessary for Chromium, but based on the numbers below I'd expect that this change will be fairly annoying for people with larger codebases.

For Chromium, this affects:

• 6 different places in v8, all due to due using enums with a spiffy BitField class (reduced example here https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c7). This BitField class takes a "size" template arg, and clang now happens to complain if that size arg is too large to hold all enum fields: If you have an enum with 8 values but try to store it in a BitField<.., 4> you now happen to get an error, because a BitField<..., 3>. (This is kind of cool!) (https://chromium-review.googlesource.com/c/v8/v8/+/3794708)

• 11 different places in chromium that are related to a macro that takes an enum (summary here: https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c11) where the workaround is to give those enums a fixed underlying type
• 4 places in unit tests that do something like constexpr auto kImpossibleEnumValue = static_cast<MyEnumType>(-1); (replaced "constexpr" with "const" as workaround)
• 1 place that checked that static_assert()ed that two enumerators from two distinct enum types have the same value, this just needed rewriting the expression slightly

After D130811, we're lucky that no code in difficult-to-change third-party repositories are affected.

It does affect 22 distinct places though, so it seems likely that other projects might be less lucky.

(But since it's no longer a problem for us, I also won't push for a warning more than I did in this comment.)

[aaron.ballman]

In D130058#3689053, @thakis wrote:

In D130058#3687868, @aaron.ballman wrote:

In D130058#3687680, @thakis wrote:

Hello, this breaks a bunch of existing code over here (and I imagine elsewhere).

Do you have an idea on how much a bunch is and whether the fixes are particularly involved or not? Did it break a system header?

Sorry for the slow reply, this was fairly involved to count. (Since this is an error, not a warning, I had to do local builds on all our supported platforms, and fix all instances before I could know I found everything.)

Thank you for helping to track those numbers down, I really appreciate it!

With D130811, the most annoying instance is resolved. I'd say it's now few enough instances in few enough repositories that this isn't necessary for Chromium, but based on the numbers below I'd expect that this change will be fairly annoying for people with larger codebases.

For Chromium, this affects:

• 6 different places in v8, all due to due using enums with a spiffy BitField class (reduced example here https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c7). This BitField class takes a "size" template arg, and clang now happens to complain if that size arg is too large to hold all enum fields: If you have an enum with 8 values but try to store it in a BitField<.., 4> you now happen to get an error, because a BitField<..., 3>. (This is kind of cool!) (https://chromium-review.googlesource.com/c/v8/v8/+/3794708)

That's neat!

• 11 different places in chromium that are related to a macro that takes an enum (summary here: https://bugs.chromium.org/p/chromium/issues/detail?id=1348574#c11) where the workaround is to give those enums a fixed underlying type
• 4 places in unit tests that do something like constexpr auto kImpossibleEnumValue = static_cast<MyEnumType>(-1); (replaced "constexpr" with "const" as workaround)
• 1 place that checked that static_assert()ed that two enumerators from two distinct enum types have the same value, this just needed rewriting the expression slightly

After D130811, we're lucky that no code in difficult-to-change third-party repositories are affected.

Glad to hear that!

It does affect 22 distinct places though, so it seems likely that other projects might be less lucky.

(But since it's no longer a problem for us, I also won't push for a warning more than I did in this comment.)

Thank you for these details! That does seem like a fairly substantial amount of broken code for one project, but it also sounds like everything it caught was a true positive (at least as far as the language standard is concerned) and none of it required major changes to fix the issues. Based on that, I think we're still okay with the functionality as-is, but if we start to find cases where the fix is really involved (or is in a system header) and the code is otherwise reasonable, we may want to reconsider.

[smeenai]

Sorry for chiming in a bit late here.

I'm working on getting a large internal codebase to compile after this change. Most of the issues are easy enough to deal with, but there's one Boost error I'd like to highlight: P8289. (We might be using a slightly older Boost version, but upgrading it would be a pretty significant undertaking, unfortunately, and current Boost appears to have similar code: https://github.com/boostorg/mpl/blob/8f10d06b9637a67b23a3a11791de5bd6bcd2e112/include/boost/mpl/aux_/integral_wrapper.hpp#L72-L73.)

There's a couple of things that make this error hard to parse:

• The note about the integer value being out of range for the enumeration doesn't appear until towards the end of the error (line 55 in my paste), even though it's the key to understanding why the argument isn't considered a constant expression
• The note doesn't mention which enumeration type the value is out of the valid range for, and with the mix of macros and templates going on in this error message, that's non-trivial to decipher

After digging through the preprocessor output, it turns out that AUX_WRAPPER_VALUE_TYPE is defined as T here (after being undefined and redefined a bunch of times, which makes this even trickier), and coupled with the types in the templates in the error you can determine that T is boost::numeric::udt_builtin_mixture_enum, but it'd be much easier if the diagnostic gave you the type directly and also appeared much earlier.

Diagnostic quality aside, I'm not really sure what to do about this error. It appears to ultimately originate from here, which will attempt to compute a prior for the first enum value, which is incorrect. I can fix the issue by giving udt_builtin_mixture_enum and int_float_mixture_enum an underlying type of int, but I lack the Boost know-how to understand if that's reasonable.

Given that we have a non-obvious (at least to me) issue in a widely used third-party library, would we consider giving users some way to opt out of this error, at least as a transition tool?

[shafik]

Given that we have a non-obvious (at least to me) issue in a widely used third-party library, would we consider giving users some way to opt out of this error, at least as a transition tool?

Thank your feedback, this is very helpful. I won't have time to dig into this in detail till tomorrow but it does not look like there is a nice fix here. I think we may want to consider turning this into a warning for some transition period.

[aaron.ballman]

In D130058#3697949, @smeenai wrote:

Given that we have a non-obvious (at least to me) issue in a widely used third-party library, would we consider giving users some way to opt out of this error, at least as a transition tool?

Thank you for the feedback! I agree that providing information like which enumeration type the value is out of range for would be super helpful. (Changing the note order might be significantly more involved but I can see how it would be helpful here.)

Because the error is happening in boost, I think we have two paths forward: 1) targeted workaround for just boost, 2) downgrade the error to a warning-defaults-to-error. Because this is the second project to have run into some pain, I'm leaning towards downgrading the diagnostic for a release. One thing I would recommend is: add information to the release notes to set expectations about the warning being upgraded (if we downgrade it but don't tell users we're going to upgrade it to an error Real Soon Now, users will come to rely on the crutch).

[smeenai]

https://godbolt.org/z/axhbj3MPE is a simpler repro in Godbolt with Boost 1.79 (the latest version).

I agree that it'll be tricky to manage expectations if we downgrade to a warning, but I also suspect that the first time many people will run into this new error is when we start doing the RCs for LLVM 16, and we'll discover lots more breakage then. I'd hope that giving the warning a sufficiently scary name (e.g. suffixing it with -will-be-removed-in-clang-17) and having a release notes entry would help us actually be able to convert this into an error in the future though and not just keep kicking the can down the road. It'd also help if we could compile the fixes for issues with this new error in common libraries (e.g. the Boost error here) somewhere, so that people using those common libraries can reference that to fix their own codebases.

[jyknight]

I've filed a bug against Boost MPL, https://github.com/boostorg/mpl/issues/69

[smeenai]

In D130058#3698213, @shafik wrote:

Given that we have a non-obvious (at least to me) issue in a widely used third-party library, would we consider giving users some way to opt out of this error, at least as a transition tool?

Thank your feedback, this is very helpful. I won't have time to dig into this in detail till tomorrow but it does not look like there is a nice fix here. I think we may want to consider turning this into a warning for some transition period.

Following up because we have an internal integration blocked on this. Are we planning to turn this into a warning soon, or should I just put in local workarounds in the meantime?

[shafik]

In D130058#3701485, @smeenai wrote:

In D130058#3698213, @shafik wrote:

Given that we have a non-obvious (at least to me) issue in a widely used third-party library, would we consider giving users some way to opt out of this error, at least as a transition tool?

Thank your feedback, this is very helpful. I won't have time to dig into this in detail till tomorrow but it does not look like there is a nice fix here. I think we may want to consider turning this into a warning for some transition period.

Following up because we have an internal integration blocked on this. Are we planning to turn this into a warning soon, or should I just put in local workarounds in the meantime?

I plan on having a PR up tomorrow and hopefully turn around will be quick.

[hokein]

+1 on downgrading to a warning, so that users can have time to do cleanup on their codebase.

We also see internal breakages caused by this change. It breaks some code using the magic_enum library, a simple repro example:

#include <iostream>                                                             
#include "magic_enum.hpp"                                
                                                                                
enum Color { Red, Black };                                                      
int main(int argc, char* argv[]) {                                              
   Color color = Black;                                                          
   switch (color) {                                                              
     case Red:                                                                   
       break;                                                                    
     default:                                                                    
       std::cout << magic_enum::enum_name(color);                                
       break;                                                                    
   }                                                                                                                                                             
   return 0;                                                                     
 }

Errors:

./magic_enum/magic_enum.hpp:436:10: note: in instantiation of function template specialization 'magic_enum::detail::values<Color, false, 0, 0UL, 1UL, 2UL, 3UL, 4UL, 5UL, 6UL, 7UL, 8UL, 9UL, 10UL, 11UL, 12UL, 13UL, 14UL, 15UL, 16UL, 17UL, 18UL, 19UL, 20UL, 21UL, 22UL, 23UL, 24UL, 25UL, 26UL, 27UL, 28UL, 29UL, 30UL, 31UL, 32UL, 33UL, 34UL, 35UL, 36UL, 37UL, 38UL, 39UL, 40UL, 41UL, 42UL, 43UL, 44UL, 45UL, 46UL, 47UL, 48UL, 49UL, 50UL, 51UL, 52UL, 53UL, 54UL, 55UL, 56UL, 57UL, 58UL, 59UL, 60UL, 61UL, 62UL, 63UL, 64UL, 65UL, 66UL, 67UL, 68UL, 69UL, 70UL, 71UL, 72UL, 73UL, 74UL, 75UL, 76UL, 77UL, 78UL, 79UL, 80UL, 81UL, 82UL, 83UL, 84UL, 85UL, 86UL, 87UL, 88UL, 89UL, 90UL, 91UL, 92UL, 93UL, 94UL, 95UL, 96UL, 97UL, 98UL, 99UL, 100UL, 101UL, 102UL, 103UL, 104UL, 105UL, 106UL, 107UL, 108UL, 109UL, 110UL, 111UL, 112UL, 113UL, 114UL, 115UL, 116UL, 117UL, 118UL, 119UL, 120UL, 121UL, 122UL, 123UL, 124UL, 125UL, 126UL, 127UL, 128UL>' requested here
  return values<E, IsFlags, reflected_min_v<E, IsFlags>>(std::make_index_sequence<range_size>{});
         ^
./magic_enum/magic_enum.hpp:440:34: note: in instantiation of function template specialization 'magic_enum::detail::values<Color, false, unsigned int>' requested here
inline constexpr auto values_v = values<E, IsFlags>();
                                 ^
./magic_enum/magic_enum.hpp:446:33: note: in instantiation of variable template specialization 'magic_enum::detail::values_v' requested here
inline constexpr auto count_v = values_v<E, IsFlags>.size();
                                ^
./magic_enum/magic_enum.hpp:567:17: note: in instantiation of variable template specialization 'magic_enum::detail::count_v' requested here
  static_assert(count_v<D, false> > 0, "magic_enum requires enum implementation and valid max and min.");
                ^
./magic_enum/magic_enum.hpp:579:1: note: in instantiation of template class 'magic_enum::detail::enable_if_enum<true, false, Color, std::string_view>' requested here
using enable_if_enum_t = typename enable_if_enum<std::is_enum_v<std::decay_t<T>>, false, T, R>::type;
^
./magic_enum/magic_enum.hpp:690:69: note: in instantiation of template type alias 'enable_if_enum_t' requested here
[[nodiscard]] constexpr auto enum_name(E value) noexcept -> detail::enable_if_enum_t<E, string_view> {
                                                                    ^
main.cc:17:20: note: while substituting deduced template arguments into function template 'enum_name' [with E = Color]
      std::cout << magic_enum::enum_name(color);

[ayermolo]

+2 turning it in to warning. It's breaking our builds, due to boost. :(

[smeenai]

In D130058#3708209, @ayermolo wrote:

+2 turning it in to warning. It's breaking our builds, due to boost. :(

This was done by D131307, which I'm hoping will land soon :)

[glandium]

This catches https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir_reader.cpp#732 but not https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir.cpp#654. Is that expected?

[glandium]

In D130058#3714672, @glandium wrote:

This catches https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir_reader.cpp#732 but not https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir.cpp#654. Is that expected?

Maybe something changed? A couple days ago it was catching https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/gfx/cairo/cairo/src/win32/cairo-dwrite-font.cpp#238 but apparently not anymore.

[glandium]

Also not caught: a cast of 0 when 0 is not a valid value in the enum.

[glandium]

Having looked around in the Firefox codebase for cases that now fail to build because of this, and looking at the clarification in DR2338, I wonder if enums in extern "C" sections should be treated as if they had an explicit type of int as if it were e.g. enum Foo: int { ... }.

[aaron.ballman]

In D130058#3714880, @glandium wrote:

In D130058#3714672, @glandium wrote:

This catches https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir_reader.cpp#732 but not https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/ir.cpp#654. Is that expected?

Maybe something changed? A couple days ago it was catching https://searchfox.org/mozilla-central/rev/c77834ec635c523f2ba0092fcd1728c9b1c3005b/gfx/cairo/cairo/src/win32/cairo-dwrite-font.cpp#238 but apparently not anymore.

Correct, the diagnostic was previously over-eager, but that's been fixed.

In D130058#3714882, @glandium wrote:

Also not caught: a cast of 0 when 0 is not a valid value in the enum.

I don't think that situation will ever be UB. When the underlying type of the enum is not fixed, the range of values it can represent is whatever values fit into a hypothetical bit-field that is large enough to cover the full range of stated values (https://eel.is/c++draft/enum#dcl.enum-8.sentence-2). 0 is something that can always be represented in such a bit-field (there's a special provision for empty enumerations or one that can only store 0).

In D130058#3715183, @glandium wrote:

Having looked around in the Firefox codebase for cases that now fail to build because of this, and looking at the clarification in DR2338, I wonder if enums in extern "C" sections should be treated as if they had an explicit type of int as if it were e.g. enum Foo: int { ... }.

That doesn't match the C++ language rules for constant expression evaluation, so I don't think we should go that route unless WG21 decides they want to.

[erichkeane]

In D130058#3714882, @glandium wrote:

Also not caught: a cast of 0 when 0 is not a valid value in the enum.

I don't think that situation will ever be UB. When the underlying type of the enum is not fixed, the range of values it can represent is whatever values fit into a hypothetical bit-field that is large enough to cover the full range of stated values (https://eel.is/c++draft/enum#dcl.enum-8.sentence-2). 0 is something that can always be represented in such a bit-field (there's a special provision for empty enumerations or one that can only store 0).

Correct here. A 'valid value' of an enum is NOT just the list of values listed. It is every value that would be represent-able by the smallest-bit-sized bitfield required to represent all of the possible enumerators. Since 0 is represent-able by all 1+ bit integers, zero is always valid.

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_constexpr_delete_base_nonvirt_dtor : Note<
   "base class type %0 with non-virtual destructor">;
 def note_constexpr_memory_leak : Note<
   "allocation performed here was not deallocated"
   "%plural{0:|: (along with %0 other memory leak%s0)}0">;
 def note_constexpr_unsupported_layout : Note<
   "type %0 has unexpected layout">;
 def note_constexpr_unsupported_flexible_array : Note<
   "flexible array initialization is not yet supported">;
+def note_constexpr_unscoped_enum_out_of_range : Note<
+  "store of value outside of the range of unscoped enum, valid values %0 to %1">;
 def err_experimental_clang_interp_failed : Error<
   "the experimental clang interpreter failed to evaluate an expression">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
 def warn_fixedpoint_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,

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().CPlusPlus20)
+      if (const EnumType *ET = dyn_cast<EnumType>(DestType)) {
+        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 End;
+
+          unsigned Bitwidth = Info.Ctx.getIntWidth(DestType);
+          unsigned NumNegativeBits = ED->getNumNegativeBits();
+          unsigned NumPositiveBits = ED->getNumPositiveBits();
+
+          if (NumNegativeBits) {
+            unsigned NumBits = std::max(NumNegativeBits, NumPositiveBits + 1);
+            End = llvm::APInt(Bitwidth, 1) << (NumBits - 1);
+            Min = -End;
+          } else {
+            End = llvm::APInt(Bitwidth, 1) << NumPositiveBits;
+            Min = llvm::APInt::getZero(Bitwidth);
+          }
+
+          if (NumNegativeBits &&
+              (End.sle(Result.getInt()) || Min.sgt(Result.getInt())))
+            CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
+                << Min.getSExtValue() << (End.getSExtValue() - 1);
+          else if (!NumNegativeBits && End.ule(Result.getInt()))
+            CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
+                << Min.getZExtValue() << (End.getZExtValue() - 1);
+        }
+      }
+
     return Success(HandleIntToIntCast(Info, E, DestType, SrcType,
                                       Result.getInt()), E);
   }
 
   case CK_PointerToIntegral: {
     CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
 
     LValue LV;

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

namespace PR45879 {
   // Only syntactic assignments change the active union member.
   constexpr bool g() { // expected-error {{never produces a constant expression}}
     C c = {.n = 1};
     c.a.operator=(B{2}.a); // expected-note 2{{member call on member 'a' of union with active member 'n' is not allowed in a constant expression}}
     return c.a.n == 2;
   }
   static_assert(g()); // expected-error {{constant expression}} expected-note {{in call}}
 }
+
+enum E { e1=-4, e2=4};
+void testValueInRangeOfEnumerationValues() {
+  constexpr E x1 = static_cast<E>(8); // expected-error {{must be initialized by a constant expression}}
+  // expected-note@-1 {{store of value outside of the range of unscoped enum, valid values -8 to 7}}
+  constexpr E x2 = static_cast<E>(-8);
+}