Skip to content

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jun 7, 2017

This code

static CYTHON_INLINE PyObject* __Pyx_PyObject_CallOneArg(PyObject *func, PyObject *arg) {
#if CYTHON_FAST_PYCALL
    if (PyFunction_Check(func)) {
        return __Pyx_PyFunction_FastCall(func, &arg, 1);
    }
#endif
#ifdef __Pyx_CyFunction_USED
    if (likely(PyCFunction_Check(func) || PyObject_TypeCheck(func, __pyx_CyFunctionType))) {
#else
    if (likely(PyCFunction_Check(func))) {
#endif
        if (likely(PyCFunction_GET_FLAGS(func) & METH_O)) {
            // fast and simple case that we are optimising for
            return __Pyx_PyObject_CallMethO(func, arg);
#if CYTHON_FAST_PYCCALL
        } else if (PyCFunction_GET_FLAGS(func) & METH_FASTCALL) {
            return __Pyx_PyCFunction_FastCall(func, &arg, 1);
#endif
        }
    }
    return __Pyx__PyObject_CallOneArg(func, arg);
}

is confusing bound and unbound methods. In particular, it can happen that func is a cyfunction (which is like an unbound method) but that __Pyx_PyObject_CallMethO(func, arg); is called which assumes that func is a bound method.

In particular, the following Cython code is buggy when compiled with binding=True:

cdef class TestMethodOneArg:
    def meth(self, arg):
        pass

def call_meth(x):
    return x.meth()

The expected result of call_meth(TestMethodOneArg()) is a TypeError because meth() is called without arg. The actual result is that meth() is called with self=TestMethodOneArg.meth.__func__ and arg=x.

It's not entirely clear what should be fixed (maybe it's already a bug that the unbound method func has the METH_O flag set?), but simply dropping the check for cyfunctions in the C code above fixes the problem for me.

@jdemeyer jdemeyer changed the title [WIP] Don't use __Pyx_PyObject_CallMethO on cyfunctions Don't use __Pyx_PyObject_CallMethO on cyfunctions Jun 7, 2017
@robertwb robertwb added this to the 0.26 milestone Jul 9, 2017
@robertwb
Copy link
Contributor

robertwb commented Jul 9, 2017

@scoder Do you want to try and fix this, or should we disable this for now?

@scoder
Copy link
Contributor

scoder commented Jul 9, 2017

It's currently also unclear to me what exactly needs fixing. I'll accept this change and think about it later.

@scoder scoder merged commit e8a6ad7 into cython:master Jul 9, 2017
@jdemeyer
Copy link
Contributor Author

As I said in the ticket, I haven't investigated this fully. I'm not sure whether this is the right fix, but at least it fixes the testcase.

@robertwb
Copy link
Contributor

This is certainly a safe fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants