From 453b60e01163b5d0d8fe0c706bcdc92a6931e32c Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 4 Feb 2017 13:16:19 +0100 Subject: [PATCH] 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 `cmp` which the correct behavior per spec. Fixes #136. --- CHANGELOG.rst | 10 ++++++- docs/api.rst | 12 ++++---- docs/examples.rst | 2 +- docs/extending.rst | 4 +-- src/attr/__init__.py | 2 +- src/attr/_make.py | 62 ++++++++++++++++++++++++++++++++++------ tests/test_dark_magic.py | 10 ++++--- tests/test_dunders.py | 58 ++++++++++++++++++++++++++++++++++--- tests/test_make.py | 30 ++++++++----------- tests/test_slots.py | 14 ++++----- tests/utils.py | 6 ++-- 11 files changed, 155 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index bea56f0a5..1513c07ca 100644 --- a/CHANGELOG.rst +++ b/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 `_. + 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 `_ 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 `_ + `#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). diff --git a/docs/api.rst b/docs/api.rst index d24bb0804..acfc458de 100644 --- a/docs/api.rst +++ b/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 (got '1' that is a ).", Attribute(name='x', default=NOTHING, validator=>, repr=True, cmp=True, hash=True, init=True), , '1') + TypeError: ("'x' must be (got '1' that is a ).", Attribute(name='x', default=NOTHING, validator=>, repr=True, cmp=True, hash=None, init=True), , '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 (got None that is a ).", Attribute(name='x', default=NOTHING, validator=>, repr=True, cmp=True, hash=True, init=True), , None) + TypeError: ("'x' must be (got None that is a ).", Attribute(name='x', default=NOTHING, validator=>, repr=True, cmp=True, hash=None, init=True), , None) .. autofunction:: attr.validators.provides diff --git a/docs/examples.rst b/docs/examples.rst index a68a85cb3..cc74a74ef 100644 --- a/docs/examples.rst +++ b/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() diff --git a/docs/extending.rst b/docs/extending.rst index 604fb0f68..e7a20ad93 100644 --- a/docs/extending.rst +++ b/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) diff --git a/src/attr/__init__.py b/src/attr/__init__.py index 2d8c403e1..dd85b4989 100644 --- a/src/attr/__init__.py +++ b/src/attr/__init__.py @@ -48,8 +48,8 @@ "Factory", "NOTHING", "asdict", - "astuple", "assoc", + "astuple", "attr", "attrib", "attributes", diff --git a/src/attr/_make.py b/src/attr/_make.py index fae40f619..b29020171 100644 --- a/src/attr/_make.py +++ b/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 \ + `_ + and the `GitHub issue that led to the default behavior \ + `_ 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): """ diff --git a/tests/test_dark_magic.py b/tests/test_dark_magic.py index 2652a84c8..ab68b7d7f 100644 --- a/tests/test_dark_magic.py +++ b/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]) diff --git a/tests/test_dunders.py b/tests/test_dunders.py index 841b622a6..f27cceacf 100644 --- a/tests/test_dunders.py +++ b/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,15 +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) + 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): """ @@ -243,6 +279,20 @@ def test_hash_works(self, cls): """ 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): """ diff --git a/tests/test_make.py b/tests/test_make.py index 264935169..8142c3ada 100644 --- a/tests/test_make.py +++ b/tests/test_make.py @@ -159,7 +159,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 @@ -213,7 +213,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): @@ -262,29 +262,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__"), @@ -294,7 +289,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. @@ -635,7 +630,6 @@ class TestMetadata(object): """ Tests for metadata handling. """ - @given(sorted_lists_of_attrs) def test_metadata_present(self, list_of_attrs): """ diff --git a/tests/test_slots.py b/tests/test_slots.py index f2ab8aef0..a850575ae 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -31,7 +31,7 @@ def staticmethod(): return "staticmethod" -@attr.s(slots=True) +@attr.s(slots=True, hash=True) class C1Slots(object): x = attr.ib(validator=attr.validators.instance_of(int)) y = attr.ib() @@ -106,7 +106,7 @@ def test_inheritance_from_nonslots(): Note that a slots class inheriting from an ordinary class loses most of the benefits of slots classes, but it should still work. """ - @attr.s(slots=True) + @attr.s(slots=True, hash=True) class C2Slots(C1): z = attr.ib() @@ -159,7 +159,7 @@ def staticmethod(): return "staticmethod" C2Slots = attr.s(these={"x": attr.ib(), "y": attr.ib(), "z": attr.ib()}, - init=False, slots=True)(SimpleOrdinaryClass) + init=False, slots=True, hash=True)(SimpleOrdinaryClass) c2 = C2Slots(x=1, y=2, z="test") assert 1 == c2.x @@ -190,11 +190,11 @@ def test_inheritance_from_slots(): """ Inheriting from an attr slot class works. """ - @attr.s(slots=True) + @attr.s(slots=True, hash=True) class C2Slots(C1Slots): z = attr.ib() - @attr.s(slots=True) + @attr.s(slots=True, hash=True) class C2(C1): z = attr.ib() @@ -264,11 +264,11 @@ def classmethod(cls): def staticmethod(): return "staticmethod" - @attr.s(slots=True) + @attr.s(slots=True, hash=True) class C2Slots(C1BareSlots): z = attr.ib() - @attr.s(slots=True) + @attr.s(slots=True, hash=True) class C2(C1Bare): z = attr.ib() diff --git a/tests/utils.py b/tests/utils.py index e6e8f4c9d..ac6a1d7cb 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -17,18 +17,20 @@ from attr._make import NOTHING, make_class -def simple_class(cmp=False, repr=False, hash=False, str=False, slots=False): +def simple_class(cmp=False, repr=False, hash=False, str=False, slots=False, + frozen=False): """ Return a new simple class. """ return make_class( "C", ["a", "b"], cmp=cmp, repr=repr, hash=hash, init=True, slots=slots, str=str, + frozen=frozen, ) def simple_attr(name, default=NOTHING, validator=None, repr=True, - cmp=True, hash=True, init=True): + cmp=True, hash=None, init=True): """ Return an attribute with a name and no other bells and whistles. """