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

[FEAT] Cannot implicitly convert from None. #2778

Closed
crisluengo opened this issue Jan 9, 2021 · 8 comments · Fixed by #3059
Closed

[FEAT] Cannot implicitly convert from None. #2778

crisluengo opened this issue Jan 9, 2021 · 8 comments · Fixed by #3059

Comments

@crisluengo
Copy link
Contributor

I am going to address this issue one more time. It was first brought up in #642, then again in #1382.

In short, we cannot make a custom type that implicitly converts from None. @bstaletic suggested a solution that doesn't work.

Consider the following example code (paraphrased, might have typos):

#include "pydip.h"

class A {
public:
   int value = 0;
   A() = default;
   A(int v) : value(v) {};
}

int X(A const&) {
   return A.value;
}

PYBIND11_MODULE(mymodule, m) {
   auto a = py::class_<A>(m, "A");
   a(py::init<>());
   a("__init__", [](py::object const& o) { // @bstaletic's solution
      if (o.is_none()) {
         return A{};
      }
      throw py::type_error("boom");
   });
   //a(py::init([](py::none const&) { return A{}; }));  // I had tried this one too
   py::implicitly_convertible<py::none, A>();
   a(py::init<int>());
   py::implicitly_convertible<int, A>();

   m.def("X", X);
}

I can now do in Python:

a = A(None)
a = A(1)
X(a)
X(1)

But I cannot do

X(None)

However, if I edit pybind11/cast.h, at the top of bool load_impl(handle src, bool convert), where it says:

        if (src.is_none()) {
            // Defer accepting None to other overloads (if we aren't in convert mode):
            if (!convert) return false;
            value = nullptr;
            return true;
        }

If I remove the value = nullptr; return true; lines, then the above all works correctly.

I have not found any downsides to doing so. This means that the custom converter function gets a py::none object as input, it can choose to reject that or accept that. Any conversion function already tests what the type of the input object is, so this should not affect any existing code.

I have been using this "fix" for some months now, but would prefer a solution that doesn't require patching pybind11.


In real life, the above example is for an image class where the default-constructed image has no pixels, and is used to signal "I don't need this input" to specific functions. In C++ one can use {} for a default-constructed object, None is the most Pythonic substitution. See this other comment for details.

Sure, I could write a function for X(None), but this is for a large project with a lot of functions, and creating multiple versions of each for various inputs being None is not doable.

@crisluengo crisluengo changed the title Cannot implicitly convert from None. [FEAT] Cannot implicitly convert from None. Jan 9, 2021
@YannickJadoul
Copy link
Collaborator

A or A & or const A & is not the type of an optional parameter in C++. You cannot pass nothing to it. So pybind11 is perfectly following that. A * or std::optional<A> is a type that allows you to not pass an A object, and pybind11 will convert None to A * (as nullptr) or to std::optional<A> (as std::nullopt).

In C++ one can use {} for a default-constructed object.

That's still constructing an A object. None is not an A object, but the lack thereof.

@YannickJadoul
Copy link
Collaborator

If I remove the value = nullptr; return true; lines, then the above all works correctly.

I have not found any downsides to doing so.

I'm fairly certain (haven't tested; it's too late. I can poke around tomorrow) that this would stop None from being converted to A *. I don't see which other part of pybind11 would handle it (but again, haven't checked).
Have you run the test suite, after this change?

@crisluengo
Copy link
Contributor Author

I haven’t compiled the code here, I have made this change in my copy of pybind11 and have had it working on Python bindings to a library with a couple of thousand public symbols, including several hundred functions functions that take all different types of standard containers as arguments.

I will try running the test cases tonight.

Sure, {} constructs an object. But in Python you cannot do that. You’d have to type the name of the constructor (dip.Image()) in my case, which makes the whole thing rather verbose and unappealing. The meaning of the default-initialized object is closest to None, and I just can’t understand why it sould be not allowed to do this implicit cast.

@YannickJadoul
Copy link
Collaborator

You’d have to type the name of the constructor (dip.Image()) in my case, which makes the whole thing rather verbose and unappealing.

Definitely.

The meaning of the default-initialized object is closest to None

But I completely disagree with this. Again, const A & is saying it's expecting an A object, None is not an A object. Any resemblance between easy of writing None and {} are very much syntactical/superficial, and not related to the actual semantics.

I just can’t understand why it sould be not allowed to do this implicit cast.

I will agree that it's slightly weird that this implicit cast isn't working. But the whole py::object hierarchy is a special one, since it's just an interface to actual Python objects.

However, exposing your function as m.def("X", [](const A *a) { return X(a ? *a : A{}); }); should perfectly work, and translates your "I need an A object" C++ interface to an "I'll accept an A object or nothing" Python interface. You are changing the meaning of what this function accepts, here.

@YannickJadoul
Copy link
Collaborator

I'm surprised by how little test breakage this change causes, but this is as minimal you can get to demonstrate:

#include <pybind11/pybind11.h>

namespace py = pybind11;

struct Foo {};

PYBIND11_MODULE(example, m) {
    py::class_<Foo>(m, "Foo")
        .def(py::init<>());
    m.def("bar", [](Foo *foo) { return 42; });
}

Without your changes:

Python 3.9.1 (default, Dec  8 2020, 03:24:52) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import example
>>> example.bar(example.Foo())
42
>>> example.bar(None)
42

With your changes:

Python 3.9.1 (default, Dec  8 2020, 03:24:52) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import example
>>> example.bar(example.Foo())
42
>>> example.bar(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: bar(): incompatible function arguments. The following argument types are supported:
    1. (arg0: example.Foo) -> int

Invoked with: None

So, there's a world outside your several hundred functions.

@crisluengo
Copy link
Contributor Author

But I completely disagree with this. Again, const A & is saying it's expecting an A object, None is not an A object. Any resemblance between easy of writing None and {} are very much syntactical/superficial, and not related to the actual semantics.

But you can say exactly the same thing about 1, which is not an A object, but I allow the implicit cast. It is just about convenience for the user.

I don’t see anyone willing to write several hundred functions just to allow None being input there.

@crisluengo
Copy link
Contributor Author

Oh, the test breaking is unfortunate.

I’ll look a bit more into that, see if I understand why. There must be a solution.

Thanks!

@YannickJadoul
Copy link
Collaborator

But you can say exactly the same thing about 1, which is not an A object, but I allow the implicit cast. It is just about convenience for the user.

That's true. I did say that the lack of possibility to have an implicit conversion is slightly weird.

I’ll look a bit more into that, see if I understand why. There must be a solution.

It's not an easy thing, though, in the current design. pybind11 sees None, says "Ah, that's a valid argument for Class" (as the caster is shared between Class, Class &, Class *, const Class &, ...), accepts it as successful conversion and doesn't even consider any implicit conversion. However, in the next step, pybind11 sees you asked for a Class & and panics, because it has a nullptr, which is not a valid Class &.

crisluengo added a commit to crisluengo/pybind11 that referenced this issue Jun 25, 2021
rwgk pushed a commit that referenced this issue Jun 26, 2021
…wrapped types (#3059)

* Allow casting from None to a custom object, closes #2778

* ci.yml patch from the smart_holder branch for full CI coverage.
rwgk pushed a commit that referenced this issue Jun 26, 2021
…wrapped types (#3059)

* Allow casting from None to a custom object, closes #2778

* ci.yml patch from the smart_holder branch for full CI coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants