[libc++] [ranges] SFINAE away ranges::cbegin(const T&&) for non-borrowed T.

[• Quuxplusone]

Summary

Fixes https://github.com/llvm/llvm-project/issues/52952 — thanks @CaseyCarter for the bug report!

I believe the issue here is fundamentally similar to https://quuxplusone.github.io/blog/2021/07/30/perfect-forwarding-call-wrapper/ — we have an overload set where we really want only one of the overloads to ever be viable, but sometimes it can happen that the overload we want to select is SFINAEd away but another overload can be viable with just a slight conversion of the argument type (for example, in this case, converting the argument type const BeginMember&& to the viable overload's parameter type const BeginMember&.

[CaseyCarter]

FWIW, the updated tests pass when run against MSVC STL.

libcxx/include/__ranges/access.h

166

&& is extraneous - _Tp is an lvalue reference if and only if _Tp&& is an lvalue reference. (Also on 194.)

171

I'd use remove_reference_t instead of __uncvref_t to preserve volatile. (6 places, including lines 197-199)

[• Quuxplusone]

libcxx/include/__ranges/access.h

166

Yes, but I think this will be easier on the reader. The standard says "If E is an lvalue...", and here the type of E is _Tp&&, so, I say let's just stick with that instead of being any more "clever."

Notice that a "clever" usage of _Tp instead of _Tp&& was the root cause of https://github.com/llvm/llvm-project/issues/52953 .

171

Hm, I guess that depends on the interpretation of https://eel.is/c++draft/range.access.cbegin . We're supposed to take a "subexpression E of type T" and cast it to const T&. Suppose the expression is of type U=int& — then const U& would be int&, but in fact we want to say that T=int. So "of type T" strips ref-qualification; does it also strip cv-qualification? Is a reference to a const int an expression "of type int" or "of type const int"? Is a reference to a volatile int an expression "of type int" or "of type volatile int"?

I originally only picked __uncvref_t over remove_reference_t because it was shorter to type. I'll change it, although I wonder if we should test this volatile business, or ask LWG the intent, or what.

[CaseyCarter]

libcxx/include/__ranges/access.h

166

There are no expressions of reference type. The type of E is remove_reference_t<_Tp>. E is an lvalue if is_lvalue_reference_v<_Tp> and an rvalue otherwise. We can't determine if an rvalue E is a prvalue or an xvalue, because we bound a reference to it forcing materialization of prvalues into xvalues. The argument to this function is in any case a glvalue, which denotes the object the Standard refers to as "the reified object of E".

That said, the suggested change is less important to me than being pedantically correct in our discussion of the change =)

171

Hm, I guess that depends on the interpretation of https://eel.is/c++draft/range.access.cbegin . We're supposed to take a "subexpression E of type T" and cast it to const T&. Suppose the expression is of type U=int& — then const U& would be int&, but in fact we want to say that T=int. So "of type T" strips ref-qualification; does it also strip cv-qualification?

Again, there are no expressions of reference type - it's important that we don't conflate the type of an expression with the type-and-value-category-encoding decltype((meow)). When decltype((E)) is int&, E is an lvalue of type int - const T& is const int&.

Is a reference to a const int an expression "of type int" or "of type const int"?

Super-pedantically, a reference isn't an expression anymore than an array or a class is an expression. An id-expression that names a variable of type const int& (or a function-call expression that invokes a function whose return type is const int&, or ...) is an expression, specifically an lvalue of type const int.

Is a reference to a volatile int an expression "of type int" or "of type volatile int"?

Ditto-ish. In general, when E is an expression, remove_reference_t<decltype((E))> is its type, and it is an lvalue if is_lvalue_reference_v<decltype((E))>, an xvalue if is_rvalue_reference_v<decltype((E))>, or a prvalue if !is_reference_v<decltype((E))>.

I originally only picked __uncvref_t over remove_reference_t because it was shorter to type. I'll change it, although I wonder if we should test this volatile business, or ask LWG the intent, or what.

In my experience, LWG mostly wants to pretend that volatile doesn't exist. That said, it's as easy to preserve the volatile here as not, and I'd prefer errors to be at the call to ranges::begin instead of inside.

[• Quuxplusone]

s/__uncvref_t/remove_reference_t/

[CaseyCarter]

LGTM. (Shout if I shouldn't approve - I vaguely recall problems in the past were non-approver approvals broke someone's Phabricator workflow.)

[• Quuxplusone]

@CaseyCarter: Approving is fine and good AFAIK. :) The protocol is still for me to wait until @ldionne approves, or two other libc++ approvers approve, before landing this. (@ldionne ping for rubberstamp! :))

