Merge pull request #1387 from kmod/instancemethods

Fix instancemethod comparisons

src/runtime/types.cpp

@@ -1872,25 +1872,35 @@ static Box* instancemethodRepr(Box* b) {
     return result;
 }
 
-Box* instancemethodEq(BoxedInstanceMethod* self, Box* rhs) {
-    if (rhs->cls != instancemethod_cls) {
-        return boxBool(false);
-    }
+static int instancemethod_compare(BoxedInstanceMethod* a, BoxedInstanceMethod* b) noexcept {
+    int cmp;
+    cmp = PyObject_Compare(a->func, b->func);
+    if (cmp)
+        return cmp;
 
-    BoxedInstanceMethod* rhs_im = static_cast<BoxedInstanceMethod*>(rhs);
-    if (self->func == rhs_im->func) {
-        if (self->obj == NULL && rhs_im->obj == NULL) {
-            return boxBool(true);
-        } else {
-            if (self->obj != NULL && rhs_im->obj != NULL) {
-                return compareInternal<NOT_REWRITABLE>(self->obj, rhs_im->obj, AST_TYPE::Eq, NULL);
-            } else {
-                return boxBool(false);
-            }
-        }
-    } else {
-        return boxBool(false);
-    }
+    if (a->obj == b->obj)
+        return 0;
+    if (a->obj == NULL || b->obj == NULL)
+        return (a->obj < b->obj) ? -1 : 1;
+    else
+        return PyObject_Compare(a->obj, b->obj);
+}
+
+Box* instancemethodHash(BoxedInstanceMethod* self) {
+    long x, y;
+    if (self->obj == NULL)
+        x = PyObject_Hash(Py_None);
+    else
+        x = PyObject_Hash(self->obj);
+    if (x == -1)
+        throwCAPIException();
+    y = PyObject_Hash(self->func);
+    if (y == -1)
+        throwCAPIException();
+    x = x ^ y;
+    if (x == -1)
+        x = -2;
+    return boxInt(x);
 }
 
 Box* sliceRepr(BoxedSlice* self) {
@@ -4530,8 +4540,8 @@ void setupRuntime() {
                                                               { NULL }));
     instancemethod_cls->giveAttr(
         "__repr__", new BoxedFunction(BoxedCode::create((void*)instancemethodRepr, STR, 1, "instancemethod.__repr__")));
-    instancemethod_cls->giveAttr(
-        "__eq__", new BoxedFunction(BoxedCode::create((void*)instancemethodEq, UNKNOWN, 2, "instancemethod.__eq__")));
+    instancemethod_cls->giveAttr("__hash__", new BoxedFunction(BoxedCode::create((void*)instancemethodHash, UNKNOWN, 1,
+                                                                                 "instancemethod.__hash__")));
     instancemethod_cls->giveAttr("__get__",
                                  new BoxedFunction(BoxedCode::create((void*)instancemethodGet, UNKNOWN, 3, false, false,
                                                                      "instancemethod.__get__")));
@@ -4548,6 +4558,9 @@ void setupRuntime() {
     // TODO: this should be handled via a getattro instead (which proxies to the function):
     instancemethod_cls->giveAttrDescriptor("__doc__", im_doc, NULL);
 
+    instancemethod_cls->tp_compare = (cmpfunc)instancemethod_compare;
+
+    add_operators(instancemethod_cls);
     instancemethod_cls->freeze();
 
     slice_cls->giveAttr("__new__",

test/extra/sqlalchemy_0.5_smalltest.py

@@ -201,6 +201,10 @@ for run_idx in xrange(1):
                 if (clsname == 'ReconstitutionTest' and t == 'test_copy'):
                     continue
 
+                if (clsname == 'InstrumentationCollisionTest' and t == 'test_diamond_b2'):
+                    # Test needs instancemethod checking (due to a bug in the test)
+                    continue
+
                 # This test is flaky since it depends on set ordering.
                 # (It causes sporadic failures in cpython as well.)
                 if clsname == "SelectTest" and t == 'test_binds':

test/tests/instance_methods.py

@@ -45,3 +45,28 @@ f(C.foo)
 f(C().foo)
 
 C().foo.__call__()
+
+
+# Check comparisons and hashing:
+class C(object):
+    def foo(self):
+        pass
+
+assert C.foo is not C.foo # This could be fine, but if it's true then the rest of the checks here don't make sense
+assert C.foo == C.foo
+assert not (C.foo != C.foo)
+assert not (C.foo < C.foo)
+assert not (C.foo > C.foo)
+assert C.foo >= C.foo
+assert C.foo <= C.foo
+assert len({C.foo, C.foo}) == 1
+
+c = C()
+assert c.foo is not c.foo # This could be fine, but if it's true then the rest of the checks here don't make sense
+assert c.foo == c.foo
+assert not (c.foo != c.foo)
+assert not (c.foo < c.foo)
+assert not (c.foo > c.foo)
+assert c.foo >= c.foo
+assert c.foo <= c.foo
+assert len({c.foo, c.foo}) == 1