[libc++] fix views::all hard error on lvalue move only views instead of SFINAE

[huixie90]

Summary

For an lvalue reference to a move only view x, views::all(x) gives hard error because the expression inside noexcept is not well formed and it is not SFINAE friendly.

Given a move only view type V, and a concept

template <class R>
concept can_all = requires {
    std::views::all(std::declval<R>());
};

The expression can_all<V&> returns
libstdc++: false
msvc stl : false
libc++ : error: static_cast from 'V' to 'typename decay<decltype((std::forward<V &>(__t)))>::type' (aka 'V') uses deleted function

noexcept(noexcept(_LIBCPP_AUTO_CAST(std::forward<_Tp>(__t))))

The standard spec has its own problem, the spec says it is expression equivalent to decay-copy(E) but the spec of decay-copy does not have any constraint, which means the expression decay-copy(declval<V&>()) is well-formed and the concept can_all<V&> should return true and should error when instantiating the function body of decay-copy. This is clearly wrong behaviour in the spec and we will probably create an LWG issue. But the libc++'s behaviour is clearly not correct. The noexcept is an "extension" in libc++ which is not in the spec, but the expression inside noexpect triggers hard error, which is not right.

[jloser]

Thanks for the fix and nice explanation. This looks good from my perspective.

[var-const]

Thanks a lot for the fix! The header change LGTM, but I have one question regarding the test.

libcxx/include/__ranges/all.h

42

I wonder if we have any other cases where we don't write the return type 3 times and consequently don't get SFINAE.

libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp

153

Is it important for the view to be move-only? From the patch description, I got the impression that any reference should fail, but perhaps I'm missing something?

[jloser]

libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp

153

The call operator already requires the template type be a view, so an lvalue reference to a view is the simplest case to make it go boom AFAICT.

[EricWF]

libcxx/include/__ranges/all.h

42

Is this a case of "not getting SFINAE" or "auto" return types not deducing to reference types? Does this work if we use decltype(auto) instead?

Also this changes the return type from being by-value to being by-reference, which is the intended change I assume?

[huixie90]

libcxx/include/__ranges/all.h

42

This function always return by value because _LIBCPP_AUTO_CAST always yield the decayed type

#define _LIBCPP_AUTO_CAST(expr) static_cast<typename decay<decltype((expr))>::type>(expr)

In the spec, it says expression equivalent to decay-copy(E), and _LIBCPP_AUTO_CAST is the libc++ way of implementing decay-copy

In any case, the return type should be the same as decay-copy (expression equivalent), and this is why decltype(_LIBCPP_AUTO_CAST(...)) is correct

decltype(auto) won't work because decltype(auto) doesn't have any effect on SFINAE

libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp

153

"move-only" is important here because _LIBCPP_AUTO_CAST (which is used in the all's function body, and noexcept specification) is going to make a copy.

see definition here

#define _LIBCPP_AUTO_CAST(expr) static_cast<typename decay<decltype((expr))>::type>(expr)

If we pass an lvalue reference to a copyable view, the expression above is just making a copy of the view, which is fine. If the view is move-only, the above expression is ill-formed and cause noexcept(decay-copy(v)) go boom

[var-const]

libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp

153

Thanks for explaining! Can you please add a condensed form of this explanation as a comment here?

[huixie90]

added comments to the test to explain why it should not be invocable with views::all

[huixie90]

pull rebase

[EricWF]

libcxx/include/__ranges/all.h

42

Oh, based on the name I had assumed it did something entirely different. Like forward<T&&>(t), but without the need to write the type.
Maybe it needs a better name.

Also, this is a lesson in using auto return types. I think it's clear that:

1. We shouldn't use them in any place the standard doesn't mandate.
2. We really shouldn't use them in any place we also use noexcept(noexcept(...)), because it can poison an entire overload set as your discovering.

[huixie90]

libcxx/include/__ranges/all.h

42

yeah, i was expecting a __decay_copy function but it turned out to have this macro. I guess the name was inspired by the C++23 auto(x) syntax which is almost the same as this macro except when x is a prvalue, this macro will do an extra move.

[ldionne]

I'm fine with this (and thanks a lot for the writeup). I think it is possible that we have the same issue elsewhere, but I'm not sure how to audit or find out about those.

[ldionne]

libcxx/include/__ranges/all.h

42

@huixie90 Yes, _LIBCPP_AUTO_CAST was inspired by auto(x). In fact, we used to have a __decay_copy function and we removed it in favour of _LIBCPP_AUTO_CAST in D115686.

[huixie90]

libcxx/include/__ranges/all.h

42

Thanks. correction for my previous comment. __decay_copy will do an extra move for prvalue inputs, auto(x) and _LIBCPP_AUTO_CAST will not do the extra move

libcxx/include/__ranges/all.h

 
 namespace __all {
   struct __fn : __range_adaptor_closure<__fn> {
     template<class _Tp>
       requires ranges::view<decay_t<_Tp>>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(std::forward<_Tp>(__t))))
+      -> decltype(_LIBCPP_AUTO_CAST(std::forward<_Tp>(__t)))
     {
       return _LIBCPP_AUTO_CAST(std::forward<_Tp>(__t));
     }
 
     template<class _Tp>
       requires (!ranges::view<decay_t<_Tp>>) &&
                requires (_Tp&& __t) { ranges::ref_view{std::forward<_Tp>(__t)}; }
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI

libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp

struct CopyableView : std::ranges::view_base {
   CopyableView& operator=(CopyableView const&) noexcept(IsNoexcept) = default;
   constexpr explicit CopyableView(int start) noexcept : start_(start) {}
   constexpr int* begin() const { return globalBuff + start_; }
   constexpr int* end() const { return globalBuff + 8; }
 };
 static_assert(std::ranges::view<CopyableView<true>>);
 static_assert(std::ranges::view<CopyableView<false>>);
 
+struct MoveOnlyView : std::ranges::view_base{
+  MoveOnlyView() = default;
+  MoveOnlyView(const MoveOnlyView&) = delete;
+  MoveOnlyView& operator=(const MoveOnlyView&) = delete;
+  MoveOnlyView(MoveOnlyView&&) = default;
+  MoveOnlyView& operator=(MoveOnlyView&&) = default;
+
+  int* begin() const;
+  int* end() const;
+};
+
 struct Range {
   int start_;
   constexpr explicit Range(int start) noexcept : start_(start) {}
   constexpr int* begin() const { return globalBuff + start_; }
   constexpr int* end() const { return globalBuff + 8; }
 };
 
 struct BorrowableRange {
constexpr bool test() {
     assert(base(std::ranges::begin(own)) == globalBuff);
     assert(base(base(std::ranges::end(own))) == globalBuff + 8);
   }
 
   // Check SFINAE friendliness of the call operator
   {
     static_assert(!std::is_invocable_v<decltype(std::views::all)>);
     static_assert(!std::is_invocable_v<decltype(std::views::all), RandomAccessRange, RandomAccessRange>);
+
+    // `views::all(v)` is expression equivalent to `decay-copy(v)` if the decayed type 
+    // of `v` models `view`. If `v` is an lvalue-reference to a move-only view, the 
+    // expression should be ill-formed because `v` is not copyable
+    static_assert(!std::is_invocable_v<decltype(std::views::all), MoveOnlyView&>);
   }
 
   // Test that std::views::all is a range adaptor
   {
     // Test `v | views::all`
     {
       Range range(0);
       auto result = range | std::views::all;