[libc++] Re-introduce special support for narrowing conversions to bool in variant #73121
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: None (bgra8) Changes. Full diff: https://github.com/llvm/llvm-project/pull/73121.diff 6 Files Affected:
diff --git a/libcxx/docs/Status/Cxx20Papers.csv b/libcxx/docs/Status/Cxx20Papers.csv
index 7aff860c68cf98e..cf7cf7f6dbdef0c 100644
--- a/libcxx/docs/Status/Cxx20Papers.csv
+++ b/libcxx/docs/Status/Cxx20Papers.csv
@@ -173,7 +173,7 @@
"`P1868R2 <https://wg21.link/P1868R2>`__","LWG","width: clarifying units of width and precision in std::format","Prague","|Complete|","14.0"
"`P1937R2 <https://wg21.link/P1937R2>`__","CWG","Fixing inconsistencies between constexpr and consteval functions","Prague","* *",""
"`P1956R1 <https://wg21.link/P1956R1>`__","LWG","On the names of low-level bit manipulation functions","Prague","|Complete|","12.0"
-"`P1957R2 <https://wg21.link/P1957R2>`__","CWG","Converting from ``T*``\ to bool should be considered narrowing (re: US 212)","Prague","|Complete|","18.0"
+"`P1957R2 <https://wg21.link/P1957R2>`__","CWG","Converting from ``T*``\ to bool should be considered narrowing (re: US 212)","Prague","* *",""
"`P1963R0 <https://wg21.link/P1963R0>`__","LWG","Fixing US 313","Prague","* *","",""
"`P1964R2 <https://wg21.link/P1964R2>`__","LWG","Wording for boolean-testable","Prague","|Complete|","13.0"
"`P1970R2 <https://wg21.link/P1970R2>`__","LWG","Consistency for size() functions: Add ranges::ssize","Prague","|Complete|","15.0","|ranges|"
diff --git a/libcxx/include/variant b/libcxx/include/variant
index d89c0596b1db0de..414cdc8b85469df 100644
--- a/libcxx/include/variant
+++ b/libcxx/include/variant
@@ -1252,6 +1252,24 @@ struct __overload {
auto operator()(_Tp, _Up&&) const -> __check_for_narrowing<_Tp, _Up>;
};
+#ifdef _LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT
+template <class _Tp, size_t>
+struct __overload_bool {
+ template <class _Up, class _Ap = __remove_cvref_t<_Up>>
+ auto operator()(bool, _Up&&) const
+ -> enable_if_t<is_same_v<_Ap, bool>, __type_identity<_Tp>>;
+};
+
+template <size_t _Idx>
+struct __overload<bool, _Idx> : __overload_bool<bool, _Idx> {};
+template <size_t _Idx>
+struct __overload<bool const, _Idx> : __overload_bool<bool const, _Idx> {};
+template <size_t _Idx>
+struct __overload<bool volatile, _Idx> : __overload_bool<bool volatile, _Idx> {};
+template <size_t _Idx>
+struct __overload<bool const volatile, _Idx> : __overload_bool<bool const volatile, _Idx> {};
+#endif
+
template <class ..._Bases>
struct __all_overloads : _Bases... {
void operator()() const;
diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
index c63626090c2fe9a..de9853f6930c6df 100644
--- a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
@@ -146,8 +146,13 @@ void test_T_assignment_sfinae() {
};
static_assert(!std::is_assignable<V, X>::value,
"no boolean conversion in operator=");
+#if defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
+ static_assert(!std::is_assignable<V, std::false_type>::value,
+ "no converted to bool in operator=");
+#else
static_assert(std::is_assignable<V, std::false_type>::value,
"converted to bool in operator=");
+#endif
}
{
struct X {};
@@ -296,6 +301,7 @@ void test_T_assignment_performs_assignment() {
#endif // TEST_HAS_NO_EXCEPTIONS
}
+#if !defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
void test_T_assignment_vector_bool() {
std::vector<bool> vec = {true};
std::variant<bool, int> v;
@@ -303,6 +309,7 @@ void test_T_assignment_vector_bool() {
assert(v.index() == 0);
assert(std::get<0>(v) == true);
}
+#endif
int main(int, char**) {
test_T_assignment_basic();
@@ -310,7 +317,9 @@ int main(int, char**) {
test_T_assignment_performs_assignment();
test_T_assignment_noexcept();
test_T_assignment_sfinae();
+#if !defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
test_T_assignment_vector_bool();
+#endif
return 0;
}
diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp
index d93d429e262c05b..f927db744e297c8 100644
--- a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp
@@ -33,8 +33,13 @@ int main(int, char**)
static_assert(!std::is_assignable<std::variant<int, bool>, decltype("meow")>::value, "");
static_assert(!std::is_assignable<std::variant<int, const bool>, decltype("meow")>::value, "");
-
+#if defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
+ static_assert(!std::is_assignable<std::variant<int, const volatile bool>, decltype("meow")>::value, "");
+ static_assert(!std::is_assignable<std::variant<bool>, std::true_type>::value, "");
+#else
static_assert(std::is_assignable<std::variant<bool>, std::true_type>::value, "");
+#endif
+
static_assert(!std::is_assignable<std::variant<bool>, std::unique_ptr<char> >::value, "");
static_assert(!std::is_assignable<std::variant<bool>, decltype(nullptr)>::value, "");
diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
index 04329f4f412b06a..4a0a31fdf320e66 100644
--- a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
@@ -80,8 +80,13 @@ void test_T_ctor_sfinae() {
};
static_assert(!std::is_constructible<V, X>::value,
"no boolean conversion in constructor");
+#if defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
+ static_assert(!std::is_constructible<V, std::false_type>::value,
+ "no converted to bool in constructor");
+#else
static_assert(std::is_constructible<V, std::false_type>::value,
"converted to bool in constructor");
+#endif
}
{
struct X {};
@@ -199,12 +204,14 @@ void test_construction_with_repeated_types() {
static_assert(std::is_constructible<V, Bar>::value, "");
}
+#if !defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
void test_vector_bool() {
std::vector<bool> vec = {true};
std::variant<bool, int> v = vec[0];
assert(v.index() == 0);
assert(std::get<0>(v) == true);
}
+#endif
int main(int, char**) {
test_T_ctor_basic();
@@ -212,6 +219,8 @@ int main(int, char**) {
test_T_ctor_sfinae();
test_no_narrowing_check_for_class_types();
test_construction_with_repeated_types();
+#if !defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
test_vector_bool();
+#endif
return 0;
}
diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp
index 7a4c68b5dacca7f..e4ee27a614f35d9 100644
--- a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp
@@ -32,8 +32,13 @@ int main(int, char**)
static_assert(!std::is_constructible<std::variant<int, bool>, decltype("meow")>::value, "");
static_assert(!std::is_constructible<std::variant<int, const bool>, decltype("meow")>::value, "");
-
+#if defined(_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT)
+ static_assert(!std::is_constructible<std::variant<int, const volatile bool>, decltype("meow")>::value, "");
+ static_assert(!std::is_constructible<std::variant<bool>, std::true_type>::value, "");
+#else
static_assert(std::is_constructible<std::variant<bool>, std::true_type>::value, "");
+#endif
+
static_assert(!std::is_constructible<std::variant<bool>, std::unique_ptr<char> >::value, "");
static_assert(!std::is_constructible<std::variant<bool>, decltype(nullptr)>::value, "");
|
|
I don't get what exactly went wrong with the |
You can ignore that for the time being. We're trying to figure this out. |
|
Can you explain why you want to make this change? 170810f made our implementation conforming, and this makes us non-conforming with the macro defined. |
We are seeing large fallout (@google) from 170810f preventing us to advance our testing of the compiler. This is a temporary workaround until we fix our code. |
libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp
Outdated
Show resolved
Hide resolved
The problem is that 170810f causes not just compilation errors, but also runtime behavior change. We see thousands of build breakages and hundreds or even thousands of test failures root-causing to this change. Bringing libc++ to conformance is a good cause, but this was a breaking change with very widespread effect. |
|
I see, thanks for the context. In the future, I suggest including that information in the original PR message since you should expect us to ask this very question. In order to determine what we want to do with this, I'd like to have a bit more information:
At the moment, I'm trying to figure out whether this is serious enough that we need to offer a transition path for everyone for one release, or whether this is even more serious and we need to bring this up in WG21. Or whether this is actually OK, none of the issues are vexing and basically this isn't something we'd want to work around upstream. Everything's on the table, but I'd like more information to help inform that decision. |
Most of the breakages are caused by clearly wrong code in the first place: https://godbolt.org/z/j6Tj864jT
We are actively working for reproducers here. Are you OK if we post them later? |
I am a bit confused by this Godbolt -- it seems like that code wasn't accepted by Clang 16 either, so this is probably not related to this change. https://godbolt.org/z/xjYj1eccq
Like I said, we need to understand the situation before we can make a decision about how to handle this. There's no desire to delay this more than needed, but at the moment we have no information. If this patch needs to land within minutes in order to unblock you downstream, that's fair but in that case you should apply it downstream first while we work out what to do upstream, which may take a bit more than just a few minutes. |
The paper explored the impact of the language change. At that time pre- 170810f libc++ is not being used. The breakage seems to be caused by later added code that counts on the pre- 170810f behavior which doesn't belong to any standard, nor is shared by other implementations. As @bgra8 says, those codes would be clearly wrong, but Google may need time to figure out. |
|
@bgra8 Are you OK if I push to your branch directly with changes that implement what I think is a reasonable way forward? Basically I'd add a flag for one release but do it in a way to make the cleanup really easy. |
Sorry my bad here! The missing bit is that we need to pass Not sure what
|
Sure!!! |
|
Oh my, I just noticed that Anyway, we'll honor |
|
I'm OK with the changes in the current state of this patch -- with my amendment. I will merge this once the CI is green. |
|
@bgra8 We would still need reproducers for the runtime issues you've seen. I'm not saying a reproducer for each, but we'd like to understand the general class of issues this can introduce, since if that's really serious we may want to take an action different from what we're doing here. |
|
Thanks @ldionne !! I'll wait for the bots to finish their checks and submit this. |
Sure, we're working on this. |
ldionne
changed the title
_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT.
|
This is all a bit of a mess. :-) Here's what I think is going on. (NB: I may have some details wrong here.) As I understand it, there are four different
Prior to my PR, #71502 changed libc++'s defaults from p0608r3 to p1957r2, the correct behavior. The motivation on my end was Chrome (which doesn't plan to set the flag) ran into the The delta from mostly-pre-p0608r3 back into pre-p0608r3 seems to be a bit turbulent. The delta from p0608r3 to p1957r2 seems to be fine? At least, we haven't seen any evidence yet that those changes cause much problems. The causes that hit the two "pre" cases are mostly all rejected by the narrowing rule anyway. Since that's a lot of text, here are some tables to summarize the mess. :-)
Observe that the rightmost column is the only one with no sad faces. Also a slight footnote on this table is that p1957r2 changed both language behavior and library behavior. AIUI, the library change is only safe (i.e. doesn't regress the
|
bgra8
force-pushed
the
revert_variant
branch
from
26b9763 to
b8e3985
Compare
|
Wow, thanks a ton for this summary. This is amazing and extremely useful since this is indeed very confusing. So I think we're all on the same page. Basically most people in the wild will see their code go from p0608r3 to p1957r2, which seems like it wouldn't cause a lot of issues. Google, who has been enabling the flag, will be allowed to stay in mostly-pre-p0608r3 world for a bit more time, and then they'll hop on the standard p1957r2 behavior. They'll have roughly until we branch for LLVM 18 to do this, which should happen some time in January of February. It seems like the real offender here is that the work to switch away from mostly-pre-p0608r3 behavior was never completed at Google (for ~4 years) since there was the option not to (via the macro). I think this is totally understandable, honestly the same would happen around here. However, I think it is a great showcase of why we have to be careful about accepting debt like this upstream since it often removes the incentive for downstreams to actually pay it off, and it harms the project in the long run. Anyway, I think what we have here is a sensible transition plan. |
That's right. I should say, this is more by absence of evidence than anything else. The big internal Google repository is on mostly-pre-p0608r3, so no data from that. Chromium (where I spend my time) is also no data because it's currently on pre-p0608r3 by way of But given that GCC implemented p1957r2 in 2020 seemingly with no problems, hopefully it's fine? |
Because you made the column. Not accepting I'm pretty sure Google will be able to handle the revert here sooner rather than later. We'll just want to always enable the narrowing check for boolean. When I surveyed the Google code base a few years ago, bool was the only type that caused surprising conversion behavior. Every other "narrowing" conversion was either a literal, or obviously what the developer intended. Bool was the only type which caused a problem. The reason Google (read EricWF) was so willing to punt on this issue is that it was a moving target. There are four columns in the table, which is a lot of times to get broken while the committee makes up their mind. |
Fair enough. I think that's a different level of sad, compare to the ones which silently pick a surprising alternate. But the way the language forgets which things were literals is definitely not great. It usually requires just a single character to fix (0.0f and 0u), but definitely awkward. Perhaps a better table would have had both 😟 and 😭 entries. Really the root problem here, IMO, is that a giant overload set is a bad way to do sum types. Every other language names the alternates and then has the caller say the name of the one they wanted. Then things like implicit conversions are no more awkward than usual. For that matter, language-level sum type support could also avoid the issue with literals. But that's not really an option at this point. (Clearly we need to define |
Well, it's a little interesting because the bool special case isn't a narrowing check, but an exact match. That was because, prior to the language change half of p1957r2, a narrowing check on bool wasn't sufficient. But, in working around that, other things went wrong like with vector. Now that the language change happened, that's no longer needed and the actual narrowing check suffices. The problem is just that, when the narrowing check is suppressed, the bool special case was a bit more load bearing. It's all a mess. |
Indeed. I was rather keen to address at least the boolean conversion issues. I just wish we had the bool narrowing changes in the language at the time. It's certainly much nicer to accept
And in that table we'll find the essence of C++.
Amen. |
This patch re-introduces special support for narrowing conversions to bool
in std::variant, which was removed in 170810f in order to make libc++
Standards-conforming.
The special support is gated by the
_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANTmacro and will be supported for LLVM 18 only as a courtesy to help large
code bases migrate over to the Standard behavior.