[libcxx-commits] [libcxx] [libcxx] improves diagnostics for containers with bad value types (PR #106296)

[Aaron Ballman via libcxx-commits]

================
@@ -635,8 +640,136 @@ void __forward_list_base<_Tp, _Alloc>::clear() _NOEXCEPT {
   __before_begin()->__next_ = nullptr;
 }
 
+// begin-diagnostic-helpers
+// std::forward_list can only have non-cv-qualified object types as its value type, which we diagnose.
+// In order to short-cirucit redundant (and cryptic) diagnostics, std::forward_list must be the one
+// to fire the static_assert. Since std::forward_list inherits from __forward_list_base, we also need
+// to create specialisations for the rejected types, so that everything appears "good" to std::forward_list
+// (otherwise we'll get lots of unhelpful diagnostics that suppress the one std::forward_list offers).
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp const, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp volatile, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&&, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class... _Args, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp(_Args...), _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<void, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+// end-diagnostic-helpers
+
 template <class _Tp, class _Alloc /*= allocator<_Tp>*/>
 class _LIBCPP_TEMPLATE_VIS forward_list : private __forward_list_base<_Tp, _Alloc> {
+  static_assert(!is_const<_Tp>::value, "'std::forward_list' can only hold non-const types");
----------------
AaronBallman wrote:

> I've very often thought to myself that the diagnostics quality would be greatly improved if we just had -ferror-limit=1 be the default.

Personal opinion: I think that's a worse user experience in a lot of cases; much of the follow-on diagnostic problems are language-specific (C++ and templates are not known for excellent diagnostic behavior). Trickling out the errors one at a time may be reasonable when doing many quick edit/recompile cycles with small changes where it's unlikely to add multiple errors, but may not be reasonable for someone throwing a giant pile of code at the compiler one time. I think it would be more palatable to take a bunch of cases where we're emitting too many follow-on diagnostics to see if we can identify a pattern, and reduce the number of follow-ons in that specific case. e.g., we may decide "oh, class template instantiation issue? Let's just bail out early instead of marking the node as invalid and continuing".

Lead maintainer opinion: that would have to go through an RFC to see whether the community would accept it or not.

https://github.com/llvm/llvm-project/pull/106296