Update finally to support value channel references

[ccotter]

This aligns with the general idea that senders should be able to send references and values, so the adaptor algorithms should preserve the types in value channels when there is no specific requirement to modify the value channel type.

In making this change, the CreateTest.AwaitTest test failed to compile. This appears to be the create operation accepting universal reference Ts&&... arguments which are then forwarded to the underlying receiver's set_value. However, in this test, the type of result is int&, but the create sender only sends int. Before this change (and before the schedule affine task), something else ended up decaying the parameter's type further downstream. With the scheduler affine task and its reliance on finally, the finally receiver's set_value strictly enforces that it's only called with the expected type, which caused the compilation error with my changes. Fundamentally, the create sender was incorrectly sending int& despite advertising it only sends int. I updated the create algorithm to faithfully send exactly what it advertises to fix the problem.

I left finally's existing behavior of decay_t-ing error types since I ran into other issues trying to get that to work, and it seems less useful/likely (though, I don't see why it shouldn't be done later).

Note that per discussion in #541, this is a source and behavior breaking change.

[ispeters]

I have a big question about the type conversion; once we resolve that, I think there might be knock-on effects for the rest of the code so back to you for now.

[ispeters]

I have a few thoughts.

First, I think there's a chance that the conversion from Ts&& to ValueTypes might throw, right? If so, the current noexcept declaration is incomplete.

Second, I intend to change set_value be noexcept (it simplifies many things, saves binary size, and brings us into better alignment with P2300) so I think, rather than refining the noexcept clause, it'd probably be better to just make it noexcept and forward exceptions to set_error.

Finally, we've been standardizing on an unstated coding style: use std::move() for moves and static_cast<Dest>(value) to replace std::forward, and don't use C-style casts. As I understand it, the C-style casts are in the code to cut down on compile time by avoiding template instantiations. We've written a handful of bugs by following this style, though, so I'm not convinced the build-time speedups are worth the maintenance risk; we may yet migrate from static_cast to std::forward.

[ccotter]

First, I think there's a chance that the conversion from Ts&& to ValueTypes might throw, right? If so, the current noexcept declaration is incomplete.

Thanks, good catch.

Second, I intend to change set_value be noexcept

Done. Makes sense. Once set_value is mandated to be noexcept, we can do the same cosntexpr conditional to avoid the try/catch if the conversion is nonthrowing, but I have not made that change here.

Finally, we've been standardizing on an unstated coding style: use std::move() for moves and static_cast(value) to replace std::forward, and don't use C-style casts.

I made one step towards this within this PR, although I think I have at least one more place to update within my changes. Thanks for clarifying the standard within this project. I will note, clang-tidy has been adding CppCoreGuidelines checks for correct move/forward/universal parameter usage (e.g., F.18) and I think the checks work best (or in some cases, only work) when move and forward are used rather than casts.

[ispeters]

I will note, clang-tidy [fixes this]

Huh. We should take advantage. I've filed issue #548.

[ispeters]

The conversion from Ts&& to ValueTypes has me a bit squirrely. The whole point of this change is to preserve the cvref qualifiers on the type parameters to create<>() so we obviously need to perform this conversion anytime set_value is invoked with inexactly-typed parameters, but I'm not sure I fully understand all the corner cases.

I think it's right for the function to take arbitrary forwarding references (rather than, say, ValueTypes&&...) so that:

• natural implicit conversions can happen (e.g. a create<double> that is completed with set_value(0) ought to work), and
• any exceptions that arise happen inside set_value so that we can capture-and-forward them automatically.

But, I worry that we'll copy or move more often than necessary if we get the details wrong.

Maybe the solution is to factor the conversion into a functor that performs the operation that convertible_to checks for?

// defined at namespace scope
template <typename From, typename To, typename = void>
struct do_convert {
  // default to no conversion
  To operator()(From&&) = delete;
};

template <typename From>
struct do_convert<From, From, void> {
  // don't construct anything when "conversion" is the identity
  From&& operator()(From&& obj) noexcept {
    return static_cast<From&&>(obj);
  }
};

template <typename From, typename To>
struct do_convert<From, To, std::enable_if_t<convertible_to<From, To>>> {
  // explicitly perform the operation that we've checked is implicitly allowed
  To operator()(From&& obj) {
    return static_cast<From&&>(obj);
  }
};

template(typename T...) //
    (requires (convertible_to<T, ValueTypes> && ...)) //
void set_value(T&&... values) noexcept {
  UNIFEX_TRY {
    unifex::set_value(std::move(rec_), do_convert<T, ValueTypes>{}(static_cast<T&&>(values))...);
  }
  UNIFEX_CATCH(...) {
    unifex::set_error(std::move(rec_), std::current_exception());
  }
}

