[Bug libstdc++/51558] New: Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[Bug libstdc++/51558] New: Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[jyasskin at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

             Bug #: 51558
           Summary: Declaration of unspecialized
                    std::hash<_Tp>::operator()(_Tp) turns compile-time
                    errors into link-time errors
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jyasskin@gcc.gnu.org


Created attachment 26090
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26090
Declare std::hash<T> instead of defining it.

libstdc++'s current definition of the unspecialized std::hash template gives
bad error messages when a user forgets to define a hash function for their
type. Specifically, providing a declaration but no definition of operator()
moves the error from compile to link time:

$ cat test.cc
#include <unordered_set>
struct MyStruct {
  int i;
};
bool operator==(const MyStruct& lhs, const MyStruct& rhs) {
  return lhs.i == rhs.i;
}
int main() {
  std::unordered_set<MyStruct> s;
  s.insert(MyStruct{3});
}
$ g++ -g -std=c++11 test.cc -o test
/tmp/cclzhwaU.o: In function `std::__detail::_Hash_code_base<MyStruct,
MyStruct, std::_Identity<MyStruct>, std::equal_to<MyStruct>,
std::hash<MyStruct>, std::__detail::_Mod_range_hashing,
std::__detail::_Default_ranged_hash, true>::_M_hash_code(MyStruct const&)
const':
.../include/c++/4.7.0/bits/hashtable_policy.h:702: undefined reference to
`std::hash<MyStruct>::operator()(MyStruct) const'
collect2: error: ld returned 1 exit status

With the attached patch, the error is much more informative, if not exactly
concise:

$ g++ -g -std=c++11 test.cc -o test
In file included from .../include/c++/4.7.0/bits/hashtable.h:35:0,
                 from .../include/c++/4.7.0/unordered_set:45,
                 from test.cc:1:
.../include/c++/4.7.0/bits/hashtable_policy.h: In instantiation of ‘struct
std::__detail::_Hash_code_base<MyStruct, MyStruct, std::_Identity<MyStruct>,
std::equal_to<MyStruct>, std::hash<MyStruct>,
std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, true>’:
.../include/c++/4.7.0/bits/hashtable.h:149:11:   required from ‘class
std::_Hashtable<MyStruct, MyStruct, std::allocator<MyStruct>,
std::_Identity<MyStruct>, std::equal_to<MyStruct>, std::hash<MyStruct>,
std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash,
std::__detail::_Prime_rehash_policy, true, true, true>’
.../include/c++/4.7.0/bits/unordered_set.h:46:11:   required from ‘class
std::__unordered_set<MyStruct, std::hash<MyStruct>, std::equal_to<MyStruct>,
std::allocator<MyStruct>, true>’
.../include/c++/4.7.0/bits/unordered_set.h:277:11:   required from ‘class
std::unordered_set<MyStruct>’
test.cc:9:32:   required from here
.../include/c++/4.7.0/bits/hashtable_policy.h:740:20: error:
‘std::__detail::_Hash_code_base<_Key, _Value, _ExtractKey, _Equal, _H1, _H2,
std::__detail::_Default_ranged_hash, true>::_M_h1’ has incomplete type
...

In particular, the "required from here" line points at the actual source
location that needs to be able to find the definition.


I believe the patch is allowed by C++11 since I can't find a specification of
the contents of the unspecialized template, and libc++ uses basically the same
technique:
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__functional_base?view=markup.

Another way to accomplish something similar would be to delete the operator()
declaration from the std::hash<T> definition. I believe that's not as good
because it produces error messages saying that std::hash lacks an operator()
rather than that the template has incomplete type. Better yet would be finding
a way to include "please specialize me" in the error message.

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-14 22:57:11 UTC ---
Could we put a static_assert in the default operator() definition?

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-12-15
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
     Ever Confirmed|0                           |1

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 01:38:11 UTC ---
For a while, back in tr1 times, we definitely had just a declaration for the
primary hash. Then at some point things changed, I can't remember in which
circumstance. If nothing breaks I think we can certainly go back to it. I'll
regtest the patchlet.

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot
                   |                            |com

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 01:44:34 UTC ---
Evidently the patchlet has not been tested ;) The library doesn't even build
because we have code in src/ specializing the operator which would have to be
adjusted.

All in all, I guess what Jon suggested should not lead to worse error messages,
and seems very easily doable and safe. Jon, would you like to suggest the text
of the static_assert?

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-12-15 02:01:24 UTC ---
Would the condition of the static_assert need to depend on the template
parameter somehow, to avoid the case where no valid specialization can ever be
generated? (Which would be ill-formed, no diagnostic required, by [temp.res]
p8.)

e.g.

static_assert( integral_constant<int, sizeof(_Tp)>::value,
  "std::hash must be specialized for this type" )

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 02:24:08 UTC ---
Uhmm, I don't know, probably yes. If we are in doubt we can as well use what
I'm going to attach, which resolves the src/ fallback. Which change leads to
the best error messages?

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 02:25:03 UTC ---
Created attachment 26098
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26098
Completed patch

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
   Target Milestone|---                         |4.7.0

--- Comment #7 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 10:52:00 UTC ---
The static_assert definitely improves the error message. I'm going to prepare a
combined patch (testcases in 23_containers/unordered_* have also to be tweaked)

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

--- Comment #8 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2011-12-15 22:15:28 UTC ---
Author: paolo
Date: Thu Dec 15 22:15:21 2011
New Revision: 182392

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182392
Log:
2011-12-15  Paolo Carlini  <paolo.carlini@oracle.com>
        Jonathan Wakely  <jwakely.gcc@gmail.com>

    PR libstdc++/51558
    * include/bits/functional_hash.h (struct hash): Add static_assert.
    * src/compatibility-c++0x.cc: Adjust compatibility definitions.
    * testsuite/23_containers/unordered_map/erase/51142.cc: Adjust.
    * testsuite/23_containers/unordered_set/erase/51142.cc: Likewise.
    * testsuite/23_containers/unordered_multimap/erase/51142.cc: Likewise.
    * testsuite/23_containers/unordered_multiset/erase/51142.cc: Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/functional_hash.h
    trunk/libstdc++-v3/src/compatibility-c++0x.cc
    trunk/libstdc++-v3/testsuite/23_containers/unordered_map/erase/51142.cc
   
trunk/libstdc++-v3/testsuite/23_containers/unordered_multimap/erase/51142.cc
   
trunk/libstdc++-v3/testsuite/23_containers/unordered_multiset/erase/51142.cc
    trunk/libstdc++-v3/testsuite/23_containers/unordered_set/erase/51142.cc

[Bug libstdc++/51558] Declaration of unspecialized std::hash<_Tp>::operator()(_Tp) turns compile-time errors into link-time errors

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-12-15 22:16:27 UTC ---
Done.