Fix the datetime comparison conundrum.

The special-casing of other objects with a timetuple attribute is gone.
Let's hope Tim agrees.

Author: gvanrossum

Lib/test/test_datetime.py

@@ -954,41 +954,60 @@ def test_compare(self):
 
     def test_mixed_compare(self):
         our = self.theclass(2000, 4, 5)
+
+        # Our class can be compared for equality to other classes
+        self.assertEqual(our == 1, False)
+        self.assertEqual(1 == our, False)
+        self.assertEqual(our != 1, True)
+        self.assertEqual(1 != our, True)
+
+        # But the ordering is undefined
+        self.assertRaises(TypeError, lambda: our < 1)
+        self.assertRaises(TypeError, lambda: 1 < our)
         self.assertRaises(TypeError, cmp, our, 1)
         self.assertRaises(TypeError, cmp, 1, our)
 
-        class AnotherDateTimeClass(object):
-            def __cmp__(self, other):
-                # Return "equal" so calling this can't be confused with
-                # compare-by-address (which never says "equal" for distinct
-                # objects).
-                return 0
-
-        # This still errors, because date and datetime comparison raise
-        # TypeError instead of NotImplemented when they don't know what to
-        # do, in order to stop comparison from falling back to the default
-        # compare-by-address.
-        their = AnotherDateTimeClass()
+        # Repeat those tests with a different class
+
+        class SomeClass:
+            pass
+
+        their = SomeClass()
+        self.assertEqual(our == their, False)
+        self.assertEqual(their == our, False)
+        self.assertEqual(our != their, True)
+        self.assertEqual(their != our, True)
+        self.assertRaises(TypeError, lambda: our < their)
+        self.assertRaises(TypeError, lambda: their < our)
         self.assertRaises(TypeError, cmp, our, their)
-        # Oops:  The next stab raises TypeError in the C implementation,
-        # but not in the Python implementation of datetime.  The difference
-        # is due to that the Python implementation defines __cmp__ but
-        # the C implementation defines tp_richcompare.  This is more pain
-        # to fix than it's worth, so commenting out the test.
-        # self.assertEqual(cmp(their, our), 0)
-
-        # But date and datetime comparison return NotImplemented instead if the
-        # other object has a timetuple attr.  This gives the other object a
-        # chance to do the comparison.
-        class Comparable(AnotherDateTimeClass):
-            def timetuple(self):
-                return ()
-
-        their = Comparable()
-        self.assertEqual(cmp(our, their), 0)
-        self.assertEqual(cmp(their, our), 0)
-        self.failUnless(our == their)
-        self.failUnless(their == our)
+        self.assertRaises(TypeError, cmp, their, our)
+
+        # However, if the other class explicitly defines ordering
+        # relative to our class, it is allowed to do so
+
+        class LargerThanAnything:
+            def __lt__(self, other):
+                return False
+            def __le__(self, other):
+                return isinstance(other, LargerThanAnything)
+            def __eq__(self, other):
+                return isinstance(other, LargerThanAnything)
+            def __ne__(self, other):
+                return not isinstance(other, LargerThanAnything)
+            def __gt__(self, other):
+                return not isinstance(other, LargerThanAnything)
+            def __ge__(self, other):
+                return True
+
+        their = LargerThanAnything()
+        self.assertEqual(our == their, False)
+        self.assertEqual(their == our, False)
+        self.assertEqual(our != their, True)
+        self.assertEqual(their != our, True)
+        self.assertEqual(our < their, True)
+        self.assertEqual(their < our, False)
+        self.assertEqual(cmp(our, their), -1)
+        self.assertEqual(cmp(their, our), 1)
 
     def test_bool(self):
         # All dates are considered true.
@@ -3217,10 +3236,10 @@ def test_bug_1028306(self):
 
         # Neverthelss, comparison should work with the base-class (date)
         # projection if use of a date method is forced.
-        self.assert_(as_date.__eq__(as_datetime))
+        self.assertEqual(as_date.__eq__(as_datetime), True)
         different_day = (as_date.day + 1) % 20 + 1
-        self.assert_(not as_date.__eq__(as_datetime.replace(day=
-                                                     different_day)))
+        as_different = as_datetime.replace(day= different_day)
+        self.assertEqual(as_date.__eq__(as_different), False)
 
         # And date should compare with other subclasses of date.  If a
         # subclass wants to stop this, it's up to the subclass to do so.

Modules/datetimemodule.c

@@ -1387,7 +1387,7 @@ build_struct_time(int y, int m, int d, int hh, int mm, int ss, int dstflag)
  * Miscellaneous helpers.
  */
 
-/* For obscure reasons, we need to use tp_richcompare instead of tp_compare.
+/* For various reasons, we need to use tp_richcompare instead of tp_compare.
  * The comparisons here all most naturally compute a cmp()-like result.
  * This little helper turns that into a bool result for rich comparisons.
  */
@@ -1705,31 +1705,23 @@ delta_subtract(PyObject *left, PyObject *right)
 	return result;
 }
 
