Fix attribute collection (#635)

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

changelog.d/428.change.rst

@@ -0,0 +1,6 @@
+Added ``@attr.s(collect_by_mro=False)`` argument that if set to ``True`` fixes the collection of attributes from base classes.
+
+It's only necessary for certain cases of multiple-inheritance but is kept off for now for backward-compatibility reasons.
+It will be turned on by default in the future.
+
+As a side-effect, ``attr.Attribute`` now *always* has an ``inherited`` attribute indicating whether an attribute on a class was directly defined or inherited.

changelog.d/635.change.rst

@@ -0,0 +1,6 @@
+Added ``@attr.s(collect_by_mro=False)`` argument that if set to ``True`` fixes the collection of attributes from base classes.
+
+It's only necessary for certain cases of multiple-inheritance but is kept off for now for backward-compatibility reasons.
+It will be turned on by default in the future.
+
+As a side-effect, ``attr.Attribute`` now *always* has an ``inherited`` attribute indicating whether an attribute on a class was directly defined or inherited.

docs/api.rst

@@ -16,7 +16,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=None, cmp=None, hash=None, init=None, slots=False, frozen=False, weakref_slot=True, str=False, auto_attribs=False, kw_only=False, cache_hash=False, auto_exc=False, eq=None, order=None, auto_detect=False)
+.. autofunction:: attr.s(these=None, repr_ns=None, repr=None, cmp=None, hash=None, init=None, slots=False, frozen=False, weakref_slot=True, str=False, auto_attribs=False, kw_only=False, cache_hash=False, auto_exc=False, eq=None, order=None, auto_detect=False, collect_by_mro=False)
 
    .. note::
 
@@ -102,7 +102,7 @@ Core
       ... class C(object):
       ...     x = attr.ib()
       >>> attr.fields(C).x
-      Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False)
 
 
 .. autofunction:: attr.make_class
@@ -178,9 +178,9 @@ Helpers
       ...     x = attr.ib()
       ...     y = attr.ib()
       >>> attr.fields(C)
-      (Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False))
+      (Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False), Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False))
       >>> attr.fields(C)[1]
-      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False)
       >>> attr.fields(C).y is attr.fields(C)[1]
       True
 
@@ -195,9 +195,9 @@ Helpers
       ...     x = attr.ib()
       ...     y = attr.ib()
       >>> attr.fields_dict(C)
-      {'x': Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), 'y': Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)}
+      {'x': Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False), 'y': Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False)}
       >>> attr.fields_dict(C)['y']
-      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False)
       >>> attr.fields_dict(C)['y'] is attr.fields(C).y
       True
 

docs/extending.rst

@@ -16,7 +16,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, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False),)
+   (Attribute(name='a', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False),)
 
 
 .. warning::

src/attr/_make.py

@@ -343,12 +343,74 @@ def _counter_getter(e):
     return e[1].counter
 
 
-def _transform_attrs(cls, these, auto_attribs, kw_only):
+def _collect_base_attrs(cls, taken_attr_names):
+    """
+    Collect attr.ibs from base classes of *cls*, except *taken_attr_names*.
+    """
+    base_attrs = []
+    base_attr_map = {}  # A dictionary of base attrs to their classes.
+
+    # Traverse the MRO and collect attributes.
+    for base_cls in reversed(cls.__mro__[1:-1]):
+        for a in getattr(base_cls, "__attrs_attrs__", []):
+            if a.inherited or a.name in taken_attr_names:
+                continue
+
+            a = a._assoc(inherited=True)
+            base_attrs.append(a)
+            base_attr_map[a.name] = base_cls
+
+    # For each name, only keep the freshest definition i.e. the furthest at the
+    # back.  base_attr_map is fine because it gets overwritten with every new
+    # instance.
+    filtered = []
+    seen = set()
+    for a in reversed(base_attrs):
+        if a.name in seen:
+            continue
+        filtered.insert(0, a)
+        seen.add(a.name)
+
+    return filtered, base_attr_map
+
+
+def _collect_base_attrs_broken(cls, taken_attr_names):
+    """
+    Collect attr.ibs from base classes of *cls*, except *taken_attr_names*.
+
+    N.B. *taken_attr_names* will be mutated.
+
+    Adhere to the old incorrect behavior.
+
+    Notably it collects from the front and considers inherited attributes which
+    leads to the buggy behavior reported in #428.
+    """
+    base_attrs = []
+    base_attr_map = {}  # A dictionary of base attrs to their classes.
+
+    # Traverse the MRO and collect attributes.
+    for base_cls in cls.__mro__[1:-1]:
+        for a in getattr(base_cls, "__attrs_attrs__", []):
+            if a.name in taken_attr_names:
+                continue
+
+            a = a._assoc(inherited=True)
+            taken_attr_names.add(a.name)
+            base_attrs.append(a)
+            base_attr_map[a.name] = base_cls
+
+    return base_attrs, base_attr_map
+
+
+def _transform_attrs(cls, these, auto_attribs, kw_only, collect_by_mro):
     """
     Transform all `_CountingAttr`s on a class into `Attribute`s.
 
     If *these* is passed, use that and don't look for them on the class.
 
+    *collect_by_mro* is True, collect them in the correct MRO order, otherwise
+    use the old -- incorrect -- order.  See #428.
+
     Return an `_Attributes`.
     """
     cd = cls.__dict__
@@ -405,24 +467,14 @@ def _transform_attrs(cls, these, auto_attribs, kw_only):
         for attr_name, ca in ca_list
     ]
 
-    base_attrs = []
-    base_attr_map = {}  # A dictionary of base attrs to their classes.
-    taken_attr_names = {a.name: a for a in own_attrs}
-
-    # Traverse the MRO and collect attributes.
-    for base_cls in cls.__mro__[1:-1]:
-        sub_attrs = getattr(base_cls, "__attrs_attrs__", None)
-        if sub_attrs is None:
-            continue
-
-        for a in sub_attrs:
-            prev_a = taken_attr_names.get(a.name)
-            # Only add an attribute if it hasn't been defined before.  This
-            # allows for overwriting attribute definitions by subclassing.
-            if prev_a is None:
-                base_attrs.append(a)
-                taken_attr_names[a.name] = a
-                base_attr_map[a.name] = base_cls
+    if collect_by_mro:
+        base_attrs, base_attr_map = _collect_base_attrs(
+            cls, {a.name for a in own_attrs}
+        )
+    else:
+        base_attrs, base_attr_map = _collect_base_attrs_broken(
+            cls, {a.name for a in own_attrs}
+        )
 
     attr_names = [a.name for a in base_attrs + own_attrs]
 
@@ -498,9 +550,10 @@ def __init__(
         kw_only,
         cache_hash,
         is_exc,
+        collect_by_mro,
     ):
         attrs, base_attrs, base_map = _transform_attrs(
-            cls, these, auto_attribs, kw_only
+            cls, these, auto_attribs, kw_only, collect_by_mro,
         )
 
         self._cls = cls
