attrs._make should document why it is more restrictive with __eq__ than other comparisons

[Julian]

The type check in __eq__ for generated classes is different than in other comparison methods.

For all other methods, isinstance(other, self.__class__) is used, which means subclasses will participate in the "happy" branch of the comparison.

For __eq__ though, other.__class__ is self.__class__ is used, so a trivial subclass will not compare equal, leading to the quite confusing:

>>> import attr; Parent = attr.make_class("Parent", dict(foo=attr.ib())); Child = type("Child", (Parent,), {}); print (Parent(foo=1) == Parent(foo=1), Parent(foo=1) == Child(foo=1), Parent(foo=1) < Parent(foo=2), Child(foo=1) < Parent(foo=2))
(True, False, True, True)

This strikes me as a bug (the incongruity), and that __eq__ should use the same check, but even if it isn't, it likely bears mentioning that there's a difference.

It even seems like dataclasses have even done something oddly similar, maybe just straight copying the code here?

https://github.com/python-attrs/attrs/commit/d134ce45fc98323576a19f03e39669dce615c4e1 looks like it's the commit that originally made the change (though since then __eq__ now is hand-generated).

[wsanchez]

Agreed; a reasonable developer would be surprised by the asymmetry described.

(FWIW I am; though I try not to subclass, so hadn't noticed this.)

[Julian]

Well yeah same, I think attrs should even have a attr.s(..., allow_subclassing=True|False) and default it to false :P, but that's a different story.

[mahmoud]

Agree on all counts.

And while it may be a bit off topic, I just wanted to echo a similar thought (experiment), that inheritance might be somewhat redeemed in a world where most types aren't open to extension by default. Not sure how deep attrs wants to dip into metaclasses.

[wsanchez]

I suggest we start with some understanding about why this difference exists at all; without that, it's not obvious to me that it's even intentional. (Though I want to assume that it is.)

[hynek]

Well yeah, this one even made it to LWN: https://lwn.net/Articles/740153/

The question is in what direction we want to go. I find it hard to come to terms that two objects can be equal if they have different types. So my gut feeling would be to add a warning if someone compares instances that have different types and remove it in a year.

Bonus points for an option to allow to configure the behavior for both?

[Julian]

Well yeah, this one even made it to LWN: https://lwn.net/Articles/740153/

Fun :)

So my gut feeling would be to add a warning if someone compares instances that have different types and remove it in a year.

+1

Bonus points for an option to allow to configure the behavior for both?

If I steal my own idea about repr, one could support this by having attr.s(cmp=lambda self, other: isinstance(other, self.__class__)) (vs cmp=lambda self, other: self.__class__ is other.__class__, or lambda self, other: False / True), though I'm not inventive enough at the minute to think of any reason someone would want something other than those four options.

[wsanchez]

@hynek I think I'm hearing that the existing difference is, in fact, accidental? Anyway, I like the idea of warning/deprecating the isinstance behavior in the default comparators.

I like the @Julian's idea of cmp accepting a callable, and have be thinking about that for a while… but I'd like to consider other uses than the type precondition. For example, I'd like to be able to change the compared data from the default of calling attrs_to_tuple on each object. In my case I've wanted to change the order of the attributes, or have one of them sorted the other direction than the default ("descending"), and right now I have to implement my own comparators in order to do that. More generally, one might want cmp to accept a callable like Python 2's __cmp__ operator function…

[Julian]

If you want to swap out the whole implementation wouldn't you just use cmp=False?

[wsanchez]

Well, it's a lot more… boilerplate to implement all of the __foo__ methods than it is to provide an alternative to attrs_to_tuple.

Probably I should fess up and show an example. I did this as a mix-in such that all I need to do in classes that inherit from the mix-in is implement _cmpValue. Here's a simple example in which all I do is change the order of the compared values because I want description to be factored in last. Here's another when I just invert one of the values. This is a lot easier than implementing all of the dunders in each class.

Sorry if this is a tangent; it probably should be a separate ticket, but it effects this if we want to overload cmp, so just trying to get ahead of that.

[altendky]

I'm not quite sure which way this thread is leaning at this point. is or isinstance()? If configurable, then which default?

[Julian]

@wsanchez ah I see, well, I guess if we're dragging tangents into things I am pretty strongly against the way the current implementation combines equality with ordering, which is at least partially relevant for what you're doing there. Possibly yet a third ticket (or maybe it exists already I forget) though.

[wsanchez]

@altendky My read of @hynek's comment is that we'd prefer is as the default, we should deprecate the current use of isinstance, and configurability is a bonus. (And I agree with all that.)

[wsanchez]

@Julian yeah, I agree that combining the two is problematic. Th reason I think it's relevant here is that I suspect any configuration option we pick here (if we do) could make fixing that problem harder.

[altendky]

I guess I don't follow how is would be expected behavior when inheriting. I get tending away from inheritance (more or less strongly) but trivially inheriting a class shouldn't make it not equal, should it? Wouldn't LSP encourage isinstance()? I'm split between thinking I'm missing something and thinking that a dislike of inheritance is going to result in breaking it for those that use it.

