Is the behaviour of std::get on rvalue-reference tuples dangerous?

Question

The following code:

#include <tuple>

int main ()
{
  auto f = [] () -> decltype (auto)
  {
    return std::get<0> (std::make_tuple (0));
  };

  return f ();
}

(Silently) generates code with undefined behaviour - the temporary rvalue returned by make_tuple is propagated through the std::get<> and through the decltype(auto) onto the return type. So it ends up returning a reference to a temporary that has gone out of scope. See it here https://godbolt.org/g/X1UhSw.

Now, you could argue that my use of decltype(auto) is at fault. But in my generic code (where the type of the tuple might be std::tuple<Foo &>) I don't want to always make a copy. I really do want to extract the exact value or reference from the tuple.

My feeling is that this overload of std::get is dangerous:

template< std::size_t I, class... Types >
constexpr std::tuple_element_t<I, tuple<Types...> >&& 
get( tuple<Types...>&& t ) noexcept;

Whilst propagating lvalue references onto tuple elements is probably sensible, I don't think that holds for rvalue references.

I'm sure the standards committee thought this through very carefully, but can anyone explain to me why this was considered the best option?

Ecg

"I don't think that holds for rvalue references" - It does if you forward_as_tuple. You want the value category preserved. Or if you want to extract one value from a functions return value. – Aliform

Thanks, but I'm not really sure that it is preserving the value category. For example, if I have a function returning a std::tuple<int> and I call std::get<0> on it I would expect to get a plain int, not an int&&. Can you give me a concrete example where you'd want something else? – Ecg

Isn’t this overload of get needed for effective structured bindings? – Prefecture

I'm certainly willing to be convinced, but examples I've tried with a "safe" version of the overload (which just returns the unadorned tuple_element type when passed an rvalue reference tuple) seem to work fine. What am I missing? – Ecg

Answer 1

Consider the following example:

void consume(foo&&);

template <typename Tuple>
void consume_tuple_first(Tuple&& t)
{
   consume(std::get<0>(std::forward<Tuple>(t)));
}

int main()
{
    consume_tuple_first(std::tuple{foo{}});
}

In this case, we know that std::tuple{foo{}} is a temporary and that it will live for the entire duration of the consume_tuple_first(std::tuple{foo{}}) expression.

We want to avoid any unnecessary copy and move, but still propagate the temporarity of foo{} to consume.

The only way of doing that is by having std::get return an rvalue reference when invoked with a temporary std::tuple instance.

live example on wandbox

---

Changing std::get<0>(std::forward<Tuple>(t)) to std::get<0>(t) produces a compilation error (as expected) (on wandbox).

---

Having a get alternative that returns by value results in an additional unnecessary move:

template <typename Tuple>
auto myget(Tuple&& t)
{
    return std::get<0>(std::forward<Tuple>(t));
}

template <typename Tuple>
void consume_tuple_first(Tuple&& t)
{
   consume(myget(std::forward<Tuple>(t)));
}

live example on wandbox

---

but can anyone explain to me why this was considered the best option?

Because it enables optional generic code that seamlessly propagates temporaries rvalue references when accessing tuples. The alternative of returning by value might result in unnecessary move operations.

Rolfrolfe

Ah I see. So my "safe_get" which returns the unadorned type causes an extra move. IMHO that would be a price worth paying for safety, but I can at least see it's debatable. Many thanks for your answer! – Ecg

Answer 2

IMHO this is dangerous and quite sad since it defeats the purpose of the "most important const":

Normally, a temporary object lasts only until the end of the full expression in which it appears. However, C++ deliberately specifies that binding a temporary object to a reference to const on the stack lengthens the lifetime of the temporary to the lifetime of the reference itself, and thus avoids what would otherwise be a common dangling-reference error.

In light of the quote above, for many years C++ programmers have learned that this was OK:

X const& x = f( /* ... */ );

Now, consider this code:

struct X {
    void hello() const { puts("hello"); }
    ~X() { puts("~X"); }
};

auto make() {
    return std::variant<X>{};
}

int main() {
    auto const& x = std::get<X>(make()); // #1
    x.hello();
}

I believe anyone should be forgiven for thinking that line #1 is OK. However, since std::get returns a reference to an object that is going to be destroyed, x is a dangling reference. The code above outputs:

~X
hello

which shows that the object that x binds to is destroyed before hello() is called. Clang gives a warning about the issue but gcc and msvc don't. The same issue happens if (as in the OP) we use std::tuple instead of std::variant but, sadly enough, clang doesn't issues a warning for this case.

A similar issue happens with std::optional and this value overload:

constexpr T&& value() &&;

This code, which uses the same X above, illustrates the issue:

auto make() {
    return std::optional{X{}};
}

int main() {
    auto const& x = make().value();
    x.hello();
}

The output is:

~X
~X
hello

Brace yourself for more of the same with C++23's std::except and its methods value() and error():

constexpr T&& value() &&;
constexpr E&& error() && noexcept;

I'd rather pay the price of the move explained in Vittorio Romeo's post. Sure, I can avoid the issue by removing & from lines #1 and #2. My point is that the rule for the "most important const" just became more complicated and we need to consider if the expression involves std::get, std::optional::value, std::expected::value, std::expected::error, ...

Favourable

"just became more complicated" If by "just", you mean eleven years ago when C++11 came out. Maybe citing something from over a decade ago isn't a great example of good C++ practice today. – Apothegm

Maybe an old practice is still good and I'm open to be convinced otherwise by a good argument other than ageism. – Favourable

The fact that something works well is what makes it a "good practice. If the language changes such that it no longer works well, then it stop being a good practice. – Apothegm

Sure but perhaps, breaking something that works well is not a good idea. – Favourable

Pre-C++11, boost::get as applied to an rvalue of a boost::tuple would have the exact same problem. auto const& x = std::get<X>(boost::tuple<...>()); would cause the same dangling reference problem. So by your logic, we just shouldn't have tuples at all, or get shouldn't be able to work on an rvalue of a tuple? The latter would mean that we wouldn't be able to get from any const& of a tuple. Basically, the "rule" you're citing, even before C++11, was never as firm as you believe it was. – Apothegm

Your speculation about my logic is wrong. I never said that we should not have std::tuple and we should not be able to work on an rvalue tuple. My post says"I'd rather pay the price of the move" (so I agree with the comment by Mat on Vittorio's answer) ie I'd prefer if std::get returned by value and not by reference. I agree with you when you say that "Maybe citing something from over a decade ago isn't a great example of good C++ practice today" perhaps this applies to boost::get. Being old is almost irrelevant to me but some form of backward compatibility (including practices) is beneficial. – Favourable