co_return to accept same expressions and types as return #2977
Conversation
Current implementation of `co_return` behaves differently from plain `return` when interpreting the expression returned. This includes both - `co_return` accepting expressions `return` would reject; - `co_return` rejecting expressions `return` would accept. Unexpectedly accepted expressions allow bugs from accidental misuse: - a value of a strongly-defined type may be implicitly instantiated from its underlying type thus defeating the purpose, e.g. a coroutine declared to return `future<std::chrono::seconds>`, may `co_return 42;` - instead of a value of an enum type used in the future, a value of an unrelated enum type may be used if it uses the same underlying type; - obscure convertions, such as a number to `std::vector`, may be applied implicitly. Unexpectedly rejected expressions include: - those rejected due to false ambiguity, where compiler cannot decide between a reasonable interpretation and one of the situtaions described above; - brace-enclosed initializer list without type specified. This happens because we save the `co_return`'ed value in a state by: - calling a chain of unnecessarily templated functions; - implicitly calling the type ctor even if it is marked explicit. The fix is to avoid the above.
bashtanov
changed the title
| void set_strict(T v) { | ||
| assert(_u.st == state::future); | ||
| new (this) future_state(strict_type_ready_future_marker(), std::forward<T>(v)); | ||
| } |
There was a problem hiding this comment.
Please don't change future.hh as a side effect. This is the main header and changes here have to be deliberate.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
| /// 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)); | ||
| } |
There was a problem hiding this comment.
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:
- rename set_value() to emplace_value(Args&&...) to construct T in place
- add set_value(T).
Unfortunately this is a breaking change and must be done by introducing a new API_LEVEL.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
bashtanov
force-pushed
the
strict-co_return-upstream
branch
from
c784720 to
b888cc9
Compare
| public: | ||
| uninitialized_wrapper() noexcept = default; | ||
| void uninitialized_set_strict(T v) { | ||
| new (&_v.value) auto(std::forward<T>(v)); |
There was a problem hiding this comment.
I'll need to change this to support reference types
2 participants
Current implementation of
co_returnbehaves differently from plainreturnwhen interpreting the expression returned. This includes bothco_returnaccepting expressionsreturnwould reject;co_returnrejecting expressionsreturnwould accept.Unexpectedly accepted expressions allow bugs from accidental misuse:
its underlying type thus defeating the purpose, e.g. a coroutine
declared to return
future<std::chrono::seconds>, mayco_return 42;unrelated enum type may be used if it uses the same underlying type;
std::vector, may be appliedimplicitly.
Unexpectedly rejected expressions include:
between a reasonable interpretation and one of the situtaions described
above;
This happens because we save the
co_return'ed value in a state by:The fix is to avoid the above.
One of the test cases was shamelessly stolen from #2971 as my change fixes their use case.