Repository: python-attrs/attrs (python)

Issue:
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).

Pull request:
Raise DeprecationWarning when comparing subclasses
Fixes #387 

The [docs](http://www.attrs.org/en/stable/api.html#attr.s) were always clear on this:

> But the attributes are *only* compared, if the type of both classes is *identical*!

(which sounds weird, so I fix it in this PR too)

Patch:
diff --git a/changelog.d/394.deprecation.rst b/changelog.d/394.deprecation.rst
new file mode 100644
index 0000000..d04399d
--- /dev/null
+++ b/changelog.d/394.deprecation.rst
@@ -0,0 +1,4 @@
+Comparing subclasses using ``<``, ``>``, ``<=``, and ``>=`` is now deprecated.
+The docs always claimed that instances are only compared if the types are identical, so this is a first step to conform to the docs.
+
+Equality operators (``==`` and ``!=``) were always strict in this regard.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 8d0b1c6..905f309 100644
--- 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 _make_cmp(attrs):
         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 _make_cmp(attrs):
         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 _make_cmp(attrs):
         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 _make_cmp(attrs):
         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
index 7d267b8..e407b9f 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -1249,3 +1249,42 @@ class TestClassBuilder(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]