Fix hashing behavior

Our previous default behavior was incorrect.  Adding __hash__ has to be
deliberate action and only makes sense for immutable classes.

By default, hash now mirrors eq which the correct behavior.

Fixes #136.

CHANGELOG.rst

@@ -14,7 +14,12 @@ Changes:
 Backward-incompatible changes:
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-*none*
+- ``attrs`` will set the ``__hash__`` method to ``None`` by default now.
+  The way hashes were handled before was in conflict with `Python's specification <https://docs.python.org/3/reference/datamodel.html#object.__hash__>`_.
+  This *may* break some software although this breakage is most likely just surfacing of latent bugs.
+  You can always make ``attrs`` create the ``__hash__`` method using ``@attr.s(hash=True)``.
+  See `#136 <https://github.com/hynek/attrs/issues/136>`_ for the rationale of this change.
+- Correspondingly, ``attr.ib``'s ``hash`` argument is ``None`` by default too and mirrors the ``cmp`` argument as it should.
 
 
 Deprecations:
@@ -26,6 +31,9 @@ Deprecations:
 Changes:
 ^^^^^^^^
 
+- Fix hashing behavior by setting ``__hash__`` to ``None`` by default and mirror ``cmp`` and ``hash`` in attributes.
+  `#136 <https://github.com/hynek/attrs/issues/136>`_
+  `#142 <https://github.com/hynek/attrs/issues/142>`_
 - Add ``attr.evolve`` that, given an instance of an ``attrs`` class and field changes as keyword arguments, will instantiate a copy of the given instance with the changes applied.
   ``evolve`` replaces ``assoc``, which is now deprecated.
   ``evolve`` is significantly faster than ``assoc``, and requires the class have an initializer that can take the field values as keyword arguments (like ``attrs`` itself can generate).

docs/api.rst

@@ -18,7 +18,7 @@ What follows is the API explanation, if you'd like a more hands-on introduction,
 Core
 ----
 
-.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=True, hash=True, init=True, slots=False, frozen=False, str=False)
+.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=True, hash=None, init=True, slots=False, frozen=False, str=False)
 
    .. note::
 
@@ -71,7 +71,7 @@ Core
       ... class C(object):
       ...     x = attr.ib()
       >>> C.x
-      Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({}))
+      Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({}))
 
 
 .. autofunction:: attr.make_class
@@ -127,9 +127,9 @@ Helpers
       ...     x = attr.ib()
       ...     y = attr.ib()
       >>> attr.fields(C)
-      (Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({})), Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({})))
+      (Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({})), Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({})))
       >>> attr.fields(C)[1]
-      Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({}))
+      Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({}))
       >>> attr.fields(C).y is attr.fields(C)[1]
       True
 
@@ -217,7 +217,7 @@ See :ref:`asdict` for examples.
       >>> attr.validate(i)
       Traceback (most recent call last):
          ...
-      TypeError: ("'x' must be <type 'int'> (got '1' that is a <type 'str'>).", Attribute(name='x', default=NOTHING, validator=<instance_of validator for type <type 'int'>>, repr=True, cmp=True, hash=True, init=True), <type 'int'>, '1')
+      TypeError: ("'x' must be <type 'int'> (got '1' that is a <type 'str'>).", Attribute(name='x', default=NOTHING, validator=<instance_of validator for type <type 'int'>>, repr=True, cmp=True, hash=None, init=True), <type 'int'>, '1')
 
 
 Validators can be globally disabled if you want to run them only in development and tests but not in production because you fear their performance impact:
@@ -254,7 +254,7 @@ Validators
       >>> C(None)
       Traceback (most recent call last):
          ...
