[libc++] Implement LWG3843 (std::expected<T,E>::value() & assumes E is copy constructible)

[yronglin]

Summary

Implement LWG3843 (std::expected<T,E>::value() & assumes E is copy constructible)
https://wg21.link/LWG3843

[yronglin]

Fix ci

[yronglin]

Fix ci

[crtrott]

LGTM, but I haven't been here long enough to feel like I can accept this on behalf of libc++ team :-)

[yronglin]

In D154110#4471709, @crtrott wrote:

LGTM, but I haven't been here long enough to feel like I can accept this on behalf of libc++ team :-)

Many thanks for your review! @crtrott let's wait for the sign off from libc++ team.

[ldionne]

Thanks for the patch! I really appreciate the added .verify.pass.cpp tests. I have a few questions but this basically LGTM.

libcxx/include/__expected/expected.h

565

I don't think it's possible to test this added as_const, since IIUC this is a no-op given that this member function has a const& qualifier. Right?

573

I don't think we're testing the added as_const here. I think we could test this by using an error type with two constructors:

Error(Error const&);
Error(Error&);

We'd make sure that the first one got called, not the second one. Does that make sense?

libcxx/test/libcxx/utilities/expected/expected.expected/value.observers.verify.cpp

124–129

Right?

[yronglin]

Address comments, and undo the change of ReleastNotes.rst

[yronglin]

Thanks for your review @ldionne !

libcxx/include/__expected/expected.h

565

Yes, as you said, this member function has a const& qualifier.

573

Agree. I have add a test case to check this.

libcxx/test/libcxx/utilities/expected/expected.expected/value.observers.verify.cpp

124–129

Yes!

[ldionne]

libcxx/test/std/utilities/expected/expected.expected/observers/value.pass.cpp

23–39

I don't think this tests what you intend to test. If you remove as_const in the & overload, does this test suddenly fail? I don't think it would, since you're still providing the declaration of the member function.

Instead, I think what you want is this:

bool ref_called = true;
bool const_ref_called = true;

struct Foo {
  Foo(Foo const&) { const_ref_called = true; }
  Foo(Foo&) { ref_called = true; }
};

