[libc++] Don't require complete types in vector<T>::empty()

[ldionne]

This was previously not required, but the patch to introduce a new size-based vector layout unintentionally added this new requirement. We almost certainly not want to promise this guarantee going forward, but we should actually land this change explicitly and consider the transition story, not do it as a fallout of another refactoring.

Fixes #210732

[llvmorg-github-actions]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This was previously not required, but the patch to introduce a new size-based vector layout unintentionally added this new requirement. We might or might not want to promise this guarantee going forward, but we should make an explicit decision, not as a fallout of another refactoring.

Fixes #210732

---

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

3 Files Affected:

• (modified) libcxx/include/__vector/layout.h (+11)
• (modified) libcxx/include/__vector/vector.h (+1-1)
• (added) libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp (+27)

diff --git a/libcxx/include/__vector/layout.h b/libcxx/include/__vector/layout.h
index 3318a13a8ede1..af03556dc2636 100644
--- a/libcxx/include/__vector/layout.h
+++ b/libcxx/include/__vector/layout.h
@@ -199,6 +199,7 @@ class __vector_layout {
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __size() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __capacity() const _NOEXCEPT;
+  [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool __empty() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer __end_ptr() _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_pointer __end_ptr() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer __capacity_ptr() _NOEXCEPT;
@@ -313,6 +314,11 @@ __vector_layout<_Tp, _Alloc>::__capacity() const _NOEXCEPT {
   return __capacity_;
 }
 
+template <class _Tp, class _Alloc>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__empty() const _NOEXCEPT {
+  return __size_ == 0;
+}
+
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename __vector_layout<_Tp, _Alloc>::pointer
 __vector_layout<_Tp, _Alloc>::__end_ptr() _NOEXCEPT {
@@ -425,6 +431,11 @@ __vector_layout<_Tp, _Alloc>::__capacity() const _NOEXCEPT {
   return static_cast<size_type>(__capacity_ - __begin_);
 }
 
+template <class _Tp, class _Alloc>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__empty() const _NOEXCEPT {
+  return __begin_ == __end_;
+}
+
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename __vector_layout<_Tp, _Alloc>::pointer
 __vector_layout<_Tp, _Alloc>::__end_ptr() _NOEXCEPT {
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 8226a7f87a119..5e9fa4a7d0030 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -398,7 +398,7 @@ class vector {
     return __layout_.__capacity();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool empty() const _NOEXCEPT {
-    return size() == 0;
+    return __layout_.__empty();
   }
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type max_size() const _NOEXCEPT {
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
new file mode 100644
index 0000000000000..029d29eb437f8
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/vector/incomplete_type.compile.pass.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// 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(); }

[philnik777]

We might or might not want to promise this guarantee going forward, but we should make an explicit decision, not as a fallout of another refactoring.

I really disagree here. Unless we make explicit guarantees I don't think we should need to consider every time whether we break something users could rely on. I understand that vector is a very commonly used type and we want to ease the transition, but I don't think "it happened to work before" is enough to require a decision to be made. I don't think this would be the response for any somewhat less commonly used type. As such, I don't think an all-out revert is the appropriate response, but rather an opt-out flag that we remove after a while, like we do for other similar things.

[ldionne]

We might or might not want to promise this guarantee going forward, but we should make an explicit decision, not as a fallout of another refactoring.

I really disagree here. Unless we make explicit guarantees I don't think we should need to consider every time whether we break something users could rely on. I understand that vector is a very commonly used type and we want to ease the transition, but I don't think "it happened to work before" is enough to require a decision to be made. I don't think this would be the response for any somewhat less commonly used type.

I think the type in question is actually at the core of the issue here. Indeed, we might not make a big fuss if this happened to e.g. condition_variable or something less commonly used. But:

• We changed this behavior unknowingly, and
• it impacts one of the most widely used types in the library, and
• a user has complained about it, meaning it does have impact, and
• importantly, this is happening right before a release, and a release where we already made several breaking changes.

If we want to do it, we should definitely de-risk this and land it as a separate intentional change, in LLVM 24.

In general, adhering to a strict interpretation of the Standard is fine, but that's not a reason to ship breaking changes bundled with other non-intended-breaking changes once we know about it.

As such, I don't think an all-out revert is the appropriate response, but rather an opt-out flag that we remove after a while, like we do for other similar things.

We shipped this change unknowingly. This reverts to the previous state (which is uncontroversial), so that we can then have a proper and non-rushed discussion about how we want to deal with it. What I'd do:

1. Land this, put the fire out.
2. Discuss whether we want to provide this as an extension (I don't think we do).
3. Discuss how we want to actually make the breakage (e.g. diagnose?)
4. Discuss whether we want this to break loud in all ABI configurations
5. Then, actually land that, potentially with an escape hatch for one release

I think we're mostly aligned -- but what I want is to revert to a known good state first so we can carry the above steps without a fire burning.

[philnik777]

For the record, I just talked with Louis, and with the wording change I'm happy.

[ldionne]

/cherry-pick 17ac8fd

[nico]

If you reland the behavior change, please land it so that it changes independent of _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED. At the moment, before this PR, we only required complete types with _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED turned off.

[llvmbot]

/pull-request #211026