Be smarter, also fix attributes

CHANGELOG.rst

@@ -18,6 +18,8 @@ Backward-incompatible changes:
   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:
@@ -29,8 +31,9 @@ Deprecations:
 Changes:
 ^^^^^^^^
 
-- Fix hashing behavior by setting ``__hash__`` to ``None`` by default.
+- 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).
@@ -44,6 +47,7 @@ Changes:
 - The ``validator`` argument of ``@attr.s`` now can take a ``list`` of validators that all have to pass.
   `#138 <https://github.com/hynek/attrs/issues/138>`_
 
+
 ----
 
 

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::
 
@@ -69,7 +69,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
@@ -125,9 +125,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
 
@@ -215,7 +215,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:
@@ -252,7 +252,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

@@ -502,7 +502,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/_make.py

@@ -48,7 +48,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.
@@ -91,8 +91,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
@@ -103,10 +106,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,
@@ -244,18 +253,22 @@ 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 hash: If ``True``, create a ``__hash__`` method that returns the
-        :func:`hash` of a tuple of all ``attrs`` attribute values.  Please note
-        that this makes only sense if and only if the class and *all of it's
-        attributes* are never mutated!
-
-        If set to ``False``, ``attrs`` won't create a ``__hash__`` method for
-        you.  Use this if you want to use the ``__hash__`` method of the
-        superclass (including ``object``).
-
-        The default of this argument is ``None`` which sets the ``__hash__``
-        method to ``None`` and marks it unhashable which is true for most
-        classes in practice.
+    :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__>`_
@@ -317,10 +330,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)
-        elif hash is None:
+        else:
             cls.__hash__ = None
+
         if init is True:
             cls = _add_init(cls, frozen)
         if frozen is True:
@@ -364,7 +385,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
 
@@ -86,9 +88,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])
@@ -135,9 +137,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,16 +230,48 @@ 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, 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):
         """

tests/test_make.py

@@ -108,7 +108,7 @@ class C(object):
             "No mandatory attributes allowed after an attribute with a "
             "default value or factory.  Attribute in question: Attribute"
             "(name='y', default=NOTHING, validator=None, repr=True, "
-            "cmp=True, hash=True, init=True, convert=None, "
+            "cmp=True, hash=None, init=True, convert=None, "
             "metadata=mappingproxy({}))",
         ) == e.value.args
 
@@ -162,7 +162,7 @@ class E(D):
 
 class TestAttributes(object):
     """
-    Tests for the `attributes` class decorator.
+    Tests for the `attrs`/`attr.s` class decorator.
     """
     @pytest.mark.skipif(not PY2, reason="No old-style classes in Py3")
     def test_catches_old_style(self):
@@ -211,29 +211,24 @@ def test_immutable(self, attr, attr_name):
     ])
     def test_adds_all_by_default(self, method_name):
         """
-        If no further arguments are supplied, all add_XXX functions are
-        applied.
+        If no further arguments are supplied, all add_XXX functions except
+        add_hash are applied.  __hash__ is set to None.
         """
         # Set the method name to a sentinel and check whether it has been
         # overwritten afterwards.
         sentinel = object()
 
-        class C1(object):
-            x = attr()
-
-        setattr(C1, method_name, sentinel)
-
-        C1 = attributes(C1)
-
-        class C2(object):
+        class C(object):
             x = attr()
 
-        setattr(C2, method_name, sentinel)
+        setattr(C, method_name, sentinel)
 
-        C2 = attributes(C2)
+        C = attributes(C)
+        meth = getattr(C, method_name)
 
-        assert sentinel != getattr(C1, method_name)
-        assert sentinel != getattr(C2, method_name)
+        assert sentinel != meth
+        if method_name == "__hash__":
+            assert meth is None
 
     @pytest.mark.parametrize("arg_name, method_name", [
         ("repr", "__repr__"),
@@ -243,7 +238,7 @@ class C2(object):
     ])
     def test_respects_add_arguments(self, arg_name, method_name):
         """
-        If a certain `add_XXX` is `True`, XXX is not added to the class.
+        If a certain `add_XXX` is `False`, `__XXX__` is not added to the class.
         """
         # Set the method name to a sentinel and check whether it has been
         # overwritten afterwards.
@@ -584,7 +579,6 @@ class TestMetadata(object):
     """
     Tests for metadata handling.
     """
-
     @given(sorted_lists_of_attrs)
     def test_metadata_present(self, list_of_attrs):
         """