libcxx/include/__ranges/access.h

 } // namespace ranges
 
 // [range.access.cbegin]
 
 namespace ranges {
 namespace __cbegin {
   struct __fn {
     template <class _Tp>
+      requires is_lvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
-    constexpr auto operator()(_Tp& __t) const
-      noexcept(noexcept(ranges::begin(static_cast<const _Tp&>(__t))))
-      -> decltype(      ranges::begin(static_cast<const _Tp&>(__t)))
-      { return          ranges::begin(static_cast<const _Tp&>(__t)); }
+    constexpr auto operator()(_Tp&& __t) const
+      noexcept(noexcept(ranges::begin(static_cast<const remove_reference_t<_Tp>&>(__t))))
+      -> decltype(      ranges::begin(static_cast<const remove_reference_t<_Tp>&>(__t)))
+      { return          ranges::begin(static_cast<const remove_reference_t<_Tp>&>(__t)); }
 
     template <class _Tp>
       requires is_rvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::begin(static_cast<const _Tp&&>(__t))))
       -> decltype(      ranges::begin(static_cast<const _Tp&&>(__t)))
       { return          ranges::begin(static_cast<const _Tp&&>(__t)); }
   };
 }
 
 inline namespace __cpo {
   inline constexpr auto cbegin = __cbegin::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.access.cend]
 
 namespace ranges {
 namespace __cend {
   struct __fn {
     template <class _Tp>
+      requires is_lvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
-    constexpr auto operator()(_Tp& __t) const
-      noexcept(noexcept(ranges::end(static_cast<const _Tp&>(__t))))
-      -> decltype(      ranges::end(static_cast<const _Tp&>(__t)))
-      { return          ranges::end(static_cast<const _Tp&>(__t)); }
+    constexpr auto operator()(_Tp&& __t) const
+      noexcept(noexcept(ranges::end(static_cast<const remove_reference_t<_Tp>&>(__t))))
+      -> decltype(      ranges::end(static_cast<const remove_reference_t<_Tp>&>(__t)))
+      { return          ranges::end(static_cast<const remove_reference_t<_Tp>&>(__t)); }
 
     template <class _Tp>
       requires is_rvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::end(static_cast<const _Tp&&>(__t))))
       -> decltype(      ranges::end(static_cast<const _Tp&&>(__t)))
       { return          ranges::end(static_cast<const _Tp&&>(__t)); }

libcxx/test/std/ranges/range.access/begin.pass.cpp

 // Ensure that we can't call with rvalues with borrowing disabled.
 static_assert( std::is_invocable_v<RangeBeginT, BeginMember &>);
 static_assert(!std::is_invocable_v<RangeBeginT, BeginMember &&>);
 static_assert( std::is_invocable_v<RangeBeginT, BeginMember const&>);
 static_assert(!std::is_invocable_v<RangeBeginT, BeginMember const&&>);
 static_assert( std::is_invocable_v<RangeCBeginT, BeginMember &>);
 static_assert(!std::is_invocable_v<RangeCBeginT, BeginMember &&>);
 static_assert( std::is_invocable_v<RangeCBeginT, BeginMember const&>);
-static_assert( std::is_invocable_v<RangeCBeginT, BeginMember const&&>);
+static_assert(!std::is_invocable_v<RangeCBeginT, BeginMember const&&>);
 
 constexpr bool testReturnTypes() {
   {
     int *x[2];
     ASSERT_SAME_TYPE(decltype(std::ranges::begin(x)), int**);
     ASSERT_SAME_TYPE(decltype(std::ranges::cbegin(x)), int* const*);
   }
   {

libcxx/test/std/ranges/range.access/end.pass.cpp

 struct Incomplete;
 static_assert(!std::is_invocable_v<RangeEndT, Incomplete(&&)[]>);
 static_assert(!std::is_invocable_v<RangeEndT, Incomplete(&&)[42]>);
 static_assert(!std::is_invocable_v<RangeCEndT, Incomplete(&&)[]>);
 static_assert(!std::is_invocable_v<RangeCEndT, Incomplete(&&)[42]>);
 
 struct EndMember {
   int x;
-  constexpr const int *begin() const { return nullptr; }
+  const int *begin() const;
   constexpr const int *end() const { return &x; }
 };
 
 // Ensure that we can't call with rvalues with borrowing disabled.
-static_assert( std::is_invocable_v<RangeEndT,  EndMember &>);
-static_assert( std::is_invocable_v<RangeEndT,  EndMember const&>);
-static_assert(!std::is_invocable_v<RangeEndT,  EndMember &&>);
+static_assert( std::is_invocable_v<RangeEndT, EndMember &>);
+static_assert(!std::is_invocable_v<RangeEndT, EndMember &&>);
+static_assert( std::is_invocable_v<RangeEndT, EndMember const&>);
+static_assert(!std::is_invocable_v<RangeEndT, EndMember const&&>);
 static_assert( std::is_invocable_v<RangeCEndT, EndMember &>);
+static_assert(!std::is_invocable_v<RangeCEndT, EndMember &&>);
 static_assert( std::is_invocable_v<RangeCEndT, EndMember const&>);
+static_assert(!std::is_invocable_v<RangeCEndT, EndMember const&&>);
 
 constexpr bool testReturnTypes() {
   {
     int *x[2];
     ASSERT_SAME_TYPE(decltype(std::ranges::end(x)), int**);
     ASSERT_SAME_TYPE(decltype(std::ranges::cend(x)), int* const*);
   }
   {