Fix backward compatibility with pickles before v22.2.0 (#1085)

* Remove unnecessary parameterization of tests

* Fix backward compatibility with pickles before v22.2.0

Unfortunately we're using `django-redis` to cache some `attrs` instances
and the change to make `__getstate__` and `__setstate__` use `dict`
values instead of `tuple` values prevents reading any values from the
cache when updating to `attrs>=22.2.0`. We get an `AttributeError`
raised for all attributes.

This happens because the tuple of values stored in the original pickle
is checked for the name of the attribute and so none of the attributes
are set on unpickle.

To address this I've made a change so that `__setstate__` will still be
able to handle values pickled as `tuple`s allowing existing pickles to
be unpickled.

The test just contains a pre-pickled value due to the complexities
around tying to mock methods on a slotted as documented[^1].

I realise that documentation also mentions that we should "think twice"
before using pickle, but sometimes those decisions predate our
involvement in a project! In addition, pretty much all of Django's cache
implementations are built around using pickle by default. This change
will allow an easier path to upgrade by unpickling and repickling all
values in the cache.

Regression in 89a890a.

[^1]: https://www.attrs.org/en/latest/glossary.html#term-slotted-classes

* Update 1085.change.md

Co-authored-by: Hynek Schlawack <hs@ox.cx>

changelog.d/1085.change.md

@@ -0,0 +1 @@
+Restored ability to unpickle instances pickled before 22.2.0.

src/attr/_make.py

@@ -934,9 +934,15 @@ def slots_setstate(self, state):
             Automatically created by attrs.
             """
             __bound_setattr = _obj_setattr.__get__(self)
-            for name in state_attr_names:
-                if name in state:
-                    __bound_setattr(name, state[name])
+            if isinstance(state, tuple):
+                # Backward compatibility with attrs instances pickled with
+                # attrs versions before v22.2.0 which stored tuples.
+                for name, value in zip(state_attr_names, state):
+                    __bound_setattr(name, value)
+            else:
+                for name in state_attr_names:
+                    if name in state:
+                        __bound_setattr(name, state[name])
 
             # The hash code cache is not included when the object is
             # serialized, but it still needs to be initialized to None to

tests/test_slots.py

@@ -754,13 +754,12 @@ class A:
     c = attr.ib()
 
 
-@pytest.mark.parametrize("cls", [A])
-def test_slots_unpickle_after_attr_removed(cls):
+def test_slots_unpickle_after_attr_removed():
     """
     We don't assign attributes we don't have anymore if the class has
     removed it.
     """
-    a = cls(1, 2, 3)
+    a = A(1, 2, 3)
     a_pickled = pickle.dumps(a)
     a_unpickled = pickle.loads(a_pickled)
     assert a_unpickled == a
@@ -778,12 +777,11 @@ class NEW_A:
         assert not hasattr(new_a, "b")
 
 
-@pytest.mark.parametrize("cls", [A])
-def test_slots_unpickle_after_attr_added(cls, frozen):
+def test_slots_unpickle_after_attr_added(frozen):
     """
     We don't assign attribute we haven't had before if the class has one added.
     """
-    a = cls(1, 2, 3)
+    a = A(1, 2, 3)
     a_pickled = pickle.dumps(a)
     a_unpickled = pickle.loads(a_pickled)
 
@@ -803,3 +801,20 @@ class NEW_A:
         assert new_a.b == 2
         assert new_a.c == 3
         assert not hasattr(new_a, "d")
+
+
+def test_slots_unpickle_is_backward_compatible(frozen):
+    """
+    Ensure object pickled before v22.2.0 can still be unpickled.
+    """
+    a = A(1, 2, 3)
+
+    a_pickled = (
+        b"\x80\x04\x95&\x00\x00\x00\x00\x00\x00\x00\x8c\x10"
+        + a.__module__.encode()
+        + b"\x94\x8c\x01A\x94\x93\x94)\x81\x94K\x01K\x02K\x03\x87\x94b."
+    )
+
+    a_unpickled = pickle.loads(a_pickled)
+
+    assert a_unpickled == a