Skip to content

util: Add some more Unexpected and Expected methods #34032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
maflcko wants to merge 8 commits into
base: master
Choose a base branch
from
+146 −40
Open

util: Add some more Unexpected and Expected methods #34032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fad4a9f
Set bugprone-unused-return-value.AllowCastToVoid
Dec 9, 2025
faa109f
test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK ==
Dec 10, 2025
fa1de11
util: Add Unexpected::error()
Dec 9, 2025
fa5a49c
util: Make Expected::value() throw
Dec 9, 2025
fa696c4
util: Add Expected<void, E> specialization
Dec 9, 2025
fa71c13
util: Implement Expected::value()&& and Expected::error()&&
Dec 9, 2025
fa9aad7
util: Implement Expected::operator*()&&
Dec 9, 2025
fa874c9
util: Add Expected::swap()
Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/.clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,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.
2 changes: 1 addition & 1 deletion src/bench/coin_selection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/httpserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
4 changes: 1 addition & 3 deletions src/ipc/test/ipc_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
84 changes: 75 additions & 9 deletions src/test/util_expected_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -17,25 +22,39 @@ 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;
(*e).x += 1;
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)
Expand All @@ -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()
90 changes: 65 additions & 25 deletions src/util/expected.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand All @@ -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
Expand All @@ -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()); }
Copy link
Contributor

@hodlinator hodlinator Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this method overload is not calling itself recursively?

Tested adding BOOST_CHECK_EQUAL((Expected<void, int>{Unexpected{1}}.error()), 1); to the tests to confirm it doesn't but don't understand why.

Is this in some kind of &-mode, making it call the method above?

Copy link
Member Author

@maflcko maflcko Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in some kind of &-mode, making it call the method above?

It is not this, but (*this), see https://eel.is/c++draft/expr.prim#id.general-2

Copy link
Contributor

@hodlinator hodlinator Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't read that. 😁

Copy link
Member Author

@maflcko maflcko Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't read that. 😁

i'd say it says that error() in this context is (*this).error(). So it calls error()& and not error()&&. If you want to (incorrectly) recursively call error(), you'd have to add a cast: std::move(*this).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(); }
Comment on lines +93 to +98
Copy link
Contributor

@hodlinator hodlinator Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Might be nice to have these assert before failing due to an exception being thrown inside of noexcept.

Copy link
Member Author

@maflcko maflcko Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Just seems more code for no reason?

Copy link
Contributor

@hodlinator hodlinator Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about failing with a clearer stack trace/message. An exception will unwind the stack before giving a stack-trace (depending upon how the compiler treats exceptions within noexcept?).

Tried adding *e in the unit tests and it doesn't pinpoint the offending line, but gets by on checkpoints.

₿ ./build/bin/test_bitcoin -t util_expected_tests
Running 9 test cases...
terminate called after throwing an instance of 'util::BadExpectedAccess'
  what():  Bad util::Expected access
unknown location(0): fatal error: in "util_expected_tests/expected_value_throws": signal: SIGABRT (application abort requested)
../src/test/util_expected_tests.cpp(73): last checkpoint

Copy link
Member Author

@maflcko maflcko Dec 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is also the behavior that you get in C++23 (if you are lucky). If you are unlucky, you'll just get the silent UB:

https://godbolt.org/z/jYxqvoejn

/../include/c++/v1/__expected/expected.h:811: libc++ Hardening assertion this->__has_val() failed: expected::operator* requires the expected to contain a value
Program terminated with signal: SIGSEGV

Also, this is the behavior with an assert/abort, so I don't see the difference!?

Copy link
Contributor

@hodlinator hodlinator Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we don't need to spoil ourselves before the switcharoo to std::expected. :)

};

template <class E>
class Expected<void, E>
Comment on lines +101 to +102
Copy link
Contributor

@ryanofsky ryanofsky Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "util: Add Expected<void, E> specialization" (fa3d6c7)

This specialization is more complicated than it needs to be. Would suggest simplifying with inheritance:

//! Specialization for void returning void from value methods.
template <class E>
class Expected<void, E> : public Expected<std::monostate, E>
{
    using Base = Expected<std::monostate, E>;
public:
    using Expected<std::monostate, E>::Expected;
    constexpr void operator*() const noexcept { Base::operator*(); }
    constexpr void value() const& { Base::value(); }
    constexpr void value() && { Base::value(); }
    void value_or() = delete;
};

Copy link
Member Author

@maflcko maflcko Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just brings back the problem we are trying to solve: Solve exposing monostate externally?

+ Expected<void, std::string> e{std::monostate{}};

{
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

constexpr bool has_value() const noexcept { return m_data.index() == 0; }
constexpr explicit operator bool() const noexcept { return has_value(); }

constexpr void operator*() const noexcept {}
constexpr void value() const
{
assert(!has_value());
return std::get<1>(m_data);
if (!has_value()) {
throw BadExpectedAccess{};
}
}

constexpr ValueType& operator*() LIFETIMEBOUND { return value(); }
constexpr const ValueType& operator*() const LIFETIMEBOUND { return value(); }

constexpr ValueType* operator->() LIFETIMEBOUND { return &value(); }
constexpr const ValueType* operator->() const LIFETIMEBOUND { return &value(); }
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
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/fuzz/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading