[libc++] Uniformly require complete types in vector member functions

[ldionne]

The Standard allows vector's value type to be incomplete when instantiating the vector class, but it requires it to be complete before instantiating any member function. Libc++ would produce an error for most functions, but it would silently work for a few methods like empty() (however only in the non size-based vector).

To avoid portability traps for users, uniformly require complete types under all configurations for all vector methods.

See also #210754

[ldionne]

CC @nico

[llvmorg-github-actions]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

The Standard allows vector's value type to be incomplete when instantiating the vector class, but it requires it to be complete before instantiating any member function. Libc++ would produce an error for most functions, but it would silently work for a few methods like empty() (however only in the non size-based vector).

To avoid portability traps for users, uniformly require complete types under all configurations for all vector methods.

See also #210754

---

Full diff: https://github.com/llvm/llvm-project/pull/211110.diff

7 Files Affected:

• (modified) libcxx/docs/ReleaseNotes/24.rst (+4)
• (modified) libcxx/include/CMakeLists.txt (+1)
• (added) libcxx/include/__utility/require_complete.h (+42)
• (modified) libcxx/include/__vector/vector.h (+17)
• (modified) libcxx/include/module.modulemap.in (+1)
• (removed) libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp (-27)
• (added) libcxx/test/libcxx/containers/sequences/vector/incomplete_type.verify.cpp (+30)

diff --git a/libcxx/docs/ReleaseNotes/24.rst b/libcxx/docs/ReleaseNotes/24.rst
index 70c704ff9d326..612fb0d03d0b2 100644
--- a/libcxx/docs/ReleaseNotes/24.rst
+++ b/libcxx/docs/ReleaseNotes/24.rst
@@ -57,6 +57,10 @@ Potentially breaking changes
   but causes programs which rely on these includes to not compile anymore. The ``_LIBCPP_KEEP_TRANSITIVE_INCLUDES_LLVM23``
   macro that was provided in LLVM 23 to ease the transition has been removed in this release.
 
+- Libc++ now diagnoses when a method of ``std::vector<T>`` is used and ``T`` is an incomplete type. The Standard requires
+  ``T`` to be complete before any member function is used, however this worked unreliably for a few member functions. In
+  LLVM 24, this is diagnosed uniformly.
+
 Announcements About Future Releases
 -----------------------------------
 
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index b40f586161e62..3f7be893a4450 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -953,6 +953,7 @@ set(files
   __utility/priority_tag.h
   __utility/private_constructor_tag.h
   __utility/rel_ops.h
+  __utility/require_complete.h
   __utility/scope_guard.h
   __utility/small_buffer.h
   __utility/swap.h
diff --git a/libcxx/include/__utility/require_complete.h b/libcxx/include/__utility/require_complete.h
new file mode 100644
index 0000000000000..5cb9ea74b500b
--- /dev/null
+++ b/libcxx/include/__utility/require_complete.h
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 _LIBCPP___UTILITY_REQUIRE_COMPLETE_H
+#define _LIBCPP___UTILITY_REQUIRE_COMPLETE_H
+
+#include <__config>
+#include <__cstddef/size_t.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template <class _Tp, size_t = sizeof(_Tp)>
+_LIBCPP_CONSTEXPR void __require_complete_impl(int) {}
+
+template <class _Tp, bool _False = false>
+_LIBCPP_CONSTEXPR void __require_complete_impl(long) {
+  static_assert(_False, "Type is required to be complete");
+}
+
+// Produce a compiler error if the given type is not complete.
+template <class _Tp>
+_LIBCPP_CONSTEXPR void __require_complete() {
+  std::__require_complete_impl<_Tp>(0);
+}
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___UTILITY_REQUIRE_COMPLETE_H
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 5e9fa4a7d0030..6749ae1aacc03 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -65,6 +65,7 @@
 #include <__utility/is_pointer_in_range.h>
 #include <__utility/move.h>
 #include <__utility/pair.h>
+#include <__utility/require_complete.h>
 #include <__utility/swap.h>
 #include <initializer_list>
 #include <limits>
@@ -348,43 +349,55 @@ class vector {
   // Iterators
   //
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator begin() _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __make_iter(__add_alignment_assumption(this->__layout_.__begin_ptr()));
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator begin() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __make_iter(__add_alignment_assumption(this->__layout_.__begin_ptr()));
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator end() _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __make_iter(__add_alignment_assumption(__layout_.__end_ptr()));
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator end() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __make_iter(__add_alignment_assumption(__layout_.__end_ptr()));
   }
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reverse_iterator rbegin() _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return reverse_iterator(end());
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reverse_iterator
   rbegin() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return const_reverse_iterator(end());
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reverse_iterator rend() _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return reverse_iterator(begin());
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reverse_iterator rend() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return const_reverse_iterator(begin());
   }
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator cbegin() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return begin();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator cend() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return end();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reverse_iterator
   crbegin() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return rbegin();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reverse_iterator crend() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return rend();
   }
 
