[libc++][test] Fix invalid test for views::view_interface

[jloser]

Summary

The type MoveOnlyForwardRange violates the precondition stated in
view.interface.general. Specifically, the type passed to
view_interface shall model the view concept. In turn, this requires the
type to satisfy movable concept (and others), but this type
MoveOnlyForwardRange does not satisfy the movable concept.

Add a move assignment operator so that MoveOnlyForwardRange satisfies the
movable concept. While we're here, ensure the neighboring types that inherit
from view_interface also satisfy the view concept to avoid similar issues.

Fixes https://bugs.llvm.org/show_bug.cgi?id=50720

[cjdb]

Nice catch, thanks.

I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

105

I don't mind if it's done in this patch or a second one, but could you please strip the const_casts? A const-qualified range shouldn't be returning a mutable iterator.

[jloser]

In D112631#3090641, @cjdb wrote:

Nice catch, thanks.

I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

Do you mind elaborating on your suggestion here? In view.interface.general note 2 states: that "D shall be complete, and model both derived_­from<view_­interface<D>> and view". So, I'm assuming you're referring to the completeness and view requirements? Note the requirement of completeness is dependent on where it's checked because D can be an incomplete type as it mentions in the note.

I think an improvement in the QoI that may be a nice middle-ground is to put a static_assert(view<_Derived>, "Derived class must be a view."); in view_interface. However, this breaks several ranges tests - specifically, subrange which inherits from view_interface. I haven't looked too closely, but at the time of the inheritance, subrange wouldn't be a complete type, so we may have a bug somewhere with incompleteness of types and some view/range concepts somewhere. I'd recommend we investigate that in a follow-up. What do you think?

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

105

Good spot. I'm happy to do that in a separate patch as the const_casts are present in a few of the range test types here and it's orthogonal to this particular test fix. :)

[cjdb]

In D112631#3090888, @jloser wrote:

In D112631#3090641, @cjdb wrote:

Nice catch, thanks.

I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

Do you mind elaborating on your suggestion here? In view.interface.general note 2 states: that "D shall be complete, and model both derived_­from<view_­interface<D>> and view". So, I'm assuming you're referring to the completeness and view requirements? Note the requirement of completeness is dependent on where it's checked because D can be an incomplete type as it mentions in the note.

Ah, yes, I forgot about the completeness requirement, although I'm not really concerned about that one. All of our requirements hinge on completeness.

I think an improvement in the QoI that may be a nice middle-ground is to put a static_assert(view<_Derived>, "Derived class must be a view."); in view_interface. However, this breaks several ranges tests - specifically, subrange which inherits from view_interface. I haven't looked too closely, but at the time of the inheritance, subrange wouldn't be a complete type, so we may have a bug somewhere with incompleteness of types and some view/range concepts somewhere. I'd recommend we investigate that in a follow-up. What do you think?

We can't do a static assert because the special member functions are allowed to exist. What I'm proposing is:

constexpr bool empty() requires forward_­range<D> and view<D> and derived_from<D, view_interface<D>>;

[jloser]

In D112631#3090909, @cjdb wrote:

In D112631#3090888, @jloser wrote:

In D112631#3090641, @cjdb wrote:

Nice catch, thanks.

I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

Do you mind elaborating on your suggestion here? In view.interface.general note 2 states: that "D shall be complete, and model both derived_­from<view_­interface<D>> and view". So, I'm assuming you're referring to the completeness and view requirements? Note the requirement of completeness is dependent on where it's checked because D can be an incomplete type as it mentions in the note.

Ah, yes, I forgot about the completeness requirement, although I'm not really concerned about that one. All of our requirements hinge on completeness.

I think an improvement in the QoI that may be a nice middle-ground is to put a static_assert(view<_Derived>, "Derived class must be a view."); in view_interface. However, this breaks several ranges tests - specifically, subrange which inherits from view_interface. I haven't looked too closely, but at the time of the inheritance, subrange wouldn't be a complete type, so we may have a bug somewhere with incompleteness of types and some view/range concepts somewhere. I'd recommend we investigate that in a follow-up. What do you think?

We can't do a static assert because the special member functions are allowed to exist. What I'm proposing is:

constexpr bool empty() requires forward_­range<D> and view<D> and derived_from<D, view_interface<D>>;

