diff --git a/changelog.d/428.change.rst b/changelog.d/428.change.rst new file mode 100644 index 00000000..5709b3e2 --- /dev/null +++ b/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. diff --git a/changelog.d/635.change.rst b/changelog.d/635.change.rst new file mode 100644 index 00000000..5709b3e2 --- /dev/null +++ b/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. diff --git a/docs/api.rst b/docs/api.rst index 23840928..0ddbc4ad 100644 --- a/docs/api.rst +++ b/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 diff --git a/docs/extending.rst b/docs/extending.rst index c92e1fd9..2a8bdafd 100644 --- a/docs/extending.rst +++ b/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:: diff --git a/src/attr/_make.py b/src/attr/_make.py index 7f0f137d..995555bd 100644 --- a/src/attr/_make.py +++ b/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 @@ class _ClassBuilder(object): 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 `_ 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 attrs( 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 @@ class Attribute(object): cmp, # XXX: unused, remove along with other cmp code. hash, init, + inherited, metadata=None, type=None, converter=None, @@ -1948,6 +2017,7 @@ class Attribute(object): ) 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 @@ class Attribute(object): "validator", "default", "type", + "inherited", ) # exclude methods and deprecated alias } return cls( @@ -1978,6 +2049,7 @@ class Attribute(object): default=ca._default, type=type, cmp=None, + inherited=False, **inst_dict ) @@ -2042,6 +2114,7 @@ _a = [ 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 diff --git a/tests/test_dark_magic.py b/tests/test_dark_magic.py index ac90f761..d86cfed8 100644 --- a/tests/test_dark_magic.py +++ b/tests/test_dark_magic.py @@ -131,6 +131,7 @@ class TestDarkMagic(object): order=True, hash=None, init=True, + inherited=False, ), Attribute( name="y", @@ -142,6 +143,7 @@ class TestDarkMagic(object): order=True, hash=None, init=True, + inherited=False, ), ) == attr.fields(cls) @@ -199,6 +201,7 @@ class TestDarkMagic(object): order=True, hash=None, init=True, + inherited=False, ), Attribute( name="b", @@ -210,6 +213,7 @@ class TestDarkMagic(object): order=True, hash=None, init=True, + inherited=False, ), ) == attr.fields(PC) diff --git a/tests/test_make.py b/tests/test_make.py index 96ef8a3c..310faf0e 100644 --- a/tests/test_make.py +++ b/tests/test_make.py @@ -167,7 +167,7 @@ class TestTransformAttrs(object): Does not attach __attrs_attrs__ to the class. """ C = make_tc() - _transform_attrs(C, None, False, False) + _transform_attrs(C, None, False, False, True) assert None is getattr(C, "__attrs_attrs__", None) @@ -176,7 +176,7 @@ class TestTransformAttrs(object): Transforms every `_CountingAttr` and leaves others (a) be. """ C = make_tc() - attrs, _, _ = _transform_attrs(C, None, False, False) + attrs, _, _ = _transform_attrs(C, None, False, False, True) assert ["z", "y", "x"] == [a.name for a in attrs] @@ -190,7 +190,7 @@ class TestTransformAttrs(object): pass assert _Attributes(((), [], {})) == _transform_attrs( - C, None, False, False + C, None, False, False, True ) def test_transforms_to_attribute(self): @@ -198,7 +198,7 @@ class TestTransformAttrs(object): All `_CountingAttr`s are transformed into `Attribute`s. """ C = make_tc() - attrs, base_attrs, _ = _transform_attrs(C, None, False, False) + attrs, base_attrs, _ = _transform_attrs(C, None, False, False, True) assert [] == base_attrs assert 3 == len(attrs) @@ -215,14 +215,14 @@ class TestTransformAttrs(object): y = attr.ib() with pytest.raises(ValueError) as e: - _transform_attrs(C, None, False, False) + _transform_attrs(C, None, False, False, True) assert ( "No mandatory attributes allowed after an attribute with a " "default value or factory. Attribute in question: 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)", + "kw_only=False, inherited=False)", ) == e.value.args def test_kw_only(self): @@ -245,7 +245,7 @@ class TestTransformAttrs(object): x = attr.ib(default=None) y = attr.ib() - attrs, base_attrs, _ = _transform_attrs(C, None, False, True) + attrs, base_attrs, _ = _transform_attrs(C, None, False, True, True) assert len(attrs) == 3 assert len(base_attrs) == 1 @@ -268,7 +268,7 @@ class TestTransformAttrs(object): y = attr.ib() attrs, base_attrs, _ = _transform_attrs( - C, {"x": attr.ib()}, False, False + C, {"x": attr.ib()}, False, False, True ) assert [] == base_attrs @@ -300,9 +300,9 @@ class TestTransformAttrs(object): assert "C(a=1, b=2)" == repr(C()) - def test_multiple_inheritance(self): + def test_multiple_inheritance_old(self): """ - Order of attributes doesn't get mixed up by multiple inheritance. + Old multiple inheritance attributre collection behavior is retained. See #285 """ @@ -337,6 +337,92 @@ class TestTransformAttrs(object): "d2='d2', e1='e1', e2='e2')" ) == repr(E()) + def test_overwrite_proper_mro(self): + """ + The proper MRO path works single overwrites too. + """ + + @attr.s(collect_by_mro=True) + class C(object): + x = attr.ib(default=1) + + @attr.s(collect_by_mro=True) + class D(C): + x = attr.ib(default=2) + + assert "D(x=2)" == repr(D()) + + def test_multiple_inheritance_proper_mro(self): + """ + Attributes are collected according to the MRO. + + See #428 + """ + + @attr.s + class A(object): + a1 = attr.ib(default="a1") + a2 = attr.ib(default="a2") + + @attr.s + class B(A): + b1 = attr.ib(default="b1") + b2 = attr.ib(default="b2") + + @attr.s + class C(B, A): + c1 = attr.ib(default="c1") + c2 = attr.ib(default="c2") + + @attr.s + class D(A): + d1 = attr.ib(default="d1") + d2 = attr.ib(default="d2") + + @attr.s(collect_by_mro=True) + class E(C, D): + e1 = attr.ib(default="e1") + e2 = attr.ib(default="e2") + + assert ( + "E(a1='a1', a2='a2', d1='d1', d2='d2', b1='b1', b2='b2', c1='c1', " + "c2='c2', e1='e1', e2='e2')" + ) == repr(E()) + + def test_mro(self): + """ + Attributes and methods are looked up the same way. + + See #428 + """ + + @attr.s + class A(object): + + x = attr.ib(10) + + def xx(self): + return 10 + + @attr.s + class B(A): + y = attr.ib(20) + + @attr.s + class C(A): + x = attr.ib(50) + + def xx(self): + return 50 + + @attr.s(collect_by_mro=True) + class D(B, C): + pass + + d = D() + + assert d.x == d.xx() + class TestAttributes(object): """ @@ -1339,7 +1425,7 @@ class TestClassBuilder(object): pass b = _ClassBuilder( - C, None, True, True, False, False, False, False, False + C, None, True, True, False, False, False, False, False, True ) assert "<_ClassBuilder(cls=C)>" == repr(b) @@ -1353,7 +1439,7 @@ class TestClassBuilder(object): x = attr.ib() b = _ClassBuilder( - C, None, True, True, False, False, False, False, False + C, None, True, True, False, False, False, False, False, True ) cls = ( @@ -1428,6 +1514,7 @@ class TestClassBuilder(object): is_exc=False, kw_only=False, cache_hash=False, + collect_by_mro=True, ) b._cls = {} # no __module__; no __qualname__ diff --git a/tests/utils.py b/tests/utils.py index 86fa3d2b..ad3fb578 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -46,6 +46,7 @@ def simple_attr( init=True, converter=None, kw_only=False, + inherited=False, ): """ Return an attribute with a name and no other bells and whistles. @@ -60,7 +61,8 @@ def simple_attr( hash=hash, init=init, converter=converter, - kw_only=False, + kw_only=kw_only, + inherited=inherited, )