let_value decay-copies its final result

[ispeters]

See discussion on #545 where I discovered the problem. The successor receiver in let_value tries to protect itself against the successor invoking set_value (and, incidentally, set_error) with a reference to a value that lives in the successor operation state by decay-copying set_value's arguments onto the stack, which breaks let_value's contract with its receiver if let_value has advertised that it's going to produce lvalue references; if we're going to distinguish between producing rvalue references and producing values, it breaks the contract when we've promised rvalue references, too, since the result object identities change.

Copying from my comment on #545, this test in finally_test.cpp repros the build error:

+
+TEST(Finally, CombinedWithLetValue) {
+  const int i{42};
+  auto ret = let_value(
+                 just(&i),
+                 [](const int* pi) noexcept {
+                   return just(pi) |
+                       then([](const int* pi) -> const int& { return *pi; });
+                 }) |
+      finally(just()) | then([](const int& i) { return &i; }) | sync_wait();
+
+  ASSERT_TRUE(ret.has_value());
+  EXPECT_EQ(&i, *ret);
+}

The problem is this decay-copy in let_value:

  template <typename... SuccessorValues>
  void set_value(SuccessorValues&&... values) && noexcept {
    auto& op = op_;
    UNIFEX_TRY {
      // Taking by value here to force a copy on the offchance the value
      // objects lives in the operation state (e.g., just), in which
      // case the call to cleanup() would invalidate them.
      [&](auto... copies) {
        cleanup();
        unifex::set_value(
            std::move(op.receiver_), (decltype(copies) &&) copies...);
      } ((SuccessorValues&&) values...);
    } UNIFEX_CATCH (...) {
      unifex::set_error(std::move(op.receiver_), std::current_exception());
    }
  }

The let_value in the new test case is advertising that it produces a const int& but then it produces int/int&&.

[ccotter]

Good find! Lvalue refs are indeed proving to be a challenge.

I originally thought the same trick with create of casting to the expected/actual value types in #545 would work here, but I don't think it can since the sender returned from let_value's Invocable can possibly send an overloaded set of values, and even consider the diabolical case of

struct SendsValueOrRef { // satisfies sender
     template <template <typename...> class Variant
                       template <typename...> class Tuple>
    using value_types = Variant<Tuple<int>, Tuple<int&>, Tuple<int&&>>;

    ...
};
let_value(just(), [] {
    return SendsValueOrRef{};
});

One possible solution is to decay copy each SuccessorValue only if it is not an lvalue reference; for lvalue references, we'd forward it inside set_value. This means rvalue reference sent by the Successor sender (the one returned from SuccessorFactory) would be decay copied unfortunately.

[ispeters]

I asked whether the stdexec implementation of let_value and finally have this unfortunate interaction and the answer was no, because, apparently, their let_value doesn't clean up the successor operation before invoking set_value so there's no need to worry about receiving a reference to a subobject of the successor operation state.

I'm a little bit worried about deferring destruction of the successor operation to the destructor of the let_value operation, but it's really just a vague anxiety. It's probably worth trying. I'll fool around with the problem today and see what I can learn.

[ispeters]

I have a local change that makes the above-described unit test build and pass, but I'm finding more problems elsewhere that I haven't minimized, yet. I'll publish a let_value PR today, but we're going to have to track down all the lvalue reference problems to set this straight, I think.

[ispeters]

This particular issue is fixed with PR #557. We'll have to file follow-up issues for the other algorithms that have similar bugs.