Bug 90943 - Visiting inherited variants no longer works in 9.1
Summary: Visiting inherited variants no longer works in 9.1
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 9.1.0
: P3 normal
Target Milestone: 11.3
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-19 16:25 UTC by Barry Revzin
Modified: 2021-10-01 19:53 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-06-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Revzin 2019-06-19 16:25:07 UTC
Here's a short repro:

#include <variant>

struct V : std::variant<int> {
    using std::variant<int>::variant;
};

namespace std {
    template <>
    struct variant_size<V> 
    : integral_constant<size_t, 1>
    { };

    template <>
    struct variant_alternative<0, V>
    {
        using type = int;
    };
}

V v = 42;
int i = std::visit([](int){ return 17;}, v);

This worked fine in gcc 8, but breaks in gcc 9 with:

In file included from <source>:1:
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant: In instantiation of 'constexpr const bool std::__detail::__variant::_Extra_visit_slot_needed<int, V&>::value':
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:809:50:   required from 'constexpr const int std::__detail::__variant::_Multi_array<int (*)(<lambda(int)>&&, V&), 1>::__do_cookie'
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:821:53:   required from 'struct std::__detail::__variant::_Multi_array<int (*)(<lambda(int)>&&, V&), 1>'
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:1016:36:   required from 'struct std::__detail::__variant::__gen_vtable<true, int, <lambda(int)>&&, V&>'
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:1637:23:   required from 'constexpr decltype(auto) std::__do_visit(_Visitor&&, _Variants&& ...) [with bool __use_index = false; bool __same_return_types = true; _Visitor = <lambda(int)>; _Variants = {V&}]'
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:1653:24:   required from 'constexpr decltype(auto) std::visit(_Visitor&&, _Variants&& ...) [with _Visitor = <lambda(int)>; _Variants = {V&}]'
<source>:21:43:   required from here
/opt/compiler-explorer/gcc-9.1.0/include/c++/9.1.0/variant:778:5: error: incomplete type 'std::__detail::__variant::_Extra_visit_slot_needed<int, V&>::_Variant_never_valueless<V>' used in nested name specifier
  778 |  && !_Variant_never_valueless<__remove_cvref_t<_Variant>>::value;
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I realize that this kind of stuff isn't technically or officially sanctioned -- but doing this makes it very convenient to write an easy-to-use recursive variant without having to implement our own variant.

Tim says you should call this NAD, but I'm being pretty optimistic :-)
Comment 1 Jonathan Wakely 2019-06-19 23:48:19 UTC
This is all it takes to compile your example:

--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -795,7 +795,7 @@ namespace __variant
   template <typename _Maybe_variant_cookie, typename _Variant>
     struct _Extra_visit_slot_needed
     {
-      template <typename> struct _Variant_never_valueless;
+      template <typename> struct _Variant_never_valueless : false_type { };
 
       template <typename... _Types>
        struct _Variant_never_valueless<variant<_Types...>>

This just assumes that unrecognized types are not never-valueless variants, which means some extra code gets generated to handle the valueless case even if your variant base class will never be valueless.

I haven't considered if anything else would break by doing that.
Comment 2 Barry Revzin 2019-06-20 02:20:43 UTC
What if we did something like (using pretty names for a sec):

template <typename _Maybe_variant_cookie, typename _Variant>
struct _Extra_visit_slot_needed
{
  template <typename... _Types>
  static bool_constant<__never_valueless<_Types...>()>
  __impl(const variant<_Types...>&);
  static false_type __impl(...);

  using _Variant_never_valueless = decltype(__impl(declval<_Variant>()));


  static constexpr bool value =
    (is_same_v<_Maybe_variant_cookie, __variant_cookie>
     || is_same_v<_Maybe_variant_cookie, __variant_idx_cookie>)
     && !_Variant_never_valueless::value;
};

This should (modulo typos) work for real variant instantiations and also anything that inherits from some variant instantiation?
Comment 3 Jonathan Wakely 2019-06-20 13:47:53 UTC
https://wg21.link/LWG3052 would forbid us from supporting this.
Comment 4 Jakub Jelinek 2019-08-12 08:53:19 UTC
GCC 9.2 has been released.
Comment 5 Jakub Jelinek 2020-03-12 11:59:03 UTC
GCC 9.3.0 has been released, adjusting target milestone.
Comment 6 Jonathan Wakely 2021-02-23 11:46:54 UTC
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2162r2.html says we should support inherited variants.
Comment 7 Jonathan Wakely 2021-04-19 14:11:08 UTC Comment hidden (obsolete)
Comment 8 Jonathan Wakely 2021-05-12 09:54:44 UTC
Corrected URL for downstream fix:
https://gitlab.com/jonathan-wakely/gcc/-/commit/486d89e403a18ef78f05f2efb1bc86bbd396899c
Comment 9 CVS Commits 2021-10-01 19:38:50 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:c46ecb0112e91c80ee111439e79a58a953e4479d

commit r12-4067-gc46ecb0112e91c80ee111439e79a58a953e4479d
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Apr 19 14:49:12 2021 +0100

    libstdc++: Allow visiting inherited variants [PR 90943]
    
    Implement the changes from P2162R2 (as a DR for C++17).
    
    Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/90943
            * include/std/variant (__cpp_lib_variant): Update value.
            (__detail::__variant::__as): New helpers implementing the
            as-variant exposition-only function templates.
            (visit, visit<R>): Use __as to upcast the variant parameters.
            * include/std/version (__cpp_lib_variant): Update value.
            * testsuite/20_util/variant/visit_inherited.cc: New test.
Comment 10 Jonathan Wakely 2021-10-01 19:53:31 UTC
Done for GCC 12, but I intend to backport it.