Note: I don't think I've inherited from an attrs class yet so I'm not arguing because of any direct effect I expect it to have on me.

[wsanchez]

@altendky It's a fair point, because it's really non-obvious and I don't know of a settled best practice here, but the is check is simply being conservative, because the parent class' implementation of, say __eq__ can't be sure that a subclass is equal any more than it can of a rando object.

If the subclass adds a field (certainly not uncommon), that field… wait… yes?… that field is not factored into the equality test. (I hesitate because I'm looking at attrs_to_tuple and it's a closure that uses the same attrs for both self and other, but I'm not super familiar with that code.)

The thing is that the default implementation provided by attrs doesn't know what a subclass considers equal, so returning NotImplemented is just saying "I dunno".

It would be nice if one could explicitly declare a subclass as considering its instances equal to those of the parent class with equal field values, but I'm a little wary of an implementation that assumes this is true.

[altendky]

I need to get a better understanding of LSP. It seems to be part of 'proper' inheritance, but it also seems to hog tie subclasses.

[hynek]

Random thought: isinstance also means that if you compare a and b and b is a subclass of a, you get different results for a == b and b == a.

[Julian]

Not sure now I'm following the last few comments --

the two options though would have to be is, or isinstance plus a field length check, because a subclass that adds fields seems pretty clearly like it shouldn't be == according to anyone, so you can't use just isinstance.

But in the latter case (isinstance + field length check) you get associativity and transitivity, so not sure I follow there, maybe I'm missing something.

And if I didn't mention it earlier, yeah personally I'd lean towards that one, but since I am in the "don't use inheritance" camp I wouldn't argue heavily for it :P

[altendky]

I guess comparisons are a well known exception to LSP? Or LSP is just considered not relevant anymore?

I still need to study it...

[Julian]

No your argument is good IMHO :)

[altendky]

Then even isinstance(other, type(self)) is too restrictive because a trivial child will reject a parent with matching attribute values. Unless LSP is only about full substitution of all instances to the sun type and not about mixing parent and child instances. I'd think this would agree with both other languages where a parent comparison method wouldn't even be aware of any new attributes on a child class as well as aligning with duck typing in Python.

[hynek]

Then even isinstance(other, type(self)) is too restrictive because a trivial child will reject a parent with matching attribute values.

This is what I meant in https://github.com/python-attrs/attrs/issues/387#issuecomment-395295204 btw. I guess we’d have to check both ways and that is kind of getting silly.

The more I think about it, the more I’m convinced we should go the is route and only add more options if there’s enough people complaining.

[altendky]

What about using super()? If that passes and isinstance(other, type(self)) then extra attributes of the subclass are compared. I think that would pass. I think that to satisfy LSP that any attributes added in a subclass would have to have defaults.

[Julian]

That won't work when the parent is on the left of the comparison -- it would call super, and not return NotImplemented, and then proceed to only compare to a subset of the subclass's fields.

Though again I must not be following because this is the one option that I think shouldn't be on the table -- the only reasonable option that uses isinstance is returning NotImplemented for not isinstance(other, self.__class__) or attr.fields(self) != attr.fields(other), just using the first part of the conditional there I think definitely produces nonsense for subclasses that add extra fields.

[altendky]

@Julian I'll try an example and see what I see. It does seem odd though that we are talking about inheritance and the one thing to not consider using is super().

[wsanchez]

For what it's worth, there's also:

@attr.s
class Rectangle(object):
    width: float
    height: float

    def area(self) ➜ float:
        return self.width * self.height

@attr.s
class RedRectangle(object):
    pass

@attr.s
class BlueRectangle(object):
    pass

In which case, one might assert RedRectangle(1,2) != BlueRectangle(1,2) and perhaps even assert RedRectangle(1,2) != Rectangle(1,2). This is obviously contrived, but I don't think one can really just assume that subclasses with the same fields are equal.

Also: I see the auto-correct typo above, but Python really should accept ➜ as equivalent to ->.

[xgid]

Just my two cents here.

The most accurate definition of LSP I've ever found is this one:

Objects of subtypes should behave like those of supertypes if used via supertype methods.

For me it is essentially the same than duck typing. And the important point here is the "if used via supertype methods" part.

This has nothing to do with which fields do the subtypes have. So taking into account the _fields_ in the comparison would go in the wrong direction in respect to LSP... but, does this really have to do with LSP at all? I don't think so, but I'm also not so sure as to make an statement.

What I'm sure of (and I hope anyone seriously following this thread) is that Identity is not a simple problem, but a really complex one because it involves what you mean by Identity. It depends on what you give "relevance" to and what you "filter out" of the comparison.

So what to do in cases like this one? When in doubt, I tend to apply the Fail-soon principle: implement by default the most restrictive code, the one that will throw an error sooner than others (in this case, the is comparison) so one will hit the problem the first time "it just doesn't work as I thought it worked".

