Skip to content

co_return to accept same expressions and types as return #2977

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
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

co_return to accept same expressions and types as return #2977

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12666b3
core/{coroutine,future}: make `co_return` more strict with types
bashtanov Aug 13, 2025
daa25fd
tests/unit/coroutines: co_return interprets expressions like return does
bashtanov Sep 9, 2025
b888cc9
tests/unit/coroutine: work around gcc bug
bashtanov Sep 12, 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
10 changes: 2 additions & 8 deletions include/seastar/core/coroutine.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,8 @@ public:
promise_type(promise_type&&) = delete;
promise_type(const promise_type&) = delete;

template<typename... U>
void return_value(U&&... value) {
_promise.set_value(std::forward<U>(value)...);
void return_value(T value) {
_promise.set_value_strict(std::forward<T>(value));
}

void return_value(coroutine::exception ce) noexcept {
Expand All @@ -59,11 +58,6 @@ public:
_promise.set_exception(std::move(eptr));
}

[[deprecated("Forwarding coroutine returns are deprecated as too dangerous. Use 'co_return co_await ...' until explicit syntax is available.")]]
void return_value(future<T>&& fut) noexcept {
fut.forward_to(std::move(_promise));
}

void unhandled_exception() noexcept {
_promise.set_exception(std::current_exception());
}
Expand Down
36 changes: 35 additions & 1 deletion include/seastar/core/future.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,9 @@ struct uninitialized_wrapper {

public:
uninitialized_wrapper() noexcept = default;
void uninitialized_set_strict(T v) {
new (&_v.value) auto(std::forward<T>(v));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll need to change this to support reference types

}
template<typename... U>
requires (!std::same_as<std::tuple<std::remove_cv_t<U>...>, std::tuple<tuple_type>>)
void
Expand Down Expand Up @@ -546,6 +549,7 @@ inline void future_state_base::any::check_failure() noexcept {
}

struct ready_future_marker {};
struct strict_type_ready_future_marker {};
struct exception_future_marker {};
struct future_for_get_promise_marker {};

Expand Down Expand Up @@ -601,6 +605,13 @@ struct future_state : public future_state_base, private internal::uninitialized
move_it(std::move(x));
return *this;
}
future_state(strict_type_ready_future_marker, T v) noexcept : future_state_base(state::result) {
try {
this->uninitialized_set_strict(std::forward<T>(v));
} catch (...) {
new (this) future_state(current_exception_future_marker());
}
}
template <typename... A>
future_state(ready_future_marker, A&&... a) noexcept : future_state_base(state::result) {
try {
Expand All @@ -609,6 +620,10 @@ struct future_state : public future_state_base, private internal::uninitialized
new (this) future_state(current_exception_future_marker());
}
}
void set_strict(T v) {
assert(_u.st == state::future);
new (this) future_state(strict_type_ready_future_marker(), std::forward<T>(v));
}
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change future.hh as a side effect. This is the main header and changes here have to be deliberate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean specifically this private function, or any changes to the file?
Other than promise::set_value_strict that is being discussed below, my changes only affect future_state which is marked as internal, and classes in seastar::internal namespace.
In any case, if you can think of a way to move this logic out of this header I'd be really grateful for suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying to make the changes elsewhere, but to move them to their own commit with clear justification.

That holds triple if we rename set_value() to emplace_value() and introduce a new set_value().

template <typename... A>
void set(A&&... a) noexcept {
assert(_u.st == state::future);
Expand Down Expand Up @@ -852,7 +867,8 @@ public:
template <typename T>
class promise_base_with_type : protected internal::promise_base {
protected:
using future_state = seastar::future_state<future_stored_type_t<T>>;
using value_type = future_stored_type_t<T>;
using future_state = seastar::future_state<value_type>;
future_state* get_state() noexcept {
return static_cast<future_state*>(_state);
}
Expand All @@ -876,6 +892,13 @@ public:
}
}

void set_value_strict(value_type v) noexcept {
if (auto *s = get_state()) {
s->set_strict(std::forward<value_type>(v));
make_ready<urgent::no>();
}
}

template <typename... A>
void set_value(A&&... a) noexcept {
if (auto *s = get_state()) {
Expand Down Expand Up @@ -911,6 +934,7 @@ private:
SEASTAR_MODULE_EXPORT
template <typename T>
class promise : private internal::promise_base_with_type<T> {
using value_type = typename internal::promise_base_with_type<T>::value_type;
using future_state = typename internal::promise_base_with_type<T>::future_state;
future_state _local_state;

Expand Down Expand Up @@ -954,6 +978,16 @@ public:
/// was attached to the future, it will run.
future<T> get_future() noexcept;

/// \brief Sets the promises value using exactly declared type
///
/// Forwards the argument and makes them available to the associated
/// future. May be called either before or after \c get_future().
///
/// pr.set_value_strict(42);
void set_value_strict(value_type v) noexcept {
internal::promise_base_with_type<T>::set_value_strict(std::forward<value_type>(v));
}
Copy link
Member

Choose a reason for hiding this comment

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

Is set_value_strict needed? What's the difference compared to set_value(T(...))?

If we want set_value_strict, I think an API change is in order:

  1. rename set_value() to emplace_value(Args&&...) to construct T in place
  2. add set_value(T).

Unfortunately this is a breaking change and must be done by introducing a new API_LEVEL.

Copy link
Contributor Author

@bashtanov bashtanov Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The difference is the same as with co_return: it does not perform any unexpected type conversions.
I'm not sure if we need it TBH, if set_value was called emplace_value I wouldn't touch it.

Do we need a new API_LEVEL only for functions like this that user call explicitly, or for co_return semantics change as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need promise::set_value_strict, whether renamed or not, for coroutine_traits_base::return_value to call it though. I can try to hide it from user if we decide to go without it.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a new API_LEVEL only for functions like this that user call explicitly, or for co_return semantics change as well?

I think we'll need it for co_return too as people likely rely on it.


/// \brief Sets the promises value
///
/// Forwards the arguments and makes them available to the associated
Expand Down