This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shafik on Jul 18 2022, 9:10 PM.

Details

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.

Diff Detail

Event Timeline

shafik created this revision.Jul 18 2022, 9:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 9:10 PM
shafik requested review of this revision.Jul 18 2022, 9:10 PM
erichkeane added inline comments.Jul 19 2022, 6:32 AM
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
13522

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

13543

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 ↗(On Diff #445688)

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 added inline comments.Jul 19 2022, 6:59 AM
clang/lib/AST/ExprConstant.cpp
13543

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 ↗(On Diff #445688)

Another good test is the empty-enum case.

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
13522

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

13543

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 ↗(On Diff #445688)

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

erichkeane added inline comments.Jul 19 2022, 11:49 AM
clang/lib/AST/ExprConstant.cpp
13537

Why "End" instead of max?

13543

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 updated this revision to Diff 446045.Jul 19 2022, 10:29 PM
shafik marked 7 inline comments as done.
  • 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 added inline comments.Jul 19 2022, 10:31 PM
clang/test/Sema/aarch64-sve-intrinsics/acle_sve_imm.cpp
209

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

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 added inline comments.Jul 19 2022, 10:33 PM
clang/include/clang/AST/Decl.h
3845

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 added inline comments.Jul 20 2022, 6:08 AM
clang/lib/AST/Decl.cpp
4629

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

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

2429

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 updated this revision to Diff 447785.Jul 26 2022, 11:51 AM
shafik marked 3 inline comments as done.
  • Added documentation for getValueRange(...)
  • Added new tests to cover fix introduced by D130301
aaron.ballman added inline comments.Jul 26 2022, 12:38 PM
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 updated this revision to Diff 447825.Jul 26 2022, 2:10 PM
shafik marked an inline comment as done.
  • Changed wording for diagnostic and updated the places where it was used and checked in tests
aaron.ballman accepted this revision.Jul 27 2022, 6:32 AM

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
13526–13531
This revision is now accepted and ready to land.Jul 27 2022, 6:32 AM

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

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

2427

I see no tests for E3?

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.

erichkeane added inline comments.Jul 27 2022, 6:38 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.Jul 27 2022, 6:47 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 accepted this revision.Jul 27 2022, 6:51 AM

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

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 added inline comments.
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.Jul 27 2022, 7:59 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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

cjdb added inline comments.Jul 27 2022, 9:53 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.Jul 27 2022, 11:15 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.Jul 27 2022, 11:50 AM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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

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

erichkeane added inline comments.Jul 27 2022, 12:06 PM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

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 added inline comments.Jul 27 2022, 12:08 PM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2420

+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 updated this revision to Diff 448173.Jul 27 2022, 2:55 PM
shafik marked 14 inline comments as done.
  • Add release note
  • Reflow diagnostic
  • Update defect report status
  • Add test for E3
This revision was landed with ongoing or failed builds.Jul 27 2022, 3:03 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 3:03 PM
shafik updated this revision to Diff 448390.Jul 28 2022, 11:24 AM
  • 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 added inline comments.Jul 28 2022, 11:27 AM
clang/test/Analysis/cfg.cpp
227

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

clang/test/SemaCXX/enum-scoped.cpp
330

@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

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 added inline comments.Jul 28 2022, 11:35 AM
clang/test/SemaCXX/enum-scoped.cpp
330

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 added a subscriber: thakis.Jul 29 2022, 8:55 AM

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 added inline comments.Jul 29 2022, 9:06 AM
clang/docs/ReleaseNotes.rst
55–57

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

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.

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 added a comment.EditedJul 29 2022, 11:59 AM

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.

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 added a comment.EditedJul 30 2022, 10:17 AM

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:

  • 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.)

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:

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 added a subscriber: smeenai.Aug 3 2022, 3:22 PM

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 added a comment.Aug 3 2022, 4:51 PM

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.

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).

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.

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 added a comment.Aug 4 2022, 7:25 PM

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 added a subscriber: hokein.Aug 5 2022, 5:15 AM

+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);
dschuff added a subscriber: dschuff.Aug 5 2022, 1:57 PM

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

+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 :)

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

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 { ... }.

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

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).

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.

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.