[libc++][expected] Implement LWG3836

[yronglin]

Summary

Implement LWG3836 (https://wg21.link/LWG3836)
std::expected<bool, E1> conversion constructor expected(const expected<U, G>&) should take precedence over expected(U&&) with operator bool

[yronglin]

Seems std::optional does not have this issue https://godbolt.org/z/96T5MT695

[Mordante]

Does this have test for all changes paragraphs in the LWG issue?

libcxx/include/__expected/expected.h

181

This was needed for C++03.

227

We typically don't mention LWG numbers in the code. We use them in the tests.

libcxx/include/optional

735

likewise

[yronglin]

Thanks for your review and sorry for the very late reply!

libcxx/include/__expected/expected.h

181

Thanks for the tip, does the header file for C++23 also need this？

227

Thanks, I'll remove this.

[Mordante]

In D155701#4535643, @yronglin wrote:

Thanks for your review and sorry for the very late reply!

No problem, I was quite busy with the release the last week.

libcxx/docs/Status/Cxx23Issues.csv

293

We missed the LLVM 17 deadline.

libcxx/include/__expected/expected.h

181

In C++23 we can use >>>>.

libcxx/include/optional

735

I still see the LWG number.

[yronglin]

Address comments.

[yronglin]

In D155701#4519553, @Mordante wrote:

Does this have test for all changes paragraphs in the LWG issue?

For std::expected, seems @huixie90 has ever add a test in ctor.u.pass.cpp.
For std::optional, I can't reproduce this issue, but I have followed the standard wording.

[yronglin]

Fix CI

[Mordante]

I finally found time to have a closer look at the patch. In general happy but I would like more test coverage to see things are correct.

@huixie90 it would be great when you also can have a look at this patch.

libcxx/include/__expected/expected.h

171–180

Why not something like this? This looks closer to the wording in the issue. It saves the reader from doing mental boolean calculations when comparing the code against the wording in the standard.

I would also more the new condition down so again it's closer to the wording of the standard. The new paragraph 23.5 is also added at the end and not in between.

226–227

Not your change but this matches the order of the wording in the WP.

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

141–149

There are 5 changes in the wording, can you make sure there are tests for all changes?

[yronglin]

Address comments.

[yronglin]

Thanks a lot for your review! @Mordante

libcxx/include/__expected/expected.h

171–180

Seems the converts-from-any-cvref<T, expected<U, G>> is false only performed when if T is not cv bool it true, we can't put s_same<remove_cv_t<_Tp>, bool> and converts-from-any-cvref<T, expected<U, G>> in the same conjunction expression, because the result was not correct. Can we use _If here?, it's more closing to the wording. WDYT?

226–227

Thanks, this looks good, done!

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

141–149

Thanks for your tips, I have add more test cases.

[yronglin]

Address comment, use _If to get closer to the standard wording.

[yronglin]

ping~

[Mordante]

In D155701#4592515, @yronglin wrote:

ping~

I've been quite busy. I'll hope to have time to look at this patch and D154116 later this week.

[yronglin]

In D155701#4592617, @Mordante wrote:

In D155701#4592515, @yronglin wrote:

ping~

I've been quite busy. I'll hope to have time to look at this patch and D154116 later this week.

Thanks!

[Mordante]

LGTM after a minor fix.

libcxx/include/__expected/expected.h

178–179

Some whitespace fixed. This is not done by clang-format since that breaks in C++03.

[yronglin]

Addres comment.

[yronglin]

Thanks a lot for your review! @Mordante

libcxx/include/__expected/expected.h

178–179

Thanks, fixed.

libcxx/docs/Status/Cxx23Issues.csv

 "`3734 <https://wg21.link/LWG3734>`__","Inconsistency in ``inout_ptr`` and ``out_ptr`` for empty case","February 2023","","",""
 "`3772 <https://wg21.link/LWG3772>`__","``repeat_view``'s ``piecewise`` constructor is missing Postconditions","February 2023","","","|ranges|"
 "`3786 <https://wg21.link/LWG3786>`__","Flat maps' deduction guide needs to default ``Allocator`` to be useful","February 2023","","",""
 "`3803 <https://wg21.link/LWG3803>`__","``flat_foo`` constructors taking ``KeyContainer`` lack ``KeyCompare`` parameter","February 2023","","",""
 "`3810 <https://wg21.link/LWG3810>`__","CTAD for ``std::basic_format_args``","February 2023","|Complete|","17.0","|format|"
 "`3827 <https://wg21.link/LWG3827>`__","Deprecate ``<stdalign.h>`` and ``<stdbool.h>`` macros","February 2023","","",""
 "`3828 <https://wg21.link/LWG3828>`__","Sync ``intmax_t`` and ``uintmax_t`` with C2x","February 2023","","",""
 "`3833 <https://wg21.link/LWG3833>`__","Remove specialization ``template<size_t N> struct formatter<const charT[N], charT>``","February 2023","|Complete|","17.0","|format|"
-"`3836 <https://wg21.link/LWG3836>`__","``std::expected<bool, E1>`` conversion constructor ``expected(const expected<U, G>&)`` should take precedence over ``expected(U&&)`` with operator ``bool``","February 2023","","",""
+"`3836 <https://wg21.link/LWG3836>`__","``std::expected<bool, E1>`` conversion constructor ``expected(const expected<U, G>&)`` should take precedence over ``expected(U&&)`` with operator ``bool``","February 2023","|Complete|","18.0",""
 "`3843 <https://wg21.link/LWG3843>`__","``std::expected<T,E>::value() &`` assumes ``E`` is copy constructible","February 2023","|Complete|","17.0",""
 "`3847 <https://wg21.link/LWG3847>`__","``ranges::to`` can still return views","February 2023","|Complete|","17.0","|ranges|"
 "`3862 <https://wg21.link/LWG3862>`__","``basic_const_iterator``'s ``common_type`` specialization is underconstrained","February 2023","","",""
 "`3865 <https://wg21.link/LWG3865>`__","Sorting a range of ``pairs``","February 2023","|Complete|","17.0","|ranges|"
 "`3869 <https://wg21.link/LWG3869>`__","Deprecate ``std::errc`` constants related to UNIX STREAMS","February 2023","","",""
 "`3870 <https://wg21.link/LWG3870>`__","Remove ``voidify``","February 2023","","",""
 "`3871 <https://wg21.link/LWG3871>`__","Adjust note about ``terminate``","February 2023","","",""
 "`3872 <https://wg21.link/LWG3872>`__","``basic_const_iterator`` should have custom ``iter_move``","February 2023","","",""
 "`3875 <https://wg21.link/LWG3875>`__","``std::ranges::repeat_view<T, IntegerClass>::iterator`` may be ill-formed","February 2023","|Complete|","17.0","|ranges|"
 "`3876 <https://wg21.link/LWG3876>`__","Default constructor of ``std::layout_XX::mapping`` misses precondition","February 2023","","",""
 "`3877 <https://wg21.link/LWG3877>`__","Incorrect constraints on ``const``-qualified monadic overloads for ``std::expected``","February 2023","|Complete|","17.0",""
 "`3878 <https://wg21.link/LWG3878>`__","import ``std;`` should guarantee initialization of standard iostreams objects","February 2023","","",""
 "`3879 <https://wg21.link/LWG3879>`__","``erase_if`` for ``flat_{,multi}set`` is incorrectly specified","February 2023","","",""
 "`3880 <https://wg21.link/LWG3880>`__","Clarify ``operator+=`` complexity for ``{chunk,stride}_view::iterator``","February 2023","","","|ranges|"
 "`3881 <https://wg21.link/LWG3881>`__","Incorrect formatting of container adapters backed by ``std::string``","February 2023","|Complete|","17.0","|format|"

libcxx/include/__expected/expected.h

_LIBCPP_HIDE_FROM_ABI constexpr expected(expected&& __other)
     }
   }
 
 private:
   template <class _Up, class _OtherErr, class _UfQual, class _OtherErrQual>
   using __can_convert =
       _And< is_constructible<_Tp, _UfQual>,
             is_constructible<_Err, _OtherErrQual>,