And then let me implement a different comparison method by my own when I have a more clear idea of what _equal_ means for me (which is already possible with attrs). In case of using inheritance, you will have to do this just once at the base class, so it is not a big deal anyway.

[Julian]

@xgid I think I agree with almost all of that -- the only issue here is that there is no "failure" here -- Python doesn't throw errors when things don't implement equality, it coerces to a value no matter what.

So you do need to carefully pick the better of the two options.

[hynek]

Ehm. Your EOL Python maybe. 🙃

Edit oh wait you wrote equality...well that one is already strict so that can’t be changed anyways.

[Julian]

@hynek your shiny one too.

⊙  python3 -c '                                                                                                                                                                                                                                                                                                                                           Julian@Macnetic ●
quote> class Foo(object):
quote>     def __eq__(self, other):
quote>         return NotImplemented
quote> print(Foo() == Foo())'
False

The only thing it fixed there was the comparison operators I believe. Equality ones still always coerce.

[xgid]

@Julian Sorry, I was not explicit enough regarding the __eq__ implementation I'm proposing.

I mean just:

def __eq__(self, other):
    return  id(self) == id(other)

When I said "_the one that will throw an error sooner than others_" I was not meaning to return NotImplemented nor an exception. Just that the more restrictive implementation shown above will give you a comparison error in your code whenever you expect that something "equivalent"(1) to this will work:

assert Foo(1,2) == Foo(1,2)

instead of succeeding.

If it succeeds, you may not find the (logical) "bug" in your code until much much later. It's a kind of "defensive programming", but just for those "corner cases where things may get dangerous if you are not aware of the real implications", like this one.

(1) By "equivalent" above I mean that this may be a _simple_ if a == b in your code for which you expect that the comparison will be successful.

Edit: Just now I see that I made a mess talking about "the is comparison" at my first comment. Don't know what I was thinking about... 😳

[altendky]

@xgid That's just the object provided == isn't it? So suggesting that is basically suggesting not implementing __eq__ at all from an attrs perspective?

After chatting with @Julian and reading around a bit more I think I see what I was missing. It seems that the basic issue is a contention between satisfying both mathematical properties and Liskov. The desire to use == as a way to access the equality check which should satisfy mathematical constraints brings in the inheritance factor and therefor Liskov. So, don't use __eq__... :] My guess is that isn't an option we are looking to consider here but this does make me feel that any solution is going to be a compromise of some principal. I haven't analyzed the situation carefully but OrderedDict is a 'classic' example of this issue and a solution that is tending towards the Liskov side over the math side of correctness.

There were suggestions that the reason for is type checking making sense is that equality is a type of equivalence relation and that they are defined on members of a set. An __eq__ method on a class that can be inherited may be applied to things that we can't include in the set we are considering when writing the __eq__ because the subclasses haven't even been written yet. At least by considering only the same type for a True result you have restricted the set of things that might be equal. A bit of a helpful explanation for me at least.

Perhaps there are some options here. Default to no __eq__ method (IOW id() checking from object iirc) and force people to... implement their own? pick between a pre-existing set of options that satisfy either math or Liskov? It encourages them to check the docs and get a brief explanation of the dilemma and at least they have made the decision.

I'm really not sure what I think should happen anymore but I at least better understand the issue with where I was going. FWIW, I made a little implementation and set of tests while exploring my approach.

https://repl.it/@altendky/LSP-and-eq-1

[xgid]

@altendky

That's just the object provided == isn't it? So suggesting that is basically suggesting not implementing __eq__ at all from an attrs perspective?

Exactly! And for the exact same reasons you have explained better than me in your comment. The final goal is precisely that "_It encourages them to check the docs and get a brief explanation of the dilemma_" before they choose anything "meaninful" for them.

If I can also easily "pick between a pre-existing set of options", that would be even better, of course! I was talking only about the default implementation.

[wsanchez]

@xgid That's just the object provided == isn't it? So suggesting that is basically suggesting not implementing __eq__ at all from an attrs perspective?

Returning NotImplemented allows the other object implement a comparator, so explicitly implementing an identity test in a class defeats the design of how Python implements these operators, no?

[altendky]

@wsanchez I'm not sure what your comment means in regards to that quote. That if someone were considering not implementing __eq__ that they should instead implement it and return NotImplemented? By 'object provided ==' I was referring to the __eq__ provided by the type object that we all inherit from.

[wsanchez]

What I mean is that the way __eq__ works is that if you don't want to implement it, then don't add the method, or return NotImplemented when you don't know. If you implement it as simply return id(self) == id(other) (which is, I think, more simply written as self is other) then you are forcing an identity test to be used, instead of allowing such a test to be a fallback in the absence of, say, the other object implementing a comparator.

[altendky]

Ah, implementing __eq__ as self is other is not the same as leaving it to object's implementation because of other getting a chance in the latter case. I see the connection.

[hynek]

JFTR I’ve just noticed that the docs were always very clear on this matter: But the attributes are only compared, if the type of both classes is identical!

So technically we could even get away without a deprecation period but let’s do it anyways.