Rooba
/
attrs
mirror of https://github.com/python-attrs/attrs.git
1
1
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix attribute collection (#635) 

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
This commit is contained in:
Hynek Schlawack 2020-04-06 11:41:52 +02:00 committed by GitHub
parent f8f3f598a3
commit 33b61316f8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 221 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
changelog.d/428.change.rst Normal file
View File

@ -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.

6
changelog.d/635.change.rst Normal file
View File

@ -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.

12
docs/api.rst
View File

@ -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

2
docs/extending.rst
View File

@ -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::

117
src/attr/_make.py
View File

@ -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 <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 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

4
tests/test_dark_magic.py
View File

@ -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)

111
tests/test_make.py
View File

@ -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__

4
tests/utils.py
View File

@ -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,
)