[libc++] Make sure we don't eagerly diagnose non-const comparators for containers of incomplete types

[ldionne]

Summary

In r348529, I improved the library-defined diagnostic for using containers
with a non-const comparator/hasher. However, the check is now performed
too early, which leads to the diagnostic being emitted in cases where it
shouldn't. See PR41360 for details.

This patch moves the diagnostic to the destructor of the containers, which
means that the diagnostic will only be emitted when the container is instantiated
at a point where the comparator and the key/value are required to be complete.
We still retain better diagnostics than before r348529, because the diagnostics
are performed in the containers themselves instead of tree and hash_table.

[ldionne]

libcxx/test/libcxx/containers/unord/non_const_comparator.pass.cpp

41–42 ↗

(On Diff #194593)

Note: those two tests fail, I need to figure out why (tomorrow). I still wanted to get the patch out there to get feedback if there's any.

[EricWF]

The fix LGTM. I'll try to figure out what's going on with the tests

[zoecarver]

libcxx/test/libcxx/containers/unord/non_const_comparator.pass.cpp

41–42 ↗

(On Diff #194593)

Looked into it a bit but can't seem to figure it out either 😕. My guess is that __check_hash_requirements is being called before Derived is completed (somehow).

[EricWF]

libcxx/test/libcxx/containers/unord/non_const_comparator.pass.cpp

41–42 ↗

(On Diff #194593)

The map tests are incorrect. They pass Derived* as the key type and std::less<Base*> as the value type. That might be why they're passing but the set tests are not.

[ldionne]

Fix broken tests.

libcxx/test/libcxx/containers/unord/non_const_comparator.pass.cpp

41–42 ↗

(On Diff #194593)

Wow, thanks. IDK what I was thinking -- this needs to be Container<Key, Value, std::hash<Key>, std::equal_to<Key>>. This was all wrong. The tests pass when I fix that. Like I said, this was the end of a long day!

[EricWF]

It took me a couple of passes to spot it.

LGTM. thanks for the fix

libcxx/trunk/include/__hash_table

#ifndef _LIBCPP_CXX03_LANG
     "the specified comparator is required to be copy constructible");
 #endif
     typedef int type;
 };
 
 template <class _Key, class _Hash, class _Equal>
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Equal const&, _Key const&, _Key const&>::value,
-    "the specified comparator type does not provide a const call operator")
+    "the specified comparator type does not provide a viable const call operator")
     _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Hash const&, _Key const&>::value,
-    "the specified hash functor does not provide a const call operator")
+    "the specified hash functor does not provide a viable const call operator")
 #endif
 typename __enforce_unordered_container_requirements<_Key, _Hash, _Equal>::type
 __diagnose_unordered_container_requirements(int);
 
 // This dummy overload is used so that the compiler won't emit a spurious
 // "no matching function for call to __diagnose_unordered_xxx" diagnostic
 // when the overload above causes a hard error.
 template <class _Key, class _Hash, class _Equal>

libcxx/trunk/include/__tree

