34

The pattern that a lot of people use with C++17 / boost variants looks very similar to switch statements. For example: (snippet from cppreference.com)

std::variant<int, long, double, std::string> v = ...;

std::visit(overloaded {
    [](auto arg) { std::cout << arg << ' '; },
    [](double arg) { std::cout << std::fixed << arg << ' '; },
    [](const std::string& arg) { std::cout << std::quoted(arg) << ' '; },
}, v);

The problem is when you put the wrong type in the visitor or change the variant signature, but forget to change the visitor. Instead of getting a compile error, you will have the wrong lambda called, usually the default one, or you might get an implicit conversion that you didn't plan. For example:

v = 2.2;
std::visit(overloaded {
    [](auto arg) { std::cout << arg << ' '; },
    [](float arg) { std::cout << std::fixed << arg << ' '; } // oops, this won't be called
}, v);

Switch statements on enum classes are way more secure, because you can't write a case statement using a value that isn't part of the enum. Similarly, I think it would be very useful if a variant visitor was limited to a subset of the types held in the variant, plus a default handler. Is it possible to implement something like that?

EDIT: s/implicit cast/implicit conversion/

EDIT2: I would like to have a meaningful catch-all [](auto) handler. I know that removing it will cause compile errors if you don't handle every type in the variant, but that also removes functionality from the visitor pattern.

  • Doesn't the snippet just above this one on en.cppreference do exactly what you want? – Holt Aug 16 '17 at 07:49
  • 3
    Yesterday I learned that there is no thing called "implicit cast", because all casts are explicit. The phrase you are looking for is "implicit conversion" :) https://stackoverflow.com/a/45672844/3560202 – Post Self Aug 16 '17 at 07:52
  • @Holt, the one with constexpr if? I think it has the same pitfall. – Michał Brzozowski Aug 16 '17 at 08:01
  • 1
    @kim366 thank you. In either case this feature in C++ is an abomination. :) – Michał Brzozowski Aug 16 '17 at 08:01
  • @MichałBrzozowski Try it, and try adding any type to the variant, you'll see that your code won't compile anymore, even if the type is implicitly convertible to another one (e.g. try adding `char`). – Holt Aug 16 '17 at 08:02
  • 3
    If you provide `default` in your `switch`, you have the same issue. (which is the equivalent of `auto arg` here). – Jarod42 Aug 16 '17 at 08:02
  • Sorry that the question is confusing. What I mean is that if you put float instead of double in the variant signature, the visitor can still have a double handler, and any float value will trigger the [](auto) handler. My aim is to get a compiler error instead. @Holt: I tried the same with the if constexpr version and the same thing happens, you end up in the last else statement. – Michał Brzozowski Aug 16 '17 at 08:11
  • @MichałBrzozowski And the last `else` statement is a compile time error... I don't see what you want to do that this version does not? – Holt Aug 16 '17 at 08:12
  • @Holt: in this case yes, but you will often have a catch-all statement there, just like the default: in a switch. – Michał Brzozowski Aug 16 '17 at 08:13
  • 1
    @MichałBrzozowski So you want a compiler error when you **change** your code basically, this is not possible... Compiler do not have memory of previous compilation... How do you want the compiler to know that you want a compile-time error in this case but not in the other one? What would be the difference between adding `float` and adding `std::vector` for the compiler? – Holt Aug 16 '17 at 08:17
  • @Holt: declare an `enum class`, then write a non-exhaustive `switch` on it with a `default` handler. Then change the name of an element in the enum that you have a `case` statement for. It causes a compile error, I want the same thing here. – Michał Brzozowski Aug 16 '17 at 08:19
  • 1
    BTW, I'm not sure in real scenario, it happens often to add an new type implicitly convertible in the `variant`. – Jarod42 Aug 16 '17 at 08:19
  • @MichałBrzozowski This is not the same as adding, this is changing something, in which case this makes more sense. Simply add a `static_assert` at the beginning of the lambda to check for only possible types. – Holt Aug 16 '17 at 08:20
  • 2
    This might be helpful: https://www.youtube.com/watch?v=mqei4JJRQ7s&t=397s – Post Self Aug 16 '17 at 08:24

3 Answers3

31

If you want to only allow a subset of types, then you can use a static_assert at the beginning of the lambda, e.g.:

template <typename T, typename... Args>
struct is_one_of: 
    std::disjunction<std::is_same<std::decay_t<T>, Args>...> {};

std::visit([](auto&& arg) {
    static_assert(is_one_of<decltype(arg), 
                            int, long, double, std::string>{}, "Non matching type.");
    using T = std::decay_t<decltype(arg)>;
    if constexpr (std::is_same_v<T, int>)
        std::cout << "int with value " << arg << '\n';
    else if constexpr (std::is_same_v<T, double>)
        std::cout << "double with value " << arg << '\n';
    else 
        std::cout << "default with value " << arg << '\n';
}, v);

This will fails if you add or change a type in the variant, or add one, because T needs to be exactly one of the given types.

You can also play with your variant of std::visit, e.g. with a "default" visitor like:

template <typename... Args>
struct visit_only_for {
    // delete templated call operator
    template <typename T>
    std::enable_if_t<!is_one_of<T, Args...>{}> operator()(T&&) const = delete;
};