-/* This is more natural as a tp_compare, but doesn't work then:  for whatever
- * reason, Python's try_3way_compare ignores tp_compare unless
- * PyInstance_Check returns true, but these aren't old-style classes.
- */
 static PyObject *
-delta_richcompare(PyDateTime_Delta *self, PyObject *other, int op)
+delta_richcompare(PyObject *self, PyObject *other, int op)
 {
-	int diff = 42;	/* nonsense */
-
 	if (PyDelta_Check(other)) {
-		diff = GET_TD_DAYS(self) - GET_TD_DAYS(other);
+		int diff = GET_TD_DAYS(self) - GET_TD_DAYS(other);
 		if (diff == 0) {
 			diff = GET_TD_SECONDS(self) - GET_TD_SECONDS(other);
 			if (diff == 0)
 				diff = GET_TD_MICROSECONDS(self) -
 				       GET_TD_MICROSECONDS(other);
 		}
+		return diff_to_bool(diff, op);
+	}
+	else {
+		Py_INCREF(Py_NotImplemented);
+		return Py_NotImplemented;
 	}
-	else if (op == Py_EQ || op == Py_NE)
-		diff = 1;	/* any non-zero value will do */
-
-	else /* stop this from falling back to address comparison */
-		return cmperror((PyObject *)self, other);
-
-	return diff_to_bool(diff, op);
 }
 
 static PyObject *delta_getstate(PyDateTime_Delta *self);
