[libc++] [ranges] Simplify our idiom for testing niebloid-ness.

[• Quuxplusone]

Summary

In the test files, replace the old-style tests with a simple static_assert, matching the current style as depicted in e.g. ranges_uninitialized_default_construct.pass.cpp.

Preserve is_function_like (but renamed to is_niebloid) at @ldionne's request. The removal of this test helper will happen in D116570 if at all.

[ldionne]

The testing simplifications make sense to me, but removing __function_like does mean that we weaken our implementation w.r.t. Hyrum's law. I don't remember coming to a consensus on removing that part (and I would most likely object to that).

[• Quuxplusone]

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp

18–21

@ldionne wrote:

The testing simplifications make sense to me, but removing __function_like does mean that we weaken our implementation w.r.t. Hyrum's law. I don't remember coming to a consensus on removing that part (and I would most likely object to that).

(1) It's physically possible to land the libcxx/test/ changes without the libcxx/include/ changes. Shall I do that? (and then leave this PR open for discussion of the libcxx/include/ changes)

(2) Re Hyrum's Law: You have to keep in mind that "there's no such thing as an archetype." C++ has only leaky abstractions. Here we have to choose, for each niebloid in the library, should it be copyable or non-copyable? final or non-final? default-initializable or non-default-initializable? We cannot avoid choosing one or the other. Whichever one we choose, the user might (accidentally or on purpose) end up relying on it. They'll have some template that does one thing if the type is copyable and a different thing if it's not, and then boom, technically we can't change that without an ABI break.
The paper standard saves us here by making most of those problematic things IFNDR, so we actually can change this stuff without worrying about the breakage to users. Therefore I think our goal should be simplicity and predictability: we should do the simplest thing that works, because complexity without reason is bad.

FWIW, today, GCC accepts and MSVC rejects https://godbolt.org/z/hhae7zEf9

std::ranges::transform(v, w.begin(), std::ranges::next);

Everyone (including libc++) accepts things like

(void) std::identity()(std::ranges::next);
std::optional<decltype(std::ranges::next)> o;
std::function<int*(int*)> f = std::ref(std::ranges::next);

These are all IFNDR as far as I'm aware: the user is supposed to know that std::ranges::next (despite being a niebloid) isn't supposed to be used in these ways. This is basically analogous to the stuff that appears in SD-8: "sure you can try doing this, but the library vendor doesn't have to coddle you and you're on your own if it breaks later."

[• Quuxplusone]

Add missing "test_macros.h".

[ldionne]

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp

18–21

(1) It's physically possible to land the libcxx/test/ changes without the libcxx/include/ changes. Shall I do that? (and then leave this PR open for discussion of the libcxx/include/ changes)

Yes, please. Ideally, this review could contain the testing changes only and then we can argue on another review whether to keep __function_like or not (see below).

(2) Re Hyrum's Law: [...]

I understand your point, and I agree that __function_like isn't perfect. However, in my opinion, the comment in function_like.h summarizes it nicely:

Since these are still standard library functions, we use __function_like to eliminate most of
the properties that function objects get by default (e.g. semiregularity, addressability), to
limit the surface area of the unintended public interface, so as to curb the effect of Hyrum's
law.

The goal isn't to be perfect, it is to limit the API surface that users can unknowingly start depending on. If that leads to too much implementation divergence on properties of those niebloids (e.g. libc++ isn't copyable but libstdc++ is), and if all implementations end up using classes to implement niebloids, then the Standard should just say it and not make it IFNDR. But weakening our implementation's strictness is not something I support unless it has a strong technical benefit associated (like your other patch for removing static_asserts for incomplete types inside ranges::begin).

[• Quuxplusone]

Defer the code diff; this is now just the test diffs.

[libc++] [ranges] Simplify our idiom for testing niebloid-ness.

In the test files, replace the old-style tests with a simple static_assert,
matching the current style as depicted in e.g.
`ranges_uninitialized_default_construct.pass.cpp`.

[ldionne]

libcxx/test/support/test_standard_function.h

1

We should keep this, but make this a libc++ specific helper. We could also rename it is_niebloid.

[• Quuxplusone]

Update per discussion with @ldionne today. The plan is to keep is_niebloid (née is_function_like) for now; defer its removal until D116570; and make sure to open an LWG issue complaining about the status quo before making the final-final decision on D116570.

If CI is green, imma land this.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // ranges::advance
 
 #include <iterator>
 
-#include "test_standard_function.h"
+#include "test_macros.h"
 
