Merge bitcoin/bitcoin#34032: util: Add some more Unexpected and Expected methods

faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)

Pull request description:

  Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.

  They are currently unused, but bring the port closer to the original `std::expected` implementation:

  * Make `Expected::value()` throw when no value exists
  * Add `Unexpected::error()` methods
  * Add `Expected<void, E>` specialization
  * Add `Expected::value()&&` and `Expected::error()&&` methods
  * Add `Expected::swap()`

  Also, include a tiny tidy fixup:

  * tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check

ACKs for top commit:
  stickies-v:
    re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
  ryanofsky:
    Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
  hodlinator:
    re-ACK faa59b367985648df901bdd7b5bba69ef898ea08

Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf

src/.clang-tidy

@@ -41,3 +41,5 @@ CheckOptions:
    value: false
  - key: bugprone-unused-return-value.CheckedReturnTypes
    value: '^::std::error_code$;^::std::error_condition$;^::std::errc$;^::std::expected$;^::util::Result$;^::util::Expected$'
+ - key: bugprone-unused-return-value.AllowCastToVoid
+   value: true  # Can be removed with C++26 once the _ placeholder exists.

src/bench/coin_selection.cpp

@@ -130,7 +130,7 @@ static void BnBExhaustion(benchmark::Bench& bench)
     bench.run([&] {
         // Benchmark
         CAmount target = make_hard_case(17, utxo_pool);
-        [[maybe_unused]] auto _{SelectCoinsBnB(utxo_pool, target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT)}; // Should exhaust
+        (void)SelectCoinsBnB(utxo_pool, target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT); // Should exhaust
 
         // Cleanup
         utxo_pool.clear();

src/httpserver.cpp

@@ -330,7 +330,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
         std::unique_ptr<HTTPWorkItem> item(new HTTPWorkItem(std::move(hreq), path, i->handler));
         assert(g_work_queue);
         if (g_work_queue->Enqueue(item.get())) {
-            [[maybe_unused]] auto _{item.release()}; /* if true, queue took ownership */
+            (void)item.release(); /* if true, queue took ownership */
         } else {
             LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting");
             item->req->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");

src/ipc/test/ipc_test.cpp

@@ -63,9 +63,7 @@ void IpcPipeTest()
         auto foo_client = std::make_unique<mp::ProxyClient<gen::FooInterface>>(
             connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
             connection_client.get(), /* destroy_connection= */ true);
-        {
-            [[maybe_unused]] auto _{connection_client.release()};
-        }
+        (void)connection_client.release();
         foo_promise.set_value(std::move(foo_client));
 
         auto connection_server = std::make_unique<mp::Connection>(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) {

src/test/util_expected_tests.cpp

@@ -7,6 +7,11 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <memory>
+#include <string>
+#include <utility>
+
+
 using namespace util;
 
 BOOST_AUTO_TEST_SUITE(util_expected_tests)
@@ -17,15 +22,15 @@ BOOST_AUTO_TEST_CASE(expected_value)
         int x;
     };
     Expected<Obj, int> e{};
-    BOOST_CHECK(e.value().x == 0);
+    BOOST_CHECK_EQUAL(e.value().x, 0);
 
     e = Obj{42};
 
     BOOST_CHECK(e.has_value());
     BOOST_CHECK(static_cast<bool>(e));
-    BOOST_CHECK(e.value().x == 42);
-    BOOST_CHECK((*e).x == 42);
-    BOOST_CHECK(e->x == 42);
+    BOOST_CHECK_EQUAL(e.value().x, 42);
+    BOOST_CHECK_EQUAL((*e).x, 42);
+    BOOST_CHECK_EQUAL(e->x, 42);
 
     // modify value
     e.value().x += 1;
@@ -33,9 +38,23 @@ BOOST_AUTO_TEST_CASE(expected_value)
     e->x += 1;
 
     const auto& read{e};
-    BOOST_CHECK(read.value().x == 45);
-    BOOST_CHECK((*read).x == 45);
-    BOOST_CHECK(read->x == 45);
+    BOOST_CHECK_EQUAL(read.value().x, 45);
+    BOOST_CHECK_EQUAL((*read).x, 45);
+    BOOST_CHECK_EQUAL(read->x, 45);
+}
+
+BOOST_AUTO_TEST_CASE(expected_value_rvalue)
+{
+    Expected<std::unique_ptr<int>, int> no_copy{std::make_unique<int>(5)};
+    const auto moved{std::move(no_copy).value()};
+    BOOST_CHECK_EQUAL(*moved, 5);
+}
+
+BOOST_AUTO_TEST_CASE(expected_deref_rvalue)
+{
+    Expected<std::unique_ptr<int>, int> no_copy{std::make_unique<int>(5)};
+    const auto moved{*std::move(no_copy)};
+    BOOST_CHECK_EQUAL(*moved, 5);
 }
 
 BOOST_AUTO_TEST_CASE(expected_value_or)
@@ -48,21 +67,68 @@ BOOST_AUTO_TEST_CASE(expected_value_or)
     BOOST_CHECK_EQUAL(const_val.value_or("fallback"), "fallback");
 }
 
+BOOST_AUTO_TEST_CASE(expected_value_throws)
+{
+    const Expected<int, std::string> e{Unexpected{"fail"}};
+    BOOST_CHECK_THROW(e.value(), BadExpectedAccess);
+
+    const Expected<void, std::string> void_e{Unexpected{"fail"}};
+    BOOST_CHECK_THROW(void_e.value(), BadExpectedAccess);
+}
+
 BOOST_AUTO_TEST_CASE(expected_error)
 {
     Expected<void, std::string> e{};
     BOOST_CHECK(e.has_value());
+    [&]() -> void { return e.value(); }(); // check value returns void and does not throw
+    [&]() -> void { return *e; }();
 
     e = Unexpected{"fail"};
     BOOST_CHECK(!e.has_value());
     BOOST_CHECK(!static_cast<bool>(e));
-    BOOST_CHECK(e.error() == "fail");
+    BOOST_CHECK_EQUAL(e.error(), "fail");
 
     // modify error
     e.error() += "1";
 
     const auto& read{e};
-    BOOST_CHECK(read.error() == "fail1");
+    BOOST_CHECK_EQUAL(read.error(), "fail1");
+}
+
+BOOST_AUTO_TEST_CASE(expected_error_rvalue)
+{
+    {
+        Expected<int, std::unique_ptr<int>> nocopy_err{Unexpected{std::make_unique<int>(7)}};
+        const auto moved{std::move(nocopy_err).error()};
+        BOOST_CHECK_EQUAL(*moved, 7);
+    }
+    {
+        Expected<void, std::unique_ptr<int>> void_nocopy_err{Unexpected{std::make_unique<int>(9)}};
+        const auto moved{std::move(void_nocopy_err).error()};
+        BOOST_CHECK_EQUAL(*moved, 9);
+    }
+}
+
+BOOST_AUTO_TEST_CASE(unexpected_error_accessors)
+{
+    Unexpected u{std::make_unique<int>(-1)};
+    BOOST_CHECK_EQUAL(*u.error(), -1);
+
+    *u.error() -= 1;
+    const auto& read{u};
+    BOOST_CHECK_EQUAL(*read.error(), -2);
+
+    const auto moved{std::move(u).error()};
+    BOOST_CHECK_EQUAL(*moved, -2);
+}
+
+BOOST_AUTO_TEST_CASE(expected_swap)
+{
+    Expected<const char*, std::unique_ptr<int>> a{Unexpected{std::make_unique<int>(-1)}};
+    Expected<const char*, std::unique_ptr<int>> b{"good"};
+    a.swap(b);
+    BOOST_CHECK_EQUAL(a.value(), "good");
+    BOOST_CHECK_EQUAL(*b.error(), -1);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/util/expected.h

@@ -6,9 +6,10 @@
 #define BITCOIN_UTIL_EXPECTED_H
 
 #include <attributes.h>
+#include <util/check.h>
 
 #include <cassert>
-#include <type_traits>
+#include <exception>
 #include <utility>
 #include <variant>
 
@@ -20,8 +21,18 @@ template <class E>
 class Unexpected
 {
 public:
-    constexpr explicit Unexpected(E e) : err(std::move(e)) {}
-    E err;
+    constexpr explicit Unexpected(E e) : m_error(std::move(e)) {}
+
+    constexpr const E& error() const& noexcept LIFETIMEBOUND { return m_error; }
+    constexpr E& error() & noexcept LIFETIMEBOUND { return m_error; }
+    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(m_error); }
+
+private:
+    E m_error;
+};
+
+struct BadExpectedAccess : std::exception {
+    const char* what() const noexcept override { return "Bad util::Expected access"; }
 };
 
 /// The util::Expected class provides a standard way for low-level functions to
@@ -33,58 +44,87 @@ template <class T, class E>
 class Expected
 {
 private:
-    using ValueType = std::conditional_t<std::is_same_v<T, void>, std::monostate, T>;
-    std::variant<ValueType, E> m_data;
+    std::variant<T, E> m_data;
 
 public:
-    constexpr Expected() : m_data{std::in_place_index_t<0>{}, ValueType{}} {}
-    constexpr Expected(ValueType v) : m_data{std::in_place_index_t<0>{}, std::move(v)} {}
+    constexpr Expected() : m_data{std::in_place_index<0>, T{}} {}
+    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
     template <class Err>
-    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index_t<1>{}, std::move(u.err)}
+    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
     {
     }
 
     constexpr bool has_value() const noexcept { return m_data.index() == 0; }
     constexpr explicit operator bool() const noexcept { return has_value(); }
 
-    constexpr const ValueType& value() const LIFETIMEBOUND
+    constexpr const T& value() const& LIFETIMEBOUND
     {
-        assert(has_value());
+        if (!has_value()) {
+            throw BadExpectedAccess{};
+        }
         return std::get<0>(m_data);
     }
-    constexpr ValueType& value() LIFETIMEBOUND
+    constexpr T& value() & LIFETIMEBOUND
     {
-        assert(has_value());
+        if (!has_value()) {
+            throw BadExpectedAccess{};
+        }
         return std::get<0>(m_data);
     }
+    constexpr T&& value() && LIFETIMEBOUND { return std::move(value()); }
 
     template <class U>
-    ValueType value_or(U&& default_value) const&
+    T value_or(U&& default_value) const&
     {
         return has_value() ? value() : std::forward<U>(default_value);
     }
     template <class U>
-    ValueType value_or(U&& default_value) &&
+    T value_or(U&& default_value) &&
     {
         return has_value() ? std::move(value()) : std::forward<U>(default_value);
     }
 
-    constexpr const E& error() const LIFETIMEBOUND
+    constexpr const E& error() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
+    constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
+    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(error()); }
+
+    constexpr void swap(Expected& other) noexcept { m_data.swap(other.m_data); }
+
+    constexpr T& operator*() & noexcept LIFETIMEBOUND { return value(); }
+    constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return value(); }
+    constexpr T&& operator*() && noexcept LIFETIMEBOUND { return std::move(value()); }
+
+    constexpr T* operator->() noexcept LIFETIMEBOUND { return &value(); }
+    constexpr const T* operator->() const noexcept LIFETIMEBOUND { return &value(); }
+};
+
+template <class E>
+class Expected<void, E>
+{
+private:
+    std::variant<std::monostate, E> m_data;
+
+public:
+    constexpr Expected() : m_data{std::in_place_index<0>, std::monostate{}} {}
+    template <class Err>
+    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
     {
-        assert(!has_value());
-        return std::get<1>(m_data);
-    }
-    constexpr E& error() LIFETIMEBOUND
-    {
-        assert(!has_value());
-        return std::get<1>(m_data);
     }
 
-    constexpr ValueType& operator*() LIFETIMEBOUND { return value(); }
-    constexpr const ValueType& operator*() const LIFETIMEBOUND { return value(); }
+    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
+    constexpr explicit operator bool() const noexcept { return has_value(); }
 
-    constexpr ValueType* operator->() LIFETIMEBOUND { return &value(); }
-    constexpr const ValueType* operator->() const LIFETIMEBOUND { return &value(); }
+    constexpr void operator*() const noexcept { return value(); }
+    constexpr void value() const
+    {
+        if (!has_value()) {
+            throw BadExpectedAccess{};
+        }
+    }
+
+    constexpr const E& error() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
+    constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
+    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(error()); }
 };
 
 } // namespace util

src/wallet/test/fuzz/spend.cpp

@@ -98,7 +98,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
 
     std::optional<unsigned int> change_pos;
     if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
-    [[maybe_unused]] auto _{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)};
+    (void)CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control);
 }
 } // namespace
 } // namespace wallet