The part of the above I'm most uncertain about is the type parameters given to do_convert. Should it be

• do_convert<T, ValueTypes>,
• do_convert<T&&, ValueTypes>,
• do_convert<T, ValueTypes&&>, or
• do_convert<T&&, ValueTypes&&>?

For create<cv int&> and set_value invoked with a cv int&, they're all equivalent because of reference collapsing and they'll all pick the identity, which avoids unnecessary copies/moves and correctly forwards "the same" int from the set_value call site through to the Receiver's set_value.

For create<cv int&> and set_value invoked with an rvalue int or with anything convertible to an int, like a cvref-qualified double, all of the above options will find the default template expansion and (correctly) block the conversion.

How do create<cv int> and create<cv int&&> differ? There is a difference between

std::unique_ptr<int> foo() {
  return std::move(*this).foo_;
}

and

std::unique_ptr<int>&& foo() {
  return std::move(*this).foo_;
}

For one thing foo_ is unconditionally left null in the first one, but the second one is conditional, depending on what the caller does with the result.

Perhaps the second type argument to do_convert should be ValueTypes and not ValueTypes&& because it would allow us to distinguish between create<int> and create<int&&>.

To get the unconditional move behaviour for create<int> and the conditional move behaviour for create<int&&>, I think we need the first type argument to do_convert to be T&&; that will choose the identity "conversion" (and thus maintain reference stability) when ValueTypes is an rvalue reference, and pick the T&& -> T conversion when ValueTypes is not a reference at all.

So that leaves us with

unifex::set_value(std::move(rec_), do_convert<T&&, ValueTypes>{}(static_cast<T&&>(values))...);

Does my reasoning seem sound?

If so, is do_convert even necessary? Maybe I've just complicated a cast expression.

Should this be

Suggested change

	unifex::set_value((Receiver&&) rec_, (ValueTypes) ts...);
	UNIFEX_TRY {
	unifex::set_value(std::move(rec_), ValueTypes{static_cast<Ts&&>(ts)}...);
	}
	UNIFEX_CATCH(...) {
	unifex::set_error(std::move(rec_), std::current_exception());
	}

with a requires (convertible_to<Ts, ValueTypes> && ...) clause on the method?

[ccotter]

But, I worry that we'll copy or move more often than necessary if we get the details wrong.