@@ -2145,7 +2137,7 @@ static PyTypeObject PyDateTime_DeltaType = {
 	delta_doc,					/* tp_doc */
 	0,						/* tp_traverse */
 	0,						/* tp_clear */
-	(richcmpfunc)delta_richcompare,			/* tp_richcompare */
+	delta_richcompare,				/* tp_richcompare */
 	0,						/* tp_weaklistoffset */
 	0,						/* tp_iter */
 	0,						/* tp_iternext */
@@ -2499,31 +2491,19 @@ date_isocalendar(PyDateTime_Date *self)
 
 /* Miscellaneous methods. */
 
-/* This is more natural as a tp_compare, but doesn't work then:  for whatever
- * reason, Python's try_3way_compare ignores tp_compare unless
- * PyInstance_Check returns true, but these aren't old-style classes.
- */
 static PyObject *
-date_richcompare(PyDateTime_Date *self, PyObject *other, int op)
+date_richcompare(PyObject *self, PyObject *other, int op)
 {
-	int diff = 42;	/* nonsense */
-
-	if (PyDate_Check(other))
-		diff = memcmp(self->data, ((PyDateTime_Date *)other)->data,
-			      _PyDateTime_DATE_DATASIZE);
-
-	else if (PyObject_HasAttrString(other, "timetuple")) {
-		/* A hook for other kinds of date objects. */
+	if (PyDate_Check(other)) {
+		int diff = memcmp(((PyDateTime_Date *)self)->data,
+				  ((PyDateTime_Date *)other)->data,
+				  _PyDateTime_DATE_DATASIZE);
+		return diff_to_bool(diff, op);
+	}
+	else {
 		Py_INCREF(Py_NotImplemented);
 		return Py_NotImplemented;
 	}
-	else if (op == Py_EQ || op == Py_NE)
-		diff = 1;	/* any non-zero value will do */
-
-	else /* stop this from falling back to address comparison */
-		return cmperror((PyObject *)self, other);
-
-	return diff_to_bool(diff, op);
 }
 
 static PyObject *
@@ -2701,7 +2681,7 @@ static PyTypeObject PyDateTime_DateType = {
 	date_doc,					/* tp_doc */
 	0,						/* tp_traverse */
 	0,						/* tp_clear */
-	(richcmpfunc)date_richcompare,			/* tp_richcompare */
+	date_richcompare,				/* tp_richcompare */
 	0,						/* tp_weaklistoffset */
 	0,						/* tp_iter */
 	0,						/* tp_iternext */
@@ -3223,36 +3203,28 @@ time_strftime(PyDateTime_Time *self, PyObject *args, PyObject *kw)
  * Miscellaneous methods.
  */
 
-/* This is more natural as a tp_compare, but doesn't work then:  for whatever
- * reason, Python's try_3way_compare ignores tp_compare unless
- * PyInstance_Check returns true, but these aren't old-style classes.
- */
 static PyObject *
-time_richcompare(PyDateTime_Time *self, PyObject *other, int op)
+time_richcompare(PyObject *self, PyObject *other, int op)
 {
 	int diff;
 	naivety n1, n2;
 	int offset1, offset2;
 
 	if (! PyTime_Check(other)) {
-		if (op == Py_EQ || op == Py_NE) {
-			PyObject *result = op == Py_EQ ? Py_False : Py_True;
-			Py_INCREF(result);
-			return result;
-		}
-		/* Stop this from falling back to address comparison. */
-		return cmperror((PyObject *)self, other);
+		Py_INCREF(Py_NotImplemented);
+		return Py_NotImplemented;
 	}
-	if (classify_two_utcoffsets((PyObject *)self, &offset1, &n1, Py_None,
-				     other, &offset2, &n2, Py_None) < 0)
+	if (classify_two_utcoffsets(self, &offset1, &n1, Py_None,
+				    other, &offset2, &n2, Py_None) < 0)
 		return NULL;
 	assert(n1 != OFFSET_UNKNOWN && n2 != OFFSET_UNKNOWN);
 	/* If they're both naive, or both aware and have the same offsets,
 	 * we get off cheap.  Note that if they're both naive, offset1 ==
 	 * offset2 == 0 at this point.
 	 */
 	if (n1 == n2 && offset1 == offset2) {
-		diff = memcmp(self->data, ((PyDateTime_Time *)other)->data,
+		diff = memcmp(((PyDateTime_Time *)self)->data,
+			      ((PyDateTime_Time *)other)->data,
 			      _PyDateTime_TIME_DATASIZE);
 		return diff_to_bool(diff, op);
 	}
@@ -3474,7 +3446,7 @@ static PyTypeObject PyDateTime_TimeType = {
 	time_doc,				/* tp_doc */
 	0,					/* tp_traverse */
 	0,					/* tp_clear */
-	(richcmpfunc)time_richcompare,		/* tp_richcompare */
+	time_richcompare,			/* tp_richcompare */
 	0,					/* tp_weaklistoffset */
 	0,					/* tp_iter */
 	0,					/* tp_iternext */
@@ -4115,54 +4087,43 @@ datetime_ctime(PyDateTime_DateTime *self)
 
 /* Miscellaneous methods. */
 
-/* This is more natural as a tp_compare, but doesn't work then:  for whatever
- * reason, Python's try_3way_compare ignores tp_compare unless
- * PyInstance_Check returns true, but these aren't old-style classes.
- */
 static PyObject *
-datetime_richcompare(PyDateTime_DateTime *self, PyObject *other, int op)
+datetime_richcompare(PyObject *self, PyObject *other, int op)
 {
 	int diff;
 	naivety n1, n2;
 	int offset1, offset2;
 
 	if (! PyDateTime_Check(other)) {
-		/* If other has a "timetuple" attr, that's an advertised
-		 * hook for other classes to ask to get comparison control.
-		 * However, date instances have a timetuple attr, and we
-		 * don't want to allow that comparison.  Because datetime
-		 * is a subclass of date, when mixing date and datetime
-		 * in a comparison, Python gives datetime the first shot
-		 * (it's the more specific subtype).  So we can stop that
-		 * combination here reliably.
-		 */
-		if (PyObject_HasAttrString(other, "timetuple") &&
-		    ! PyDate_Check(other)) {
-			/* A hook for other kinds of datetime objects. */
-			Py_INCREF(Py_NotImplemented);
-			return Py_NotImplemented;
+		if (PyDate_Check(other)) {
+			/* Prevent invocation of date_richcompare.  We want to
+			   return NotImplemented here to give the other object
+			   a chance.  But since DateTime is a subclass of
+			   Date, if the other object is a Date, it would
+			   compute an ordering based on the date part alone,
+			   and we don't want that.  So force unequal or
+			   uncomparable here in that case. */
+			if (op == Py_EQ)
+				Py_RETURN_FALSE;
+			if (op == Py_NE)
+				Py_RETURN_TRUE;
+			return cmperror(self, other);
 		}
-		if (op == Py_EQ || op == Py_NE) {
-			PyObject *result = op == Py_EQ ? Py_False : Py_True;
-			Py_INCREF(result);
-			return result;
-		}
-		/* Stop this from falling back to address comparison. */
-		return cmperror((PyObject *)self, other);
+		Py_INCREF(Py_NotImplemented);
+		return Py_NotImplemented;
 	}
 
-	if (classify_two_utcoffsets((PyObject *)self, &offset1, &n1,
-				    (PyObject *)self,
-				     other, &offset2, &n2,
-				     other) < 0)
+	if (classify_two_utcoffsets(self, &offset1, &n1, self,
+				    other, &offset2, &n2, other) < 0)
 		return NULL;
 	assert(n1 != OFFSET_UNKNOWN && n2 != OFFSET_UNKNOWN);
  	/* If they're both naive, or both aware and have the same offsets,
 	 * we get off cheap.  Note that if they're both naive, offset1 ==
 	 * offset2 == 0 at this point.
 	 */
 	if (n1 == n2 && offset1 == offset2) {
-		diff = memcmp(self->data, ((PyDateTime_DateTime *)other)->data,
+		diff = memcmp(((PyDateTime_DateTime *)self)->data,
+			      ((PyDateTime_DateTime *)other)->data,
 			      _PyDateTime_DATETIME_DATASIZE);
 		return diff_to_bool(diff, op);
 	}
@@ -4568,7 +4529,7 @@ static PyTypeObject PyDateTime_DateTimeType = {
 	datetime_doc,				/* tp_doc */
 	0,					/* tp_traverse */
 	0,					/* tp_clear */
-	(richcmpfunc)datetime_richcompare,	/* tp_richcompare */
+	datetime_richcompare,			/* tp_richcompare */
 	0,					/* tp_weaklistoffset */
 	0,					/* tp_iter */
 	0,					/* tp_iternext */