Skip to content

fix universal: make utils::expected description consistent #1204

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
ddvamp wants to merge 2 commits into
base: develop
Choose a base branch
from
+92 −57
Open

fix universal: make utils::expected description consistent #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f391b1
fix universal: make utils::expected description consistent
ddvamp Apr 25, 2026
7f96faf
refactor universal: refactor utils::expected
ddvamp Apr 26, 2026
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
96 changes: 48 additions & 48 deletions universal/include/userver/utils/expected.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
/// @file userver/utils/expected.hpp
/// @brief @copybrief utils::expected

#include <stdexcept>
#include <string>
#include <utility>
#include <variant>

#include <userver/compiler/impl/lifetime.hpp>
Expand Down Expand Up @@ -79,7 +79,7 @@ class [[nodiscard]] expected {

/// @brief Return reference to the value or throws bad_expected_access
/// if it's not available
/// @throws utils::bad_expected_access if *this contain an unexpected value
/// @throws utils::bad_expected_access if the value is not available
S& value() & USERVER_IMPL_LIFETIME_BOUND;

/// @overload
Expand All @@ -88,9 +88,12 @@ class [[nodiscard]] expected {
/// @overload
const S& value() const& USERVER_IMPL_LIFETIME_BOUND;

/// @brief Check whether *this contains an error
bool has_error() const noexcept;

/// @brief Return reference to the error value or throws bad_expected_access
/// if it's not available
/// @throws utils::bad_expected_access if success value is not available
/// @throws utils::bad_expected_access if the error value is not available
E& error() USERVER_IMPL_LIFETIME_BOUND;

/// @overload
Expand Down Expand Up @@ -119,6 +122,7 @@ class [[nodiscard]] expected<void, E> {
explicit operator bool() const noexcept;
void value() const;

bool has_error() const noexcept;
E& error() USERVER_IMPL_LIFETIME_BOUND;
const E& error() const USERVER_IMPL_LIFETIME_BOUND;

Expand All @@ -128,12 +132,12 @@ class [[nodiscard]] expected<void, E> {

template <class E>
unexpected<E>::unexpected(const E& error)
: value_{error}
: value_(error)
{}

template <class E>
unexpected<E>::unexpected(E&& error)
: value_{std::forward<E>(error)}
: value_(std::move(error))
{}

template <class E>
Expand Down Expand Up @@ -165,41 +169,41 @@ constexpr expected<S, E>::expected()

template <class S, class E>
expected<S, E>::expected(const S& success)
: data_(success)
: data_(std::in_place_index<0>, success)
Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 26, 2026

Choose a reason for hiding this comment

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

it's easy to accidentally confuse constants 1 and 2. Make those a named constants kSuccessIndex and kErrorIndex and use the readable names everywhere in this file

{}

template <class S, class E>
expected<S, E>::expected(S&& success)
: data_(std::forward<S>(success))
: data_(std::in_place_index<0>, std::move(success))
{}

template <class S, class E>
expected<S, E>::expected(const unexpected<E>& error)
: data_(error.error())
: data_(std::in_place_index<1>, error.error())
{}

template <class S, class E>
expected<S, E>::expected(unexpected<E>&& error)
: data_(std::forward<unexpected<E>>(error.error()))
: data_(std::in_place_index<1>, std::move(error.error()))
{}

template <class S, class E>
template <class G>
requires std::is_convertible_v<G, E>
expected<S, E>::expected(const unexpected<G>& error)
: data_(utils::unexpected<E>(std::forward<G>(error.error())))
: data_(std::in_place_index<1>, error.error())
{}

template <class S, class E>
template <class G>
requires std::is_convertible_v<G, E>
expected<S, E>::expected(unexpected<G>&& error)
: data_(utils::unexpected<E>(std::forward<G>(error.error())))
: data_(std::in_place_index<1>, std::move(error.error()))
{}

template <class S, class E>
bool expected<S, E>::has_value() const noexcept {
return std::holds_alternative<S>(data_);
return data_.index() == 0;
}

template <class S, class E>
Expand All @@ -208,71 +212,66 @@ expected<S, E>::operator bool() const noexcept {
}

template <class S, class E>
S& expected<S, E>::value() & USERVER_IMPL_LIFETIME_BOUND {
S* result = std::get_if<S>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined value from utils::expected");
}
return *result;
S& expected<S, E>::value() & USERVER_IMPL_LIFETIME_BOUND {
return const_cast<S&>(std::as_const(*this).value());
}

template <class S, class E>
S&& expected<S, E>::value() && USERVER_IMPL_LIFETIME_BOUND {
S&& expected<S, E>::value() && USERVER_IMPL_LIFETIME_BOUND {
return std::move(value());
}

template <class S, class E>
const S& expected<S, E>::value() const& USERVER_IMPL_LIFETIME_BOUND {
const S* result = std::get_if<S>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined value from utils::expected");
if (const auto* result = std::get_if<0>(&data_)) {
return *result;
}
return *result;
throw bad_expected_access("Trying to get undefined value from utils::expected");
}

template <class S, class E>
bool expected<S, E>::has_error() const noexcept {
return data_.index() == 1;
}

template <class S, class E>
E& expected<S, E>::error() USERVER_IMPL_LIFETIME_BOUND {
auto* result = std::get_if<unexpected<E>>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined error value from utils::expected");
}
return result->error();
return const_cast<E&>(std::as_const(*this).error());
}

template <class S, class E>
const E& expected<S, E>::error() const USERVER_IMPL_LIFETIME_BOUND {
const auto* result = std::get_if<unexpected<E>>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined error value from utils::expected");
if (const auto* result = std::get_if<1>(&data_)) {
return result->error();
}
return result->error();
throw bad_expected_access("Trying to get undefined error value from utils::expected");
}

template <class E>
constexpr expected<void, E>::expected() noexcept: data_(std::in_place_index<0>) {}
constexpr expected<void, E>::expected() noexcept : data_(std::in_place_index<0>) {}

template <class E>
expected<void, E>::expected(const unexpected<E>& error)
: data_(error.error())
: data_(std::in_place_index<1>, error.error())
{}

template <class E>
expected<void, E>::expected(unexpected<E>&& error)
: data_(std::forward<unexpected<E>>(error.error()))
: data_(std::in_place_index<1>, std::move(error.error()))
{}

template <class E>
template <class G>
requires std::is_convertible_v<G, E>
expected<void, E>::expected(const unexpected<G>& error)
: data_(utils::unexpected<E>(std::forward<G>(error.error())))
: data_(std::in_place_index<1>, error.error())
{}

template <class E>
template <class G>
requires std::is_convertible_v<G, E>
expected<void, E>::expected(unexpected<G>&& error)
: data_(utils::unexpected<E>(std::forward<G>(error.error())))
: data_(std::in_place_index<1>, std::move(error.error()))
{}

template <class E>
Expand All @@ -287,27 +286,28 @@ expected<void, E>::operator bool() const noexcept {

template <class E>
void expected<void, E>::value() const {
if (!has_value()) {
throw bad_expected_access("Trying to get undefined value from utils::expected");
if (has_value()) {
return;
}
throw bad_expected_access("Trying to get undefined value from utils::expected");
}

template <class E>
bool expected<void, E>::has_error() const noexcept {
return data_.index() == 1;
}

template <class E>
E& expected<void, E>::error() USERVER_IMPL_LIFETIME_BOUND {
auto* result = std::get_if<unexpected<E>>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined error value from utils::expected");
}
return result->error();
return const_cast<E&>(std::as_const(*this).error());
}

template <class E>
const E& expected<void, E>::error() const USERVER_IMPL_LIFETIME_BOUND {
const auto* result = std::get_if<unexpected<E>>(&data_);
if (result == nullptr) {
throw bad_expected_access("Trying to get undefined error value from utils::expected");
if (const auto* result = std::get_if<1>(&data_)) {
return result->error();
}
return result->error();
throw bad_expected_access("Trying to get undefined error value from utils::expected");
}

// NOLINTEND(readability-identifier-naming)
Expand Down
53 changes: 44 additions & 9 deletions universal/src/utils/expected_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <string>
#include <utility>

#include <gtest/gtest.h>

Expand Down Expand Up @@ -42,46 +43,80 @@ TEST(Expected, ErrorCtor) {
EXPECT_FALSE(ei);
EXPECT_FALSE(ev.has_value());
EXPECT_FALSE(ev);
EXPECT_EQ(const_cast<const ExpectedInt&>(ei).error(), "string error");
EXPECT_EQ(const_cast<const ExpectedVoid&>(ev).error(), "string error");
ASSERT_TRUE(ei.has_error());
ASSERT_TRUE(ev.has_error());
ASSERT_NO_THROW(ei.error());
ASSERT_NO_THROW(ev.error());
EXPECT_EQ(std::as_const(ei).error(), "string error");
EXPECT_EQ(std::as_const(ev).error(), "string error");

ei.error() = "another error";
ev.error() = "one more error";

EXPECT_EQ(ei.error(), "another error");
EXPECT_EQ(ev.error(), "one more error");

ei = ExpectedInt{utils::unexpected<const char*>("converted error")};
ev = ExpectedVoid{utils::unexpected<const char*>("converted error")};
auto error2 = utils::unexpected<const char*>("converted error");

ei = ExpectedInt{error2};
ev = ExpectedVoid{std::move(error2)};

EXPECT_FALSE(ei.has_value());
EXPECT_FALSE(ei);
EXPECT_FALSE(ev.has_value());
EXPECT_FALSE(ev);
ASSERT_TRUE(ei.has_error());
ASSERT_TRUE(ev.has_error());
ASSERT_NO_THROW(ei.error());
ASSERT_NO_THROW(ev.error());
EXPECT_EQ(ei.error(), "converted error");
EXPECT_EQ(ev.error(), "converted error");
}

TEST(Expected, ValueThrowsIfExpectedContainsError) {
TEST(Expected, ValueThrowsIfExpectedContainsNoValue) {
auto error = utils::unexpected{std::string("string error")};

ExpectedInt ei{error};
ExpectedVoid ev{std::move(error)};

EXPECT_THROW(const_cast<const ExpectedInt&>(ei).value(), utils::bad_expected_access);
ASSERT_FALSE(ei.has_value());
ASSERT_FALSE(ev.has_value());
EXPECT_THROW(std::as_const(ei).value(), utils::bad_expected_access);
EXPECT_THROW(ei.value(), utils::bad_expected_access);
EXPECT_THROW(std::move(ei).value(), utils::bad_expected_access);
EXPECT_THROW(ev.value(), utils::bad_expected_access);
}

TEST(Expected, ErrorThrowsIfExpectedContainsValue) {
TEST(Expected, ErrorThrowsIfExpectedContainsNoError) {
ExpectedInt ei{10};
ExpectedVoid ev;

EXPECT_THROW(const_cast<const ExpectedInt&>(ei).error(), utils::bad_expected_access);
ASSERT_FALSE(ei.has_error());
ASSERT_FALSE(ev.has_error());
EXPECT_THROW(std::as_const(ei).error(), utils::bad_expected_access);
EXPECT_THROW(ei.error(), utils::bad_expected_access);
EXPECT_THROW(const_cast<const ExpectedVoid&>(ev).error(), utils::bad_expected_access);
EXPECT_THROW(std::as_const(ev).error(), utils::bad_expected_access);
EXPECT_THROW(ev.error(), utils::bad_expected_access);
}

TEST(Expected, ValuelessByException) {
struct Throw {
Throw() = default;
Throw(const Throw&) { throw 0; }
Throw& operator=(const Throw&) { throw 0; }
};
using Expected = utils::expected<Throw, int>;

Expected e(utils::unexpected(0));
try {
e = Expected{};
FAIL();
} catch (...) {}

EXPECT_FALSE(e.has_value());
EXPECT_THROW(e.value(), utils::bad_expected_access);
EXPECT_FALSE(e.has_error());
Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 26, 2026

Choose a reason for hiding this comment

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

It should behave differently http://eel.is/c++draft/expected.object.assign

Copy link
Copy Markdown
Contributor Author

@ddvamp ddvamp Apr 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Are you suggesting adding an implementation of operator= (copy/move) corresponding to std::expected::operator=?

Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 28, 2026

Choose a reason for hiding this comment

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

Yep... We are planning to remove utils::expected after a few years and replace it with std::expected

The closer the behavior of those two types - the better.

EXPECT_THROW(e.error(), utils::bad_expected_access);
}

USERVER_NAMESPACE_END