Initialization order of fields overridden in subclasses

[badicsalex]

I have a code similar to the following, that relies very heavily on mypy for correctness:

import attr

class X:
    """ Some other class from the code """

class XButBetter(X):
    """ Some other class from the code """

@attr.s(auto_attribs=True)
class XContainer:
    the_x: X
    x_metadata: int = attr.ib(init=False)
    @x_metadata.default
    def _x_metadata_default(self) -> int:
        return self.the_x.get_metadata()

    def some_useful_function(self) -> str:
        """ A useful but very complex function dealing with self.the_x """

@attr.s(auto_attribs=True)
class BetterXContainer(XContainer):
    the_x: XButBetter
    def some_function(self) -> None:
        """ some code assuming the_x is the better version """

xc = XContainer(X())
xcb = BetterXContainer(XButBetter())

The last line fails with AttributeError: 'BetterXContainer' object has no attribute 'the_x', because the initialization order of the attributes changed in the subclass. Another similar issue is described on stackoverflow.

Any plans on implementing subclassing similarly to dataclasses? Would that be a breaking API change? (Probably.)

Related attrs issue I found: #637

PS.: I realize this is subclassing issue n+1 when subclassing is frowned upon, and this is somewhat similar to the classic OOP problem of subclassing Circle from Ellipse, so feel free to just say "damn man, this is not proper OOP, so wontfix".

[hynek]

I don't think we'll do subclassing DC style, but I could be convinced by a good design for #637 I think.

(The reason I don't like DC style is that it's not apparent from BetterXContainer the_x that it's been inherited. I try to keep attrs as explicit as possible.)

[Drino]

Sounds like another approach here may be lazy factory for x_metadata:
#573

The example from the first post seems to be solvable with pure @cached_property (though it wouldn't be a part of repr/str, but I'm not sure if you really need it, as it's a part of the_x already)

[badicsalex]

The main thing I want to get out of this is construction-time type checking, so I really must be able to override at least the type annotation of the_x.

Also, I didn't mention, but I use slotted frozen classes, and @cached_property wouldn't work.

My current workaround looks like this:

@attr.s(auto_attribs=True)
class BetterXContainer(XContainer):
    the_better_x: XButBetter = attr.ib(init=False)
    @the_better_x.default
    def _the_better_x_default(self) -> XButBetter:
        assert isinstance(self.the_x, XButBetter)
        return self.the_x

    def some_function(self) -> None:
        """ some code using the_better_x instead of the_x """

My previous workaround asserted for the type of the_x in post_init, and in all extension functions, which is kind of a pain.

But now that I'm thinking about it, I might use a @property instead of a new field, so thanks for that idea.

[Drino]

Why don't use validator? Like

def validate_type(self, attr, value):
    assert isinstance(value, attr.type)

@attr.dataclass(frozen=True, slots=True)  # .dataclass( is shorter than .s(auto_attribs=True,
class XContainer:
    the_x: X = attr.ib(validator=validate_type)

    @property
    def x_metadata(self) -> XMetadata: 
        return self.the_x.get_metadata()

@attr.dataclass(frozen=True, slots=True)
class BetterXContainer(XContainer):
    # Note, that attr.ib is required, as auto_attribs=True will create empty attr.ib automatically (thus - override all validators)
    the_x: XButBetter = attr.ib(validator=validate_type)

    # Not sure if it's needed, but in case your metadata types are different you can help mypy like that:
    if typing.TYPE_CHECKING:
        @property
        def x_metadata(self) -> XButBetterMetadata: ...  

[badicsalex]

Huh, if typing.TYPE_CHECKING: is another trick I haven't thought of. I've tried

@attr.s(auto_attribs=True)                               
class BetterXContainer(XContainer):                      
    if TYPE_CHECKING:                                    
        the_x: XButBetter                                

It solves the "ton of asserts in the methods" problem while not breaking anything (like initialization order and constructor parameter order). Runtime type checking can be done in __attrs_post_init__ (which can be generalized quite easily by going through all attributes and checking what we can)

Unfortunately my main remaining concern is static type checking at construction-time, and the mypy attrs plugin does not seem to care about this if.

(see https://github.com/python/mypy/blob/master/mypy/plugins/attrs.py#L320 although the comment is extremely misleading and possibly wrong EDIT: the only one who was wrong is me. see below)

[euresti]

The comment in question says:

                # When attrs are defined twice in the same body we want to use the 2nd definition
                # in the 2nd location. So remove it from the OrderedDict.
                # Unless it's auto_attribs in which case we want the 2nd definition in the
                # 1st location.

This was definitely what I observed when I coded this up though it might've changed since then. Let me put it to the test:

import attr

@attr.s
class A:
    x = attr.ib("first")
    y = attr.ib("y")
    x = attr.ib("second")

print(A())
print(A("one", "two"))

@attr.s(auto_attribs=True)
class B:
    x: str = "first"
    y: str = "y"
    x: str = "second"

print(B())
print(B("one", "two"))

This gives me:

A(y='y', x='second')
A(y='one', x='two')
B(x='second', y='y')
B(x='one', y='two')

So in A the 2nd definition of x (i.e. attrib("second") ends up after y
But in B the 2nd definition (i.e. "second") ends up in the first place.

But I was definitely not thinking of subclassing so that's probably another story.

When you say:

the mypy attrs plugin does not seem to care about this if.

What do you mean? Usually if TYPE_CHECKING is a trick to say "this exists only for mypy's sake, but not at runtime" Is that not what you're seeing?

[badicsalex]

Hi! Thanks for joining in 😄
I'm sorry, the comment is correct, I just didn't read it properly and notice "the same body".
(Defining an attribute two times in the same class didn't make much sense to me, so I thought this was about subclasses through some mypy magic, but now I see there is specific code for that just below)

It seems that attr.ib() attributes behind if TYPE_CHECKING don't get parsed in the child class, so the constructor of the child class will still have the type signature of the parent class. E.g. this does not make mypy complain:

class X:
    pass

class XButBetter(X):
    def i_am_better(self) -> None:
        pass

@attr.s(auto_attribs=True)
class XContainer:
    the_x: X

@attr.s(auto_attribs=True)
class BetterXContainer(XContainer):
    if TYPE_CHECKING:
        the_x: XButBetter = attr.ib()

    def some_function(self) -> None:
        self.the_x.i_am_better()

xc = XContainer(X())
xcb = BetterXContainer(XButBetter())
xcb = BetterXContainer(X())

When it really should in the last line. If I omit the if, it works as expected, but breaks various aforementioned things at runtime. If I omit the whole declaration, mypy complains that in some_function, the_x does not have a method i_am_better, which is reasonable.

I guess in the attrs plugin, only simple, top-level assignments are "detected" when you do

for stmt in ctx.cls.defs.body:
    if isinstance(stmt, AssignmentStmt):

[euresti]

Oh right. We're actually trying to read code in the middle of mypy. Ok so I think the workaround might be:

if TYPE_CHECKING:
    # For mypy's sake
    @attr.s(auto_attribs=True)
    class BetterXContainer(XContainer):
        the_x: XButBetter = attr.ib()

        def some_function(self) -> None:
            ...   # That's literally a dot dot dot
else:
    # The runtime implementation
    @attr.s(auto_attribs=True)
    class BetterXContainer(XContainer):

        def some_function(self) -> None:
            self.the_x.i_am_better()

[Drino]

Oh, I've missed the whole point about order of parameters in __init__ as in most of examples there is only one attribute that can be passed in __init__.

If I'd really cared about that - I would make all non-redefinable attributes kw_only (and redefine new ones in the right order):

from inspect import signature
from numbers import Integral
from typing import Sequence, List

import attr


@attr.dataclass
class Parent:
    x: Sequence[Integral]
    special: int = attr.ib(42, kw_only=True)

@attr.dataclass
class Specific(Parent):
    x: List[int]

print(signature(Parent.__init__))  # (self, x: Sequence[numbers.Integral], *, special: int = 42) -> None
print(signature(Specific.__init__))  # (self, x: List[int], *, special: int = 42) -> None