Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attrs-decorated classes cannot participate in cooperative inheritance #640

Closed
NeilGirdhar opened this issue Apr 16, 2020 · 7 comments
Closed

Comments

@NeilGirdhar
Copy link

NeilGirdhar commented Apr 16, 2020

Dear attrs team, great project!

Summary: Because attrs-decorated classes don't call super in __init__, they cannot participate in cooperative multiple inheritance.

I might be alone in this interpretation, but I imagine that there are three fundamental kinds of inheritance patterns for methods, which I defined in my ipromise package: implementing an abstract method, overriding, and augmenting. If I had to choose, I would say that __init__ should be an augmenting pattern.

If that interpretation is correct, then __init__ should call super. Even if a user wants Y to override some behavior in X, what happens if Z inherits from Y and W? Now, Y.__init__'s decision not to call super would mean that W.__init__ would not be called. That seems like a bug. Instead, I would rather put the behavior that Y wants to override in a separate method, say X.f, which is called in X.__init__. Now, if Y.f overrides X.f, everything is okay. Even if Z inherits from Y and W, the override still works, and W.__init__ still gets called, angels sing, etc.

Long story short, am I wrong to interpret __init__ as an augmenting method and ensure that it always call super().__init__(**kwargs)? Are there any downsides to attrs-generated __init__ providing this behavior?

@hynek
Copy link
Member

hynek commented Apr 17, 2020

Generally speaking we're not super fond of subclassing (although the biggest chunk of work for the next release was fixing attrs behavior in diamond shaped hierarchies 😱) so attrs behavior is optimized for the case where you don't do subclassing at all or where you use attrs throughout (which gives you an optimized __init__ for all classes involved and with the next release it even will get the MRO right in multiple inheritance).