int main() {
  std::expected<int, Foo> e;
  try {
    e.value();
  catch (std::bad_expected_access const&) {
    assert(const_ref_called);
  }
}

Obviously this is a poor way to test this, but I think you see the point I'm trying to make?

[yronglin]

Address comments.

[yronglin]

Thanks for your review @ldionne !

libcxx/test/std/utilities/expected/expected.expected/observers/value.pass.cpp

23–39

Good catch! It's my mistake, I have update the test case.

[ldionne]

Thanks!

[yronglin]

In D154110#4482106, @ldionne wrote:

Thanks!

Thanks for your review!

libcxx/docs/Status/Cxx23Issues.csv

 "`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","","",""
-"`3843 <https://wg21.link/LWG3843>`__","``std::expected<T,E>::value() &`` assumes ``E`` is copy constructible","February 2023","","",""
+"`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","","","|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","","","|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

 #include <__type_traits/is_trivially_copy_constructible.h>
 #include <__type_traits/is_trivially_destructible.h>
 #include <__type_traits/is_trivially_move_constructible.h>
 #include <__type_traits/is_void.h>
 #include <__type_traits/lazy.h>
 #include <__type_traits/negation.h>
 #include <__type_traits/remove_cv.h>
 #include <__type_traits/remove_cvref.h>
+#include <__utility/as_const.h>
 #include <__utility/exception_guard.h>
 #include <__utility/forward.h>
 #include <__utility/in_place.h>
 #include <__utility/move.h>
 #include <__utility/swap.h>
 #include <__verbose_abort>
 #include <initializer_list>
 
_LIBCPP_HIDE_FROM_ABI constexpr _Tp&& operator*() && noexcept {
     return std::move(__union_.__val_);
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr explicit operator bool() const noexcept { return __has_val_; }
 
   _LIBCPP_HIDE_FROM_ABI constexpr bool has_value() const noexcept { return __has_val_; }
 
   _LIBCPP_HIDE_FROM_ABI constexpr const _Tp& value() const& {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be copy constructible");
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(__union_.__unex_);
+      std::__throw_bad_expected_access<_Err>(std::as_const(error()));
     }
     return __union_.__val_;
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr _Tp& value() & {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be copy constructible");
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(__union_.__unex_);
+      std::__throw_bad_expected_access<_Err>(std::as_const(error()));
     }
     return __union_.__val_;
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr const _Tp&& value() const&& {
+    static_assert(is_copy_constructible_v<_Err> && is_constructible_v<_Err, decltype(std::move(error()))>,
+                  "error_type has to be both copy constructible and constructible from decltype(std::move(error()))");
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
+      std::__throw_bad_expected_access<_Err>(std::move(error()));
     }
     return std::move(__union_.__val_);
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr _Tp&& value() && {
+    static_assert(is_copy_constructible_v<_Err> && is_constructible_v<_Err, decltype(std::move(error()))>,
+                  "error_type has to be both copy constructible and constructible from decltype(std::move(error()))");
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
+      std::__throw_bad_expected_access<_Err>(std::move(error()));
     }
     return std::move(__union_.__val_);
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr const _Err& error() const& noexcept {
     _LIBCPP_ASSERT_UNCATEGORIZED(!__has_val_, "expected::error requires the expected to contain an error");
     return __union_.__unex_;
   }

libcxx/test/libcxx/utilities/expected/expected.expected/value.observers.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// Test the mandates
+// constexpr T& value() & ;
+// constexpr const T& value() const &;
+// Mandates: is_copy_constructible_v<E> is true.
+
+// constexpr T&& value() &&;
+// constexpr const T&& value() const &&;
+// Mandates: is_copy_constructible_v<E> is true and is_constructible_v<E, decltype(std::move(error()))> is true.
+
+#include <expected>
+#include <utility>
+
+#include "MoveOnly.h"
+
+struct CopyConstructible {
+  constexpr CopyConstructible()                         = default;
+  constexpr CopyConstructible(const CopyConstructible&) = default;
+};
+
+struct CopyConstructibleButNotMoveConstructible {
+  constexpr CopyConstructibleButNotMoveConstructible()                                                 = default;
+  constexpr CopyConstructibleButNotMoveConstructible(const CopyConstructibleButNotMoveConstructible&)  = default;
+  constexpr CopyConstructibleButNotMoveConstructible(CopyConstructibleButNotMoveConstructible&&)       = delete;
+  constexpr CopyConstructibleButNotMoveConstructible(const CopyConstructibleButNotMoveConstructible&&) = delete;
+};
+
+struct CopyConstructibleAndMoveConstructible {
+  constexpr CopyConstructibleAndMoveConstructible()                                             = default;
+  constexpr CopyConstructibleAndMoveConstructible(const CopyConstructibleAndMoveConstructible&) = default;
+  constexpr CopyConstructibleAndMoveConstructible(CopyConstructibleAndMoveConstructible&&)      = default;
+};
+
+// clang-format off
+void test() {
+
+  // Test & overload
+  {
+    // is_copy_constructible_v<E> is true.
+    {
+      std::expected<int, CopyConstructible> e;
+      [[maybe_unused]] auto val = e.value();
+    }
+
+    // is_copy_constructible_v<E> is false.
+    {
+      std::expected<int, MoveOnly> e;
+      [[maybe_unused]] auto val = e.value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be copy constructible}}
+    }
+  }
+
+  // Test const& overload
+  {
+    // is_copy_constructible_v<E> is true.
+    {
+      const std::expected<int, CopyConstructible> e;
+      [[maybe_unused]] auto val = e.value();
+    }
+
+    // is_copy_constructible_v<E> is false.
+    {
+      const std::expected<int, MoveOnly> e;
+      [[maybe_unused]] auto val = e.value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be copy constructible}}
+    }
+  }
+
+  // Test && overload
+  {
+    // is_copy_constructible_v<E> is false.
+    {
+      std::expected<int, MoveOnly> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be both copy constructible and constructible from decltype(std::move(error()))}}
+    }
+
+    // is_copy_constructible_v<E> is true and is_constructible_v<E, decltype(std::move(error()))> is true.
+    {
+      std::expected<int, CopyConstructibleAndMoveConstructible> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+    }
+
+    // is_copy_constructible_v<E> is true and is_constructible_v<E, decltype(std::move(error()))> is false.
+    {
+      std::expected<int, CopyConstructibleButNotMoveConstructible> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be both copy constructible and constructible from decltype(std::move(error()))}}
+    }
+  }
+
+  // Test const&& overload
+  {
+    // is_copy_constructible_v<E> is false.
+    {
+      const std::expected<int, MoveOnly> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be both copy constructible and constructible from decltype(std::move(error()))}}
+    }
+
+    // is_copy_constructible_v<E> is true and is_constructible_v<E, decltype(std::move(error()))> is true.
+    {
+      const std::expected<int, CopyConstructibleAndMoveConstructible> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+    }
+
+    // is_copy_constructible_v<E> is true and is_constructible_v<E, decltype(std::move(error()))> is false.
+    {
+      const std::expected<int, CopyConstructibleButNotMoveConstructible> e;
+      [[maybe_unused]] auto val = std::move(e).value();
+      // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}error_type has to be both copy constructible and constructible from decltype(std::move(error()))}}
+    }
+  }
+// These diagnostics happen when we try to construct bad_expected_access from the non copy-constructible error type.
+#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+#endif
+}
+// clang-format on