-      TypeError: ("'x' must be <type 'int'> (got None that is a <type 'NoneType'>).", Attribute(name='x', default=NOTHING, validator=<instance_of validator for type <type 'int'>>, repr=True, cmp=True, hash=True, init=True), <type 'int'>, None)
+      TypeError: ("'x' must be <type 'int'> (got None that is a <type 'NoneType'>).", Attribute(name='x', default=NOTHING, validator=<instance_of validator for type <type 'int'>>, repr=True, cmp=True, hash=None, init=True), <type 'int'>, None)
 
 
 .. autofunction:: attr.validators.provides

docs/examples.rst

@@ -514,7 +514,7 @@ Slot classes are a little different than ordinary, dictionary-backed classes:
     ... class C(object):
     ...     x = attr.ib()
     >>> C.x
-    Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({}))
+    Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({}))
     >>> @attr.s(slots=True)
     ... class C(object):
     ...     x = attr.ib()

docs/extending.rst

@@ -17,7 +17,7 @@ So it is fairly simple to build your own decorators on top of ``attrs``:
    ... @attr.s
    ... class C(object):
    ...     a = attr.ib()
-   (Attribute(name='a', default=NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata=mappingproxy({})),)
+   (Attribute(name='a', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata=mappingproxy({})),)
 
 
 .. warning::
@@ -66,7 +66,7 @@ Here are some tips for effective use of metadata:
 
     >>> MY_TYPE_METADATA = '__my_type_metadata'
     >>>
-    >>> def typed(cls, default=attr.NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata={}):
+    >>> def typed(cls, default=attr.NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, convert=None, metadata={}):
     ...     metadata = dict() if not metadata else metadata
     ...     metadata[MY_TYPE_METADATA] = cls
     ...     return attr.ib(default, validator, repr, cmp, hash, init, convert, metadata)

src/attr/__init__.py

@@ -48,8 +48,8 @@
     "Factory",
     "NOTHING",
     "asdict",
-    "astuple",
     "assoc",
+    "astuple",
     "attr",
     "attrib",
     "attributes",

src/attr/_make.py

@@ -49,7 +49,7 @@ def __hash__(self):
 
 
 def attr(default=NOTHING, validator=None,
-         repr=True, cmp=True, hash=True, init=True,
+         repr=True, cmp=True, hash=None, init=True,
          convert=None, metadata={}):
     """
     Create a new attribute on a class.
@@ -92,8 +92,11 @@ def attr(default=NOTHING, validator=None,
         method.
     :param bool cmp: Include this attribute in the generated comparison methods
         (``__eq__`` et al).
-    :param bool hash: Include this attribute in the generated ``__hash__``
-        method.
+    :param hash: Include this attribute in the generated ``__hash__``
+        method.  If ``None`` (default), mirror *cmp*'s value.  This is the
+        correct behavior according the Python spec.  Setting this value to
+        anything else than ``None`` is *discouraged*.
+    :type hash: ``bool`` or ``None``
     :param bool init: Include this attribute in the generated ``__init__``
         method.  It is possible to set this to ``False`` and set a default
         value.  In that case this attributed is unconditionally initialized
@@ -104,10 +107,16 @@ def attr(default=NOTHING, validator=None,
         returned value will be used as the new value of the attribute.  The
         value is converted before being passed to the validator, if any.
     :param metadata: An arbitrary mapping, to be used by third-party
-        components.
+        components.  See :ref:`extending_metadata`.
 
     ..  versionchanged:: 17.1.0 *validator* can be a ``list`` now.
+    ..  versionchanged:: 17.1.0
+        *hash* is ``None`` and therefore mirrors *cmp* by default .
     """
