Python 2: Forbid type objects as keys.

CHANGES.rst

@@ -5,6 +5,13 @@
 4.8.0 (unreleased)
 ==================
 
+- Make Python 2 forbid the use of type objects as keys (unless a
+  custom metaclass is used that implements comparison as required by
+  BTrees.) On Python 3, types are not orderable so they were already
+  forbidden, but on Python 2 types can be ordered by memory address,
+  which makes them unsuitable for use as keys. See `issue
+  <https://github.com/zopefoundation/BTrees/issues/153>`_.
+
 - Make the ``multiunion``, ``union``, ``intersection``, and
   ``difference`` functions accept arbitrary Python iterables (that
   iterate across the correct types). Previously, the Python

src/BTrees/_datatypes.py

@@ -20,6 +20,13 @@
 from struct import Struct
 from struct import error as struct_error
 
+try:
+    from abc import ABC
+except ImportError:
+    # Python < 3.4
+    from abc import ABCMeta
+    ABC = ABCMeta('ABC', (object,), {})
+
 from ._compat import int_types
 from .utils import Lazy
 
@@ -147,6 +154,80 @@ def supports_value_union(self):
         return False
 
 
+
+class _HasDefaultComparison(ABC):
+    """
+    An ABC for checking whether an item has default comparison.
+
+    This automatically gets us some caching.
+    """
+
+    # Comparisons only use special methods defined on the
+    # type, not instance variables.
+
+    # On CPython 2, every subclass of object inherits __lt__
+    # (unless it overrides), and it has ``__objclass__`` of ``type``
+    # (of course it is also the same object as ``_object_lt`` at
+    # that point). Also, interestingly, CPython 2 classes inherit
+    # ``__lt__`` from ``type``, but *instances* do not.
+    #
+    # On CPython 3, classes inherit __lt__ with ``__objclass__`` of ``object``.
+    # On PyPy2, classes don't inherit any __lt__.
+    # On PyPy3, they do.
+    #
+    # Test these conditions at runtime and define the method variant
+    # appropriately.
+    #
+    # Remember the method is checking if the object has default comparison
+    assert '__lt__' not in DataType.__dict__
+    if hasattr(DataType, '__lt__'):
+        # CPython 2 or CPython 3 or PyPy3
+        if getattr(DataType.__lt__, '__objclass__', None) is object:
+            # CPython 3
+            @classmethod
+            def __subclasshook__(cls, C, _NoneType=type(None)):
+                if C is _NoneType:
+                    return False
+                defining_class = getattr(C.__lt__, '__objclass__', None)
+                if defining_class is None:
+                    # Implemented in Python
+                    return False
+                return C.__lt__.__objclass__ is object
+        elif getattr(DataType.__lt__, '__objclass__', None) is type:
+            # CPython 2
+            @classmethod
+            def __subclasshook__(cls, C, _NoneType=type(None)):
+                if C is _NoneType:
+                    return False
+                # If C is an old-style class, it may not even have __lt__
+                if isinstance(C, type):
+                    lt = C.__lt__
+                else:
+                    lt = getattr(C, '__lt__', None)
+                if lt is not None:
+                    defining_class = getattr(lt, '__objclass__', None)
+                    if defining_class is None:
+                        # Implemented in Python
+                        return False
+                    if defining_class is not type:
+                        return False
+                return not hasattr(C, '__cmp__')
+        else:
+            # PyPy3
+            @classmethod
+            def __subclasshook__(cls, C, _object_lt=object.__lt__, _NoneType=type(None)):
+                if C is _NoneType:
+                    return False
+                return C.__lt__ is _object_lt
+    else:
+        # PyPy2
+        @classmethod
+        def __subclasshook__(cls, C, _NoneType=type(None)):
+            if C is _NoneType:
+                return False
+            return not hasattr(C, '__lt__') and not hasattr(C, '__cmp__')
+
+
 class O(KeyDataType):
     """
     Arbitrary (sortable) Python objects.
@@ -161,58 +242,9 @@ def as_value_type(self):
     def supports_value_union(self):
         return False
 
-    @staticmethod
-    def _check_default_comparison(
-            item,
-            # PyPy2 doesn't define __lt__ on object; PyPy3 and
-            # CPython 2 and 3 do.
-            _object_lt=getattr(object, '__lt__', object())
-    ):
-        # Enforce test that key has non-default comparison.
-        # (With the exception of None, because we define a sort order
-        # for it.)
-
-        if item is None:
-            return
-
-        if type(item) is object: # pylint:disable=unidiomatic-typecheck
-            raise TypeError("Can't use object() as keys")
-
-        # Now more complicated checks to be sure we didn't
-        # inherit default comparison on any version of Python.
-
-
-        # TODO: Using a custom ABC and doing ``isinstance(item, NoDefaultComparison)``
-        # would automatically gain us caching.
-
-        # XXX: Comparisons only use special methods defined on the
-        # type, not instance variables. So shouldn't this be getattr(type(key)...)?
-        # Note that changes some things below; for example, on CPython 2,
-        # every subclass of object inherits __lt__ (unless it overrides), and it
-        # has __objclass__ of ``type`` (of course it is also the same object
-        # as ``_object_lt`` at that point). Also, weirdly, CPython 2 classes inherit
-        # ``__lt__``, but *instances* do not.
-
-        lt = getattr(item, '__lt__', None)
-        if lt is not None:
-            # CPython 2 and 3 follow PEP 252, defining '__objclass__'
-            # for methods of builtin types like str; methods of
-            # classes defined in Python don't get it. ``__objclass__``
-            if getattr(lt, '__objclass__', None) is object:
-                lt = None  # pragma: no cover Py3k
-            # PyPy3 doesn't follow PEP 252, but defines '__func__'
-            elif getattr(lt, '__func__', None) is _object_lt:
-                lt = None  # pragma: no cover PyPy3
-
-        if (lt is None
-                # TODO: Shouldn't we only check __cmp__ on Python 2?
-                # Python 3 won't use it.
-                and getattr(item, '__cmp__', None) is None):
-            raise TypeError("Object has default comparison")
-
-
     def __call__(self, item):
-        self._check_default_comparison(item)
+        if isinstance(item, _HasDefaultComparison):
+            raise TypeError("Object of class %s has default comparison" % (type(item).__name__,))
         return item
 
 

src/BTrees/objectkeymacros.h

@@ -19,14 +19,23 @@ check_argument_cmp(PyObject *arg)
       return 1;
     }
 
+
 #ifdef PY3K
     if (Py_TYPE(arg)->tp_richcompare == Py_TYPE(object_)->tp_richcompare)
 #else
-    if (Py_TYPE(arg)->tp_richcompare == NULL
-        && Py_TYPE(arg)->tp_compare == Py_TYPE(object_)->tp_compare)
+    if ((Py_TYPE(arg)->tp_richcompare == NULL
+	 && Py_TYPE(arg)->tp_compare == Py_TYPE(object_)->tp_compare)
+	/* Also exclude new-style classes. On Python 2, they can be compared,
+	   but order by address, making them not suitable for BTrees. */
+	|| PyType_CheckExact(arg)
+	/* But let classes with a meta class that implements comparison through. */
+	|| (PyType_Check(arg) && Py_TYPE(arg)->tp_richcompare == PyType_Type.tp_richcompare)
+	)
 #endif
     {
-        PyErr_SetString(PyExc_TypeError, "Object has default comparison");
+        PyErr_Format(PyExc_TypeError,
+		     "Object of class %s has default comparison",
+		     Py_TYPE(arg)->tp_name);
         return 0;
     }
     return 1;

src/BTrees/tests/common.py

@@ -1143,6 +1143,32 @@ def test_assign_value_type_float(self):
     def test_assign_value_type_None(self):
         self.__test_value(None)
 
+    def testNewStyleClassAsKeyNotAllowed(self):
+        m = self._makeOne()
+        class New(object):
+            pass
+
+        with self.assertRaises(TypeError):
+            m[New] = self.getTwoValues()[0]
+
+    def testOldStyleClassAsKeyNotALlowed(self):
+        m = self._makeOne()
+        class Old: # Python 2: Old style class
+            pass
+
+        with self.assertRaises(TypeError):
+            m[Old] = self.getTwoValues()[0]
+
+    def testNewStyleClassWithCustomMetaClassNotAllowed(self):
+
+        class Meta(type):
+            pass
+
+        cls = Meta('Class', (object,), {})
+        m = self._makeOne()
+        with self.assertRaises(TypeError):
+            m[cls] = self.getTwoValues()[0]
+
     def testEmptyFirstBucketReportedByGuido(self):
         # This was for Integer keys
         from .._compat import xrange

src/BTrees/tests/test_OOBTree.py

@@ -25,9 +25,10 @@ def test_byValue(self):
                          [(y, x) for x, y in reversed(ITEMS[22:])])
 
     def testRejectDefaultComparisonOnSet(self):
-        # Check that passing int keys w default comparison fails.
-        # Only applies to new-style class instances. Old-style
-        # instances are too hard to introspect.
+        # Check that passing in keys w default comparison fails. Only
+        # applies to new-style class instances if we're using the C
+        # extensions; old-style instances are too hard to introspect
+        # in C.
 
         # This is white box because we know that the check is being
         # used in a function that's used in lots of places.
@@ -36,13 +37,22 @@ def testRejectDefaultComparisonOnSet(self):
         from .._compat import PY2
         t = self._makeOne()
 
+        class OldStyle:
+            pass
+
+        if self._getTargetClass() is OOBTree.OOBTreePy or not PY2:
+            with self.assertRaises(TypeError):
+                t[OldStyle()] = 1
+
         class C(object):
             pass
 
         with self.assertRaises(TypeError) as raising:
             t[C()] = 1
 
-        self.assertEqual(raising.exception.args[0], "Object has default comparison")
+        self.assertEqual(
+            raising.exception.args[0],
+            "Object of class C has default comparison")
 
         if PY2: # we only check for __cmp__ on Python2
 
@@ -60,6 +70,14 @@ def __lt__(*args):
 
         c = With___lt__()
         t[c] = 1
+        t.clear()
+
+        class With___lt__Old:
+            def __lt__(*args):
+                return 1
+
+        c = With___lt__Old()
+        t[c] = 1
 
         t.clear()
 
@@ -73,6 +91,16 @@ class C(object):
         self.assertRaises(KeyError, t.__getitem__, C())
         self.assertFalse(C() in t)
 
+
+    def testNewStyleClassWithCustomMetaClassAllowed(self):
+        class Meta(type):
+            def __lt__(cls, other):
+                return 1
+
+        cls = Meta('Class', (object,), {})
+        m = self._makeOne()
+        m[cls] = self.getTwoValues()[0]
+
     def test_None_is_smallest(self):
         t = self._makeOne()
         for i in range(999): # Make sure we multiple buckets