Add auto-detection of self-implemented methods

[hynek]

This adds the feature to auto-detect existing methods so the creation doesn't have to be prevented by hand. This has been confusing our users for a while (see #324, #416). It should become True by default when the new APIs emerge (attr.auto/attrs.define – this is actually one of the last blockers for me to start working on them).

Typing requires an update to the mypy plugin to work correctly (currently it says tests/typing_example.py:193: error: Name '__init__' already defined on line 190).

There is really only one controversial open question: if a method is detected, should it just prevent the one method or all of the related ones? E.g. if I define __eq__, what about __ne__?

I have decided to take the presence of any method that belongs to a flag to set the flag to False because it's easier to reason about. I'm open to discussion tho.

[euresti]

Is the concept with __init__ that it replaces the auto-created init, or that it gets turned into post_attr_init_?

[hynek]

Replaces. In the case of init we could consider special-case it to optionally create a __attrs_init__ that the user can call whenever to fix the “how can I call super()” problem forever.

[wsanchez]

There is really only one controversial open question: if a method is detected, should it just prevent the one method or all of the related ones? E.g. if I define __eq__, what about __ne__?

As a user, I would expect it to work like functools.total_ordering.

[hynek]

There is really only one controversial open question: if a method is detected, should it just prevent the one method or all of the related ones? E.g. if I define __eq__, what about __ne__?

As a user, I would expect it to work like functools.total_ordering.

Hm that makes it a lot more complex to both implement and reason about. 🤔 The current approach is about detecting implied settings, yours would be very different and then the option should be called do_no_overwrite_own or something?

[wsanchez]

There is really only one controversial open question: if a method is detected, should it just prevent the one method or all of the related ones? E.g. if I define __eq__, what about __ne__?

As a user, I would expect it to work like functools.total_ordering.

Hm that makes it a lot more complex to both implement and reason about. 🤔 The current approach is about detecting implied settings, yours would be very different and then the option should be called do_no_overwrite_own or something?

Yeah, that's harder, but I think it does fall solidly into the "remove boilerplate" goal of attrs.

That said, if we want to keep things simple, then I'd definitely favor disabling all related methods. My reasoning there is that I suspect that the alternative is complicated in a worse way than total_ordering:

If, for example, you only see __eq__ in a class and you only disable __eq__ and leave the attrs-generated __ne__ in place, then we'd have to document the implementation of __ne__ (e.g. NotImplemented if a different class, or inverse of __eq__) and call that part of the API. That seems straightforward enough, I guess (though committing to an implementation, even a very simple one, makes me nervous).

But then what happens if you instead find a class that only defines __ne__? Does __eq__ simply invert that or do what it currently does?

[hynek]

Yeah this is exactly why I’m apprehensive of the granular version. I’m already seeing the issues and questions from confused people.

Couldn’t people actually use total_ordering?

[wsanchez]

Yeah this is exactly why I’m apprehensive of the granular version. I’m already seeing the issues and questions from confused people.

For sure. What I mean is that for your "should it just prevent the one method or all of the related ones?" question, I would opt for the latter. Otherwise, you either have to explain how what is left is implemented, or do something like total_ordering.

Couldn’t people actually use total_ordering?

Sure. Couldn't auto_detect actually use total_ordering? :-)

[hynek]

Couldn’t people actually use total_ordering?

Sure. Couldn't auto_detect actually use total_ordering? :-)

It could but it would introduce an unprecedented amount of magic. I really prefer to add one line and be clear about what's happening here.

Unless I'm missing something, I think I've made up my mind here. I should add a functional test that verifies that people actually can use total_ordering.

[hynek]

I have added an example for @total_ordering in 2cd16a2.

[pganssle]

My initial intuition here is to agree with @wsanchez. As a user, having attrs use roughly the same semantics as functools.total_ordering would be the kind of thing I'd use to pitch the library to people: look at how little boilerplate you need for this! Look how seamlessly it scales up! Truly they are as gods among men who have crafted this masterpiece!

That said, I suspect that @hynek's reluctance here is at least partially borne of having tried to do it that way in the first place and realizing all the edge cases that would be super hard to address. I agree that the two options are basically "do it like functools.total_ordering" and "implement everything or nothing".

