[libcxx-commits] [PATCH] D112665: [libc++] Ensure valid view for view_interface template parameter

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__ranges/view_interface.h:52
   constexpr _Derived const& __derived() const noexcept {
+    static_assert(sizeof(_Derived) && derived_from<_Derived, view_interface<_Derived>> && view<_Derived>);
     return static_cast<_Derived const&>(*this);
----------------
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.


================
Comment at: libcxx/include/__ranges/view_interface.h:65
     return static_cast<_Derived const&>(*this);
   }
 
----------------
jloser wrote:
> Quuxplusone wrote:
> > jloser wrote:
> > > Quuxplusone wrote:
> > > > > 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.)
> > > 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>> {
> > > ```
> > 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?
> 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.
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... 😛


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112665/new/

https://reviews.llvm.org/D112665