From 2a6de5e5256e1e942f2316e681e0724f0bb3b01b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sun, 3 Dec 2017 08:14:08 +0100 Subject: [PATCH 1/2] Fix MRO traversal with multiple inheritance --- docs/examples.rst | 2 +- src/attr/_make.py | 28 ++++++++-------------------- tests/test_make.py | 2 +- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/docs/examples.rst b/docs/examples.rst index 4432e8fcb..39bd846da 100644 --- a/docs/examples.rst +++ b/docs/examples.rst @@ -115,7 +115,7 @@ This is useful in times when you want to enhance classes that are not yours (nic ... class B(object): ... b = attr.ib() >>> @attr.s - ... class C(B, A): + ... class C(A, B): ... c = attr.ib() >>> i = C(1, 2, 3) >>> i diff --git a/src/attr/_make.py b/src/attr/_make.py index 31c5f94ce..3787bc100 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -264,7 +264,7 @@ def _transform_attrs(cls, these, auto_attribs): if isinstance(attr, _CountingAttr) ), key=lambda e: e[1].counter) - non_super_attrs = [ + own_attrs = [ Attribute.from_counting_attr( name=attr_name, ca=ca, @@ -274,34 +274,22 @@ def _transform_attrs(cls, these, auto_attribs): in ca_list ] - # Walk *down* the MRO for attributes. While doing so, we collect the names - # of attributes we've seen in `take_attr_names` and ignore their - # redefinitions deeper in the hierarchy. super_attrs = [] - taken_attr_names = {a.name: a for a in non_super_attrs} + taken_attr_names = {a.name: a for a in own_attrs} + + # Traverse the MRO and collect attributes. for super_cls in cls.__mro__[1:-1]: sub_attrs = getattr(super_cls, "__attrs_attrs__", None) if sub_attrs is not None: - # We iterate over sub_attrs backwards so we can reverse the whole - # list in the end and get all attributes in the order they have - # been defined. - for a in reversed(sub_attrs): + 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: super_attrs.append(a) taken_attr_names[a.name] = a - elif prev_a == a: - # This happens thru multiple inheritance. We don't want - # to favor attributes that are further down in the tree - # so we move them to the back. - super_attrs.remove(a) - super_attrs.append(a) - - # Now reverse the list, such that the attributes are sorted by *descending* - # age. IOW: the oldest attribute definition is at the head of the list. - super_attrs.reverse() - attr_names = [a.name for a in super_attrs + non_super_attrs] + attr_names = [a.name for a in super_attrs + own_attrs] AttrsClass = _make_attr_tuple_class(cls.__name__, attr_names) diff --git a/tests/test_make.py b/tests/test_make.py index f8c80228a..70cfaf3ec 100644 --- a/tests/test_make.py +++ b/tests/test_make.py @@ -235,7 +235,7 @@ class D(A): d2 = attr.ib(default="d2") @attr.s - class E(D, C): + class E(C, D): e1 = attr.ib(default="e1") e2 = attr.ib(default="e2") From f312ed2b82e103962ee7c7423eefcd5b48d17fa9 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sun, 3 Dec 2017 08:15:12 +0100 Subject: [PATCH 2/2] Add newsfragments --- changelog.d/298.breaking.rst | 4 ++++ changelog.d/299.breaking.rst | 4 ++++ changelog.d/304.breaking.rst | 4 ++++ 3 files changed, 12 insertions(+) create mode 100644 changelog.d/298.breaking.rst create mode 100644 changelog.d/299.breaking.rst create mode 100644 changelog.d/304.breaking.rst diff --git a/changelog.d/298.breaking.rst b/changelog.d/298.breaking.rst new file mode 100644 index 000000000..904757d4a --- /dev/null +++ b/changelog.d/298.breaking.rst @@ -0,0 +1,4 @@ +The traversal of of MROs when using multiple inheritance was backward: +If you defined a class ``C`` that subclasses ``A`` and ``B`` like ``C(A, B)``, ``attrs`` would have collected the attributes from ``B`` *before* those of ``A``. + +This is fixed now however due to its nature, it may require fixes on your side too unfortunately. diff --git a/changelog.d/299.breaking.rst b/changelog.d/299.breaking.rst new file mode 100644 index 000000000..904757d4a --- /dev/null +++ b/changelog.d/299.breaking.rst @@ -0,0 +1,4 @@ +The traversal of of MROs when using multiple inheritance was backward: +If you defined a class ``C`` that subclasses ``A`` and ``B`` like ``C(A, B)``, ``attrs`` would have collected the attributes from ``B`` *before* those of ``A``. + +This is fixed now however due to its nature, it may require fixes on your side too unfortunately. diff --git a/changelog.d/304.breaking.rst b/changelog.d/304.breaking.rst new file mode 100644 index 000000000..904757d4a --- /dev/null +++ b/changelog.d/304.breaking.rst @@ -0,0 +1,4 @@ +The traversal of of MROs when using multiple inheritance was backward: +If you defined a class ``C`` that subclasses ``A`` and ``B`` like ``C(A, B)``, ``attrs`` would have collected the attributes from ``B`` *before* those of ``A``. + +This is fixed now however due to its nature, it may require fixes on your side too unfortunately.