private:
     template <class, class, class> friend class _LIBCPP_TEMPLATE_VIS multiset;
     template <class> friend class _LIBCPP_TEMPLATE_VIS __map_const_iterator;
 
 };
 
 template<class _Tp, class _Compare>
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_DIAGNOSE_WARNING(!std::__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
-        "the specified comparator type does not provide a const call operator")
+        "the specified comparator type does not provide a viable const call operator")
 #endif
 int __diagnose_non_const_comparator();
 
 template <class _Tp, class _Compare, class _Allocator>
 class __tree
 {
 public:
     typedef _Tp                                      value_type;

libcxx/trunk/include/map

public:
     typedef _Key                                     key_type;
     typedef _Tp                                      mapped_type;
     typedef pair<const key_type, mapped_type>        value_type;
     typedef _Compare                                 key_compare;
     typedef _Allocator                               allocator_type;
     typedef value_type&                              reference;
     typedef const value_type&                        const_reference;
 
-    static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
     static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
     class _LIBCPP_TEMPLATE_VIS value_compare
         : public binary_function<value_type, value_type, bool>
     {
         friend class map;
     protected:
#endif  // _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     map(const map& __m, const allocator_type& __a)
         : __tree_(__m.__tree_.value_comp(), typename __base::allocator_type(__a))
         {
             insert(__m.begin(), __m.end());
         }
 
     _LIBCPP_INLINE_VISIBILITY
+    ~map() {
+        static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
           iterator begin() _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator begin() const _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
           iterator end() _NOEXCEPT {return __tree_.end();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator end() const _NOEXCEPT {return __tree_.end();}
 
public:
     typedef _Key                                     key_type;
     typedef _Tp                                      mapped_type;
     typedef pair<const key_type, mapped_type>        value_type;
     typedef _Compare                                 key_compare;
     typedef _Allocator                               allocator_type;
     typedef value_type&                              reference;
     typedef const value_type&                        const_reference;
 
-    static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
     static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
     class _LIBCPP_TEMPLATE_VIS value_compare
         : public binary_function<value_type, value_type, bool>
     {
         friend class multimap;
     protected:
#endif  // _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     multimap(const multimap& __m, const allocator_type& __a)
         : __tree_(__m.__tree_.value_comp(), typename __base::allocator_type(__a))
         {
             insert(__m.begin(), __m.end());
         }
 
     _LIBCPP_INLINE_VISIBILITY
+    ~multimap() {
+        static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
           iterator begin() _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator begin() const _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
           iterator end() _NOEXCEPT {return __tree_.end();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator end() const _NOEXCEPT {return __tree_.end();}
 

libcxx/trunk/include/set

public:
     typedef _Key                                     key_type;
     typedef key_type                                 value_type;
     typedef _Compare                                 key_compare;
     typedef key_compare                              value_compare;
     typedef _Allocator                               allocator_type;
     typedef value_type&                              reference;
     typedef const value_type&                        const_reference;
 
-    static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
     static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
 private:
     typedef __tree<value_type, value_compare, allocator_type> __base;
     typedef allocator_traits<allocator_type>                  __alloc_traits;
     typedef typename __base::__node_holder                    __node_holder;
 
set& operator=(set&& __s)
         _NOEXCEPT_(is_nothrow_move_assignable<__base>::value)
         {
             __tree_ = _VSTD::move(__s.__tree_);
             return *this;
         }
 #endif  // _LIBCPP_CXX03_LANG
 
     _LIBCPP_INLINE_VISIBILITY
+    ~set() {
+        static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
           iterator begin() _NOEXCEPT       {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator begin() const _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
           iterator end() _NOEXCEPT         {return __tree_.end();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator end()   const _NOEXCEPT {return __tree_.end();}
 
public:
     typedef _Key                                      key_type;
     typedef key_type                                 value_type;
     typedef _Compare                                  key_compare;
     typedef key_compare                              value_compare;
     typedef _Allocator                                allocator_type;
     typedef value_type&                              reference;
     typedef const value_type&                        const_reference;
 
-    static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
     static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
 private:
     typedef __tree<value_type, value_compare, allocator_type> __base;
     typedef allocator_traits<allocator_type>                  __alloc_traits;
     typedef typename __base::__node_holder                    __node_holder;
 
multiset& operator=(multiset&& __s)
         _NOEXCEPT_(is_nothrow_move_assignable<__base>::value)
         {
             __tree_ = _VSTD::move(__s.__tree_);
             return *this;
         }
 #endif  // _LIBCPP_CXX03_LANG
 
     _LIBCPP_INLINE_VISIBILITY
+    ~multiset() {
+        static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
           iterator begin() _NOEXCEPT       {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator begin() const _NOEXCEPT {return __tree_.begin();}
     _LIBCPP_INLINE_VISIBILITY
           iterator end() _NOEXCEPT         {return __tree_.end();}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator end()   const _NOEXCEPT {return __tree_.end();}
 

libcxx/trunk/include/unordered_map

public:
     typedef _Hash                                          hasher;
     typedef _Pred                                          key_equal;
     typedef _Alloc                                         allocator_type;
     typedef pair<const key_type, mapped_type>              value_type;
     typedef value_type&                                    reference;
     typedef const value_type&                              const_reference;
     static_assert((is_same<value_type, typename allocator_type::value_type>::value),
                   "Invalid allocator::value_type");
-    static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
 
 private:
     typedef __hash_value_type<key_type, mapped_type>                 __value_type;
     typedef __unordered_map_hasher<key_type, __value_type, hasher>   __hasher;
     typedef __unordered_map_equal<key_type, __value_type, key_equal> __key_equal;
     typedef typename __rebind_alloc_helper<allocator_traits<allocator_type>,
                                                  __value_type>::type __allocator_type;
 
#if _LIBCPP_STD_VER > 11
     _LIBCPP_INLINE_VISIBILITY
     unordered_map(initializer_list<value_type> __il, size_type __n, const allocator_type& __a)
       : unordered_map(__il, __n, hasher(), key_equal(), __a) {}
     _LIBCPP_INLINE_VISIBILITY
     unordered_map(initializer_list<value_type> __il, size_type __n, const hasher& __hf, 
       const allocator_type& __a)
       : unordered_map(__il, __n, __hf, key_equal(), __a) {}
 #endif
-    // ~unordered_map() = default;
+    _LIBCPP_INLINE_VISIBILITY
+    ~unordered_map() {
+        static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     unordered_map& operator=(const unordered_map& __u)
     {
 #ifndef _LIBCPP_CXX03_LANG
         __table_ = __u.__table_;
 #else
         if (this != &__u) {
             __table_.clear();
public:
     typedef _Hash                                          hasher;
     typedef _Pred                                          key_equal;
     typedef _Alloc                                         allocator_type;
     typedef pair<const key_type, mapped_type>              value_type;
     typedef value_type&                                    reference;
     typedef const value_type&                              const_reference;
     static_assert((is_same<value_type, typename allocator_type::value_type>::value),
                   "Invalid allocator::value_type");
-    static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
 
 private:
     typedef __hash_value_type<key_type, mapped_type>                 __value_type;
     typedef __unordered_map_hasher<key_type, __value_type, hasher>   __hasher;
     typedef __unordered_map_equal<key_type, __value_type, key_equal> __key_equal;
     typedef typename __rebind_alloc_helper<allocator_traits<allocator_type>,
                                                  __value_type>::type __allocator_type;
 
#if _LIBCPP_STD_VER > 11
     _LIBCPP_INLINE_VISIBILITY
     unordered_multimap(initializer_list<value_type> __il, size_type __n, const allocator_type& __a)
       : unordered_multimap(__il, __n, hasher(), key_equal(), __a) {}
     _LIBCPP_INLINE_VISIBILITY
     unordered_multimap(initializer_list<value_type> __il, size_type __n, const hasher& __hf, 
       const allocator_type& __a)
       : unordered_multimap(__il, __n, __hf, key_equal(), __a) {}
 #endif
-    // ~unordered_multimap() = default;
+    _LIBCPP_INLINE_VISIBILITY
+    ~unordered_multimap() {
+        static_assert(sizeof(__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     unordered_multimap& operator=(const unordered_multimap& __u)
     {
 #ifndef _LIBCPP_CXX03_LANG
         __table_ = __u.__table_;
 #else
         if (this != &__u) {
             __table_.clear();

libcxx/trunk/include/unordered_set

public:
     typedef key_type                                                   value_type;
     typedef _Hash                                                      hasher;
     typedef _Pred                                                      key_equal;
     typedef _Alloc                                                     allocator_type;
     typedef value_type&                                                reference;
     typedef const value_type&                                          const_reference;
     static_assert((is_same<value_type, typename allocator_type::value_type>::value),
                   "Invalid allocator::value_type");
-    static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
 
 private:
     typedef __hash_table<value_type, hasher, key_equal, allocator_type> __table;
 
     __table __table_;
 
 public:
     typedef typename __table::pointer         pointer;
unordered_set(initializer_list<value_type> __il, size_type __n,
                                                       const allocator_type& __a)
         : unordered_set(__il, __n, hasher(), key_equal(), __a) {}
     inline _LIBCPP_INLINE_VISIBILITY
     unordered_set(initializer_list<value_type> __il, size_type __n, 
                                   const hasher& __hf, const allocator_type& __a)
         : unordered_set(__il, __n, __hf, key_equal(), __a) {}
 #endif
 #endif  // _LIBCPP_CXX03_LANG
-    // ~unordered_set() = default;
+    _LIBCPP_INLINE_VISIBILITY
+    ~unordered_set() {
+        static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     unordered_set& operator=(const unordered_set& __u)
     {
         __table_ = __u.__table_;
         return *this;
     }
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
public:
     typedef key_type                                                   value_type;
     typedef _Hash                                                      hasher;
     typedef _Pred                                                      key_equal;
     typedef _Alloc                                                     allocator_type;
     typedef value_type&                                                reference;
     typedef const value_type&                                          const_reference;
     static_assert((is_same<value_type, typename allocator_type::value_type>::value),
                   "Invalid allocator::value_type");
-    static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
 
 private:
     typedef __hash_table<value_type, hasher, key_equal, allocator_type> __table;
 
     __table __table_;
 
 public:
     typedef typename __table::pointer         pointer;
#if _LIBCPP_STD_VER > 11
     inline _LIBCPP_INLINE_VISIBILITY
     unordered_multiset(initializer_list<value_type> __il, size_type __n, const allocator_type& __a)
       : unordered_multiset(__il, __n, hasher(), key_equal(), __a) {}
     inline _LIBCPP_INLINE_VISIBILITY
     unordered_multiset(initializer_list<value_type> __il, size_type __n, const hasher& __hf, const allocator_type& __a)
       : unordered_multiset(__il, __n, __hf, key_equal(), __a) {}
 #endif
 #endif  // _LIBCPP_CXX03_LANG
-    // ~unordered_multiset() = default;
+    _LIBCPP_INLINE_VISIBILITY
+    ~unordered_multiset() {
+        static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     unordered_multiset& operator=(const unordered_multiset& __u)
     {
         __table_ = __u.__table_;
         return *this;
     }
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY

libcxx/trunk/test/libcxx/containers/associative/non_const_comparator.fail.cpp

bool operator()(T const& t, U const& u) {
     return t < u;
   }
 };
 
 int main(int, char**) {
   static_assert(!std::__invokable<BadCompare const&, int const&, int const&>::value, "");
   static_assert(std::__invokable<BadCompare&, int const&, int const&>::value, "");
 
-  // expected-warning@set:* 2 {{the specified comparator type does not provide a const call operator}}
-  // expected-warning@map:* 2 {{the specified comparator type does not provide a const call operator}}
+  // expected-warning@set:* 2 {{the specified comparator type does not provide a viable const call operator}}
+  // expected-warning@map:* 2 {{the specified comparator type does not provide a viable const call operator}}
   {
     using C = std::set<int, BadCompare>;
     C s;
   }
   {
     using C = std::multiset<long, BadCompare>;
     C s;
   }

libcxx/trunk/test/libcxx/containers/associative/non_const_comparator.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03
+// REQUIRES: verify-support
+
+// Test that libc++ does not generate a warning diagnostic about the comparator
+// too early for containers of incomplete types.
+//
+// See PR41360.
+
+#include <set>
+#include <map>
+#include <functional>
+
+
+template <template <typename ...> class Container>
+void test_set() {
+  struct KeyBase { };
+  struct KeyDerived;  // derives from KeyBase, but incomplete at this point
+
+  // Name the type but don't instantiate it.
+  using C = Container<KeyDerived*, std::less<KeyBase*>>;
+
+  // Instantiate it but don't ODR use any members.
+  typename C::value_type dummy; (void)dummy;
+
+  // Complete the types.
+  struct KeyDerived : KeyBase { };
+
+  C c; // ODR use it, which should be OK
+}
+
+template <template <typename ...> class Container>
+void test_map() {
+  struct Value { };
+  struct KeyBase { };
+  struct KeyDerived;
+  using C = Container<KeyDerived*, Value, std::less<KeyBase*>>;
+  typename C::value_type dummy; (void)dummy;
+  struct KeyDerived : KeyBase { };
+  C c;
+}
+
+int main(int, char**) {
+  // expected-no-diagnostics
+  test_set<std::set>();
+  test_set<std::multiset>();
+  test_map<std::map>();
+  test_map<std::multimap>();
+
+  return 0;
+}

libcxx/trunk/test/libcxx/containers/unord/non_const_comparator.fail.cpp

bool operator()(T const& t, U const& u) {
     return t == u;
   }
 };
 
 int main(int, char**) {
   static_assert(!std::__invokable<BadEqual const&, int const&, int const&>::value, "");
   static_assert(std::__invokable<BadEqual&, int const&, int const&>::value, "");
 
-  // expected-warning@unordered_set:* 2 {{the specified comparator type does not provide a const call operator}}
-  // expected-warning@unordered_map:* 2 {{the specified comparator type does not provide a const call operator}}
-  // expected-warning@unordered_set:* 2 {{the specified hash functor does not provide a const call operator}}
-  // expected-warning@unordered_map:* 2 {{the specified hash functor does not provide a const call operator}}
+  // expected-warning@unordered_set:* 2 {{the specified comparator type does not provide a viable const call operator}}
+  // expected-warning@unordered_map:* 2 {{the specified comparator type does not provide a viable const call operator}}
+  // expected-warning@unordered_set:* 2 {{the specified hash functor does not provide a viable const call operator}}
+  // expected-warning@unordered_map:* 2 {{the specified hash functor does not provide a viable const call operator}}
 
   {
     using C = std::unordered_set<int, BadHash, BadEqual>;
     C s;
   }
   {
     using C = std::unordered_multiset<long, BadHash, BadEqual>;
     C s;

libcxx/trunk/test/libcxx/containers/unord/non_const_comparator.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03
+// REQUIRES: verify-support
+
+// Test that libc++ does not generate a warning diagnostic about the comparator
+// or the hasher too early for containers of incomplete types.
+//
+// See PR41360.
+
+#include <unordered_set>
+#include <unordered_map>
+#include <functional>
+
+
+template <template <typename ...> class Container>
+void test_set() {
+  struct KeyBase { };
+  struct KeyDerived;  // derives from KeyBase, but incomplete at this point
+
+  // Name the type but don't instantiate it.
+  using C = Container<KeyDerived*, std::hash<KeyBase*>, std::equal_to<KeyBase*>>;
+
+  // Instantiate it but don't ODR use any members.
+  typename C::value_type dummy; (void)dummy;
+
+  // Complete the types.
+  struct KeyDerived : KeyBase { };
+
+  C c; // ODR use it, which should be OK
+}
+
+template <template <typename ...> class Container>
+void test_map() {
+  struct Value { };
+  struct KeyBase { };
+  struct KeyDerived;
+  using C = Container<KeyDerived*, Value, std::hash<KeyBase*>, std::equal_to<KeyBase*>>;
+  typename C::value_type dummy; (void)dummy;
+  struct KeyDerived : KeyBase { };
+  C c;
+}
+
+int main(int, char**) {
+  // expected-no-disagnostics
+  test_set<std::unordered_set>();
+  test_set<std::unordered_multiset>();
+  test_map<std::unordered_map>();
+  test_map<std::unordered_multimap>();
+
+  return 0;
+}