// then
std::visit(overloaded {
    visit_only_for<int, long, double, std::string>{}, // here
    [](auto arg) { std::cout << arg << ' '; },
    [](double arg) { std::cout << std::fixed << arg << ' '; },
    [](const std::string& arg) { std::cout << std::quoted(arg) << ' '; },
}, v);

If you add a type that is not one of int, long, double or std::string, then the visit_only_for call operator will be matching and you will have an ambiguous call (between this one and the default one).

This should also works without default because the visit_only_for call operator will be match, but since it is deleted, you'll get a compile-time error.

Holt
  • 36,600
  • 7
  • 92
  • 139
  • 3
    `visit_only_for` might have its operator deleted. (it would avoid to introduce dummy `ret_t`). – Jarod42 Aug 16 '17 at 11:40
  • @Jarod42 true, I'm not used to delete such kind of functions, but that looks nicer ;) Answer updated. – Holt Aug 16 '17 at 11:45
  • `template visit_only_for make_visit_only_for(const std::variant&) { return {}; }` might also be a good addition. – Jarod42 Aug 16 '17 at 11:50
  • @Jarod42 This would break the goal of the operation - If I use `make_visit_only_for(w)`, I'll get a `visit_only_for`, and thus the `float` visitor would not raise a compile-time error (it would fall back to the template operator). My goal here is to force programmer to explicitly write the variant types in both places so that a change in one place always result in a change in the other. – Holt Aug 16 '17 at 11:53
1

You may add an extra layer to add those extra check, for example something like:

template <typename Ret, typename ... Ts> struct IVisitorHelper;

template <typename Ret> struct IVisitorHelper<Ret> {};

template <typename Ret, typename T>
struct IVisitorHelper<Ret, T>
{
    virtual ~IVisitorHelper() = default;
    virtual Ret operator()(T) const = 0;
};

template <typename Ret, typename T, typename T2, typename ... Ts>
struct IVisitorHelper<Ret, T, T2, Ts...> : IVisitorHelper<Ret, T2, Ts...>
{
    using IVisitorHelper<Ret, T2, Ts...>::operator();
    virtual Ret operator()(T) const = 0;
};

template <typename Ret, typename V> struct IVarianVisitor;

template <typename Ret, typename ... Ts>
struct IVarianVisitor<Ret, std::variant<Ts...>> : IVisitorHelper<Ret, Ts...>
{
};

template <typename Ret, typename V>
Ret my_visit(const IVarianVisitor<Ret, std::decay_t<V>>& v, V&& var)
{
    return std::visit(v, var);
}

With usage:

struct Visitor : IVarianVisitor<void, std::variant<double, std::string>>
{
    void operator() (double) const override { std::cout << "double\n"; }
    void operator() (std::string) const override { std::cout << "string\n"; }
};


std::variant<double, std::string> v = //...;
my_visit(Visitor{}, v);
Jarod42
  • 203,559
  • 14
  • 181
  • 302
0

Somewhat based on the visit_only_for example by Holt, I'm currently trying something like this to have a drop-in "tag" to my std::visit calls that prevents forgetting explicit handlers/operators:

//! struct visit_all_types_explicitly
//!
//! If this callable is used in the overload set for std::visit
//! its templated call operator will be bound to any type
//! that is not explicitly handled by a better match.
//! Since the instantiation of operator()<T> will trigger
//! a static_assert below, using this in std::visit forces
//! the user to handle all type cases.
//! Specifically, since the templated call operator is a
//! better match than call operators found via implicit argument
//! conversion, one is forced to implement all types even if
//! they are implicitly convertible without warning.
struct visit_all_types_explicitly {
    template<class> static inline constexpr bool always_false_v = false;

    // Note: Uses (T const&) instead of (T&&) because the const-ref version
    //       is a better "match" than the universal-ref version, thereby
    //       preventing the use of this in a context where another
    //       templated call operator is supplied.
    template<typename T>
    void operator()(T const& arg) const {
        static_assert(always_false_v<T>, "There are unbound type cases! [visit_all_types_explicitly]");
    }
};

using MyVariant = std::variant<int, double>;

void test_visit() {
    const MyVariant val1 = 42;

    // This compiles:
    std::visit(
        overloaded{
            kse::visit_all_types_explicitly(),
            [](double arg) {},
            [](int arg) {},
        },
        val1
        );

    // does not compile because missing int-operator causes
    // visit_all_types_explicitly::operator()<int> to be instantiated
    std::visit(
        overloaded{
            visit_all_types_explicitly(),
            [](double arg) {},
            // [](int arg) {  },
        },
        val1
        );

    // does also not compile: (with static assert from visit_all_types_explicitly)
    std::visit(
        overloaded{
            visit_all_types_explicitly(),
            [](double arg) {},
            // [](int arg) {  },
            [](auto&& arg) {}
        },
        val1
    );

    // does also not compile: (with std::visit not being able to match the overloads)
    std::visit(
        overloaded{
            visit_all_types_explicitly(),
            [](double arg) {},
            // [](int arg) {  },
            [](auto const& arg) {}
        },
        val1
    );
}

For now, this seems to do what I want, and what the OP asked for:

Instead of getting a compile error, you will have the wrong lambda called, usually the default one, or you might get an implicit conversion that you didn't plan.

You intentionally cannot combine this with an "default" / auto handler.

Martin Ba
  • 37,187
  • 33
  • 183
  • 337