-            _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>,
+            _If<_Not<is_same<remove_cv_t<_Tp>, bool>>::value,
+                _And< _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>,
                       _Not<is_constructible<_Tp, expected<_Up, _OtherErr>>>,
                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>&>>,
                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>>>,
                       _Not<is_convertible<expected<_Up, _OtherErr>&, _Tp>>,
                       _Not<is_convertible<expected<_Up, _OtherErr>&&, _Tp>>,
                       _Not<is_convertible<const expected<_Up, _OtherErr>&, _Tp>>,
-            _Not<is_convertible<const expected<_Up, _OtherErr>&&, _Tp>>,
+                      _Not<is_convertible<const expected<_Up, _OtherErr>&&, _Tp>>>,
+                true_type>,
             _Not<is_constructible<unexpected<_Err>, expected<_Up, _OtherErr>&>>,
             _Not<is_constructible<unexpected<_Err>, expected<_Up, _OtherErr>>>,
             _Not<is_constructible<unexpected<_Err>, const expected<_Up, _OtherErr>&>>,
             _Not<is_constructible<unexpected<_Err>, const expected<_Up, _OtherErr>>> >;
 
   template <class _Func, class... _Args>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(
       std::__expected_construct_in_place_from_invoke_tag __tag, _Func&& __f, _Args&&... __args)
       : __union_(__tag, std::forward<_Func>(__f), std::forward<_Args>(__args)...), __has_val_(true) {}
 
