Add type_caster<PyObject>

[rwgk]

Description

This is to support automatic wrapping of APIs that make use of PyObject *. It is impractical to rewrite all such APIs to use py::handle or py::object (although that would of course be ideal).

Suggested changelog entry:

pybind11/type_caster_pyobject_ptr.h was added to support automatic wrapping of APIs that make use of `PyObject *`. This header needs to included explicitly (i.e. it is not included implicitly with pybind/pybind11.h).

[rwgk]

The equivalent of this PR was merged under google/pybind11clif#30021

[EthanSteinberg]

Is this actually necessary? Seems a bit unintuitive here. When are situations when obj would be a temporary?

[rwgk]

In a callback (functional.h):

• pybind11/include/pybind11/functional.h

  Line 109 in 7ab88d2

  return hfunc.f(std::forward<Args>(args)...).template cast<Return>();

That is using object::cast<PyObject *>() which winds up in py::cast<PyObject *>(handle):

• pybind11/include/pybind11/cast.h

  Lines 1136 to 1139 in 7ab88d2

  	template <typename T>
  	T object::cast() const & {
  	return pybind11::cast<T>(*this);
  	}

This actually took me a while to discover and debug.

I don't see an easy way to make the code in functional.h behave differently.

But I'm also thinking:

cast<PyObject *>(something_that_generates_a_temporary_object())

with the temporary passed as handle is a really bad trap in general (especially if the cast is actually written as cast<T> buried in the guts of some library).

By adopting the inc_ref() convention we don't have to do anything special for callbacks and we avoid the trap.

Users have to learn once that the refcount is incremented, it always applies.

This is only for users that find it necessary to actually drop down to the level of dealing with raw PyObject *s. In that context, having to be super mindful about refcounts is a given, and careful leak checking of the tests is a standard practice for me at least (while True: & top is the most reliable btw).

Another way to look at it:

• Risk of a accidental leaks (with the inc_ref()).
  vs
• Risk of UB (segfault if you're lucky).

What tipped the scale in my mind was that the inc_ref() convention means nothing else needs to change, which is usually a good sign for changes like this (fitting something slightly different into an existing system).

[Skylion007]

This is necessary incase obj is a temporary

Which is why I am advocating for the additional overload below. For C++ calls, we can determine if the object is a temporary (ie. it's an rvalue) and avoid having to increment it under those circumstances.

[rwgk]

I want to mention: in the meantime I discovered that this type_caster unlocks supporting numpy arrays with dtype='O'. It's incomplete work, although internally globally tested:

• Snapshot of WIP: Add format_descriptor<PyObject *> google/pybind11clif#30023

What you see there is all it took to add the 'O' support.

[Skylion007]

Is the any case where you want to steal the ref? Ie. shouldn't you also add this? I do not think it will be used currently by your code but if that cast path is used in the future, better to have it.

Suggested change

	}
	}
	template <typename T,
	detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value, int> = 0>
	T cast(object &&obj) {
	return obj.release().ptr();
	}

[rwgk]

Is the any case where you want to steal the ref?

Not sure. I think the best way to answer this: can we come up with a meaningful test case?

My gut feeling is that stealing a Python reference like this is much more likely to backfire than be helpful. When dropping down to the bare-metal level of PyObject *, it's basically a given that the author of the code with the raw pointers needs to think 3+ times about all nuances of lifetime management. Keeping the mechanics here as simple as possible makes that easier.

[Skylion007]

Wouldn't the current test cases cover it? Semantically, it's equivalent to what is written above, the difference is that without this the py::object gets cast up to a handle, inc_ref'd by the caster, then dec_ref'ed as the py::object is destructed. All we are doing here is removing a useless increment / decrement.

Easy example/test case:

auto *pyobject_ptr = py::cast<PyObject*>(std::move(obj));

If we include this explicit cast for callbacks, some end will be silly enough to call it so we should make the semantics as useful/efficient as possible.

[Skylion007]

Actually, a slight modification to the current one of the current test cases would definitely cover it (see below).

[Skylion007]

Won't this const name break mypy/ type parsing? * has special meaning in docstrings and Python itself cannot return pointers (they will just appear as Python objects). Also returning a null pyobject ptr will cause a SystemError in Python.

[Skylion007]

As unsatisfying as it, I think the name should be an object caster to avoid breaking anything.

[Skylion007]

It will likely break any position args etc that come after it, right? If you really want to differentiate it (and create invalid typing) just have it be PyObject

[rwgk]

As unsatisfying as it, I think the name should be an object caster to avoid breaking anything.

Wow ... that's a tough one to decide.

From a high-level perspective, using object here definitely looks much better.

But it hides an important clue: beware, this is dealing with raw pointers.

But then again, to whom is that clue useful? The author of the code with the raw pointers already needs to be super careful when writing the code, i.e. doesn't need any extra clues. For everybody else is probably a not very enlightening surprise.

I'm leaning towards thinking using object as the name will be better overall.

[rwgk]

Testing 5ccb893 now.

[Skylion007]

Actually one typing detail. Does this caster accept Py_None as meaning nullptr? That would be the one caveat and if did, it may better to list the type as typing.Optional

[rwgk]

Do you mean the load() step?

That's essentially just doing an inc_ref and nothing else:

    bool load(handle src, bool) {
        value = reinterpret_borrow<object>(src);
        return true;
    }

[Skylion007]

Is an error guaranteed here? Python may raise a system exception, but I think that might be triggered before even entering the caster. Worried about the case where a nullptr is fed in somehow but no error is set.

[rwgk]

Is an error guaranteed here?

Good question: No, but error_already_set will handle this gracefully:

pybind11/include/pybind11/pytypes.h

Lines 483 to 487 in 6de6191

	if (!m_type) {
	pybind11_fail("Internal error: " + std::string(called)
	+ " called while "
	"Python error indicator not set.");
	}

(This was one of the things I made as clean/clear as possible while working on #1895.)

[Skylion007]

This can test my custom caster by just making the py::handle a py::object and calling std::move(obj) for the cast. (if you avoid the ref count access below).

[rwgk]

FYI I just pushed a commit that expands the source code comment, before seeing your latest comment here.

I need to look/think more.

[Skylion007]

The more I think it, the more I think it's a good feature / caster to have and I do not see any reason not to include the object&& overload we already do it for other object subclasses anyhow further down in the file.

[rwgk]

I didn't find it easy to unlock that optimization: 2e5a699

Do you have ideas for achieving the same outcome with less code?

The added code complexity is a bit at odds with the really minor runtime optimization, in any real-world application.

[Skylion007]

@rwgk I am just bringing the complexity / runtime optimization inline with the other caster methods. If adding a single overload can signficantly improve performance and remove unnecessary refcount calls, I see no reason why not to include it.

[rwgk]

OK, thanks for the review!