This will never change since our opinion on subclassing will not change. :) (I'll spare you the preaching here why ;))

Currently you can run super from a __attrs_post_init__. Unfortunately that means that you can't participate with classes that require a call to super() before attributes are set. There's an issue about that somewhere and I'm open to get a more flexible hook system in place, such that users can run code before our init. But currently I'm too busy with other stuff so I hadn't time looking into it.

@NeilGirdhar
Copy link
Author

NeilGirdhar commented Apr 17, 2020

we're not super fond of subclassing

I feel you. I'm all for composition over inheritance. I avoid inheritance where possible. However, it is difficult to avoid inheritance. If a class expects a State object, and I need my state to have some behavior or extra members, then I have to use inheritance. Type annotations also induce the use of inheritance.

attrs behavior is optimized for the case where you don't do subclassing at all

So, if I understand correctly, the reason attrs doesn't call super is because the super call is too expensive? But you call __attrs_post_init__ unconditionally, and that's somehow cheaper?

you can't participate with classes that require a call to super() before attributes are set.

That would be fine except, unless I missed something, you're not able to forward **kwargs to super().__init__(**kwargs). That means no cooperative inheritance.

If optimization is the reason that you don't unconditionally delegate to super, could I humbly request a flag on attr.s that delegates to super? E.g., cooperative=True? It does seem like a bit of over-optimization to me though.

If cooperative is False, you should probably check to make sure that the decorated class does not inherit from anything. Also, the decorated class should raise in __init_subclass__ to prevent anyone inheriting from it. This is because without the super call, attrs-classes shouldn't participate in cooperative inheritance. I guess the exception is when your mro list comprises attrs-decorated classes, which you are telescoping.

@euresti
Copy link
Contributor

euresti commented Apr 17, 2020

Type annotations also induce the use of inheritance.

You might be able to use Protocols to avoid the need for inheritance in type annotations. See https://www.python.org/dev/peps/pep-0544/

@NeilGirdhar
Copy link
Author

@euresti Thanks, that's true.

@hynek
Copy link
Member

hynek commented Apr 18, 2020

So, if I understand correctly, the reason attrs doesn't call super is because the super call is too expensive? But you call __attrs_post_init__ unconditionally, and that's somehow cheaper?

We don't call __attrs_post_init__ unconditionally: we only call it if it's there. This is the whole philosophy of attrs: every code that is generated is hyper-optimized.

If optimization is the reason that you don't unconditionally delegate to super, could I humbly request a flag on attr.s that delegates to super? E.g., cooperative=True? It does seem like a bit of over-optimization to me though.

How would you expect it to work? Have you seen https://fuhm.net/super-harmful/? It's a bit more complicated than it sounds in practice.


I promised not to preach, but I can't help myself: have you seen Brandon's brilliant post https://python-patterns.guide/gang-of-four/composition-over-inheritance/? Not telling you to convince you – telling you because I have the urge to tell everybody. :)

@NeilGirdhar
Copy link
Author

NeilGirdhar commented Apr 18, 2020

I promised not to preach, but I can't help myself: have you seen Brandon's brilliant post https://python-patterns.guide/gang-of-four/composition-over-inheritance/? Not telling you to convince you – telling you because I have the urge to tell everybody. :)

I haven't seen this, but this looks very good! Great presentation of design patterns and yes to preferring composition over inheritance! I mentioned the principle earlier in this issue ("I'm all for composition over inheritance.") But like I said, composition is not always the best solution. Sometimes, you really do want inheritance. That's just a reality of design.

Have you seen https://fuhm.net/super-harmful/

I don't really like the tone of frustration of this article, but I mainly agree with its conclusions:

  • Use it consistently, and document that you use it, as it is part of the external interface for your class, like it or not.

Yes.

  • Never call super with anything but the exact arguments you received, unless you really know what you're doing.

Yes.

  • When you use it on methods whose acceptable arguments can be altered on a subclass via addition of more optional arguments, always accept *args, **kw, and call super like "super(MyClass, self).currentmethod(alltheargsideclared, *args, **kwargs)". If you don't do this, forbid addition of optional arguments in subclasses.

Close: You don't need args.

  • Never use positional arguments in __init__ or __new__. Always use keyword args, and always call them as keywords, and always pass all keywords on to super.

Close. It's okay to have positional arguments like this:

class A:
     def __init__(self, a, b, **kwargs):
        super().__init__(**kwargs)
        self.a = a
        self.b = b

class B(A):
    def __init__(self, c, d, **kwargs):
        super().__init__(**kwargs)
        self.c = c
        self.d = d

x = B(c=3, d=4, a=1, b=2)
y = B(3, 4, a=1, b=2)

Callers to B can see that they can't use positional arguments to specify a and b, which is fine. There is no problem if other classes comes along:

class C(A): ...

class D(B, C):
    def __init__(self, e, f, **kwargs):
         super().__init__(**kwargs)
         ...

I contend that __init__ is one of these methods he's talking about it. Every __init__ in a cooperative inheritance needs to delegate to super along with all kwargs. The same goes for __new__, __init_subclass__, __enter__ and __exit__, etc. I agree that it can seem a "bit complicated".

The way I see it, there are three choices for every attrs-decorated class D:

  1. Check that there are no non-attrs classes in D's ancestors that define an __init__. These would be broken by the failure to delegate.

  2. Add **kwargs to the signature of D.__init__ and call super().__init__(**kwargs) wherever you think is best in its body.

  3. Add a flag to attr.s like cooperative=True that selects between these two options.

I understand your desire to “hyper-optimize”, so maybe the best option is the flag. My personal choice would be delegation since the kinds of programs I write are not affected by such minor computational optimizations.

I mainly want attrs for the rapid prototyping. What makes me so uncomfortable though is that if I forget which classes are attrs-decorated, then I end up with subtle bugs when constructors aren't called. It's too much mental overhead to remember.

What do you think?

@NeilGirdhar
Copy link
Author

In the end, it was easier for me to just add cooperative_dataclasses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants