NVIDIA/stdexec

Temporary sender in `transform_sender` may dangle

jiixyj opened this issue · 0 comments

transform_sender is defined like this:

    struct __transform_sender {
      template <class _Self = __transform_sender, class _Domain, class _Sender, class... _Env>
      STDEXEC_ATTRIBUTE((always_inline))
      /*constexpr*/
      decltype(auto)
        operator()(_Domain __dom, _Sender&& __sndr, const _Env&... __env) const
        noexcept(__nothrow_callable<__transform_sender_1, _Domain, _Sender, const _Env&...>) {
        using _Sender2 = __call_result_t<__transform_sender_1, _Domain, _Sender, const _Env&...>;
        // If the transformation doesn't change the sender's type, then do not
        // apply the transform recursively.
        if constexpr (__decay_same_as<_Sender, _Sender2>) {
          return __transform_sender_1()(__dom, static_cast<_Sender&&>(__sndr), __env...);
        } else {
          // We transformed the sender and got back a different sender. Transform that one too.
          return _Self()(
            __dom,
            __transform_sender_1()(__dom, static_cast<_Sender&&>(__sndr), __env...),
            __env...);
        }
      }
    };

In general it takes care to "perfectly backward" the value category of any custom transform_sender implementation it calls by using decltype(auto).

But I think there is a lifetime bug in the last else clause. What happens if the return value of __transform_sender_1 is a prvalue? This is given as an argument to the call _Self()(...) and therefore materializes as an xvalue. Thus, _Self()(...) possibly returns a xvalue as well, as inside, it cannot distinguish between xvalue and prvalue. But now we are returning a reference to a temporary! Shouldn't the "prvalue-ness" of the return value of __transform_sender_1 be "perfectly backwarded" with something like this:

          if constexpr (std::is_reference_v<decltype(__transform_sender_1()(
                          __dom, static_cast<_Sender&&>(__sndr), __env...))>) {
            return _Self()(
              __dom,
              __transform_sender_1()(__dom, static_cast<_Sender&&>(__sndr), __env...),
              __env...);
          } else {
            return auto(_Self()(
              __dom,
              __transform_sender_1()(__dom, static_cast<_Sender&&>(__sndr), __env...),
              __env...));
          }

This is the relevant place in the draft: https://eel.is/c++draft/exec#snd.transform-1

Let transformed-sndr be the expression dom.transform_sender(std::forward(sndr), env...)
if that expression is well-formed; otherwise, default_domain().transform_sender(std::forward(sndr), env...)
Let final-sndr be the expression transformed-sndr if transformed-sndr and sndr have the same type ignoring cv-qualifiers; otherwise, it is the expression transform_sender(dom, transformed-sndr, env...).

I think it should say something like:

Let transformed-sndr be the expression dom.transform_sender(std::forward(sndr), env...)
if that expression is well-formed; otherwise, default_domain().transform_sender(std::forward(sndr), env...)
Let final-sndr be the expression transformed-sndr if transformed-sndr and sndr have the same type ignoring cv-qualifiers; otherwise, it is the expression auto(transform_sender(dom, transformed-sndr, env...)) if transformed-sndr is a prvalue, and transform_sender(dom, transformed-sndr, env...) otherwise.

Also, in the sender adaptors like https://eel.is/c++draft/exec#then-3 for example, it should probably say auto(transform_sender(get-domain-early(sndr), make-sender(then-cpo, f, sndr))), right? make-sender is a prvalue, and this should be preserved.