-static_assert(is_function_like<decltype(std::ranges::advance)>());
-
-// FIXME: We're bending the rules here by adding a new type to namespace std::ranges. Since this is
-// the standard library's test suite, this should be fine (we *are* the implementation), but it's
-// necessary at the time of writing since there aren't any iterators in std::ranges that we can
-// borrow for this test.
-namespace std::ranges {
-class fake_forward_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::forward_iterator_tag;
-
-  fake_forward_iterator() = default;
-
-  value_type operator*() const;
-  fake_forward_iterator& operator++();
-  fake_forward_iterator operator++(int);
-
-  bool operator==(fake_forward_iterator const&) const = default;
-};
-} // namespace std::ranges
-
-template <class I, class... Args>
-constexpr bool unqualified_lookup_works = requires(I i, Args... args) {
-  advance(i, args...);
-};
-
-static_assert(!unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ptrdiff_t>);
-static_assert(!unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ranges::fake_forward_iterator>);
-static_assert(
-    !unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ptrdiff_t, std::ranges::fake_forward_iterator>);
-
-namespace test {
-template <class>
-class forward_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::forward_iterator_tag;
-
-  forward_iterator() = default;
-
-  value_type operator*() const;
-  forward_iterator& operator++();
-  forward_iterator operator++(int);
-
-  bool operator==(forward_iterator const&) const = default;
-};
-
-template <class I>
-void advance(forward_iterator<I>&, std::ptrdiff_t) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void advance(forward_iterator<I>&, forward_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void advance(forward_iterator<I>&, std::ptrdiff_t, forward_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-} // namespace test
-
-// TODO(varconst): simply check that `advance` is a variable and not a function.
-// When found by unqualified ([basic.lookup.unqual]) name lookup for the postfix-expression in a
-// function call ([expr.call]), they inhibit argument-dependent name lookup.
-void adl_inhibition() {
-  test::forward_iterator<int*> x;
-
-  using std::ranges::advance;
-  advance(x, 0);
-  advance(x, x);
-  advance(x, 0, x);
-}
+// Because this is a variable and not a function, it's guaranteed that ADL won't be used. However,
+// implementations are allowed to use a different mechanism to achieve this effect, so this check is
+// libc++-specific.
+LIBCPP_STATIC_ASSERT(std::is_class_v<decltype(std::ranges::advance)>);

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/special_function.compile.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // ranges::next
 
 #include <iterator>
 
-#include "test_standard_function.h"
+#include "test_macros.h"
 
-static_assert(is_function_like<decltype(std::ranges::next)>());
-
-// FIXME: We're bending the rules here by adding a new type to namespace std::ranges. Since this is
-// the standard library's test suite, this should be fine (we *are* the implementation), but it's
-// necessary at the time of writing since there aren't any iterators in std::ranges that we can
-// borrow for this test.
-namespace std::ranges {
-class fake_forward_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::forward_iterator_tag;
-
-  fake_forward_iterator() = default;
-
-  value_type operator*() const;
-  fake_forward_iterator& operator++();
-  fake_forward_iterator operator++(int);
-
-  bool operator==(fake_forward_iterator const&) const = default;
-};
-} // namespace std::ranges
-
-// The function templates defined in [range.iter.ops] are not found by argument-dependent name lookup ([basic.lookup.argdep]).
-template <class I, class... Args>
-constexpr bool unqualified_lookup_works = requires(I i, Args... args) {
-  next(i, args...);
-};
-
-static_assert(!unqualified_lookup_works<std::ranges::fake_forward_iterator>);
-static_assert(!unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ptrdiff_t>);
-static_assert(!unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ranges::fake_forward_iterator>);
-static_assert(
-    !unqualified_lookup_works<std::ranges::fake_forward_iterator, std::ptrdiff_t, std::ranges::fake_forward_iterator>);
-
-namespace test {
-template <class>
-class forward_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::forward_iterator_tag;
-
-  forward_iterator() = default;
-
-  value_type operator*() const;
-  forward_iterator& operator++();
-  forward_iterator operator++(int);
-
-  bool operator==(forward_iterator const&) const = default;
-};
-
-template <class I>
-void next(forward_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void next(forward_iterator<I>, std::ptrdiff_t) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void next(forward_iterator<I>, forward_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void next(forward_iterator<I>, std::ptrdiff_t, forward_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-} // namespace test
-
-// TODO(varconst): simply check that `next` is a variable and not a function.
-// When found by unqualified ([basic.lookup.unqual]) name lookup for the postfix-expression in a
-// function call ([expr.call]), they inhibit argument-dependent name lookup.
-void adl_inhibition() {
-  test::forward_iterator<int*> x;
-
-  using std::ranges::next;
-
-  (void)next(x);
-  (void)next(x, 5);
-  (void)next(x, x);
-  (void)next(x, 6, x);
-}
+// Because this is a variable and not a function, it's guaranteed that ADL won't be used. However,
+// implementations are allowed to use a different mechanism to achieve this effect, so this check is
+// libc++-specific.
+LIBCPP_STATIC_ASSERT(std::is_class_v<decltype(std::ranges::next)>);

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/special_function.compile.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // ranges::prev
 
 #include <iterator>
 
-#include "test_iterators.h"
-#include "test_standard_function.h"
+#include "test_macros.h"
 
-static_assert(is_function_like<decltype(std::ranges::prev)>());
-
-namespace std::ranges {
-class fake_bidirectional_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::bidirectional_iterator_tag;
-
-  fake_bidirectional_iterator() = default;
-
-  value_type operator*() const;
-  fake_bidirectional_iterator& operator++();
-  fake_bidirectional_iterator operator++(int);
-  fake_bidirectional_iterator& operator--();
-  fake_bidirectional_iterator operator--(int);
-
-  bool operator==(fake_bidirectional_iterator const&) const = default;
-};
-} // namespace std::ranges
-
-// The function templates defined in [range.iter.ops] are not found by argument-dependent name lookup ([basic.lookup.argdep]).
-template <class I, class... Args>
-constexpr bool unqualified_lookup_works = requires(I i, Args... args) {
-  prev(i, args...);
-};
-
-static_assert(!unqualified_lookup_works<std::ranges::fake_bidirectional_iterator>);
-static_assert(!unqualified_lookup_works<std::ranges::fake_bidirectional_iterator, std::ptrdiff_t>);
-static_assert(!unqualified_lookup_works<std::ranges::fake_bidirectional_iterator, std::ptrdiff_t,
-                                        std::ranges::fake_bidirectional_iterator>);
-
-namespace test {
-template <class>
-class bidirectional_iterator {
-public:
-  using value_type = int;
-  using difference_type = std::ptrdiff_t;
-  using iterator_category = std::bidirectional_iterator_tag;
-
-  bidirectional_iterator() = default;
-
-  value_type operator*() const;
-  bidirectional_iterator& operator++();
-  bidirectional_iterator operator++(int);
-  bidirectional_iterator& operator--();
-  bidirectional_iterator operator--(int);
-
-  bool operator==(bidirectional_iterator const&) const = default;
-};
-
-template <class I>
-void prev(bidirectional_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void prev(bidirectional_iterator<I>, std::ptrdiff_t) {
-  static_assert(std::same_as<I, I*>);
-}
-
-template <class I>
-void prev(bidirectional_iterator<I>, std::ptrdiff_t, bidirectional_iterator<I>) {
-  static_assert(std::same_as<I, I*>);
-}
-} // namespace test
-
-// TODO(varconst): simply check that `prev` is a variable and not a function.
-// When found by unqualified ([basic.lookup.unqual]) name lookup for the postfix-expression in a
-// function call ([expr.call]), they inhibit argument-dependent name lookup.
-void adl_inhibition() {
-  test::bidirectional_iterator<int*> x;
-
-  using std::ranges::prev;
-
-  (void)prev(x);
-  (void)prev(x, 5);
-  (void)prev(x, 6, x);
-}
+// Because this is a variable and not a function, it's guaranteed that ADL won't be used. However,
+// implementations are allowed to use a different mechanism to achieve this effect, so this check is
+// libc++-specific.
+LIBCPP_STATIC_ASSERT(std::is_class_v<decltype(std::ranges::prev)>);

libcxx/test/support/test_standard_function.h

-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-#ifndef LIBCXX_TEST_SUPPORT_TEST_STANDARD_FUNCTION_H
-#define LIBCXX_TEST_SUPPORT_TEST_STANDARD_FUNCTION_H
-
-#include "test_macros.h"
-
-#if TEST_STD_VER >= 20
-template <class T>
-constexpr bool is_addressable = requires(T t) {
-  &t;
-};
-
-template <class T>
-constexpr bool is_function_like() {
-  using X = std::remove_cvref_t<T>;
-  static_assert(!is_addressable<X>);
-  static_assert(!is_addressable<X const>);
-
-  static_assert(std::destructible<X> && !std::default_initializable<X>);
-
-  static_assert(!std::move_constructible<X>);
-  static_assert(!std::assignable_from<X&, X>);
-
-  static_assert(!std::copy_constructible<X>);
-  static_assert(!std::assignable_from<X&, X const>);
-  static_assert(!std::assignable_from<X&, X&>);
-  static_assert(!std::assignable_from<X&, X const&>);
-  static_assert(std::is_final_v<X>);
-  return true;
-}
-#endif
-
-#endif // LIBCXX_TEST_SUPPORT_TEST_STANDARD_FUNCTION_H