One possibility that I think might allow us to kick the can down the road a bit: what if we made the ambiguous condition (the relevant attrs attribute is unset but you've only defined one of the dunder methods) a hard failure, with a voluminous error message explaining other ways to accomplish the goal? Assuming @functools.total_ordering actually defines all the methods, that should allow people to choose whether they want to just decorate with @functools.total_ordering or to explicitly disable the relevant method auto-generation.

If we know that it's a hard failure, a later release adding functools.total_ordering-like (or really any) semantics to the library would be fully backwards compatible.

[pganssle]

Suggested change

	If the argument implies more than one methods (e.g. ``eq=True`` creates both ``__eq__`` and ``__ne__``), it's enough for *one* of them to exist and ``attrs`` will create *neither*.
	If the argument implies more than one method (e.g. ``eq=True`` creates both ``__eq__`` and ``__ne__``), it's enough for *one* of them to exist and ``attrs`` will create *neither*.

[hynek]

Honestly to me this is much less about the complexity to implement it (that's a one-time deal mostly) and much more about the clarity and obviousness when looking at attrs-using code.

I don't think expecting people to add one line to their code is too much to ask, if it makes the whole thing more explicit and clear about what happens. Also once we implement #602 (which after this lands might be a very good opportunity), the users can directly deduct attrs behavior from the settings.

attrs was always about saving boilerplate but it also was always about being explicit and clear. Here we have to choose what is more important and my gut feeling is telling me that clarity wins.

[pganssle]

Well, my contention is that there are two very intuitive options here and I think either option would be surprising to some people because the semantics of this are simply not obvious.

One problem I have with the "specifying any one method flips the default for all methods" version is that it seems very unlikely that I would want that behavior, so I'd want to have to be explicit about it. If you make it so that specifying __lt__ silently flips ord=True to ord=False, I think you'll get a lot of users confused as to why x > y is not working as they expected.

I think some people will want to explicitly define a partial ordering, but those people will know that they want it, and they'll either implement all the methods or they can explicitly provide ord=False.

Given the asymmetry in expectations, I would think that the intuitive options are:

1. Specifying a partial ordering / partial equality with auto detection and eq/ord not explicitly set raises an exception (so anyone who thinks it works like the second option are put on notice).
2. Specifying a partial ordering / partial equality with auto detection defines a total ordering in terms of the user-defined methods.

I suspect that if you go with the first option, you'll get a bunch of people who say, "Why raise an exception, there's not really another thing you'd want to do here!" Of course, people thoughtlessly complain about everything with non-obvious semantics as if the semantics should be obvious, so that's not necessarily a lot to go on.

That said, maybe I'm completely missing some edge cases where this could lead to major footguns, so I'm totally willing to be convinced.

As I said before, if we go with option 2 (throwing in the ambiguous case), we should have the option to switch to option 1 in the future in a backwards-compatible way if it turns out I'm right about the asymmetry in use cases and intuition.

[wsanchez]

I've come around to agreeing with @hynek about users just using total_ordering explicitly if that's what they want.

I'm sympathetic to the argument that if ord=true (or unspecified, since it's normally the default), that rather than altering the default behavior when you add an __lt__ method, raising an exception may be more correct in a sense. But attrs changes the defaults in other similar cases, so I don't think that we should inconsistent about that in this one.

[pganssle]

Hm, I feel like I must be missing something, because my most recent post started out conceding the point and as I started trying to write out my reasoning for that I ended up going in exactly the opposite direction.

For me the issue comes down to what behavior users would want and what they would expect. I would think that most people who define a partial ordering want a total ordering. I can think of very few scenarios where that isn't the case, and in all of those scenarios, I'd be trying to craft specific semantics and I would test those semantics explicitly.

To me it seems like the fraction of people who don't want total order extrapolation, are expecting that defining a partial order will set ord=False and who don't carefully test their implementation is very low. On the other hand, in addition to the prior of "the user wants total ordering" being much higher, I think the fraction of those people who don't carefully test the ordering semantics is higher as well, so I'd expect a lot of broken classes being deployed unless ord=None + partial ordering = total ordering OR ord=None + partial ordering = exception.

But maybe my priors are way off. Do you disagree with my premises, my reasoning or am I maybe missing another line of reasoning here?

(Note: I will admit that the "should you generate __eq__ when only __ne__" question almost had me change my mind here, since generating __eq__ from __ne__ seems weird, but having eq do something other than ord would be very bad. In the end, I decided that these practical concerns override my squeamishness about generating an __ne__).

[hynek]

As Paul wrote correctly: no matter what way we go, someone is going to be confused. I think this is the premise that everything starts from.

---

There has been a lot of text and I'm a bit lost, but it seems to me, that Paul is arguing for an implicit total ordering?

As I've laid out before, I don't like it because it it's too much implicit behavior. I like being able to look at an attrs class and know what's happening without considering all kinds of implications and I think asking the user to add one line @functools.total_ordering is a good compromise here.

Making the existence of methods switch off a switch is a very clear semantic that can be easily communicated: "if you implement __eq__ then eq=False". This is not true if we just skip implementing certain methods based on their presence. The value of eq suddenly means nothing anymore. There's no way we make it the behavior introspectable in any way.

This feels harder to understand and harder to reason about – especially if we magically start applying a total ordering to the class.

But maybe I'm just misunderstanding? We def – at least partially – argue about different things here.

---

Random nit: in future APIs ord will be False by default I think. Most people don't need it so it's unnecessary baggage.

[pganssle]

There has been a lot of text and I'm a bit lost, but it seems to me, that Paul is arguing for an implicit total ordering?

I like implicit total ordering as the behavior of ord=True (and the default behavior if ord defaults to True) when a partial ordering is defined, but I am not sure I totally grok the arguments against it since yes it is a bit magical but I think in the sense that it will magically do what almost everyone wants and probably won't bother the people who don't want that behavior. I'm definitely willing to concede that you and @wsanchez understand the ways the semantics can get hairy better than me, so my position is "I'm probably wrong about this."

The thing I am more confident about is that if you don't go with total ordering, I think that unless ord is explicitly set to False, I think it should be an exception to define a partial ordering, because I think almost no one actually wants a partial ordering, and it's better to loudly fail and say, "If you really want a partial ordering, set ord=False, if you want a total ordering, using functools.total_ordering".

---

Making the existence of methods switch off a switch is a very clear semantic that can be easily communicated: "if you implement __eq__ then eq=False". This is not true if we just skip implementing certain methods based on their presence. The value of eq suddenly means nothing anymore. There's no way we make it the behavior introspectable in any way.

This is very fair, and I think I'm starting to understand the concern here. I was thinking that eq=True would mean "this class is guaranteed to define both __eq__ and __ne__" and ord=True would mean "this class is guaranteed to define a total ordering" and not necessarily "if x is True/False, attrs generates these methods". I think that's fairly easy to grok, but if you think that what people care about is what methods attr.s will generate or if exposing that information in a public interface is a critical design goal, then I concede the point about total ordering (though I'm still not quite convinced on the point of silent vs. exception).

[hynek]

Paul, to sum it up (and me stopping procrastinating on this): you would be (begrudgingly) OK with the current implementation, if we raise a Warning if the the user forgets to implement certain methods.

Am I understanding you correctly?

[pganssle]

@hynek Yeah, that sounds about right.

[hynek]

So I've implemented the warning and realized that there's a problem: we'd force the decorator order on the users.

@attr.s(auto_detect=True)
@total_ordering
class C:
    ...

would be fine while

@total_ordering
@attr.s(auto_detect=True)
class C:
    ...

would annoy the users.

I'm not really sure what to do here?

[pganssle]

Hm... That is indeed annoying.

I think it really depends on something I don't have a huge amount of information about, which is the baseline rates of this kind of confusion. If a lot of people are going to be silently getting broken behavior from this because they don't know they need to use functools.total_ordering with auto_attrib=True if they want to define a custom comparison, then it's probably justified to warn people who do the decorator in the wrong order (possibly with an additional line like, "Make sure the @attr.s decorator is on top to avoid this warning").

If almost everyone who writes one knows to use functools.total_ordering but tends to put it outside the attr.s decorator, then it's just annoying people for no reason.

Luckily, I don't think either decision is irreversible (at least when deciding between warning / nothing rather than deciding between exception / warning / nothing). You can always start raising a warning later if you find people are having this problem a lot, or you can start with the warning and remove it if people complain a lot.

I think you probably have a better handle than me on what attrs users will want, so I'm confident that whatever you choose is right. Sorry if all I've managed to add to this thread was delays and extra noise.

[hynek]

You can always start raising a warning later if you find people are having this problem a lot, or you can start with the warning and remove it if people complain a lot.

Yeah I think we'll go without for now – with a fat warning (that nobody will read).

I think you probably have a better handle than me on what attrs users will want, so I'm confident that whatever you choose is right.

With 26 million downloads per month, I don't think there's such a thing as an “attrs user”. :) I guess the best thing I can do is to build something I want to use.

Sorry if all I've managed to add to this thread was delays and extra noise.

Heh delays caused by food for thought are always very welcome.

[hynek]

Since I believe we have agreement on the design/API, I hope someone will find the time to give the implementation a review now. ❤️🐶

[hynek]

OK I don't think waiting any further will make anyone come forward so I'm just YOLOing it in. Otherwise I'll never get Operation import attrs done.