cython / cython

The most widely used Python to C compiler
https://cython.org
Apache License 2.0
9.52k stars 1.49k forks source link

C++ compilation error for fused type in `__cinit__` #3758

Open fhchl opened 4 years ago

fhchl commented 4 years ago

Describe the bug An extension type definition with fused type in __cinit__ leads to a C++ compilation error. I am not sure if this is because my code is not valid Cython or there is a bug.

To Reproduce Code to reproduce the behaviour:


ctypedef fused ndcomplexview_t:
    complex[::1]
    complex[:, ::1]
    complex[:, :, ::1]

cdef class Foo:
    cdef complex *data
    def __cinit__(self, ndcomplexview_t arr):
        if ndcomplexview_t is complex[::1]:
            self.data = &arr[0]
        elif ndcomplexview_t is complex[:, ::1]:
            self.data = &arr[0, 0]
        elif ndcomplexview_t is complex[:, :, ::1]:
            self.data = &arr[0, 0, 0]

throws:

cc1plus: warning: command line option '-Wstrict-prototypes' is valid for C/ObjC but not for C++
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:2918:110: error: expected primary-expression before 'static'
 static PyMethodDef __pyx_fuse_0__pyx_mdef_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_3__cinit__ = static int __pyx_fuse_0__pyx_pw_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_3__cinit__(PyObject *__pyx_v_self, PyObject *__pyx_args, PyObject *__pyx_kwds) {
                                                                                                              ^~~~~~
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:2969:1: error: expected ',' or ';' before 'static'
 static int __pyx_pf_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_2__cinit__(struct __pyx_obj_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_Foo *__pyx_v_self, __Pyx_memviewslice __pyx_v_arr) {
 ^~~~~~
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:3020:110: error: expected primary-expression before 'static'
 static PyMethodDef __pyx_fuse_1__pyx_mdef_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_5__cinit__ = static int __pyx_fuse_1__pyx_pw_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_5__cinit__(PyObject *__pyx_v_self, PyObject *__pyx_args, PyObject *__pyx_kwds) {
                                                                                                              ^~~~~~
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:3071:1: error: expected ',' or ';' before 'static'
 static int __pyx_pf_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_4__cinit__(struct __pyx_obj_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_Foo *__pyx_v_self, __Pyx_memviewslice __pyx_v_arr) {
 ^~~~~~
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:3128:110: error: expected primary-expression before 'static'
 static PyMethodDef __pyx_fuse_2__pyx_mdef_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_7__cinit__ = static int __pyx_fuse_2__pyx_pw_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_7__cinit__(PyObject *__pyx_v_self, PyObject *__pyx_args, PyObject *__pyx_kwds) {
                                                                                                              ^~~~~~
/home/fmheu/.cache/ipython/cython/_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d.cpp:3179:1: error: expected ',' or ';' before 'static'
 static int __pyx_pf_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_3Foo_6__cinit__(struct __pyx_obj_46_cython_magic_fed3fa8885216b9d3fc7a5864874ea9d_Foo *__pyx_v_self, __Pyx_memviewslice __pyx_v_arr) {
 ^~~~~~

Expected behavior I would expect an error in Cython compilation step, if above is not allowed in Cython.

Environment (please complete the following information):

da-woods commented 4 years ago

I suspect this is a duplicate of https://github.com/cython/cython/issues/1359 (but that one's a little light on details).

It probably should work, or at least fail earlier and with a clearer message.

If you're looking for a workaround you could probably do something like:

ctypedef fused ndcomplexview_t:
    complex[::1]
    complex[:, ::1]
    complex[:, :, ::1]

def actual_cinit(Foo self, ndcomplexview_t arr):
        if ndcomplexview_t is complex[::1]:
            self.data = &arr[0]
        elif ndcomplexview_t is complex[:, ::1]:
            self.data = &arr[0, 0]
        elif ndcomplexview_t is complex[:, :, ::1]:
            self.data = &arr[0, 0, 0]     

cdef class Foo:
    cdef complex *data
    def __cinit__(self, arr):
        actual_cinit(self, arr)
fhchl commented 4 years ago

Indeed, moving the type declarations out of the __cinit__ arguments avoids the problem. Thanks for the workaround!

Vis-Viva commented 1 year ago

Hey quick question regarding this workaround, the answer to which may be generally enlightening for some stuff going on under the hood within Cython. Does the required omission of typing information for this workaround incur python overhead due to the need to handle the "arr" argument in __cinit__ as a general python object? Or is such python overhead ALWAYS incurred by __cinit__ and other special methods due to them being def instead of cdef? Furthermore, if this overhead is incurred, what would be the point of redirecting to actual_cinit here, since the overhead from treating arr as a generic python object is incurred one way or the other?

da-woods commented 1 year ago

There's always Python overhead in calling __cinit__. The point of redirecting to actual_cinit is that within actual_cinit you have a known memoryview type that you can use (for example by indexing into it quickly). The point is not to avoid overhead when calling - it's to get Cython to figure out the types when you call it, and use those types.

If you want to call it from Cython with known types quickly then use that @staticmethod factory function approach, and move the work out of __[c]init__

Vis-Viva commented 1 year ago

There's always Python overhead in calling __cinit__. The point of redirecting to actual_cinit is that within actual_cinit you have a known memoryview type that you can use (for example by indexing into it quickly). The point is not to avoid overhead when calling - it's to get Cython to figure out the types when you call it, and use those types.

If you want to call it from Cython with known types quickly then use that @staticmethod factory function approach, and move the work out of __[c]init__

Right, I suppose the purpose of the redirection to somewhere where the type information could actually be made use of should have been rather obvious lol - ty for the explanation (: