[libc++] Ensure valid view for view_interface template parameter

[jloser]

Summary

Some types that inherit from view_interface do not meet the
preconditions. This came up during discussion
in https://reviews.llvm.org/D112631. Currently, the behavior is IFNDR,
but the preconditions can be easily checked, so let's do so.

In particular, we know each public member function calls the
__derived() private function, so we can do the check there. We
intentionally do it as a static_assert instead of a requires clause
to avoid hard erroring in some cases, such as with incomplete types. An
example hard error is:

llvm-project/build/include/c++/v1/__ranges/view_interface.h:48:14: note: because 'sizeof(_Tp)' would be invalid: invalid application of 'sizeof' to an incomplete type 'MoveOnlyForwardRange'
  requires { sizeof(_Tp); } &&
             ^
llvm-project/build/include/c++/v1/__ranges/view_interface.h:73:26: error: no matching member function for call to '__derived'
    return ranges::begin(__derived()) == ranges::end(__derived());
                         ^~~~~~~~~
llvm-project/libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:187:31: note: in instantiation of function template specialization 'std::ranges::view_interface<MoveOnlyForwardRange>::empty<Mov
eOnlyForwardRange>' requested here
  assert(!std::move(moveOnly).empty());

[jloser]

Rename helper concept to be less generic in its identifier

[• Quuxplusone]

LGTM % (significant) comments, but I'm sure there will be some discussion.

libcxx/include/__ranges/view_interface.h

45–50

Flashbacks to "Concepts As She Is Spoke"! :) Remember that view<_Tp> is always well-formed. Sometimes its runtime boolean value will be "false," but it's always well-formed (and has type bool). What you meant was

template<class _Tp>
concept __valid_view_for_view_interface =
  view<_Tp> && derived_from<_Tp, view_interface<_Tp>> && requires { sizeof(_Tp); };

However, I really don't think this should be a concept, because it depends on a property (completeness) that changes over the lifetime of the TU. I think you should just inline this into the one place it's used: that static_assert.

Collateral bonuses:

• no need to forward-declare view_interface on line 43
• probably no need to include the second argument to static_assert
• optionally (and I'd prefer it), use the injected-class-name derived_from<_Derived, view_interface> instead of derived_from<_Derived, view_interface<_Derived>>

54

We intentionally do it as a static_assert instead of a requires clause to avoid hard erroring in some cases.

And also because this is an implementation detail: not libc++ policy but certainly my own policy, is that we can cruft up the public API (with noexcept, requires, [[nodiscard]], etc) when the Standard forces us to, but for the private details, "behind the firewall" as it were, we should prefer simple and fast and generic.

These cases include checking a particular concept that delegates to a type trait that hard errors when passed an incomplete type.

I don't understand this sentence at all. Are you thinking of a particular concept in particular? 😛 If you can craft an example program that fails when __derived() is constrained but succeeds when __derived() is unconstrained, then please add it as a test case. (Which will also forestall future relitigation of this issue, because then we'll have a test case that fails if someone tries it.)

[cjdb]

libcxx/include/__ranges/view_interface.h

45–50

The original order is important: we want to check for completeness first, then the cheapest independent check, and so on. But yes, only the former should be in the requires clause.

template<class _Tp>
concept __valid_view_for_view_interface =
  requires { sizeof(_Tp); } &&
  derived_from<_Tp, view_interface<_Tp>> &&
  view<_Tp>;

[jloser]

Remove concept and inline the static assert use

[jloser]

In D112665#3091724, @Quuxplusone wrote:

LGTM % (significant) comments, but I'm sure there will be some discussion.

libcxx/include/__ranges/view_interface.h

45–50

Yikes, you're right. I meant what you typed.

I removed the concept (and completeness check) in favor of inlining it as you suggested.

54

This may just be an obvious error on my half, so I recommend you try it out. But if you take the equivalent boolean expression in the static_assert and naively turn it into a requires for the __derived() member functions, you'll get a hard error in the view.interface.pass existing test. I think this makes sense to me since it's hard erroring in evaluating derived_from concept with an incomplete type. An example error is:

/Users/joe/dev_local/llvm-project/build/include/c++/v1/type_traits:1661:59: error: incomplete type 'BoolConvertibleComparison<false>' used in type trait expression
    : public integral_constant<bool, __is_base_of(_Bp, _Dp)> {};
                                                          ^
/Users/joe/dev_local/llvm-project/build/include/c++/v1/type_traits:1665:38: note: in instantiation of template class 'std::is_base_of<std::ranges::view_interface<BoolConvertibleComparison<false>>, BoolConvertibleComparison<false>>' requested here
inline constexpr bool is_base_of_v = is_base_of<_Bp, _Dp>::value;
                                     ^
/Users/joe/dev_local/llvm-project/build/include/c++/v1/__concepts/derived_from.h:27:3: note: in instantiation of variable template specialization 'std::is_base_of_v<std::ranges::view_interface<BoolConvertibleComparison<false>>, BoolConvertibleComparison<false>>' requested here
  is_base_of_v<_Bp, _Dp> &&
  ^
/Users/joe/dev_local/llvm-project/build/include/c++/v1/__concepts/derived_from.h:27:3: note: while substituting template arguments into constraint expression here
  is_base_of_v<_Bp, _Dp> &&
  ^~~~~~~~~~~~~~~~~~~~~~
