Failing Test Case: std::move-ing out of variant

[daniel-j-h]

@anandthakker and I just had a discussion about move-ing types out of a variant.

Think:

variant<Container1, Container2> v;

v.match([](Container1&& c1) { },
        [](Container2&& c2) { });

Which does not compile.

I think the issue is this type alias (and related), which do not take rvalue refs into account.

[daniel-j-h]

Horrible workaround is temporarily escaping the type system

auto consume = [](auto){};

v.match([&](const T1& t1) { consume((T1&&)(t1)); },
        [&](const T2& t2) { consume((T2&&)(t2)); });

[lightmare]

Which does not compile.

I don't think your first example should compile. v is an lvalue, andvariant::match(...) & should treat its contents as an lvalue, not automagically bind to an rvalue reference.

variant<Container1, Container2> v;

std::move(v).match([](Container1&& c1) { },
                   [](Container2&& c2) { });

This, on the other hand, should work as you expect.

[daniel-j-h]

Good point, I adapted the example. Still fails to compile, though.

[kkaefer]

I just ran into a particular annoying variant of this bug: If you define auto&& as a catchall matcher along with other Type&& lambdas, the catchall lambda will always be chosen:

#include <mapbox/variant.hpp>
#include <iostream>

struct container {
    int data;
};

int main() {
    using type = mapbox::util::variant<container>;

    type value{container{42}};

    value.match(
        [](container&& val) {
            std::cout << "container&&: " << val.data << std::endl;
        },
        [](auto&& val) {
            std::cout << "auto&&" << std::endl;
        }
    );

    value.match(
        [](container& val) {
            std::cout << "container&: " << val.data << std::endl;
        },
        [](auto& val) {
            std::cout << "auto&" << std::endl;
        }
    );
}

This prints

auto&&
container&: 42

[artemp]

/cc @kkaefer @springmeyer

Current (WIP) implementation https://github.com/mapbox/variant/tree/moveable_visit

#include <mapbox/variant.hpp>
#include <iostream>
#include <type_traits>

struct container {
    int data;
};

struct container2 {
    float data;
};

struct visitor
{
    template <typename T>
    void operator() (T && val)
    {
        std::cout << "VISITOR:" << val.data
                  << " type:" << typeid(val).name()
                  << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
    }
};

int main() {
    using type = mapbox::util::variant<container, container2>;

    type value{container{42}};

    type const& ref_value = value;

    mapbox::util::apply_visitor(visitor(), std::move(value));
    mapbox::util::apply_visitor(visitor(), value);

    mapbox::util::apply_visitor(visitor(), std::move(ref_value));
    mapbox::util::apply_visitor(visitor(), ref_value);

    // value is coersed into r-value reference allowing moving out
    value.match(
        [](container&& val) {
            std::cout << "container&&: " << val.data << " type:" << typeid(val).name() << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
        },
        [](auto&& val) {
            std::cout << "auto&& " << typeid(val).name() << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
        }
        );

    // ref_value is l-value and `auto&&` take precedence (??)
    ref_value.match(
        [](container&& val) {
            std::cout << "container&&: " << val.data << " type:" << typeid(val).name() << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
        },
        [](auto&& val) {
            std::cout << "auto&& " << typeid(val).name() << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
        }
        );

    // value is coersed into const&
    value.match(
        [](container const& val) {
            std::cout << "container const&: " << val.data << " type:" << typeid(val).name() << " is R-value:" << std::is_rvalue_reference<decltype(val)>::value << std::endl;
        },
        [](auto const& val) {
            std::cout << "auto&" << std::endl;
        }
        );

    return 0;
}

[lightmare]

This prints

auto&&
container&: 42

This is correct. In the expression value.match(...), you're calling match on an l-value, and it should not bind to r-value parameter type container&&.

[artemp]

This prints

auto&&
container&: 42

This is correct. In the expression value.match(...), you're calling match on an l-value, and it should not bind to r-value parameter type container&&.

@lightmare - could you help to implement this feature correctly, have a look at https://github.com/mapbox/variant/tree/moveable_visit cheers!

[artemp]

@kkaefer @lightmare is correct ^ It's important to understand the difference between

void operator() (container && obj) const {} // expected rvalue 

and

template <typename T>
void operator() (T && obj) const {} // obj is `forwarding reference` (aka `universal reference`)

auto&& is equivalent to forwarding reference ^^ (https://en.cppreference.com/w/cpp/language/reference)

In code below value is lvalue and complier uses auto&& as only possible match.

#include <variant>
#include <iostream>

struct container
{
    int value = 123;
};

struct container2
{
    float value = 3.14159;
};

struct visitor
{
    void operator() (container&& obj) const
    {
        std::cerr << "container&& " << obj.value
                  << " is R-value:" << std::is_rvalue_reference<decltype(obj)>::value
                  << std::endl;
    }

    template <typename T>
    void operator() (T && obj) const
    {
        std::cerr << "T&& " << obj.value
                  << " is R-value:" << std::is_rvalue_reference<decltype(obj)>::value
                  << std::endl;
    }
};

// helper type for the visitor #4
template<class... Ts> struct overloaded : Ts... { using Ts::operator()...; };
// explicit deduction guide (not needed as of C++20)
template<class... Ts> overloaded(Ts...) -> overloaded<Ts...>;

int main()
{
#if __cplusplus >= 201703L

    using variant_type = std::variant<container, container2>;
    variant_type v;

    std::visit(overloaded {
            [](container && obj)
            {
                std::cerr << "container&& :" << obj.value
                          << " is R-value:" << std::is_rvalue_reference<decltype(obj)>::value
                          << std::endl;
                obj.value = 42;
            },
            [](auto&& obj)
            {
                std::cerr << "auto&& :" << obj.value
                          << " is R-value:" << std::is_rvalue_reference<decltype(obj)>::value
                          << std::endl;
                obj.value = 456;
            }}, v);

    std::visit(visitor(), v);
#else
    std::cerr << "std::variant requires c++17!" << std::endl;
#endif
    return 0;
}

[lightmare]

@artemp I took daniel's branch as the starting point, as it has the test of the feature in question. See https://github.com/lightmare/variant/commits/move-out

It looks like there's a lot of changes, but not really. The biggest chunk of changes is pushing dispatcher class template parameters down to apply function, so that they can be easily forwarded. Secondly I changed result_of_unary_visit from trait class to mere alias template, because the class was not SFINAE friendly:

template <typename F, typename V>
struct result_of_unary_visit
{
    using type = decltype(::std::declval<F>()(::std::declval<V>()));
};

In the expression result_of_unary_visit<F, V>::type, if the call inside decltype is not valid, it's not a substitution failure, but a hard error, it won't compile. But I needed this to fail silently in order to have lvalue/rvalue match overloads.

[artemp]

thanks @lightmare, I'll take a look.

[artemp]

@lightmare - your branch is looking good, thanks! I ended up ditching c++11 support in my branch in favour of auto deduction but your implementation, preserving c++11 support, wins. I'm personally a bit dubious about std::move(value).match(...) syntax but it's technically correct. I think we should add std::variant style interface e.g mapbox::util::visit(F, V) once this features is in master. Could you create a PR, please, cheers. /cc @kkaefer @springmeyer