@@ -839,6 +892,7 @@ def attrs(
     eq=None,
     order=None,
     auto_detect=False,
+    collect_by_mro=False,
 ):
     r"""
     A class decorator that adds `dunder
@@ -998,6 +1052,13 @@ def attrs(
           default value are additionally available as a tuple in the ``args``
           attribute,
         - the value of *str* is ignored leaving ``__str__`` to base classes.
+    :param bool collect_by_mro: Setting this to `True` fixes the way ``attrs``
+       collects attributes from base classes.  The default behavior is
+       incorrect in certain cases of multiple inheritance.  It should be on by
+       default but is kept off for backward-compatability.
+
+       See issue `#428 <https://github.com/python-attrs/attrs/issues/428>`_ for
+       more details.
 
     .. versionadded:: 16.0.0 *slots*
     .. versionadded:: 16.1.0 *frozen*
@@ -1024,6 +1085,7 @@ def attrs(
     .. deprecated:: 19.2.0 *cmp* Removal on or after 2021-06-01.
     .. versionadded:: 19.2.0 *eq* and *order*
     .. versionadded:: 20.1.0 *auto_detect*
+    .. versionadded:: 20.1.0 *collect_by_mro*
     """
     if auto_detect and PY2:
         raise PythonTooOldError(
@@ -1050,6 +1112,7 @@ def wrap(cls):
             kw_only,
             cache_hash,
             is_exc,
+            collect_by_mro,
         )
         if _determine_whether_to_implement(
             cls, repr, auto_detect, ("__repr__",)
@@ -1884,11 +1947,15 @@ class Attribute(object):
     *Read-only* representation of an attribute.
 
     :attribute name: The name of the attribute.
+    :attribute inherited: Whether or not that attribute has been inherited from
+        a base class.
 
     Plus *all* arguments of `attr.ib` (except for ``factory``
     which is only syntactic sugar for ``default=Factory(...)``.
 
-    For the version history of the fields, see `attr.ib`.
+    .. versionadded:: 20.1.0 *inherited*
+
+    For the full version history of the fields, see `attr.ib`.
     """
 
     __slots__ = (
@@ -1904,6 +1971,7 @@ class Attribute(object):
         "type",
         "converter",
         "kw_only",
+        "inherited",
     )
 
     def __init__(
@@ -1915,6 +1983,7 @@ def __init__(
         cmp,  # XXX: unused, remove along with other cmp code.
         hash,
         init,
+        inherited,
         metadata=None,
         type=None,
         converter=None,
@@ -1948,6 +2017,7 @@ def __init__(
         )
         bound_setattr("type", type)
         bound_setattr("kw_only", kw_only)
+        bound_setattr("inherited", inherited)
 
     def __setattr__(self, name, value):
         raise FrozenInstanceError()
@@ -1970,6 +2040,7 @@ def from_counting_attr(cls, name, ca, type=None):
                 "validator",
                 "default",
                 "type",
+                "inherited",
             )  # exclude methods and deprecated alias
         }
         return cls(
@@ -1978,6 +2049,7 @@ def from_counting_attr(cls, name, ca, type=None):
             default=ca._default,
             type=type,
             cmp=None,
+            inherited=False,
             **inst_dict
         )
 
@@ -2042,6 +2114,7 @@ def _setattrs(self, name_values_pairs):
         order=False,
         hash=(name != "metadata"),
         init=True,
+        inherited=False,
     )
     for name in Attribute.__slots__
 ]
@@ -2087,6 +2160,7 @@ class _CountingAttr(object):
             kw_only=False,
             eq=True,
             order=False,
+            inherited=False,
         )
         for name in (
             "counter",
@@ -2109,6 +2183,7 @@ class _CountingAttr(object):
             kw_only=False,
             eq=True,
             order=False,
+            inherited=False,
         ),
     )
     cls_counter = 0

tests/test_dark_magic.py

@@ -131,6 +131,7 @@ def test_fields(self, cls):
                 order=True,
                 hash=None,
                 init=True,
+                inherited=False,
             ),
             Attribute(
                 name="y",
@@ -142,6 +143,7 @@ def test_fields(self, cls):
                 order=True,
                 hash=None,
                 init=True,
+                inherited=False,
             ),
         ) == attr.fields(cls)
 
@@ -199,6 +201,7 @@ def test_programmatic(self, slots, frozen):
                 order=True,
                 hash=None,
                 init=True,
+                inherited=False,
             ),
             Attribute(
                 name="b",
@@ -210,6 +213,7 @@ def test_programmatic(self, slots, frozen):
                 order=True,
                 hash=None,
                 init=True,
+                inherited=False,
             ),
         ) == attr.fields(PC)