/Users/joe/dev_local/llvm-project/build/include/c++/v1/__ranges/view_interface.h:45:71: note: while checking the satisfaction of concept 'derived_from<BoolConvertibleComparison<false>, std::ranges::view_interface<BoolConvertibleComparison<false>>>' requested here
  constexpr _Derived& __derived() noexcept requires view<_Derived> && derived_from<_Derived, view_interface> {
                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/joe/dev_local/llvm-project/libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:127:36: note: in instantiation of template class 'std::ranges::view_interface<BoolConvertibleComparison<false>>' requested here
struct BoolConvertibleComparison : std::ranges::view_interface<BoolConvertibleComparison<IsNoexcept>> {

[jloser]

libcxx/include/__ranges/view_interface.h

45–50

Agree regarding order and checking completeness first. I removed the concept in the latest iteration (and also don't check for completeness). What do you think, @cjdb (concept vs inline, etc)?

[• Quuxplusone]

libcxx/include/__ranges/view_interface.h

54

Ah. IIUC (which I still may not), that'll be fixed by @cjdb's recommendation to put requires { sizeof(_Tp); } first and derived_from<...> afterward.
If you do that, then are there any failures to avoid?

[jloser]

Check completeness in concept.
Explain hard error with incomplete types if the concept is used in a requires clause

[jloser]

libcxx/include/__ranges/view_interface.h

54

It's still an issue. With the concept (even with it checking for completeness using sizeof first), when you convert the use from static_assert to requires, you will get a hard error for incomplete types when the type is passed to sizeof when we're checking concept constraints. This is a bit unintuitive to me. Does this make sense to you or @cjdb? See the updated commit message for an example error.

[jloser]

[NFC] Format the concept a bit nicer

[jloser]

Remove the concept yet again in favor of inlining, except we check for completeness now still

[Mordante]

LGTM module my two issues. I'll approve so @Quuxplusone can give the final approval after the remaining issues are solved.

libcxx/include/__ranges/view_interface.h

46

Two points:

• I think it would be good to add comment why you use requires { sizeof(_Derived); } instead of sizeof(_Derived);
• I don't feel the message of the static_assert adds a lot of value. It basically says the same as the code, except that the code requires a complete type and the message doesn't mention it. I would prefer to remove the entire message.

[jloser]

Remove requires clause for completeness check
Remove message from static_assert

[• Quuxplusone]

libcxx/include/__ranges/view_interface.h

52

I'd still weakly prefer

static_assert(sizeof(_Derived) && derived_from<_Derived, view_interface> && view<_Derived>);

but, if you commit it as-is, I won't care enough to change it.

54

Okay, I (assume I) have reproduced the issue: https://godbolt.org/z/YE7Mbnc6x
It smells very much like a Clang concepts bug: Clang is evaluating the constraints on member function (non-template) declarations eagerly, whereas GCC and MSVC are deferring them until they see the first use of that specific member function. I assume the majority are correct and Clang is non-conforming, which means cjdb's suggestion is intended to work; it just doesn't work in practice on Clang (so we still can't use it).

Btw, it has occurred to me that the value of view<T> also changes over the course of a translation unit: it's generally false at class-definition time, and then becomes true a few lines later after we see a specialization of enable_view. (https://godbolt.org/z/YE7Mbnc6x shows something isomorphic to this.) I'm not sure if this means there's no problem with a complete<T> concept after all, or that everything in Ranges is a huge fragile house of cards... 😛

[cjdb]

libcxx/include/__ranges/view_interface.h

45–50

What do you think, @cjdb (concept vs inline, etc)?

I like names that express intent.

[jloser]

libcxx/include/__ranges/view_interface.h

52

I landed it with your suggestion using just view_interface. I slightly prefer it the way you wrote it. One pro for the way I originally wrote it is the fact that it matches the wording in the standard "exactly" rather than relying on lookup.

54

That looks to be about right for the repro linked -- pretty nice minimal! Want me to file a clang bug, or did you already do so?

Good observation regarding the value of a concept changing over the lifetime of a TU. In general, any concept written in terms of *something* that can be specialized (such as a variable template) suffers this "problem". I say problem in quotes since it may not be a problem in reality. I'd need to think more about it; we should talk offline about it.

As far as ranges goes, I'd need to look through all the ranges section of the standard to identity cases where things rely on complete types without explicitly saying so. In the case here of view.interface.general, the standard explicitly talks about incomplete vs complete types:

The template parameter D for view_­interface may be an incomplete type. Before any member of the resulting specialization of view_­interface other than special member functions is referenced, D shall be complete, and model both derived_­from<view_­interface<D>> and view.

[jloser]

libcxx/include/__ranges/view_interface.h

54

@Quuxplusone I think this is related to why most members functions of view_interface have a defaulted template parameter that defaults to _Derived: to workaround this Clang bug. The standard doesn't say back(), front(), etc. of view_interface should be member function templates. I just tried removing the defaulted template argument and Clang failed pretty bad. :)

An example workaround to your Godbolt that uses this technique makes things consistently pass across all compilers by working around the Clang bug: https://godbolt.org/z/f6qeWYTYG

libcxx/include/__ranges/view_interface.h

 
 template<class _Tp>
 void __implicitly_convert_to(type_identity_t<_Tp>) noexcept;
 
 template<class _Derived>
   requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
 class view_interface : public view_base {
   _LIBCPP_HIDE_FROM_ABI
   constexpr _Derived& __derived() noexcept {
+    static_assert(sizeof(_Derived) && derived_from<_Derived, view_interface> && view<_Derived>);
     return static_cast<_Derived&>(*this);
   }
 
   _LIBCPP_HIDE_FROM_ABI
   constexpr _Derived const& __derived() const noexcept {
+    static_assert(sizeof(_Derived) && derived_from<_Derived, view_interface> && view<_Derived>);
     return static_cast<_Derived const&>(*this);
   }
 
 public:
   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());