[libc++] No longer support ranges::begin(x) when x is an array of incomplete type.

[• Quuxplusone]

Summary

var-const points out that ranges::begin is (non-normatively but explicitly) always supposed to return a std::input_or_output_iterator, and Incomplete* is not a std::input_or_output_iterator because it has no operator++. Therefore, we should never return Incomplete* from ranges::begin(x), even when x is Incomplete(&)[]. Instead, just SFINAE away.

(The note in question: https://eel.is/c++draft/range.access.begin#4 "Whenever ranges​::​begin(E) is a valid expression, its type models input_­or_­output_­iterator.")

[• Quuxplusone]

libcxx/include/__ranges/access.h

73

The use of _Tp (&__t)[] here instead of is_array_v is both for symmetry with ranges::end, and because we need to make sure that we never try to evaluate *__t before we're absolutely sure that __t is an array type (because if it's a class type, that'll do ADL and we don't want ADL).
__t + 0 could be just __t AFAIK, but the standard says __t + 0 and I can't see any way for that to hurt anything.

[var-const]

Thanks!

[var-const]

libcxx/include/__ranges/access.h

62

Question: end only defines the case where the array size is known. Does the case where the size is unknown need to be handled by end, or does it not apply in that case?

[var-const]

libcxx/include/__ranges/access.h

73

Question: why do we need to do sizeof(*__t) rather than sizeof(_Tp)?

[philnik]

libcxx/include/__ranges/access.h

62

Couldn't we just say input_or_output_iterator auto operator()?

[• Quuxplusone]

libcxx/include/__ranges/access.h

62

When the array size is unknown, i.e. we're asking for the ranges::end of an int[], well, we have no idea where that end would be! :) So ranges::end(int[]) is ill-formed https://eel.is/c++draft/range.access.end#2.3 (note: not even IFNDR).

62

Couldn't we just say input_or_output_iterator auto operator()?

That would have caught this bug, in the sense that it would have caused hard errors in some of our tests. (Remember a constrained return type is basically a static_assert.) However, as a matter of QoI I'd still prefer not to pay for that constraint check on every single instantiation of ranges::begin.
I did add the constrained return type locally and confirmed that all our tests pass now.

The other awkward thing about constrained auto return types is that, AFAICT, it is impossible to use both a constrained return type and a trailing -> decltype(...) return type, which means that we wouldn't be able to use constrained return types at all if we moved to a "write it three times" style like we currently do in ranges::cbegin, or a "write it four times" style like I propose in D115607.

FYI, ranges::end has a similar "kinda normative note," that its return type always satisfies sentinel_for<iterator_t<_Tp>> https://eel.is/c++draft/range.access.end#4

73

Good point, will fix. On the left-hand side, we needed *__t because _Tp& __t referred to the whole array. Now that _Tp refers to the element type, sizeof(_Tp) should be fine.

[• Quuxplusone]

sizeof(_Tp)

[ldionne]

This should be cherry-picked to 14.x

libcxx/include/__ranges/access.h

concept __unqualified_begin =
     __can_borrow<_Tp> &&
     __class_or_enum<remove_cvref_t<_Tp>> &&
     requires(_Tp && __t) {
       { _LIBCPP_AUTO_CAST(begin(__t)) } -> input_or_output_iterator;
     };
 
   struct __fn {
     template <class _Tp>
-      requires is_array_v<remove_cv_t<_Tp>>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const noexcept
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[]) const noexcept
+      requires (sizeof(*__t) != 0)  // Disallow incomplete element types.
+    {
+      return __t + 0;
+    }
+
+    template <class _Tp, size_t _Np>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
+      requires (sizeof(*__t) != 0)  // Disallow incomplete element types.
     {
-      return __t;
+      return __t + 0;
     }
 
     template <class _Tp>
       requires __member_begin<_Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(__t.begin())))
     {
       return _LIBCPP_AUTO_CAST(__t.begin());
     }

