From 53e1e5f8c14e9cc6d60d39d9178feb8d25ed67e0 Mon Sep 17 00:00:00 2001 From: drinob Date: Fri, 13 Nov 2020 21:24:12 +0300 Subject: [PATCH 1/4] Fixed slots inheritance --- src/attr/_make.py | 13 +++++++++---- tests/test_slots.py | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 49484f935..b31838c45 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -698,7 +698,6 @@ def _create_slots_class(self): """ Build and return a new class with a `__slots__` attribute. """ - base_names = self._base_names cd = { k: v for k, v in iteritems(self._cls_dict) @@ -722,12 +721,16 @@ def _create_slots_class(self): cd["__setattr__"] = object.__setattr__ break - # Traverse the MRO to check for an existing __weakref__. + # Traverse the MRO to collect existing slots + # and check for an existing __weakref__. + existing_slots_and_base_names = set(self._base_names) weakref_inherited = False for base_cls in self._cls.__mro__[1:-1]: if base_cls.__dict__.get("__weakref__", None) is not None: weakref_inherited = True - break + existing_slots_and_base_names.update( + getattr(base_cls, "__slots__", []) + ) names = self._attr_names if ( @@ -740,7 +743,9 @@ def _create_slots_class(self): # We only add the names of attributes that aren't inherited. # Setting __slots__ to inherited attributes wastes memory. - slot_names = [name for name in names if name not in base_names] + slot_names = [ + name for name in names if name not in existing_slots_and_base_names + ] if self._cache_hash: slot_names.append(_hash_cache_field) cd["__slots__"] = tuple(slot_names) diff --git a/tests/test_slots.py b/tests/test_slots.py index b57fc639d..9bdc087ae 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -232,6 +232,8 @@ def test_inheritance_from_slots(): @attr.s(slots=True, hash=True) class C2Slots(C1Slots): + # y re-defined here but it shouldn't get a slot + y = attr.ib() z = attr.ib() @attr.s(slots=True, hash=True) From 13033e0533819aedeea3b88fa00e89156b57df0a Mon Sep 17 00:00:00 2001 From: drinob Date: Fri, 13 Nov 2020 21:37:05 +0300 Subject: [PATCH 2/4] Added changelog --- changelog.d/718.change.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/718.change.rst diff --git a/changelog.d/718.change.rst b/changelog.d/718.change.rst new file mode 100644 index 000000000..a5dbfde25 --- /dev/null +++ b/changelog.d/718.change.rst @@ -0,0 +1 @@ +Fixed creation of extra slot for ``attr.ib`` when parent class already has a slot with the same name From dd0929ee7fcd33ef95ef38dffef8e802a1e9d397 Mon Sep 17 00:00:00 2001 From: drinob Date: Sat, 14 Nov 2020 18:24:07 +0300 Subject: [PATCH 3/4] Added a separate test --- tests/test_slots.py | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/tests/test_slots.py b/tests/test_slots.py index 9bdc087ae..60dd61c9b 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -232,8 +232,6 @@ def test_inheritance_from_slots(): @attr.s(slots=True, hash=True) class C2Slots(C1Slots): - # y re-defined here but it shouldn't get a slot - y = attr.ib() z = attr.ib() @attr.s(slots=True, hash=True) @@ -270,6 +268,42 @@ class C2(C1): assert {"x": 1, "y": 2, "z": "test"} == attr.asdict(c2) +def test_inheritance_from_slots_with_attribute_override(): + """ + Inheriting from a slotted class doesn't re-create existing slots + """ + + class HasXSlot(object): + __slots__ = ("x",) + + @attr.s(slots=True, hash=True) + class C2Slots(C1Slots): + # y re-defined here but it shouldn't get a slot + y = attr.ib() + z = attr.ib() + + @attr.s(slots=True, hash=True) + class NonAttrsChild(HasXSlot): + # Parent class has slot for "x" already, so we skip it + x = attr.ib() + y = attr.ib() + z = attr.ib() + + c2 = C2Slots(1, 2, "test") + assert 1 == c2.x + assert 2 == c2.y + assert "test" == c2.z + + assert {"z"} == set(C2Slots.__slots__) + + na = NonAttrsChild(1, 2, "test") + assert 1 == na.x + assert 2 == na.y + assert "test" == na.z + + assert {"y", "z"} == set(NonAttrsChild.__slots__) + + def test_bare_inheritance_from_slots(): """ Inheriting from a bare attrs slotted class works. From cf30d3ea038d56f97ec4d5cd915f68a35fe1d5ad Mon Sep 17 00:00:00 2001 From: drinob Date: Sat, 14 Nov 2020 18:50:19 +0300 Subject: [PATCH 4/4] Restored backwards-compatibility and added a test for it --- src/attr/_make.py | 26 ++++++++++++++++++++------ tests/test_slots.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index b31838c45..5bc322853 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -723,15 +723,20 @@ def _create_slots_class(self): # Traverse the MRO to collect existing slots # and check for an existing __weakref__. - existing_slots_and_base_names = set(self._base_names) + existing_slots = dict() weakref_inherited = False for base_cls in self._cls.__mro__[1:-1]: if base_cls.__dict__.get("__weakref__", None) is not None: weakref_inherited = True - existing_slots_and_base_names.update( - getattr(base_cls, "__slots__", []) + existing_slots.update( + { + name: getattr(base_cls, name) + for name in getattr(base_cls, "__slots__", []) + } ) + base_names = set(self._base_names) + names = self._attr_names if ( self._weakref_slot @@ -743,9 +748,18 @@ def _create_slots_class(self): # We only add the names of attributes that aren't inherited. # Setting __slots__ to inherited attributes wastes memory. - slot_names = [ - name for name in names if name not in existing_slots_and_base_names - ] + slot_names = [name for name in names if name not in base_names] + # There are slots for attributes from current class + # that are defined in parent classes. + # As their descriptors may be overriden by a child class, + # we collect them here and update the class dict + reused_slots = { + slot: slot_descriptor + for slot, slot_descriptor in iteritems(existing_slots) + if slot in slot_names + } + slot_names = [name for name in slot_names if name not in reused_slots] + cd.update(reused_slots) if self._cache_hash: slot_names.append(_hash_cache_field) cd["__slots__"] = tuple(slot_names) diff --git a/tests/test_slots.py b/tests/test_slots.py index 60dd61c9b..d5106ffb8 100644 --- a/tests/test_slots.py +++ b/tests/test_slots.py @@ -301,7 +301,35 @@ class NonAttrsChild(HasXSlot): assert 2 == na.y assert "test" == na.z - assert {"y", "z"} == set(NonAttrsChild.__slots__) + assert {"__weakref__", "y", "z"} == set(NonAttrsChild.__slots__) + + +def test_inherited_slot_reuses_slot_descriptor(): + """ + We reuse slot descriptor for an attr.ib defined in a slotted attr.s + """ + + class HasXSlot(object): + __slots__ = ("x",) + + class OverridesX(HasXSlot): + @property + def x(self): + return None + + @attr.s(slots=True) + class Child(OverridesX): + x = attr.ib() + + assert Child.x is not OverridesX.x + assert Child.x is HasXSlot.x + + c = Child(1) + assert 1 == c.x + assert set() == set(Child.__slots__) + + ox = OverridesX() + assert ox.x is None def test_bare_inheritance_from_slots():