Repository: python-attrs/attrs

Problem statement:
attrs._make should document why it is more restrictive with __eq__ than other comparisons
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__`](https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L867) 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](https://www.youtube.com/watch?v=T-TwcmT6Rcw#t=24m09s), 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).


Hints:
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.)
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.
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.
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.)
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?
> 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.
@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__`](https://docs.python.org/2/reference/datamodel.html?highlight=__cmp__#object.__cmp__) operator function…
If you want to swap out the whole implementation wouldn't you just use `cmp=False`?
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](https://github.com/burningmantech/ranger-ims-server/blob/master/src/ims/model/_cmp.py#L31) such that all I need to do in classes that inherit from the mix-in is implement `_cmpValue`.  [Here's a simple example](https://github.com/burningmantech/ranger-ims-server/blob/master/src/ims/model/_address.py#L105) 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](https://github.com/burningmantech/ranger-ims-server/blob/master/src/ims/model/_entry.py#L62) 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.

I'm not quite sure which way this thread is leaning at this point.  `is` or `isinstance()`?  If configurable, then which default?
@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.
@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.)
@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.
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.
@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`](https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L902) 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.
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.
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. 
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
I guess comparisons are a well known exception to LSP?  Or LSP is just considered not relevant anymore?

I still need to study it...
No your argument is good IMHO :)
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.
> 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.
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.
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.
@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()`.
For what it's worth, there's also:

```python
@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 `->`.
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.

@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.
Ehm. *Your* EOL Python maybe. 🙃

*Edit* oh wait you wrote equality...well that one is already strict so that can’t be changed anyways. 
@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.
@Julian Sorry, I was not explicit enough regarding the `__eq__` implementation I'm proposing. 

I mean just:

```python
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:

```python
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... 😳 
@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

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

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

Patch:
diff --git a/src/attr/_make.py b/src/attr/_make.py
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -671,7 +671,7 @@ def attrs(
     :param bool cmp: Create ``__eq__``, ``__ne__``, ``__lt__``, ``__le__``,
         ``__gt__``, and ``__ge__`` methods that compare the class as if it were
         a tuple of its ``attrs`` attributes.  But the attributes are *only*
-        compared, if the type of both classes is *identical*!
+        compared, if the types of both classes are *identical*!
     :param hash: If ``None`` (default), the ``__hash__`` method is generated
         according how *cmp* and *frozen* are set.
 
@@ -747,6 +747,11 @@ def attrs(
     .. versionchanged:: 18.1.0
        If *these* is passed, no attributes are deleted from the class body.
     .. versionchanged:: 18.1.0 If *these* is ordered, the order is retained.
+    .. deprecated:: 18.2.0
+       ``__lt__``, ``__le__``, ``__gt__``, and ``__ge__`` now raise a
+       :class:`DeprecationWarning` if the classes compared are subclasses of
+       each other. ``__eq`` and ``__ne__`` never tried to compared subclasses
+       to each other.
     """
 
     def wrap(cls):
@@ -885,6 +890,12 @@ def __ne__(self, other):
     return not result
 
 
+WARNING_CMP_ISINSTANCE = (
+    "Comparision of subclasses using __%s__ is deprecated and will be removed "
+    "in 2019."
+)
+
+
 def _make_cmp(attrs):
     attrs = [a for a in attrs if a.cmp]
 
@@ -938,6 +949,10 @@ def __lt__(self, other):
         Automatically created by attrs.
         """
         if isinstance(other, self.__class__):
+            if other.__class__ is not self.__class__:
+                warnings.warn(
+                    WARNING_CMP_ISINSTANCE % ("lt",), DeprecationWarning
+                )
             return attrs_to_tuple(self) < attrs_to_tuple(other)
         else:
             return NotImplemented
@@ -947,6 +962,10 @@ def __le__(self, other):
         Automatically created by attrs.
         """
         if isinstance(other, self.__class__):
+            if other.__class__ is not self.__class__:
+                warnings.warn(
+                    WARNING_CMP_ISINSTANCE % ("le",), DeprecationWarning
+                )
             return attrs_to_tuple(self) <= attrs_to_tuple(other)
         else:
             return NotImplemented
@@ -956,6 +975,10 @@ def __gt__(self, other):
         Automatically created by attrs.
         """
         if isinstance(other, self.__class__):
+            if other.__class__ is not self.__class__:
+                warnings.warn(
+                    WARNING_CMP_ISINSTANCE % ("gt",), DeprecationWarning
+                )
             return attrs_to_tuple(self) > attrs_to_tuple(other)
         else:
             return NotImplemented
@@ -965,6 +988,10 @@ def __ge__(self, other):
         Automatically created by attrs.
         """
         if isinstance(other, self.__class__):
+            if other.__class__ is not self.__class__:
+                warnings.warn(
+                    WARNING_CMP_ISINSTANCE % ("ge",), DeprecationWarning
+                )
             return attrs_to_tuple(self) >= attrs_to_tuple(other)
         else:
             return NotImplemented


Test patch:
diff --git a/tests/test_make.py b/tests/test_make.py
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -1249,3 +1249,42 @@ class C(object):
             )
 
         assert C() == copy.deepcopy(C())
+
+
+class TestMakeCmp:
+    """
+    Tests for _make_cmp().
+    """
+
+    @pytest.mark.parametrize(
+        "op", ["__%s__" % (op,) for op in ("lt", "le", "gt", "ge")]
+    )
+    def test_subclasses_deprecated(self, recwarn, op):
+        """
+        Calling comparison methods on subclasses raises a deprecation warning;
+        calling them on identical classes does not..
+        """
+
+        @attr.s
+        class A(object):
+            a = attr.ib()
+
+        @attr.s
+        class B(A):
+            pass
+
+        getattr(A(42), op)(A(42))
+        getattr(B(42), op)(B(42))
+
+        assert [] == recwarn.list
+
+        getattr(A(42), op)(B(42))
+
+        w = recwarn.pop()
+
+        assert [] == recwarn.list
+        assert isinstance(w.message, DeprecationWarning)
+        assert (
+            "Comparision of subclasses using %s is deprecated and will be "
+            "removed in 2019." % (op,)
+        ) == w.message.args[0]