ericniebler / range-v3

Range library for C++14/17/20, basis for C++20's std::ranges
Other
4.06k stars 437 forks source link

abstract iterator value_types's and `zip_view` fail to compile #1680

Open ingo-loehken opened 2 years ago

ingo-loehken commented 2 years ago

I found out, that iterators with an abstract value_type fail to compile, because concepts checks on invocable try to pass the unqouted value_type, which ofc cannot be instantiated.

#include <range/v3/view/zip.hpp>

struct Abstract
{
  virtual void dummy() = 0;
};

using Container = std::vector<Abstract>; // just get an iterator, not meaningful at all

void foo()
{
  for([[maybe_unsued]] auto const& [x, y] : ranges::zip_view(std::declval<Container const&>(), std::declval<Container const&>()))
  {

  }
}

Minimal reproduction scenario with msvc vs2019 (16.11.5), C++17. Please refer to example in compiler explorer.

example.cpp
The contents of <span> are available only with C++20 or later.
C:/data/msvc/14.30.30528-Pre/include\utility(329): error C2259: 'Abstract': cannot instantiate abstract class
<source>(3): note: see declaration of 'Abstract'
C:/data/msvc/14.30.30528-Pre/include\utility(329): note: due to following members:
C:/data/msvc/14.30.30528-Pre/include\utility(329): note: 'void Abstract::dummy(void)': is abstract
<source>(5): note: see declaration of 'Abstract::dummy'
C:/data/msvc/14.30.30528-Pre/include\type_traits(1385): note: see reference to class template instantiation 'std::pair<Abstract,Abstract>' being compiled
C:/data/msvc/14.30.30528-Pre/include\type_traits(1384): note: while compiling class template member function 'std::pair<Abstract,Abstract> std::_Invoker_functor::_Call<T&,ranges::copy_tag,From,From>(_Callable,ranges::copy_tag &&,From &&,From &&) noexcept(<expr>)'
        with
        [
            T=ranges::detail::indirect_zip_fn_,
            From=std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<Abstract>>>,
            _Callable=ranges::detail::indirect_zip_fn_ &
        ]
C:/data/msvc/14.30.30528-Pre/include\type_traits(1608): note: see reference to alias template instantiation 'std::_Decltype_invoke_nonzero<ranges::detail::indirect_zip_fn_&,ranges::copy_tag,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>>' being compiled
        with
        [
            _Ty=Abstract
        ]
C:/data/libraries/installed/x64-windows/include\range/v3/functional/concepts.hpp(33): note: see reference to variable template 'const bool is_invocable_v<ranges::detail::indirect_zip_fn_ &,ranges::copy_tag,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<Abstract> > >,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<Abstract> > > >' being compiled
C:/data/msvc/14.30.30528-Pre/include\utility(329): note: see reference to variable template 'const bool invocable<ranges::detail::indirect_zip_fn_ &,ranges::copy_tag,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<Abstract> > >,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<Abstract> > > >' being compiled
C:/data/msvc/14.30.30528-Pre/include\utility(329): note: see reference to variable template 'const bool zippable_with<ranges::detail::indirect_zip_fn_,ranges::ref_view<std::vector<Abstract,std::allocator<Abstract> > const > const ,ranges::ref_view<std::vector<Abstract,std::allocator<Abstract> > const > const >' being compiled
C:/data/libraries/installed/x64-windows/include\range/v3/view/facade.hpp(46): note: see reference to alias template instantiation 'ranges::detail::begin_cursor_t<const ranges::iter_zip_with_view<ranges::detail::indirect_zip_fn_,ranges::ref_view<const Container>,ranges::ref_view<const Container>>>' being compiled
C:/data/libraries/installed/x64-windows/include\range/v3/view/facade.hpp(106): note: see reference to alias template instantiation 'ranges::detail::facade_iterator_t<const ranges::iter_zip_with_view<ranges::detail::indirect_zip_fn_,ranges::ref_view<const Container>,ranges::ref_view<const Container>>>' being compiled
C:/data/libraries/installed/x64-windows/include\range/v3/view/facade.hpp(106): note: while compiling class template member function 'ranges::basic_iterator<decay<unknown-type>::type> ranges::view_facade<ranges::iter_zip_with_view<ranges::detail::indirect_zip_fn_,ranges::ref_view<const Container>,ranges::ref_view<const Container>>,ranges::finite>::begin(void) const'
C:/data/msvc/14.30.30528-Pre/include\utility(330): error C2259: 'Abstract': cannot instantiate abstract class
<source>(3): note: see declaration of 'Abstract'
C:/data/msvc/14.30.30528-Pre/include\utility(330): note: due to following members:
C:/data/msvc/14.30.30528-Pre/include\utility(330): note: 'void Abstract::dummy(void)': is abstract
<source>(5): note: see declaration of 'Abstract::dummy'
Compiler returned: 2
Compiler Explorer uses cookies and other related techs to serve you

