Pybind11: Casting from an unregistered type does not throw.

[nyibbang]

Hello,

Issue description

I have this issue on pybind11 v2.5.0.

In the documentation, in object.rst, section Casting back and forth, the following is stated:

When conversion fails, both directions throw the exception cast_error.

But when trying to cast an unregistered custom type, it does not. Instead it seems to be setting an error (PyErr_Occurred returns a TypeError) and it returns a None object.

Reproducible example code

#include <pybind11/pybind11.h>
#include <pybind11/embed.h>
#include <cassert>

struct F {};

int main() {
  pybind11::scoped_interpreter interp;
  F f;
  try {
    auto obj = pybind11::cast(f);
  }
  catch(...) {
    return 0;
  }
  assert(((void)"cast did not throw an exception", false));
  return -1;
}

According to the documentation, we should not get an assertion failure with this code.

[YannickJadoul]

I can reproduce and confirm the bug. Minor correction: I seems like obj is a nullptr and not None?

[YannickJadoul]

Aaaaaaah, that's what this old patch I had lying around fixes! I forgot what it actually does, but it seems this fixes things?

diff --git a/pybind11/include/pybind11/cast.h b/pybind11/include/pybind11/cast.h
index fab985873..d94005328 100644
--- a/pybind11/include/pybind11/cast.h
+++ b/pybind11/include/pybind11/cast.h
@@ -719,7 +719,8 @@ public:
         detail::clean_type_id(tname);
         std::string msg = "Unregistered type : " + tname;
         PyErr_SetString(PyExc_TypeError, msg.c_str());
-        return {nullptr, nullptr};
+        throw error_already_set();
+        // return {nullptr, nullptr};
     }

     const type_info *typeinfo = nullptr;

I need to check whether this doesn't break anything else that is expecting a {nullptr, nullptr} to be returned, though.

[nyibbang]

Thank you, so you're confirming that this should be the expected behavior ?

One workaround for me is to do the following:

    auto obj = pybind11::cast(f);
    if (PyErr_Occurred())
       throw pybind11::error_already_set();

If too tedious, it's a fairly simple thing to put in a function.

[YannickJadoul]

Thank you, so you're confirming that this should be the expected behavior ?

I believe so. Calling PyErr_SetString without throwing a pybind11::error_already_set() is a mistake.

One workaround for me is to do the following:

Yes, definitely, but that's exactly what pybind11 is trying to abstract away from you by using error_already_set. So somewhere, pybind11 should throw that exception itself. The main question (I just need half an hour or an hour tonight or tomorrow) is whether my patch is in the right spot.

But this patch also works for you, right?

Spoiler alert: I just tried, and some tests áre failing, so this is not the full solution.

[nyibbang]

But this patch also works for you, right?

It does yes.

[YannickJadoul]

Still working on this, btw. But it turns out to be a lot harder than the 2 line patch I wrote above...

[nyibbang]

Okay, no worry. For me it's not a blocking issue, I applied a workaround and I've been using it, so it's not a big deal, no rush. :wink: