Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: implicitly_convertible with templated constructor #5271

Open
3 tasks done
jeanchristopheruel opened this issue Jul 26, 2024 · 2 comments
Open
3 tasks done

[BUG]: implicitly_convertible with templated constructor #5271

jeanchristopheruel opened this issue Jul 26, 2024 · 2 comments
Labels
triage New bug, unverified

Comments

@jeanchristopheruel
Copy link

jeanchristopheruel commented Jul 26, 2024

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

2.13.1

Problem description

Could we make implicitly_convertible explicitly instantiate templated constructors?

Consider this type erasure pattern (remark the template constructor of A for implicit conversion).

#pragma once

#include <memory>
#include <iostream>

// Concept and Model classes for type erasure
namespace detail {
    struct Concept {
        virtual ~Concept() = default;
        virtual void print() const = 0;
        virtual std::unique_ptr<Concept> clone() const = 0;
    };

    template <typename T>
    struct Model : Concept {
        T data;
        Model(T const& data) : data(data) {}
        void print() const override { data.print(); }
        std::unique_ptr<Concept> clone() const override { return std::make_unique<Model<T>>(*this); }
    };
}

// Type-Erased Class A
class A {
    std::unique_ptr<detail::Concept> pimpl;

public:
    template <typename T>
    A(T const& x) : pimpl(std::make_unique<detail::Model<T>>(x)) {}

    A(A const& other) : pimpl(other.pimpl->clone()) {}
    A& operator=(A const& other) { pimpl = other.pimpl->clone(); return *this; }
    A(A&& other) noexcept = default;
    A& operator=(A&& other) noexcept = default;

    void print() const { pimpl->print(); }
};

// Specialized Class B
class B {
public:
    void print() const {
        std::cout << "I am B" << std::endl;
    }
};

// Specialized Class C
class C {
public:
    void print() const {
        std::cout << "I am C" << std::endl;
    }
};

void print(const A& foo) {
   foo.print();
}

The python binding:

PYBIND11_MODULE(example, m) {
    py::class_<A>(m, "A")
        .def("print", &A::print);

    py::class_<B>(m, "B")
        .def(py::init<>())
        .def("print", &B::print);

    py::class_<C>(m, "C")
        .def(py::init<>())
        .def("print", &C::print);

    py::implicitly_convertible<B, A>();
    py::implicitly_convertible<C, A>();
    
    m.def("print", &print);
}

I would expect the python binding to behave like:

import example

b = example.B()
print_from_b = example.print(b)  # This should print "I am B" due to implicit conversion

c = example.C()
print_from_c = example.print(c)  # This should print "I am C" due to implicit conversion

The error traceback:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: printA(): incompatible function arguments. The following argument types are supported:
    1. (arg0: example.A) -> None

Invoked with: <example.B object at 0x75f99e0c3130>

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

Not a regression

@jeanchristopheruel jeanchristopheruel added the triage New bug, unverified label Jul 26, 2024
@ManifoldFR
Copy link

Hey,

I'm trying to do a similar thing with nanobind right now. I think one would need to actually create their own template<typename Concrete> implicit_convertible_with_concept(py::class_<A>&cl) which would take the exposing py::class_ object as an argument and defs the constructor:

template<typename Derived> implicit_convertible_with_concept(py::class_<A>&cl)
{
  cl.def(py::init<Derived const&>());
  py::implicitly_convertible<Derived, A>();
}

@jeanchristopheruel
Copy link
Author

Good idea! And maybe we could add Parameter pack for multiple Derived types.

'''
implicit_convertible_with_concept<Derived1, Derived2, ...>(py::class_&cl)
'''

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

No branches or pull requests

2 participants