@@ -392,16 +405,20 @@ class vector {
   // [vector.capacity], capacity
   //
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __layout_.__size();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type capacity() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __layout_.__capacity();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool empty() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return __layout_.__empty();
   }
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type max_size() const _NOEXCEPT {
+    std::__require_complete<_Tp>();
     return std::min<size_type>(__alloc_traits::max_size(__layout_.__alloc()), numeric_limits<difference_type>::max());
   }
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void reserve(size_type __n);
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 39b4e0bb986c6..3a2615da37505 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -2229,6 +2229,7 @@ module std {
     module priority_tag                    { header "__utility/priority_tag.h" }
     module private_constructor_tag         { header "__utility/private_constructor_tag.h" }
     module rel_ops                         { header "__utility/rel_ops.h" }
+    module require_complete                { header "__utility/require_complete.h" }
     module scope_guard                     { header "__utility/scope_guard.h" }
     module small_buffer                    { header "__utility/small_buffer.h" }
     module swap                            { header "__utility/swap.h" }
diff --git a/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp b/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp
deleted file mode 100644
index 029d29eb437f8..0000000000000
--- a/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp
+++ /dev/null
@@ -1,27 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// <vector>
-
-// This test pins down the current libc++ behavior that vector<T>::empty() can be
-// called even when T is an incomplete type. The standard does not require this:
-// [vector.overview] only guarantees that an incomplete type may be used to
-// instantiate vector, and requires the type to be complete before any method is
-// called.
-//
-// However, libc++ made that work previously, and this test pins down that behavior
-// to avoid breaking it unintentionally. Note that this is not a guarantee to users
-// that we will support this in the future: this merely guards against changing this
-// behavior unknowingly.
-
-#include <vector>
-
-struct Incomplete;
-
-bool call_empty(std::vector<Incomplete>& v) { return v.empty(); }
-bool call_empty_const(const std::vector<Incomplete>& v) { return v.empty(); }
diff --git a/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.verify.cpp b/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.verify.cpp
new file mode 100644
index 0000000000000..caabd64c3b279
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.verify.cpp
@@ -0,0 +1,30 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <vector>
+
+// This test ensures that we diagnose when an incomplete type is used in one of
+// vector's methods. The Standard requires that to be the case, and we want to
+// uniformly produce an error for that. Note that producing the same diagnostic
+// in all cases is difficult, but we at least want to fail to fight back against
+// Hyrum's law.
+
+#include <vector>
+
+struct Incomplete;
+
+void f(std::vector<Incomplete>& v) {
+  (void)v.empty();  // expected-error@*:* {{}}
+  (void)v.size();   // expected-error@*:* {{}}
+  (void)v.begin();  // expected-error@*:* {{}}
+  (void)v.end();    // expected-error@*:* {{}}
+  (void)v.cbegin(); // expected-error@*:* {{}}
+  (void)v.cend();   // expected-error@*:* {{}}
+
+  // etc for other APIs
+}

[ldionne]

This is more difficult to test than I anticipated.

This test currently happens to pass because we produce errors, but the errors are not all static asserts. In fact, most of them are other, non static_assert errors.

Furthermore, if I remove the __require_complete call from empty() (which is arguably the only one that needs something), this test still passes because some of these statements produce more than one error.

So, I'm not certain how to best test this. One test per file would technically work, but there's no way I can convince myself to do that.

[ldionne]

Another question is which methods should we test here. Some methods like push_back(T const&) clearly already imply a complete T, so I don't think there's a point in testing those. I'm not sure where we should draw the line.

[philnik777]

#211231 is an alternative to this approach, which also improves diagnostics of incomplete types in general. Assuming the Clang folks are happy with it I think that's the superior approach.