[libc++] Disallow specializing common_reference

[frederick-vs-ja]

common_reference isn't an exception for [meta.rqmts]/4, so it's better to disallow users to specialize it.

indirectly_readable.compile.pass.cpp was a bit problematic. It attempted to opt-out common reference type in some wrong ways. Also, the standard effectively forbids opting-out common reference type for T& and T&&. This patch removes and adjusts some problematic cases.

[frederick-vs-ja]

I'm moving basic_common_reference here, because it's a standard library component and it's a bit weird to me to leave it in an "implementation detail code section".

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

common_reference isn't an exception for [meta.rqmts]/4, so it's better to disallow users to specialize it.

indirectly_readable.compile.pass.cpp was a bit problematic. It attempted to opt-out common reference type in some wrong ways. Also, the standard effectively forbids opting-out common reference type for T& and T&&. This patch removes and adjusts some problematic cases.

---

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

3 Files Affected:

• (modified) libcxx/include/__type_traits/common_reference.h (+10-5)
• (modified) libcxx/test/libcxx/type_traits/no_specializations.verify.cpp (+7)
• (modified) libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp (+5-26)

diff --git a/libcxx/include/__type_traits/common_reference.h b/libcxx/include/__type_traits/common_reference.h
index c27da5251b9b7..a2e360405ecf6 100644
--- a/libcxx/include/__type_traits/common_reference.h
+++ b/libcxx/include/__type_traits/common_reference.h
@@ -109,11 +109,18 @@ struct __common_ref {};
 // Note C: For the common_reference trait applied to a parameter pack [...]
 
 template <class...>
-struct common_reference;
+struct _LIBCPP_NO_SPECIALIZATIONS common_reference;
 
 template <class... _Types>
 using common_reference_t = typename common_reference<_Types...>::type;
 
+template <class, class, template <class> class, template <class> class>
+struct basic_common_reference {};
+
+_LIBCPP_DIAGNOSTIC_PUSH
+#  if __has_warning("-Winvalid-specialization")
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winvalid-specialization")
+#  endif
 // bullet 1 - sizeof...(T) == 0
 template <>
 struct common_reference<> {};