I had a similar concern. My original logic of set_value((Receiver&&) rec_, (ValueTypes) ts...) definitely incurs unnecessary copies or moves (I've added tests in my latest commit to capture this number of copies or moves to ensure no unnecessary copies or moves are done).

I'm having a hard time understanding whether do_convert<Ts&&, ValueTypes> or do_convert<Ts, ValueTypes> is right. Consider calling set_value<AnObject> on create<AnObject>. I think this will not hit the do_convert<From, From, void> specialization if do_convert<Ts&&, ValueTypes> is used and instead will end up calling the other which leads to a copy of the Object. In the latest commit I pushed, my new tests pass with both versions, so I don't think I have a test that exposes the right condition where Ts&& vs Ts in do_convert matters. I did add a test for the "conditional move" behavior (create<TrackingObject&&>).

Ts	ValueTypes	Desired Result passed to the inner set_value	do_convert<Ts, ValueTypes>	do_convert<Ts&&, ValueTypes>
int	int	int&&	int&&	int
int&	int	int	int	int

[ccotter]

Note, I added Co-authored-by: Ian Petersen <ispeters@fb.com> in the commit where I used do_convert. LMK if you are OK with this.

[ispeters]

I'm having a hard time understanding whether do_convert<Ts&&, ValueTypes> or do_convert<Ts, ValueTypes> is right. Consider calling set_value<AnObject> on create<AnObject>. I think this will not hit the do_convert<From, From, void> specialization if do_convert<Ts&&, ValueTypes> is used and instead will end up calling the other which leads to a copy of the Object.

I'm currently leaning towards "create<AnObject> ought to always construct a new object" (whether by copy or by move is up to the value category of the argument to set_value). In other words, do_convert<Ts&&, ValuteTypes> not matching do_convert<From, From, void> is, I think, the right choice. If there are scenarios where create<AnObject> doesn't create a new object then I think it's indistinguishable from create<AnObject&&>, right?

If I think of set_value as "how Sender/Receiver implements return" then requiring create<AnObject> to always create a new object is akin to saying "Sender/Receiver doesn't have RVO," which I think is just true.

I think there are two choices here with different trade-offs:

1. create<AnObject> always creates a new object and is distinguishable from create<AnObject&&>, or
2. create<AnObject> is the same as create<AnObject&&> and the algorithm never constructs a new object for you except when performing an implicit conversion.

I think the danger of 1 is unintentional copy/move, which can be "fixed" by changing to create<AnObject&&> and the danger of 2 is unintentional aliasing, which can be "fixed" by deliberately passing a temporary to set_value (i.e. set_value(AnObject{anObject})) or by receiving the value(s) by value (e.g. | then([](auto a) { return a; })).

I feel like the danger of 1 is less dangerous than the danger of 2 and the cost of safety isn't that high, but what do you think?

Also, if we make create<AnObject> and create<AnObject&&> the same, have we thrown away the ability to send values in the pursuit of the ability to send references?

[ispeters]

Re: giving me co-authorship credit, that's kind of you. I feel like I'm playing the role of editor here, not co-author, so it feels a bit like undue credit, but I don't know what the open source conventions are around this kind of thing.

[ccotter]

Ok, now I see - I think create<AnObject> should construct an object, that seems like the right semantics for "how Sender/Receiver implement return." Thanks for explaining the key difference. I added a test CreateObjectLeadsToNewObject which only works with do_convert<Ts&&, ValueTypes>, which ensures that the move constructor is invoked at least once (it's never invoked with do_convert<Ts, ValueTypes>).

Re: credit - im not sure of the convention either. Not a big deal, and fair point about being "editor," so I rebased and removed it.

[ispeters]

Looking good! Thanks for working through all the back-and-forth.

A few more nits and we can merge this.

[ispeters]

I think the void_t is redundant here; I would expect enable_if_t to already alias void when the condition is true.

Suggested change

	struct _do_convert<From, To, std::void_t<std::enable_if_t<convertible_to<From, To>>>> {
	struct _do_convert<From, To, std::enable_if_t<convertible_to<From, To>>> {

[ccotter]

The code ran into ambiguous template error when From=To (e.g., do_convert<int&,int&>), and void_t appeared to make this template more specialization, but to be honest, I am not great with SFINAE in this case. What did end up working was std::enable_if_t<!std::is_same_v<From, To> && convertible_to<From, To>> without the void_t.

Unsurprisingly, a C++20 constraints version worked with just template <typename From, typename To> requires convertible_to<From, To> struct _do_convert<From, To> { ...

In any case, I don't think we need _do_convert.

[ispeters]

Now that we've agreed on an implementation of _do_convert and what type parameters to pass to it, is this any different?

Suggested change

	unifex::set_value(std::move(rec_), _do_convert<Ts&&, ValueTypes>{}(static_cast<Ts&&>(ts))...);
	unifex::set_value(std::move(rec_), ValueTypes{static_cast<Ts&&>(ts)}...);

If it's the same behaviour, do you see value in the different spelling? I might be convinced that the _do_convert version makes the intention clearer.

[ccotter]

Ah yes, static_cast<ValueTypes>(static_cast<Ts&&>(ts)...) works but the ValueTypes{...}does not (I get -Wc++11-narrowing with the int -> double conversion test, and the create<A> test fails since set_value<B> tries to call A's aggregate initialization with a B which does not work (I think that's what is going on at least). B is convertible to A via the operator A() const member of B.

[ispeters]

In trying to sync this PR internally, we're getting build errors like this (truncated for reasons):

include/unifex/manual_lifetime_union.hpp:55:5: error: static assertion failed due to requirement 'is_one_of_v<std::tuple<bool &>, std::tuple<bool>>'
    static_assert(is_one_of_v<T, Ts...>);
    ^             ~~~~~~~~~~~~~~~~~~~~~
include/unifex/finally.hpp:108:35: note: in instantiation of function template specialization 'unifex::manual_lifetime_union<std::tuple<bool>>::get<std::tuple<bool &>>' requested here
              op->value_.template get<std::tuple<Values...>>());
                                  ^

I'll see if I can create a minimal repro as a unit test with a fix tomorrow.

[ispeters]

I've figured out what's broken, but not how to fix it. It's similar to the problem that @ccotter fixed in create<>() in this PR. There's a set_value that takes a parameter pack of forwarding references and it's inconvenient when the deduced types of the actual arguments are different from the declared types produced by the predecessor Sender.

[ccotter]

@ispeters curious...did you determine a fix for the internal issue you found above?

[ispeters]

Sort of. The problem is similar to the one you fixed in create<> where the Sender advertises a value_types of one kind and then perfect forwarding delivers something else.

This Godbolt is something I cooked up just before a week of PTO in hopes that a teammate might be able to extend it to a solution, but they've had other priorities, and I've been firefighting since I got back to work.

[ispeters]

OK, finally had time to minimize the problem. 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&&.