Background:
Container is a custom type holding an std::vector<std::unique_ptr<Abstract>> and providing overloads for begin and end to a user-defined iterator with value_type Abstract to be used in range-based-for loops or with ranges library.

I looked though git issues and checked articles and ranges papers and proposals, but did not find any hint. Reverse Egineering of ranges-v3 internals just came up with the same issue.

Any ideas are welcome ? Current solution is to use iterators. But I don't like to recommend using workarounds.

brevzin commented 2 years ago

Container is a custom type holding an std::vector<std::unique_ptr<Abstract>> and providing overloads for begin and end to a user-defined iterator with value_type Abstract to be used in range-based-for loops or with ranges library.

Having an abstract value_type doesn't really make much sense. The value_type of a vector<unique_ptr<Abstract>> should be unique_ptr<Abstract>, not Abstract. The point of value_type is to be able construct objects of that type (e.g. ranges::min(r) returns r's value_type), and if that's impossible then that's not very useful?

CaseyCarter commented 2 years ago

Container is a custom type holding an std::vector<std::unique_ptr<Abstract>> and providing overloads for begin and end to a user-defined iterator with value_type Abstract to be used in range-based-for loops or with ranges library.

Having an abstract value_type doesn't really make much sense. The value_type of a vector<unique_ptr<Abstract>> should be unique_ptr<Abstract>, not Abstract. The point of value_type is to be able construct objects of that type (e.g. ranges::min(r) returns r's value_type), and if that's impossible then that's not very useful?

What should the value type of std::vector<std::unique_ptr<Abstract>>{} | ranges::transform([](auto&& x) -> auto&& { return *x; }) be, if not Abstract? (https://godbolt.org/z/5Paq9Tve7)

Requirements come from algorithms, and there are more algorithms (in the Standard, and IIRC range-v3 as well) that don't need to construct objects of a range's value type than algorithms that do. Ideally zip shouldn't need to instantiate its value type, either. I see copy_tag in the stack of "why I'm instantiating std::pair<Abstract, Abstract>" messages, I suspect the problem has something to do with the weirdness in the implementation of zip_with.

brevzin commented 2 years ago

Container is a custom type holding an std::vector<std::unique_ptr<Abstract>> and providing overloads for begin and end to a user-defined iterator with value_type Abstract to be used in range-based-for loops or with ranges library.

Having an abstract value_type doesn't really make much sense. The value_type of a vector<unique_ptr<Abstract>> should be unique_ptr<Abstract>, not Abstract. The point of value_type is to be able construct objects of that type (e.g. ranges::min(r) returns r's value_type), and if that's impossible then that's not very useful?