@@ -145,9 +152,6 @@ struct __common_reference_sub_bullet1<_Tp, _Up> {
 
 // sub-bullet 2 - Otherwise, if basic_common_reference<remove_cvref_t<T1>, remove_cvref_t<T2>, XREF(T1), XREF(T2)>::type
 // is well-formed, then the member typedef `type` denotes that type.
-template <class, class, template <class> class, template <class> class>
-struct basic_common_reference {};
-
 template <class _Tp, class _Up>
 using __basic_common_reference_t _LIBCPP_NODEBUG =
     typename basic_common_reference<remove_cvref_t<_Tp>,
@@ -180,10 +184,11 @@ struct __common_reference_sub_bullet3 : common_type<_Tp, _Up> {};
 template <class _Tp, class _Up, class _Vp, class... _Rest>
   requires requires { typename common_reference_t<_Tp, _Up>; }
 struct common_reference<_Tp, _Up, _Vp, _Rest...> : common_reference<common_reference_t<_Tp, _Up>, _Vp, _Rest...> {};
+_LIBCPP_DIAGNOSTIC_POP
 
 // bullet 5 - Otherwise, there shall be no member `type`.
 template <class...>
-struct common_reference {};
+struct _LIBCPP_NO_SPECIALIZATIONS common_reference{};
 
 #endif // _LIBCPP_STD_VER >= 20
 
diff --git a/libcxx/test/libcxx/type_traits/no_specializations.verify.cpp b/libcxx/test/libcxx/type_traits/no_specializations.verify.cpp
index 38560161f162e..897ae89365014 100644
--- a/libcxx/test/libcxx/type_traits/no_specializations.verify.cpp
+++ b/libcxx/test/libcxx/type_traits/no_specializations.verify.cpp
@@ -186,5 +186,12 @@ struct std::enable_if<true, S>; // expected-error {{cannot be specialized}}
 #  if TEST_STD_VER >= 20
 template <>
 struct std::integral_constant<S, {}>; // expected-error {{cannot be specialized}}
+
+template <>
+struct std::common_reference<S>; // expected-error {{cannot be specialized}}
+template <>
+struct std::common_reference<S, S>; // expected-error {{cannot be specialized}}
+template <>
+struct std::common_reference<S, S, S>; // expected-error {{cannot be specialized}}
 #  endif
 #endif
diff --git a/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp b/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
index 5fc6ffd37caf1..086e79b799c36 100644
--- a/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
+++ b/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.readable/indirectly_readable.compile.pass.cpp
@@ -78,16 +78,10 @@ struct missing_iter_value_t {
 };
 static_assert(!check_indirectly_readable<missing_iter_value_t>());
 
-struct unrelated_lvalue_ref_and_rvalue_ref {};
-
-struct iter_ref1 {};
-template <>
-struct std::common_reference<iter_ref1&, iter_ref1&&> {};
-
-template <>
-struct std::common_reference<iter_ref1&&, iter_ref1&> {};
-
-static_assert(!std::common_reference_with<iter_ref1&, iter_ref1&&>);
+struct iter_ref1 {
+  iter_ref1(const iter_ref1&) = delete;
+  iter_ref1(iter_ref1&&)      = delete;
+};
 
 struct bad_iter_reference_t {
   using value_type = int;
@@ -128,24 +122,9 @@ struct different_reference_types_with_common_reference {
 static_assert(check_indirectly_readable<different_reference_types_with_common_reference>());
 
 struct iter_ref4 {
-  operator iter_rvalue_ref() const;
+  operator iter_rvalue_ref();
 };
 
-template <template <class> class XQual, template <class> class YQual>
-struct std::basic_common_reference<iter_ref4, iter_rvalue_ref, XQual, YQual> {
-  using type = iter_rvalue_ref;
-};
-template <template <class> class XQual, template <class> class YQual>
-struct std::basic_common_reference<iter_rvalue_ref, iter_ref4, XQual, YQual> {
-  using type = iter_rvalue_ref;
-};
-
-// FIXME: This is UB according to [meta.rqmts], and there is no exception for common_reference.
-template <>
-struct std::common_reference<iter_ref4 const&, iter_rvalue_ref&&> {};
-template <>
-struct std::common_reference<iter_rvalue_ref&&, iter_ref4 const&> {};
-
 static_assert(std::common_reference_with<iter_ref4&&, iter_rvalue_ref&&>);
 static_assert(!std::common_reference_with<iter_ref4 const&, iter_rvalue_ref&&>);
 

[ldionne]

LGTM but can you add a release note since this may break some code?

[frederick-vs-ja]

LGTM but can you add a release note since this may break some code?

We didn't add release note in #118167. If release note is desired, perhaps it's better to also add one to 20.rst.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ldionne]

LGTM but can you add a release note since this may break some code?

We didn't add release note in #118167. If release note is desired, perhaps it's better to also add one to 20.rst.

Looking quickly through #118167, it seems that we only added [[no_specializations]] to templates that were very uncontroversial, i.e. where the likelihood of a user specializing the template was very low. Since common_type is intended to be specialized, I have a feeling that common_reference might be more commonly specialized too (by mistake). That's why I think this one could use a release note. WDYT?

[frederick-vs-ja]

LGTM but can you add a release note since this may break some code?

We didn't add release note in #118167. If release note is desired, perhaps it's better to also add one to 20.rst.

Looking quickly through #118167, it seems that we only added [[no_specializations]] to templates that were very uncontroversial, i.e. where the likelihood of a user specializing the template was very low. Since common_type is intended to be specialized, I have a feeling that common_reference might be more commonly specialized too (by mistake). That's why I think this one could use a release note. WDYT?

Agreed. I added release note just for common_reference.