add PyErr::set_traceback

[Icxolu]

This adds PyErr::set_traceback in an attempt to fix #5348. For Python 3.12+ this is pretty straight forward. For <3.12 it's a bit more tricky because we track it ourselves. For now used a Mutex to get the interior mutability, but since we always have a Python token available (and there is no free-threading in this scenario) it should also be possible to use an UnsafeCell and synchronize on the GIL.

Closes #5348

[davidhewitt]

Thanks for looking into this! I wonder if there's a way to add a test?

I have some crazy ideas, both which seem slightly high-risk and might be safer to avoid shipping in 0.26 if it's going out imminently...

[davidhewitt]

An alternative on Python 3.11+ could be to use call .add_note() to attach a note along the lines of "this happened while processing argument X". This would also have the upside that we could do it for all exception types.

... if so, it might even be good enough to just drop the "remapping" completely even on old Python versions, and just do nothing on old versions where the notes don't exist.

[Icxolu]

Interesting, I did not know about add_note. This would of course be much simpler (at the expense of going through the general call api, I don't think there is a C version). This is how it would look like

Traceback (most recent call last):
  File "G:\RustProjects\pyo3-workspace\pyo3-scratch\foo.py", line 7, in <module>
    test(Foo())
    ~~~~^^^^^^^
  File "G:\RustProjects\pyo3-workspace\pyo3-scratch\foo.py", line 4, in foo
    raise TypeError("wrong type")
TypeError: wrong type
while processing `bar`

or

Traceback (most recent call last):
  File "G:\RustProjects\pyo3-workspace\pyo3-scratch\foo.py", line 7, in <module>
    test(Foo())
    ~~~~^^^^^^^
TypeError: 'str' object cannot be interpreted as an integer
while processing `bar`

[davidhewitt]

Agreed, we'd have to go via the Python call, but at least we could optimize that to be a "vectorcall". I think given this is already the error pathway it's not the end of the world if it's a little slower.

What do you think of this option? I like the fact that it's applicable to all errors and simplifies, though I worry about possible silent breakage downstream.

[Icxolu]

I'm open to it. The ability to apply it to all exceptions is pretty appealing. Also there is also context that we currently not transfer to the remapped exception (and I guess args as well). Not sure if there would still be an observable difference if we added that as well.

Depending on the wording the newline of add_note might be a bit annoying, but maybe with something like this it would be acceptable 🤔

Traceback (most recent call last):
  File "G:\RustProjects\pyo3-workspace\pyo3-scratch\foo.py", line 7, in <module>
    test(Foo())
    ~~~~^^^^^^^
TypeError: 'str' object cannot be interpreted as an integer
Note: This occurred while processing argument `bar`.

though I worry about possible silent breakage downstream

What kind of breakage do you have in mind here? Just someone relying on the exact error message? I think the type would be the same, right?

[davidhewitt]

Yeah, dependency on the error message. I would argue that it's generally bad practice to depend on error message content (maybe aside from in tests), but you never know and hard to inform users of changes 😬

That said, I think I like it enough that we should move forward with the .add_note()? But I am not sure enough that I want to rush to get it into 0.26 🤔

[Icxolu]

I just noticed that while these notes are shown on unhandled exceptions, they are not part final error message. So catching the exception and just printing it out will not show them:

from pyo3_scratch import test
class Foo:
    def foo(self):
        raise TypeError("wrong type")

try:
    test(Foo())
except Exception as e:
    print(e) # does not show the note
    # wrong type

test(Foo()) 
# TypeError: wrong type
# Note: This occurred while processing argument 'bar'.

For the same reason the notes are not shown in the Debug or Display impls for PyErr. For PyErr we could query the notes and add them manually, but then these would be quite different from what Python shows. That does not feel particularly great and makes me question again whether add_note is the way forward here...

I guess this is at least a good argument for taking a bit more time here and not land this in 0.26.

[davidhewitt]

Ah, that's a good (and unfortunate) point.

[davidhewitt]

Mild slight crazy idea I've wondered about in the past, I have wondered if there is a use case for a type which is something like AtomicPy (maybe including the Option, not sure, would need it in this case I guess) which would allow for swapping the contained objects using atomics. Could avoid locking? 🤔

[Icxolu]

I actually had the same idea that AtomicPtr should be possible here, but without a proper wrapper it's to hairy.

An AtomicPy could also be useful for frozen pyclasses that want to swap out a Py field. We would need to figure out whether it's possible to write a safe interface around it. I guess it would not be possible to borrow it, we would always have to work with "owned" pointers.

[davidhewitt]

Ah yes I think frozen classes might be where I was wondering about this in the past too.

I would definitely be open to exploring that further, I have no idea how it would feel in practice!

[davidhewitt]

I guess it would not be possible to borrow it, we would always have to work with "owned" pointers.

I guess so, but it's not completely stuck. &AtomicPy would work, but the semantics would presumably be that someone else might swap the object that it points to until you .clone_ref(py) to get a Bound, I guess.

[Icxolu]

I played a bit to find out how the interface could look like. I put that experiment in #5356