I see. Yeah, we can (and still be conforming AFAIK) add the additional requirements of view<D> and derived_from<D, view_interface<D>> to be checked on every member function of view_interface . This does have additional compile-time costs as you know, but does ensure a bit of safety. I'm generally in favor of this kind of tradeoff. What's the status quo with other types (ranges or others) with regard to these requirements on classes? Right now, it's IFNDR which isn't great from a user perspective. Thoughts, @ldionne @Quuxplusone?

As a proof of concept, I just sprinkled the requirements to each member function of view_interface and all of the ranges test pass just fine FWIW.

[Mordante]

In D112631#3090943, @jloser wrote:

In D112631#3090909, @cjdb wrote:

In D112631#3090888, @jloser wrote:

In D112631#3090641, @cjdb wrote:

Nice catch, thanks.

I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

Do you mind elaborating on your suggestion here? In view.interface.general note 2 states: that "D shall be complete, and model both derived_­from<view_­interface<D>> and view". So, I'm assuming you're referring to the completeness and view requirements? Note the requirement of completeness is dependent on where it's checked because D can be an incomplete type as it mentions in the note.

Ah, yes, I forgot about the completeness requirement, although I'm not really concerned about that one. All of our requirements hinge on completeness.

I think an improvement in the QoI that may be a nice middle-ground is to put a static_assert(view<_Derived>, "Derived class must be a view."); in view_interface. However, this breaks several ranges tests - specifically, subrange which inherits from view_interface. I haven't looked too closely, but at the time of the inheritance, subrange wouldn't be a complete type, so we may have a bug somewhere with incompleteness of types and some view/range concepts somewhere. I'd recommend we investigate that in a follow-up. What do you think?

We can't do a static assert because the special member functions are allowed to exist. What I'm proposing is:

constexpr bool empty() requires forward_­range<D> and view<D> and derived_from<D, view_interface<D>>;

I see. Yeah, we can (and still be conforming AFAIK) add the additional requirements of view<D> and derived_from<D, view_interface<D>> to be checked on every member function of view_interface . This does have additional compile-time costs as you know, but does ensure a bit of safety. I'm generally in favor of this kind of tradeoff. What's the status quo with other types (ranges or others) with regard to these requirements on classes? Right now, it's IFNDR which isn't great from a user perspective. Thoughts, @ldionne @Quuxplusone?

Personally I would prefer to stick to the wording of the Standard. Since all these functions call the exposition only function derived, how about adding these additional requirements to these two functions?

IMO the proposed changes to the requirements should be done in a separate patch. For this one LGTM!

[• Quuxplusone]

IIUC, Chris proposes changing view_interface<D>::empty from

template<class _D2 = _Derived>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
  noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
  requires forward_range<_D2>
{
  return ranges::begin(__derived()) == ranges::end(__derived());
}

to

template<class _D2 = _Derived>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
  noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
  requires forward_range<_D2> && view<_D2> && derived_from<_D2, view_interface>
{
  return ranges::begin(__derived()) == ranges::end(__derived());
}

This doesn't seem useful to me. It diverges from the Standard wording, slows down compilation, suffers from the "more code = more chances for bugs" problem, and also seems like it might hide user-code bugs (by having these members simply SFINAE away in the library-UB case).
I don't particularly object to a static_assert like

template<class _D2 = _Derived>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
  noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
  requires forward_range<_D2>
{
  static_assert(view<_D2> && derived_from<_D2, view_interface>);
  return ranges::begin(__derived()) == ranges::end(__derived());
}

but even that suffers from the middle two disadvantages above (slows down compilation, suffers from the "more code = more chances for bugs" problem). So I'm (as usual) in favor of the status quo.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

75

Consider eliminating line 75, as long as you're here.

[cjdb]

There seems to be some confusion about whether or not this deviates from standard wording. It doesn't. I would very much prefer that our implementation catch user errors whenever we're in a position to do so, rather than let our users get false positives for a valid C++ program.

[jloser]

In D112631#3091418, @cjdb wrote:

There seems to be some confusion about whether or not this deviates from standard wording. It doesn't. I would very much prefer that our implementation catch user errors whenever we're in a position to do so, rather than let our users get false positives for a valid C++ program.

Agreed. I'm going to land this as-is for now, and I'll open a new review which adds a helpful static_assert to the member functions of view_interface soon.

[Mordante]

In D112631#3091418, @cjdb wrote:

There seems to be some confusion about whether or not this deviates from standard wording. It doesn't. I would very much prefer that our implementation catch user errors whenever we're in a position to do so, rather than let our users get false positives for a valid C++ program.