+    if hash is not None and hash is not True and hash is not False:
+        raise TypeError(
+            "Invalid value for hash.  Must be True, False, or None."
+        )
     return _CountingAttr(
         default=default,
         validator=validator,
@@ -216,7 +225,7 @@ def _frozen_delattrs(self, name):
 
 
 def attributes(maybe_cls=None, these=None, repr_ns=None,
-               repr=True, cmp=True, hash=True, init=True,
+               repr=True, cmp=True, hash=None, init=True,
                slots=False, frozen=False, str=False):
     r"""
     A class decorator that adds `dunder
@@ -245,8 +254,28 @@ def attributes(maybe_cls=None, these=None, repr_ns=None,
         ``__gt__``, and ``__ge__`` methods that compare the class as if it were
         a tuple of its ``attrs`` attributes.  But the attributes are *only*
         compared, if the type of both classes is *identical*!
-    :param bool hash: Create a ``__hash__`` method that returns the
-        :func:`hash` of a tuple of all ``attrs`` attribute values.
+    :param hash: If ``None`` (default), the ``__hash__`` method is generated
+        according how *cmp* and *frozen* are set.  You will receive one if
+        *both* are ``True``.
+
+        Although not recommended, you can decide for yourself and force
+        ``attrs`` to create one (e.g. if the class is immutable even though you
+        didn't freeze it programmatically) by passing ``True`` or not (e.g. if
+        you want to use the superclass's ``__hash__`` method be it Python's
+        build-in id-based hashing or your own).  Both of these cases are rather
+        special and should be used carefully.
+
+        Please note that setting *hash* to ``False`` means that the
+        superclass's ``__hash__`` function is used. If you set it to ``None``,
+        and your class is not *both* ``cmp=True`` and ``frozen=True``, the
+        ``__hash__`` method is set to ``None``, making it not hashable (which
+        it is).
+
+        See the `Python documentation \
+        <https://docs.python.org/3/reference/datamodel.html#object.__hash__>`_
+        and the `GitHub issue that led to the default behavior \
+        <https://github.com/hynek/attrs/issues/136>`_ for more details.
+    :type hash: ``bool`` or ``None``
     :param bool init: Create a ``__init__`` method that initialiazes the
         ``attrs`` attributes.  Leading underscores are stripped for the
         argument name.  If a ``__attrs_post_init__`` method exists on the
@@ -273,6 +302,9 @@ def attributes(maybe_cls=None, these=None, repr_ns=None,
     ..  versionadded:: 16.0.0 *slots*
     ..  versionadded:: 16.1.0 *frozen*
     ..  versionadded:: 16.3.0 *str*, and support for ``__attrs_post_init__``.
+    ..  versionchanged::
+            17.1.0 *hash* supports ``None`` as value which is also the default
+            now.
     """
     def wrap(cls):
         if getattr(cls, "__class__", None) is None:
@@ -303,8 +335,18 @@ def wrap(cls):
             cls.__str__ = cls.__repr__
         if cmp is True:
             cls = _add_cmp(cls)
-        if hash is True:
+
+        if hash is not True and hash is not False and hash is not None:
+            raise TypeError(
+                "Invalid value for hash.  Must be True, False, or None."
+            )
+        elif hash is False:
+            pass
+        elif hash is True or (hash is None and cmp is True and frozen is True):
             cls = _add_hash(cls)
+        else:
+            cls.__hash__ = None
+
         if init is True:
             cls = _add_init(cls, effectively_frozen)
         if effectively_frozen is True:
@@ -369,7 +411,9 @@ def _add_hash(cls, attrs=None):
     Add a hash method to *cls*.
     """
     if attrs is None:
-        attrs = [a for a in cls.__attrs_attrs__ if a.hash]
+        attrs = [a
+                 for a in cls.__attrs_attrs__
+                 if a.hash is True or (a.hash is None and a.cmp is True)]
 
     def hash_(self):
         """

tests/test_dark_magic.py

@@ -1,7 +1,9 @@
 from __future__ import absolute_import, division, print_function
+
 import pickle
 
 import pytest
+
 from hypothesis import given
 from hypothesis.strategies import booleans
 
@@ -91,9 +93,9 @@ def test_fields(self, cls):
         """
         assert (
             Attribute(name="x", default=foo, _validator=None,
-                      repr=True, cmp=True, hash=True, init=True),
+                      repr=True, cmp=True, hash=None, init=True),
             Attribute(name="y", default=attr.Factory(list), _validator=None,
-                      repr=True, cmp=True, hash=True, init=True),
+                      repr=True, cmp=True, hash=None, init=True),
         ) == attr.fields(cls)
 
     @pytest.mark.parametrize("cls", [C1, C1Slots])
@@ -140,9 +142,9 @@ def test_programmatic(self, slots, frozen):
         PC = attr.make_class("PC", ["a", "b"], slots=slots, frozen=frozen)
         assert (
             Attribute(name="a", default=NOTHING, _validator=None,
-                      repr=True, cmp=True, hash=True, init=True),
+                      repr=True, cmp=True, hash=None, init=True),
             Attribute(name="b", default=NOTHING, _validator=None,
-                      repr=True, cmp=True, hash=True, init=True),
+                      repr=True, cmp=True, hash=None, init=True),
         ) == attr.fields(PC)
 
     @pytest.mark.parametrize("cls", [Sub, SubSlots])

tests/test_dunders.py

@@ -29,8 +29,11 @@
 CmpCSlots = simple_class(cmp=True, slots=True)
 ReprC = simple_class(repr=True)
 ReprCSlots = simple_class(repr=True, slots=True)
+
+# HashC is hashable by explicit definition while HashCSlots is hashable
+# implicitly.
 HashC = simple_class(hash=True)
-HashCSlots = simple_class(hash=True, slots=True)
+HashCSlots = simple_class(hash=None, cmp=True, frozen=True, slots=True)
 
 
 class InitC(object):
@@ -227,22 +230,69 @@ class TestAddHash(object):
     """
     Tests for `_add_hash`.
     """
+    def test_enforces_type(self):
+        """
+        The `hash` argument to both attrs and attrib must be None, True, or
+        False.
+        """
+        exc_args = ("Invalid value for hash.  Must be True, False, or None.",)
+
+        with pytest.raises(TypeError) as e:
+            make_class("C", {}, hash=1),
+
+        assert exc_args == e.value.args
+
+        with pytest.raises(TypeError) as e:
+            make_class("C", {"a": attr(hash=1)}),
+
+        assert exc_args == e.value.args
+
     @given(booleans())
-    def test_hash(self, slots):
+    def test_hash_attribute(self, slots):
         """
-        If `hash` is False, ignore that attribute.
+        If `hash` is False on an attribute, ignore that attribute.
         """
-        C = make_class("C", {"a": attr(hash=False), "b": attr()}, slots=slots)
+        C = make_class("C", {"a": attr(hash=False), "b": attr()},
+                       slots=slots, hash=True)
 
         assert hash(C(1, 2)) == hash(C(2, 2))
 
+    @given(booleans())
+    def test_hash_attribute_mirrors_cmp(self, cmp):
+        """
+        If hash is None, the hash generation mirrors cmp.
+        """
+        C = make_class("C", {"a": attr(cmp=cmp)}, cmp=True, frozen=True)
+
+        if cmp:
+            assert C(1) != C(2)
+            assert hash(C(1)) != hash(C(2))
+            assert hash(C(1)) == hash(C(1))
+        else:
+            assert C(1) == C(2)
+            assert hash(C(1)) == hash(C(2))
+
     @pytest.mark.parametrize("cls", [HashC, HashCSlots])
     def test_hash_works(self, cls):
         """
         __hash__ returns different hashes for different values.
         """
         assert hash(cls(1, 2)) != hash(cls(1, 1))
 
+    def test_hash_default(self):
+        """
+        Classes are not hashable by default.
+        """
+        C = make_class("C", {})
+
+        with pytest.raises(TypeError) as e:
+            hash(C())
+
+        assert e.value.args[0] in (
+            "'C' objects are unhashable",  # PyPy
+            "unhashable type: 'C'",  # CPython
+        )
+
 
 class TestAddInit(object):
     """