libcxx/test/libcxx/ranges/range.access/begin.incomplete_type.sh.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// RUN: %{cxx} %{flags} %{compile_flags} -c %s -o %t.tu1.o -DTU1
-// RUN: %{cxx} %{flags} %{compile_flags} -c %s -o %t.tu2.o -DTU2
-// RUN: %{cxx} %t.tu1.o %t.tu2.o %{flags} %{link_flags} -o %t.exe
-// RUN: %{exec} %t.exe
-
-// UNSUPPORTED: c++03, c++11, c++14, c++17
-// UNSUPPORTED: libcpp-no-concepts
-// UNSUPPORTED: libcpp-has-no-incomplete-ranges
-
-// Test the libc++-specific behavior that we handle the IFNDR case for ranges::begin
-// by returning the beginning of the array-of-incomplete-type.
-// Use two translation units so that `Incomplete` really is never completed
-// at any point within TU2, but the array `bounded` is still given a definition
-// (in TU1) to avoid an "undefined reference" error from the linker.
-// All of the actually interesting stuff takes place within TU2.
-
-#include <ranges>
-#include <cassert>
-
-#include "test_macros.h"
-
-#if defined(TU1)
-
-struct Incomplete {};
-Incomplete bounded[10];
-Incomplete unbounded[10];
-
-#else // defined(TU1)
-
-struct Incomplete;
-
-constexpr bool test()
-{
-    {
-    extern Incomplete bounded[10];
-    assert(std::ranges::begin(bounded) == bounded);
-    assert(std::ranges::cbegin(bounded) == bounded);
-    assert(std::ranges::begin(std::as_const(bounded)) == bounded);
-    assert(std::ranges::cbegin(std::as_const(bounded)) == bounded);
-    ASSERT_SAME_TYPE(decltype(std::ranges::begin(bounded)), Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::cbegin(bounded)), const Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::begin(std::as_const(bounded))), const Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::cbegin(std::as_const(bounded))), const Incomplete*);
-    }
-    {
-    extern Incomplete unbounded[];
-    assert(std::ranges::begin(unbounded) == unbounded);
-    assert(std::ranges::cbegin(unbounded) == unbounded);
-    assert(std::ranges::begin(std::as_const(unbounded)) == unbounded);
-    assert(std::ranges::cbegin(std::as_const(unbounded)) == unbounded);
-    ASSERT_SAME_TYPE(decltype(std::ranges::begin(unbounded)), Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::cbegin(unbounded)), const Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::begin(std::as_const(unbounded))), const Incomplete*);
-    ASSERT_SAME_TYPE(decltype(std::ranges::cbegin(std::as_const(unbounded))), const Incomplete*);
-    }
-
-    return true;
-}
-
-int main(int, char**)
-{
-    test();
-    static_assert(test());
-    return 0;
-}
-
-#endif // defined(TU1)

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

 
 static_assert(!std::is_invocable_v<RangeBeginT, int (&&)[10]>);
 static_assert( std::is_invocable_v<RangeBeginT, int (&)[10]>);
 static_assert(!std::is_invocable_v<RangeBeginT, int (&&)[]>);
 static_assert( std::is_invocable_v<RangeBeginT, int (&)[]>);
 
 struct Incomplete;
 static_assert(!std::is_invocable_v<RangeBeginT, Incomplete(&&)[]>);
-static_assert(!std::is_invocable_v<RangeBeginT, Incomplete(&&)[42]>);
+static_assert(!std::is_invocable_v<RangeBeginT, const Incomplete(&&)[]>);
 static_assert(!std::is_invocable_v<RangeCBeginT, Incomplete(&&)[]>);
-static_assert(!std::is_invocable_v<RangeCBeginT, Incomplete(&&)[42]>);
+static_assert(!std::is_invocable_v<RangeCBeginT, const Incomplete(&&)[]>);
+
+static_assert(!std::is_invocable_v<RangeBeginT, Incomplete(&&)[10]>);
+static_assert(!std::is_invocable_v<RangeBeginT, const Incomplete(&&)[10]>);
+static_assert(!std::is_invocable_v<RangeCBeginT, Incomplete(&&)[10]>);
+static_assert(!std::is_invocable_v<RangeCBeginT, const Incomplete(&&)[10]>);
+
+// This case is IFNDR; we handle it SFINAE-friendly.
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeBeginT, Incomplete(&)[]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeBeginT, const Incomplete(&)[]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeCBeginT, Incomplete(&)[]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeCBeginT, const Incomplete(&)[]>);
+
+// This case is IFNDR; we handle it SFINAE-friendly.
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeBeginT, Incomplete(&)[10]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeBeginT, const Incomplete(&)[10]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeCBeginT, Incomplete(&)[10]>);
+LIBCPP_STATIC_ASSERT(!std::is_invocable_v<RangeCBeginT, const Incomplete(&)[10]>);
 
 struct BeginMember {
   int x;
   constexpr const int *begin() const { return &x; }
 };
 
 // Ensure that we can't call with rvalues with borrowing disabled.
 static_assert( std::is_invocable_v<RangeBeginT, BeginMember &>);