Fixed slots inheritance (#718)

* Fixed slots inheritance

* Added changelog

* Added a separate test

* Restored backwards-compatibility and added a test for it

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

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

src/attr/_make.py

@@ -703,7 +703,6 @@ class _ClassBuilder(object):
         """
         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)
@@ -727,12 +726,21 @@ class _ClassBuilder(object):
                         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 = 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
-                break
+            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 (
@@ -746,6 +754,17 @@ class _ClassBuilder(object):
         # 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]
+        # 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)

tests/test_slots.py

@@ -268,6 +268,70 @@ def test_inheritance_from_slots():
     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 {"__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():
     """
     Inheriting from a bare attrs slotted class works.