cython icon indicating copy to clipboard operation
cython copied to clipboard

[bug] issues with "cppclass overloadable methods"

Open da-woods opened this issue 10 months ago • 2 comments

I ran into a couple of problems today that I was able to trace back to this PR. I was able to fix both of them, and I wasn't sure whether to open issues because I strongly suspect that they're simply unsupported use cases that happened to work previously and are better caught after the changes in this PR, so I thought I'd ask here first rather than opening an issue. Apologies if that's not your preferred approach, I wasn't sure what would be easier for you.

The first problem came from code that was declaring C++ template APIs in a pxd file as explicit template instantiations rather than as templates, i.e. roughly this

from libcpp.memory cimport unique_ptr
cdef extern from *:
    """
    #include <memory>

    template <typename T>
    class myclass {};

    template <typename T>
    void std::unique_ptr<myclass<T>> f() {}

    template class myclass<T>;
    template std::unique_ptr<myclass<T>>
    """"
    cdef cppclass myclass[T]:
        pass

    cdef unique_ptr[myclass[int]] f[int]() # Note the explicit `int` instead of `T` here

There's something more to it than just that since this MRE compiles for me, but changing the function declaration to use a template typename instead of a real type fixed compilation, and I've never seen the real type used there before, so I don't even know if this was intended to work.

The second problem came from a case where some code was shadowing a (non-templated) C function with a template C++ function:

from libc.stdlib cimport free

cdef extern from *:
    """
    template <typename T>
    void free(...) {}
    """
    cdef void free[T](...)

free[int](...)  # This used to work, but started throwing errors after this PR.

I wouldn't expect this to be valid Cython either, but apparently it worked before.

Do either of these changes appear to merit any follow-up to you? If so, I'm happy to open up an issue or two with proper MREs, but I wasn't sure if that was worthwhile here. Thanks!

Originally posted by @vyasr in https://github.com/cython/cython/issues/3235#issuecomment-2024105616

da-woods avatar Mar 28 '24 08:03 da-woods

The second one:

What's happening is that previously the templated free completely overrode the non-templated free I think. Therefore explicitly indexing it worked fine. It's now trying to explicitly index the non-templated free.

The simple solution is to go through the available alternatives when indexing and pick one with a matching number of templates. That seems to work fine and is probably no worse than what we had before.

However, it's splitting the "find the right alternative" method into two parts - a simple one that applies to explicitly indexed template functions (and requires a single exact match) and a more complicated one that applies to non-index functions.

Will try to decide how complicated it's worth making this...

da-woods avatar Apr 14 '24 12:04 da-woods

(Comment mainly for my own future reference) My WIP for the second issue is at https://github.com/da-woods/cython/tree/cpp-overloaded-alternatives-try. It fixes the issue but I suspect it should be done differently.

da-woods avatar Apr 27 '24 07:04 da-woods