GorNishanov / coroutines-ts

20 stars 2 forks source link

XX025: && parameters in coroutines - very likely result in UB #13

Open GorNishanov opened 7 years ago

GorNishanov commented 7 years ago

This coroutine follows a perfect forwarding pattern, but, results in UB

template <typename... Args>
generator<int> f(Args&&... args) {
  g(std::forward<Args>(args)...);
  co_return;
}

Should we change the rules for how && parameters are captured in coroutines?

lewissbaker commented 7 years ago

You can kind of work around it with something like this, which move-constructs copies of r-value reference parameters and passes l-value references through.

template<typename... Args>
generator<int> f(Args&&... args) {
  return [](Args... args) -> generator<int> {
    g(std::forward<Args>(args)...);
    co_return;
  }(std::forward<Args>(args)...);
}
GorNishanov commented 7 years ago

Cool workaround! Though, I think this is something that evolution group might have want to have some input on.

lewissbaker commented 7 years ago

I do worry that if we come up with a rule like "rvalue references are always move-copied" then we'll penalise use cases like std::expected or std::optional for which there would not be undefined behavior for rvalue references.

What if we let the promise object or coroutine_traits decide how parameters are captured? Eg by defining some nested typedef in std::experimental::coroutine_traits<RET, ARGS...>

Something like this:

template<typename T, typename... ARGS>
class coroutine_traits<generator<T>, ARGS...>
{
  using arg_capture = void(std::remove_rvalue_reference_t<ARGS>...);
};

This could possibly be a solution to allowing capture of 'this' by value too? If you can determine that call is to a member function from coroutine_traits.

GorNishanov commented 7 years ago

Possibly some constexpr member in the promise_type? That way we don't have to do the traits if we control the return type

lewissbaker commented 7 years ago

So were you thinking maybe something like this:

struct generator_promise<T>
{
  generator<T> get_return_object();
  suspend_always initial_suspend();
  suspend_always final_suspend();
  void unhandled_exception();

  // Should this be a non-static member function?
  template<typename ARG>
  static constexpr ARG parameter_transform(ARG&& arg) { return std::forward<ARG>(arg); }
};

Then within the coroutine a reference to the parameter is replaced with a reference to decltype(auto) varWithinCoroutine = promise_type::parameter_transform(var);

How would such a facility affect the ability for the compiler to elide copies if the parameter is not referenced after the first suspend-point? If provided, would the compiler still be required to call parameter_transform() even if not referenced after first suspend-point?

Would the parameter_transform() function be allowed to return a value of a different type than the parameter? eg. could you add an overload like std::string parameter_transform(int x) { return std::to_string(x); }?

This could lead to particularly surprising (and difficult to understand) code. eg.

generator<int> f(int a, int b)
{
  static_assert(std::is_same_v<decltype(a), std::string>);
  auto ab = a + b; // <--- string concatenation here!
  co_return;
}

I can, however, see potential in the ability to, say, translate std::reference_wrapper<T> to T&.

For non-static member-function coroutines, would *this be passed to promise_type::parameter_transform() to capture implicit this?

GorNishanov commented 7 years ago

Good thinking. I had parameter_transform in early design for coroutine customization points that I later dropped to trying to make the design leaner and simpler. This is something that can be added in the future in a non-breaking fashion. I'd like to explore simpler alternatives for C++20.

lewissbaker commented 7 years ago

So something ugly like constexpr bool promise_type::capture_rvalue_reference_parameters_by_value = true;?

Or do we just recommend that generator coroutine functions (and other coroutines that are suspended before executing the body) take all parameters by value rather than trying to use perfect-forwarding? Then have l-value references opt-in by passing std::reference_wrapper<T> values using std::ref/cref.

eg.

template<typename... ARGS>
generator<int> f(ARGS... args)
{
  g(std::move(args)...); // or possibly some other std::forward-like thing that unwraps std::reference_wrapper if required.
  co_return;
}
lewissbaker commented 7 years ago

Another idea that came up during a discussion on #coroutines slack channel was to just prevent r-value references.

template<typename T, typename... ARGS>
struct coroutine_traits<generator<T>, ARGS...>
{
  static_assert(
    !std::disjunction<std::is_rvalue_reference_t<ARGS>...>::value,
    "Passing parameters by rvalue reference is not allowed for generator<T> coroutines");
  using promise_type = generator_promise<T>;
};

Possibly a bit of a sledgehammer, though.