What should the value type of std::vector<std::unique_ptr<Abstract>>{} | ranges::transform([](auto&& x) -> auto&& { return *x; }) be, if not Abstract? (https://godbolt.org/z/5Paq9Tve7)

Requirements come from algorithms, and there are more algorithms (in the Standard, and IIRC range-v3 as well) that don't need to construct objects of a range's value type than algorithms that do. Ideally zip shouldn't need to instantiate its value type, either. I see copy_tag in the stack of "why I'm instantiating std::pair<Abstract, Abstract>" messages, I suspect the problem has something to do with the weirdness in the implementation of zip_with.

OK, was not thinking. I take it back. Here's a more full repro:

#include <range/v3/view/zip.hpp>

struct Abstract {
    virtual auto f() -> int = 0;
};

struct V {
    auto begin() -> Abstract*;
    auto end() -> Abstract*;
};

void use(V v) {
    auto z = ranges::views::zip(v);
    z.begin();
}

Stack trace makes little sense to me so far.

brevzin commented 2 years ago

Ok neither gcc nor clang's stack trace are remotely helpful (neither even mention zip_with.hpp anywhere???), but if you comment out the declaration of value_type here (lines 212-213): https://github.com/ericniebler/range-v3/blob/83783f578e0e6666d68a3bf17b0038a80e62530e/include/range/v3/view/zip_with.hpp#L198-L213

then it works.

brevzin commented 2 years ago

Alright here's a further reduction which doesn't even involve range-v3 @CaseyCarter:

#include <tuple>

struct Abstract {
    virtual auto f() -> int = 0;
};

struct indirect_value_fn {
    template <typename... Ts>
    auto operator()(Ts*... ) const -> std::tuple<Ts...>;
};

#if BAD
template <typename F, typename... Args>
auto lame_invoke(F&& f, Args&&... args)
    noexcept(noexcept(((F&&) f)((Args&&) args...)))
    -> decltype(((F&&) f)((Args&&) args...))
{
    return ((F&&) f)((Args&&) args...);
}

// not fine
using T = decltype(lame_invoke(indirect_value_fn{}, (Abstract*)nullptr));
#else
// fine
using T = decltype(indirect_value_fn{}((Abstract*)nullptr));
#endif

I'm not sure where actually things are supposed to be instantiated. That's kind of... murky.

CaseyCarter commented 2 years ago

Notably this compiles with BAD defined if we comment out the noexcept-specifier on lame_invoke:

template <typename F, typename... Args>
auto lame_invoke(F&& f, Args&&... args)
    // noexcept(noexcept(((F&&) f)((Args&&) args...)))
    -> decltype(((F&&) f)((Args&&) args...))
{
    return ((F&&) f)((Args&&) args...);
}

Could we workaround by using a function without conditional noexcept to determine the value type? (I'd hate to have to duplicate the definition of invoke, but IIRC range-v3 uses a short-and-sweet invoke.)

brevzin commented 2 years ago

In this case we wouldn't even need to bother, since we're calling the function with copy_tag as the 1st argument, so the function can't very well be a pointer to member of copy_tag.

So I tried this, in zip_with.hpp:

- using value_type = detail::decay_t<invoke_result_t<
-     fun_ref_ &, copy_tag, iterator_t<meta::const_if_c<Const, Rngs>>...>>;
+ using value_type = detail::decay_t<decltype(
+     fun_(copy_tag{}, std::declval<iterator_t<meta::const_if_c<Const, Rngs>>>()...))>;

Now we're not going through ranges::invoke_result_t, so we avoid that particular problem. But this still instantiates tuple<Abstract> somewhere.

igoloe commented 2 years ago

Did you found any fix, that might be incorporated into a release? There are a lot of issues open regarding zip_view and move only types.

If zip_view and others would be part of C++20 , fixes wont be required, but schedule is for C++23. And thats a long way.

ericniebler commented 2 years ago

Range-v3 doesn't have support for move only iterators or views. Adding that support and tests for it throughout would be a big job. Spot fixes for the concepts are not even half measures. I simply don't have the time to do this, and I haven't convinced anybody yet to take this on.