I agree we should catch it, but I dislike to change the public interface from the wording in the Standard. Hence my suggestion to add these additional constraints to the exposition only functions. IMO that's the better solution:

• The public interface follows the wording of the Standard.
• The additional constrains only need to be applied to two functions instead of all public functions.

So I prefer

_LIBCPP_HIDE_FROM_ABI
constexpr _Derived& __derived() noexcept requires view<D> && derived_from<D, view_interface<D>>
{
  return static_cast<_Derived&>(*this);
}

over

template<class _D2 = _Derived>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
  noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
  requires forward_range<_D2> && requires view<D> && derived_from<D, view_interface<D>>
{
  return ranges::begin(__derived()) == ranges::end(__derived());
}

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

constexpr std::ptrdiff_t operator-(const ForwardIter& x, const ForwardIter& y) {
     return x.base() - y.base();
 }
 
 struct ForwardRange : std::ranges::view_interface<ForwardRange> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
 };
+static_assert(std::ranges::view<ForwardRange>);
 
 struct MoveOnlyForwardRange : std::ranges::view_interface<MoveOnlyForwardRange> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   MoveOnlyForwardRange(MoveOnlyForwardRange const&) = delete;
   MoveOnlyForwardRange(MoveOnlyForwardRange &&) = default;
+  MoveOnlyForwardRange& operator=(MoveOnlyForwardRange &&) = default;
   MoveOnlyForwardRange() = default;
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
 };
+static_assert(std::ranges::view<MoveOnlyForwardRange>);
 
 struct EmptyIsTrue : std::ranges::view_interface<EmptyIsTrue> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
   constexpr bool empty() const { return true; }
 };
+static_assert(std::ranges::view<EmptyIsTrue>);
 
 struct SizeIsTen : std::ranges::view_interface<SizeIsTen> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
   constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
   constexpr size_t size() const { return 10; }
 };
+static_assert(std::ranges::view<SizeIsTen>);
 
 using RAIter = random_access_iterator<int*>;
 
 struct RARange : std::ranges::view_interface<RARange> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr RAIter begin() const { return RAIter(const_cast<int*>(buff)); }
   constexpr RAIter end() const { return RAIter(const_cast<int*>(buff) + 8); }
 };
+static_assert(std::ranges::view<RARange>);
 
 using ContIter = contiguous_iterator<const int*>;
 
 struct ContRange : std::ranges::view_interface<ContRange> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ContIter begin() const { return ContIter(buff); }
   constexpr ContIter end() const { return ContIter(buff + 8); }
 };
+static_assert(std::ranges::view<ContRange>);
 
 struct DataIsNull : std::ranges::view_interface<DataIsNull> {
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ContIter begin() const { return ContIter(buff); }
   constexpr ContIter end() const { return ContIter(buff + 8); }
   constexpr const int *data() const { return nullptr; }
 };
+static_assert(std::ranges::view<DataIsNull>);
 
 template<bool IsNoexcept>
 struct BoolConvertibleComparison : std::ranges::view_interface<BoolConvertibleComparison<IsNoexcept>> {
   struct ResultType {
     bool value;
     constexpr operator bool() const noexcept(IsNoexcept) { return value; }
   };
 
   struct SentinelType {
     int *base_;
     SentinelType() = default;
     explicit constexpr SentinelType(int *base) : base_(base) {}
     friend constexpr ResultType operator==(ForwardIter const& iter, SentinelType const& sent) noexcept { return {iter.base() == sent.base_}; }
     friend constexpr ResultType operator==(SentinelType const& sent, ForwardIter const& iter) noexcept { return {iter.base() == sent.base_}; }
     friend constexpr ResultType operator!=(ForwardIter const& iter, SentinelType const& sent) noexcept { return {iter.base() != sent.base_}; }
     friend constexpr ResultType operator!=(SentinelType const& sent, ForwardIter const& iter) noexcept { return {iter.base() != sent.base_}; }
   };
 
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   constexpr ForwardIter begin() const noexcept { return ForwardIter(const_cast<int*>(buff)); }
   constexpr SentinelType end() const noexcept { return SentinelType(const_cast<int*>(buff) + 8); }
 };
+static_assert(std::ranges::view<BoolConvertibleComparison<true>>);
+static_assert(std::ranges::view<BoolConvertibleComparison<false>>);
 
 template<class T>
 concept EmptyInvocable = requires (T const& obj) { obj.empty(); };
 
 template<class T>
 concept BoolOpInvocable = requires (T const& obj) { bool(obj); };
 
 constexpr bool testEmpty() {