From 1a90857109794f27e4cf5914bb9ae4617993e880 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Thu, 31 Jan 2019 08:19:26 +0100 Subject: [PATCH 01/12] Use new flake8 pre-commit --- .pre-commit-config.yaml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9636062e1..18f244790 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -7,10 +7,18 @@ repos: # override until resolved: https://github.com/ambv/black/issues/402 files: \.pyi?$ types: [] + + - repo: https://gitlab.com/pycqa/flake8 + rev: '3.7.3' + hooks: + - id: flake8 + language_version: python3.7 + - repo: https://github.com/asottile/seed-isort-config rev: v1.5.0 hooks: - id: seed-isort-config + - repo: https://github.com/pre-commit/mirrors-isort rev: v4.3.4 hooks: @@ -23,5 +31,3 @@ repos: - id: trailing-whitespace - id: end-of-file-fixer - id: debug-statements - - id: flake8 - language_version: python3.7 From d2be76c964aea3b0648d547c930f9633f577b257 Mon Sep 17 00:00:00 2001 From: Ryan Gabbard Date: Sat, 2 Feb 2019 08:32:01 -0500 Subject: [PATCH 02/12] Clear cache hash on de-serialization (#489) * Don't cache hash codes across deserialization. Because the hash code cache field gets serialized and deserialized by Pickle, previously when you deserialize a cache_hash=True attrs object, the hashcode will be the hashcode the object had at serialization-time. However, if your object had fields with hash codes which were not deterministic between interpreter runs, then on a new interpreter run your deserialized object would have a hash code which differs from a newly created identical object. This commit fixes that by clearing the cache on deserialization. It needs to override the __setstate__ method to do so, so this commit also forbids using a custom __setstate__ on a cache_hash=True object. Closes #482 . * Improve exception type * Minor tweaks to comments formatting * Fix test for Python 2.7 * Fix error in comment * Make nomenclature consistent for slotted/dict classes * Remove hasattr() * Improve comments and error message for custom __setstate__ + cache_hash=True * Drop use of for test classes for cache hash serialization * Make changelog note this is a breaking change * Make exception message point to tracking issue * Fix test classes for Python 2.7 --- changelog.d/482.breaking.rst | 4 ++ src/attr/_make.py | 29 ++++++++++++++ tests/test_dunders.py | 78 ++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 changelog.d/482.breaking.rst diff --git a/changelog.d/482.breaking.rst b/changelog.d/482.breaking.rst new file mode 100644 index 000000000..0eb5cb4fc --- /dev/null +++ b/changelog.d/482.breaking.rst @@ -0,0 +1,4 @@ +Fixed a bug where deserialized objects with ``cache_hash=True`` could have incorrect hash code values. +This change breaks classes with ``cache_hash=True`` when a custom ``__setstate__`` is present. +An exception will be thrown when applying the ``attrs`` annotation to such a class. +This limitation is tracked in issue https://github.com/python-attrs/attrs/issues/494 . diff --git a/src/attr/_make.py b/src/attr/_make.py index 6d72df4af..1373536af 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -529,6 +529,26 @@ def _patch_original_class(self): for name, value in self._cls_dict.items(): setattr(cls, name, value) + # Attach __setstate__. This is necessary to clear the hash code + # cache on deserialization. See issue + # https://github.com/python-attrs/attrs/issues/482 . + # Note that this code only handles setstate for dict classes. + # For slotted classes, see similar code in _create_slots_class . + if self._cache_hash: + existing_set_state_method = getattr(cls, "__setstate__", None) + if existing_set_state_method: + raise NotImplementedError( + "Currently you cannot use hash caching if " + "you specify your own __setstate__ method." + "See https://github.com/python-attrs/attrs/issues/494 ." + ) + + def cache_hash_set_state(chss_self, _): + # clear hash code cache + setattr(chss_self, _hash_cache_field, None) + + setattr(cls, "__setstate__", cache_hash_set_state) + return cls def _create_slots_class(self): @@ -581,6 +601,8 @@ def slots_getstate(self): """ return tuple(getattr(self, name) for name in state_attr_names) + hash_caching_enabled = self._cache_hash + def slots_setstate(self, state): """ Automatically created by attrs. @@ -588,6 +610,13 @@ def slots_setstate(self, state): __bound_setattr = _obj_setattr.__get__(self, Attribute) for name, value in zip(state_attr_names, state): __bound_setattr(name, value) + # Clearing the hash code cache on deserialization is needed + # because hash codes can change from run to run. See issue + # https://github.com/python-attrs/attrs/issues/482 . + # Note that this code only handles setstate for slotted classes. + # For dict classes, see similar code in _patch_original_class . + if hash_caching_enabled: + __bound_setattr(_hash_cache_field, None) # slots and frozen require __getstate__/__setstate__ to work cd["__getstate__"] = slots_getstate diff --git a/tests/test_dunders.py b/tests/test_dunders.py index 59d5764a1..468a56982 100644 --- a/tests/test_dunders.py +++ b/tests/test_dunders.py @@ -5,6 +5,7 @@ from __future__ import absolute_import, division, print_function import copy +import pickle import pytest @@ -453,6 +454,83 @@ def __hash__(self): assert 2 == uncached_instance.hash_counter.times_hash_called assert 1 == cached_instance.hash_counter.times_hash_called + def test_cache_hash_serialization(self): + """ + Tests that the hash cache is cleared on deserialization to fix + https://github.com/python-attrs/attrs/issues/482 . + """ + + # First, check that our fix didn't break serialization without + # hash caching. + # We don't care about the result of this; we just want to make sure we + # can do it without exceptions. + hash(pickle.loads(pickle.dumps(HashCacheSerializationTestUncached))) + + def assert_hash_code_not_cached_across_serialization(original): + # Now check our fix for #482 for when hash caching is enabled. + original_hash = hash(original) + round_tripped = pickle.loads(pickle.dumps(original)) + # What we want to guard against is having a stale hash code + # when a field's hash code differs in a new interpreter after + # deserialization. This is tricky to test because we are, + # of course, still running in the same interpreter. So + # after deserialization we reach in and change the value of + # a field to simulate the field changing its hash code. We then + # check that the object's hash code changes, indicating that we + # don't have a stale hash code. + # This could fail in two ways: (1) pickle.loads could get the hash + # code of the deserialized value (triggering it to cache) before + # we alter the field value. This doesn't happen in our tested + # Python versions. (2) "foo" and "something different" could + # have a hash collision on this interpreter run. But this is + # extremely improbable and would just result in one buggy test run. + round_tripped.foo_string = "something different" + assert original_hash != hash(round_tripped) + + # Slots and non-slots classes implement __setstate__ differently, + # so we need to test both cases. + assert_hash_code_not_cached_across_serialization( + HashCacheSerializationTestCached() + ) + assert_hash_code_not_cached_across_serialization( + HashCacheSerializationTestCachedSlots() + ) + + # Test that a custom __setstate__ is disallowed on a + # cache_hash=True object. + # This is needed because we handle clearing the cache after + # deserialization with a custom __setstate__. It is possible + # to make both work, but it requires some thought about how to go + # about it, so it has not yet been implemented. + with pytest.raises( + NotImplementedError, + message="Currently you cannot use hash caching if you " + "specify your own __setstate__ method. This is tracked" + "by https://github.com/python-attrs/attrs/issues/494", + ): + + @attr.attrs(hash=True, cache_hash=True) + class NoCacheHashAndCustomSetState(object): + def __setstate__(self, state): + pass + + +# these are for use in TestAddHash.test_cache_hash_serialization +# they need to be out here so they can be un-pickled +@attr.attrs(hash=True, cache_hash=False) +class HashCacheSerializationTestUncached(object): + foo_string = attr.ib(default="foo") + + +@attr.attrs(hash=True, cache_hash=True) +class HashCacheSerializationTestCached(object): + foo_string = attr.ib(default="foo") + + +@attr.attrs(slots=True, hash=True, cache_hash=True) +class HashCacheSerializationTestCachedSlots(object): + foo_string = attr.ib(default="foo") + class TestAddInit(object): """ From 40edb30d3a5393f3334cbf38e0019ce18d2549e6 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 2 Feb 2019 14:33:29 +0100 Subject: [PATCH 03/12] Make nicer link --- changelog.d/482.breaking.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/482.breaking.rst b/changelog.d/482.breaking.rst index 0eb5cb4fc..694b714be 100644 --- a/changelog.d/482.breaking.rst +++ b/changelog.d/482.breaking.rst @@ -1,4 +1,4 @@ Fixed a bug where deserialized objects with ``cache_hash=True`` could have incorrect hash code values. This change breaks classes with ``cache_hash=True`` when a custom ``__setstate__`` is present. An exception will be thrown when applying the ``attrs`` annotation to such a class. -This limitation is tracked in issue https://github.com/python-attrs/attrs/issues/494 . +This limitation is tracked in issue `#494 `_. From 3421b4bd971bb3ee6467fcaa1be89f7c7062fb85 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 2 Feb 2019 14:36:56 +0100 Subject: [PATCH 04/12] Split test and fix warning message is deprecated. --- tests/test_dunders.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/test_dunders.py b/tests/test_dunders.py index 468a56982..b9337b2eb 100644 --- a/tests/test_dunders.py +++ b/tests/test_dunders.py @@ -496,17 +496,20 @@ def assert_hash_code_not_cached_across_serialization(original): HashCacheSerializationTestCachedSlots() ) - # Test that a custom __setstate__ is disallowed on a - # cache_hash=True object. - # This is needed because we handle clearing the cache after - # deserialization with a custom __setstate__. It is possible - # to make both work, but it requires some thought about how to go - # about it, so it has not yet been implemented. + def test_caching_and_custom_setstate(self): + """ + The combination of a custom __setstate__ and cache_hash=True is caught + with a helpful message. + + This is needed because we handle clearing the cache after + deserialization with a custom __setstate__. It is possible to make both + work, but it requires some thought about how to go about it, so it has + not yet been implemented. + """ with pytest.raises( NotImplementedError, - message="Currently you cannot use hash caching if you " - "specify your own __setstate__ method. This is tracked" - "by https://github.com/python-attrs/attrs/issues/494", + match="Currently you cannot use hash caching if you " + "specify your own __setstate__ method.", ): @attr.attrs(hash=True, cache_hash=True) From 87b2bb2e4850e5314a26d3dc77d29026048f7018 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Tue, 5 Feb 2019 06:45:27 +0100 Subject: [PATCH 05/12] Add warning about subclassing behavior (#485) Fixes #483 --- docs/hashing.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/hashing.rst b/docs/hashing.rst index fd495b4ee..b0a61c342 100644 --- a/docs/hashing.rst +++ b/docs/hashing.rst @@ -37,6 +37,16 @@ Because according to the definition_ from the official Python docs, the returned The *correct way* to achieve hashing by id is to set ``@attr.s(cmp=False)``. Setting ``@attr.s(hash=False)`` (that implies ``cmp=True``) is almost certainly a *bug*. + .. warning:: + + Be careful when subclassing! + Setting ``cmp=False`` on class whose base class has a non-default ``__hash__`` method, will *not* make ``attrs`` remove that ``__hash__`` for you. + + It is part of ``attrs``'s philosophy to only *add* to classes so you have the freedom to customize your classes as you wish. + So if you want to *get rid* of methods, you'll have to do it by hand. + + The easiest way to reset ``__hash__`` on a class is adding ``__hash__ = object.__hash__`` in the class body. + #. If two object are not equal, their hash **should** be different. While this isn't a requirement from a standpoint of correctness, sets and dicts become less effective if there are a lot of identical hashes. From 2e2748aee3b704ef9022ce3d8bf70e676c07cf00 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 13:34:48 +0100 Subject: [PATCH 06/12] Clarify execution order in init (#488) * Clarify execution order in init Fixes #461 * Clarify attributes are processed in the order of declaration * Simple past is good enough --- docs/init.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/init.rst b/docs/init.rst index 41a86ca77..85fcf39b6 100644 --- a/docs/init.rst +++ b/docs/init.rst @@ -350,6 +350,23 @@ If you need to set attributes on a frozen class, you'll have to resort to the :r Note that you *must not* access the hash code of the object in ``__attrs_post__init__`` if ``cache_hash=True``. + +Order of Execution +------------------ + +If present, the hooks are executed in the following order: + +1. For each attribute, in the order it was declared: + + a. default factory + b. converter + +2. *all* validators +3. ``__attrs_post_init__`` + +Notably this means, that you can access all attributes from within your validators, but your converters have to deal with invalid values and have to return a valid value. + + .. _`Wiki page`: https://github.com/python-attrs/attrs/wiki/Extensions-to-attrs .. _`get confused`: https://github.com/python-attrs/attrs/issues/289 .. _`there is no such thing as a private argument`: https://github.com/hynek/characteristic/issues/6 From bb9b83ceacebf9335c2b3afecb28dde1bcb4b661 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 13:55:48 +0100 Subject: [PATCH 07/12] Clarify that annotation-only doesn't work w/ decorators (#486) Fixes #466 --- docs/init.rst | 3 ++- docs/types.rst | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/init.rst b/docs/init.rst index 85fcf39b6..80bf82369 100644 --- a/docs/init.rst +++ b/docs/init.rst @@ -97,6 +97,7 @@ This is when default values come into play: C(a=42, b=[], c=[], d={}) It's important that the decorated method -- or any other method or property! -- doesn't have the same name as the attribute, otherwise it would overwrite the attribute definition. +You also cannot use type annotations to elide the :func:`attr.ib` call for ``d`` as explained in :doc:`types`. Please note that as with function and method signatures, ``default=[]`` will *not* do what you may think it might do: @@ -161,7 +162,7 @@ If the value does not pass the validator's standards, it just raises an appropri ... ValueError: x must be smaller or equal to 42 -Again, it's important that the decorated method doesn't have the same name as the attribute. +Again, it's important that the decorated method doesn't have the same name as the attribute and that you can't elide the call to :func:`attr.ib`. Callables diff --git a/docs/types.rst b/docs/types.rst index 60db3f352..798d864e4 100644 --- a/docs/types.rst +++ b/docs/types.rst @@ -23,9 +23,19 @@ That means that on modern Python versions, the declaration part of the example f >>> attr.fields(SomeClass).a_number.type -You can still use :func:`attr.ib` for advanced features, but you don't have to. +You will still need :func:`attr.ib` for advanced features, but not for the common cases. -Please note that these types are *only metadata* that can be queried from the class and they aren't used for anything out of the box! +One of those features are the decorator-based features like defaults. +It's important to remember that ``attrs`` doesn't do any magic behind your back. +All the decorators are implemented using an object that is returned by the call to :func:`attr.ib`. + +Attributes that only carry a class annotation do not have that object so trying to call a method on it will inevitably fail. + +***** + +Please note that types -- however added -- are *only metadata* that can be queried from the class and they aren't used for anything out of the box! + +In practice, their biggest usefulness shows in combination with mypy. mypy From d21eed51656b6046307ba3ad09c97a98e6c798cc Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 13:59:16 +0100 Subject: [PATCH 08/12] Use isolated builds --- pyproject.toml | 1 + tox.ini | 1 + 2 files changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index e39808341..866a34f28 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,6 @@ [build-system] requires = ["setuptools", "wheel"] +build-backend = "setuptools.build_meta" [tool.black] diff --git a/tox.ini b/tox.ini index 6af1fe3bd..5d94364e8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,6 @@ [tox] envlist = typing,lint,py27,py34,py35,py36,py37,pypy,pypy3,manifest,docs,doctest,pypi-description,changelog,coverage-report +isolated_build = True [testenv] From d4a002f64d8db7e05b9f5dd6e955fd0f5c8c2d58 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 14:28:01 +0100 Subject: [PATCH 09/12] try to fix build --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index 5d94364e8..a09784be7 100644 --- a/tox.ini +++ b/tox.ini @@ -86,6 +86,7 @@ commands = towncrier --draft [testenv:typing] +isolated_build = false basepython = python3.7 deps = mypy commands = mypy tests/typing_example.py From f8f4aef5a8bc0e3c403ae343b71b2400978c56d7 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 14:47:35 +0100 Subject: [PATCH 10/12] Revert isolated builds because they break mypy --- pyproject.toml | 1 - tox.ini | 2 -- 2 files changed, 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 866a34f28..e39808341 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,5 @@ [build-system] requires = ["setuptools", "wheel"] -build-backend = "setuptools.build_meta" [tool.black] diff --git a/tox.ini b/tox.ini index a09784be7..6af1fe3bd 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,5 @@ [tox] envlist = typing,lint,py27,py34,py35,py36,py37,pypy,pypy3,manifest,docs,doctest,pypi-description,changelog,coverage-report -isolated_build = True [testenv] @@ -86,7 +85,6 @@ commands = towncrier --draft [testenv:typing] -isolated_build = false basepython = python3.7 deps = mypy commands = mypy tests/typing_example.py From 2f63f2c5fe1edfa8bd868d01c7096263868092de Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 9 Feb 2019 18:36:30 +0100 Subject: [PATCH 11/12] Use isolated builds (#499) * Use isolated builds * try to upgrade all the things * Try to upgrade only setuptools --- .travis.yml | 2 +- pyproject.toml | 3 ++- tox.ini | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 09f1f4018..cd4a62f51 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,7 +60,7 @@ matrix: install: - - pip install tox + - pip install --upgrade tox script: diff --git a/pyproject.toml b/pyproject.toml index e39808341..5657791df 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,6 @@ [build-system] -requires = ["setuptools", "wheel"] +requires = ["setuptools>=40.6.0", "wheel"] +build-backend = "setuptools.build_meta" [tool.black] diff --git a/tox.ini b/tox.ini index 6af1fe3bd..5d94364e8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,6 @@ [tox] envlist = typing,lint,py27,py34,py35,py36,py37,pypy,pypy3,manifest,docs,doctest,pypi-description,changelog,coverage-report +isolated_build = True [testenv] From 7b37354aa4c81ba299427972a20c55da848d5bfd Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sun, 10 Feb 2019 07:32:19 +0100 Subject: [PATCH 12/12] Add Python 2 statement (#498) --- docs/index.rst | 1 + docs/python-2.rst | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 docs/python-2.rst diff --git a/docs/index.rst b/docs/index.rst index 18ce36302..8e88be0d2 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -60,6 +60,7 @@ Day-to-Day Usage license backward-compatibility + python-2 contributing changelog diff --git a/docs/python-2.rst b/docs/python-2.rst new file mode 100644 index 000000000..4fdf2c9b2 --- /dev/null +++ b/docs/python-2.rst @@ -0,0 +1,25 @@ +Python 2 Statement +================== + +While ``attrs`` has always been a Python 3-first package, we the maintainers are aware that Python 2 will not magically disappear in 2020. +We are also aware that ``attrs`` is an important building block in many people's systems and livelihoods. + +As such, we do **not** have any immediate plans to drop Python 2 support in ``attrs``. +We intend to support is as long as it will be technically feasible for us. + +Feasibility in this case means: + +1. Possibility to run the tests on our development computers, +2. and **free** CI options. + +This can mean that we will have to run our tests on PyPy, whose maintainters have unequivocally declared that they do not intend to stop the development and maintenance of their Python 2-compatible line at all. +And this can mean that at some point, a sponsor will have to step up and pay for bespoke CI setups. + +**However**: there is no promise of new features coming to ``attrs`` running under Python 2. +It is up to our discretion alone, to decide whether the introduced complexity or awkwardness are worth it, or whether we choose to make a feature available on modern platforms only. + + +Summary +------- + +We will do our best to support existing users, but nobody is entitled to the latest and greatest features on a platform that is officially end of life.