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

Fri Oct 29 16:40:51 PDT 2021

jloser added inline comments.

================
Comment at: libcxx/include/__ranges/view_interface.h:65
     return static_cast<_Derived const&>(*this);
   }

----------------
jloser wrote:
> Quuxplusone wrote:
> > 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... 😛
> 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.
> 
@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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112665