libcxx/test/std/utilities/expected/expected.expected/observers/value.pass.cpp

 // constexpr const T&& value() const &&;
 
 #include <cassert>
 #include <concepts>
 #include <expected>
 #include <type_traits>
 #include <utility>
 
-#include "MoveOnly.h"
 #include "test_macros.h"
 
 constexpr bool test() {
   // non-const &
   {
     std::expected<int, int> e(5);
     decltype(auto) x = e.value();
     static_assert(std::same_as<decltype(x), int&>);
     assert(&x == &(*e));
     assert(x == 5);
   }
 
   // const &
   {
     const std::expected<int, int> e(5);
     decltype(auto) x = e.value();
     static_assert(std::same_as<decltype(x), const int&>);
     assert(&x == &(*e));
     assert(x == 5);
   }
 
   // non-const &&
   {
     std::expected<int, int> e(5);
     decltype(auto) x = std::move(e).value();
     static_assert(std::same_as<decltype(x), int&&>);
     assert(&x == &(*e));
// int
     try {
       (void) e.value();
       assert(false);
     } catch (const std::bad_expected_access<int>& ex) {
       assert(ex.error() == 5);
     }
   }
 
-  // MoveOnly
+#endif // TEST_HAS_NO_EXCEPTIONS
+}
+
+void testAsConst() {
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  struct Error {
+    enum { Default, MutableRefCalled, ConstRefCalled } From = Default;
+    Error()                                                 = default;
+    Error(const Error&) { From = ConstRefCalled; }
+    Error(Error&) { From = MutableRefCalled; }
+    Error(Error&& e) { From = e.From; }
+  };
+
+  // Test & overload
   {
-    std::expected<int, MoveOnly> e(std::unexpect, 5);
+    std::expected<int, Error> e(std::unexpect, Error());
     try {
-      (void) std::move(e).value();
+      (void)e.value();
       assert(false);
-    } catch (const std::bad_expected_access<MoveOnly>& ex) {
-      assert(ex.error() == 5);
+    } catch (const std::bad_expected_access<Error>& ex) {
+      assert(ex.error().From == Error::ConstRefCalled);
     }
   }
 
+  // There are no effects for `const &` overload.
+
 #endif // TEST_HAS_NO_EXCEPTIONS
 }
 
 int main(int, char**) {
   test();
   static_assert(test());
   testException();
+  testAsConst();
+
   return 0;
 }