pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

[BUG]: implicitly_convertible with templated constructor

Open jeanchristopheruel opened this issue 1 year ago • 2 comments

Required prerequisites

  • [X] Make sure you've read the documentation. Your issue may be addressed there.
  • [X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
  • [X] Consider asking first in the Gitter chat room or in a Discussion.

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 avatar Jul 26 '24 19:07 jeanchristopheruel

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>();
}

ManifoldFR avatar Aug 22 '24 19:08 ManifoldFR

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

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

jeanchristopheruel avatar Aug 22 '24 20:08 jeanchristopheruel