if (__has_val_) {
       std::construct_at(std::addressof(__union_.__val_), std::move(__other.__union_.__val_));
     } else {
       std::construct_at(std::addressof(__union_.__unex_), std::move(__other.__union_.__unex_));
     }
   }
 
   template <class _Up = _Tp>
     requires(!is_same_v<remove_cvref_t<_Up>, in_place_t> && !is_same_v<expected, remove_cvref_t<_Up>> &&
-             !__is_std_unexpected<remove_cvref_t<_Up>>::value && is_constructible_v<_Tp, _Up>)
+             is_constructible_v<_Tp, _Up> && !__is_std_unexpected<remove_cvref_t<_Up>>::value &&
+             (!is_same_v<remove_cv_t<_Tp>, bool> || !__is_std_expected<remove_cvref_t<_Up>>::value))
   _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up, _Tp>)
-  expected(_Up&& __u)
-    noexcept(is_nothrow_constructible_v<_Tp, _Up>) // strengthened
+      expected(_Up&& __u) noexcept(is_nothrow_constructible_v<_Tp, _Up>) // strengthened
       : __has_val_(true) {
     std::construct_at(std::addressof(__union_.__val_), std::forward<_Up>(__u));
   }
 
-
   template <class _OtherErr>
     requires is_constructible_v<_Err, const _OtherErr&>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)
   expected(const unexpected<_OtherErr>& __unex)
     noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened
       : __has_val_(false) {
     std::construct_at(std::addressof(__union_.__unex_), __unex.error());
   }

libcxx/include/optional

struct _CheckOptionalArgsConstructor {
       _LIBCPP_HIDE_FROM_ABI static constexpr bool __enable_explicit() {
           return is_constructible_v<_Tp, _Up&&> &&
                  !is_convertible_v<_Up&&, _Tp>;
       }
     };
     template <class _Up>
     using _CheckOptionalArgsCtor = _If<
         _IsNotSame<__remove_cvref_t<_Up>, in_place_t>::value &&
-        _IsNotSame<__remove_cvref_t<_Up>, optional>::value,
+        _IsNotSame<__remove_cvref_t<_Up>, optional>::value &&
+        (!is_same_v<remove_cv_t<_Tp>, bool> || !__is_std_optional<__remove_cvref_t<_Up>>::value),
         _CheckOptionalArgsConstructor,
         __check_tuple_constructor_fail
     >;
     template <class _QualUp>
     struct _CheckOptionalLikeConstructor {
       template <class _Up, class _Opt = optional<_Up>>
       using __check_constructible_from_opt = _Or<
           is_constructible<_Tp, _Opt&>,
struct _CheckOptionalLikeConstructor {
           is_assignable<_Tp&, _Opt&>,
           is_assignable<_Tp&, _Opt const&>,
           is_assignable<_Tp&, _Opt&&>,
           is_assignable<_Tp&, _Opt const&&>
       >;
       template <class _Up, class _QUp = _QualUp>
       _LIBCPP_HIDE_FROM_ABI static constexpr bool __enable_implicit() {
           return is_convertible<_QUp, _Tp>::value &&
-              !__check_constructible_from_opt<_Up>::value;
+                 (is_same_v<remove_cv_t<_Tp>, bool> || !__check_constructible_from_opt<_Up>::value);
       }
       template <class _Up, class _QUp = _QualUp>
       _LIBCPP_HIDE_FROM_ABI static constexpr bool __enable_explicit() {
           return !is_convertible<_QUp, _Tp>::value &&
-              !__check_constructible_from_opt<_Up>::value;
+                 (is_same_v<remove_cv_t<_Tp>, bool> || !__check_constructible_from_opt<_Up>::value);
       }
       template <class _Up, class _QUp = _QualUp>
       _LIBCPP_HIDE_FROM_ABI static constexpr bool __enable_assign() {
           // Construction and assignability of _QUp to _Tp has already been
           // checked.
           return !__check_constructible_from_opt<_Up>::value &&
               !__check_assignable_from_opt<_Up>::value;
       }

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

 struct CopyOnly {
   int i;
   constexpr CopyOnly(int ii) : i(ii) {}
   CopyOnly(const CopyOnly&) = default;
   CopyOnly(CopyOnly&&)      = delete;
   friend constexpr bool operator==(const CopyOnly& mi, int ii) { return mi.i == ii; }
 };
 
+struct BaseError {};
+struct DerivedError : BaseError {};
+
 template <class T>
 constexpr void testInt() {
   std::expected<T, int> e(5);
   assert(e.has_value());
   assert(e.value() == 5);
 }
 
 template <class T>
struct Bar {
       constexpr Bar(int ii, int jj) : i(ii), j(jj) {}
     };
 
     std::expected<Bar, int> e({5, 6});
     assert(e.value().i == 5);
     assert(e.value().j == 6);
   }
 
-  // this is a confusing example, but the behaviour
-  // is exactly what is specified in the spec
-  // see https://cplusplus.github.io/LWG/issue3836
+  // https://cplusplus.github.io/LWG/issue3836
+
+  // Test &
   {
-    struct BaseError {};
-    struct DerivedError : BaseError {};
+    std::expected<bool, DerivedError> e1(false);
+    std::expected<bool, BaseError> e2(e1);
+    assert(e2.has_value());
+    assert(!e2.value()); // yes, e2 holds "false" since LWG3836
+  }
 
+  // Test &&
+  {
     std::expected<bool, DerivedError> e1(false);
+    std::expected<bool, BaseError> e2(std::move(e1));
+    assert(e2.has_value());
+    assert(!e2.value()); // yes, e2 holds "false" since LWG3836
+  }
+
+  // Test const&
+  {
+    const std::expected<bool, DerivedError> e1(false);
     std::expected<bool, BaseError> e2(e1);
     assert(e2.has_value());
-    assert(e2.value()); // yes, e2 holds "true"
+    assert(!e2.value()); // yes, e2 holds "false" since LWG3836
+  }
+
+  // Test const&&
+  {
+    const std::expected<bool, DerivedError> e1(false);
+    std::expected<bool, BaseError> e2(std::move(e1));
+    assert(e2.has_value());
+    assert(!e2.value()); // yes, e2 holds "false" since LWG3836
   }
   return true;
 }
 
 void testException() {
 #ifndef TEST_HAS_NO_EXCEPTIONS
   struct Except {};
 

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/copy.pass.cpp

TestTypes::TestType::reset();
         test_reference_extension();
     }
     {
         constexpr std::optional<int> o1{4};
         constexpr std::optional<int> o2 = o1;
         static_assert( *o2 == 4, "" );
     }
 
+    // LWG3836 https://wg21.link/LWG3836
+    // std::optional<bool> conversion constructor optional(const optional<U>&)
+    // should take precedence over optional(U&&) with operator bool
+    {
+        std::optional<bool> o1(false);
+        std::optional<bool> o2(o1);
+        assert(!o2.value());
+    }
+
   return 0;
 }