Fix exception handling when pybind11::weakref() fails.

[hawkinsp]

The weakref() constructor calls pybind11_fail() without clearing any
Python interpreter error state. If a client catches the C++ exception
thrown by pybind11_fail(), the Python interpreter will be left in an
error state.

I found this problem while fixing code that called pybind11::weakref(obj) where obj is not weak-referenceable. In that case, I wanted to fallback to another code path by catching the exception raised by pybind11_fail, e.g.,

try {
  x = py::weakref(obj);
} catch (std::runtime_error& e) {
 ... do something else
}

However, if you do this, then the Python error flag is left set in the catch case and not cleared. This causes later calls into the Python interpreter to fail.

It is possible that this PR should be more aggressive: all calls to pybind11_fail() should clear the error flag. If we are going to throw a C++ exception, then presumably that is replacing any Python error state.

Suggested changelog entry:

* Fix exception handling when ``pybind11::weakref()`` fails

[Skylion007]

What is the PyErr flag set to? I see two possible alternative solutions:

1. add a Debug assert that assert(!PyErr_Occurred()); to pybind11_fail.
2. always use raise_from in pybind11 so more information is passed via the __cause__ exception object. Should be easy to do now that we have dropped Python2 support and I am pretty sure that ensures the error is properly cleared.

I gave a stab of 1. on master in Debug mode, but it didn't trigger any assert failures which tells me that this error isn't caught by our current test suite. @hawkinsp can you verify this is the case and add an appropriate unit test.

@henryiii @rwgk I'd appreciate hearing your input on what the best way to harden pybind11_fail to this would be.

[hawkinsp]

@Skylion007 I may not have time to add a test to the PR, but the idea is like this:

In [32]: class C:
    ...:     __slots__ = []
    ...:

In [33]: import weakref

In [34]: c = C()

In [35]: weakref.ref(c)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-35-6812bda72132> in <module>
----> 1 weakref.ref(c)

TypeError: cannot create weak reference to 'C' object

You can reproduce by simply calling pybind11::weakref from C++ on c instead of weakref.ref() from Python.

[rwgk]

@Skylion007 wrote:

always use raise_from in pybind11 so more information is passed via the cause exception object. Should be easy to do now that we have dropped Python2 support and I am pretty sure that ensures the error is properly cleared.

I really like that idea.

Yesterday on chat @hawkinsp had the idea to move the change into the pybind11_fail implementation.

I haven't looked, could use raise_from from there? That would seem like an ideal solution.

[Skylion007]

I really feel like the proper thing to do here is just remove our pybind11_fail and propogate the typeerror with error_already_set(). Thoughts @henryiii @rwgk ?

It's a very minor breaking change, but one that will match PyBInd11 functionality with Python a bit better. I actually don't like raise_from in this context UNLESS we really have to preserve backwards compatability as the errors will be of different types if it's from PyBind11 vs Python (Runtime vs TypeError).

[henryiii]

I'm all for closer matching of pybind11 with Python, personally. We can mention it in the upgrade guide.

[rwgk]

It's a very minor breaking change

We're going from 2.9 to 2.10 anyway, that would seem fine.

[henryiii]

Is it best to test that PyPy doesn't show this behavior (as done now), or skip/xfail the test on PyPy? Guessing this behavior is likely to remain unchanged on PyPy, which would be the assumption in the current implementation, I think that's fine.

[Skylion007]

@henryiii CPython says weakref is an implementation detail. If PyPy behavior changes when we update PyPy than we can make it an xfail.

[rwgk]

Awesome, thanks a lot @Skylion007 for hammering it into such great shape!

[rwgk]

Is the _ important here? (Guessing yes, but asking JIC it's a leftover from some experiment.)

[Skylion007]

I just wanted to annotate that there was a return value we were throwing away.

[rwgk]

Makes sense! Thanks for the explanation.

[rwgk]

I'll run the global testing for this PR as soon as I can. Unfortunately the previous attempt is still stuck in the queue (bad infrastructure day).

[rwgk]

Globally tested with the 2022-02-18 00:00 batch. All good!

[Skylion007]

Whoops, @henryiii forgot to fix the initial PR to include a changelog. What's the best way to retrofit?

[henryiii]

? We don't ever include changelogs in the PR. Just make sure it's in the description so that nox -s make_changelog pulls it from GitHub.

[henryiii]

(done)