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]

A related observation here: the current code may wrap BaseException as a TypeError, which is probably always a bug.

Xref #5457 (comment)

[davidhewitt]

I have been thinking more about this today. I think I'm still in favour of using error notes. For example, pytest will render the note properly:

=================================== FAILURES ===================================
________________________________ test_err_note _________________________________

    def test_err_note():
        e = ValueError("This is an error message")
        e.add_note("Note: this error occurred extracting argument 'foo'")
>       raise e
E       ValueError: This is an error message
E       Note: this error occurred extracting argument 'foo'

So for me, I think the upsides are:

• We can add notes to all errors in a consistent way
• Adding a note to an existing error is probably more efficient than creating a new error object to substitute / wrap it
• In user-facing contexts (full tracebacks, interactive use, pytest output) they will get more consistent feedback than with our current implementation which only enriches TypeError (e.g. ValueError, OverflowError come to mind immediately).
• I might even be tempted to argue that not mangling the orginal exception message might make it easier for users to search for e.g. TypeError: 'str' object cannot be interpreted as an integer

[flying-sheep]

If I understand correctly, add_note is indeed better: no need to have two TypeErrors with the same traceback visible. Since cause is set, the current way of doing things will probably be displayed as something like:

[traceback]
TypeError: wrong type

The above exception was the direct cause of the following exception:

[the exact same traceback]
TypeError: argument 'foo': wrong type

If I’m right that invalidates your last argument about Ctrl-F-ing, but you’re totally right about the rest!

[flying-sheep]

Hi! this would be wonderful to have, can we get this unstalled?

[davidhewitt]

Circled back to this... here's some further thoughts.

[davidhewitt]

I have been thinking more about this today. I think I'm still in favour of using error notes. For example, pytest will render the note properly:

=================================== FAILURES ===================================
________________________________ test_err_note _________________________________

    def test_err_note():
        e = ValueError("This is an error message")
        e.add_note("Note: this error occurred extracting argument 'foo'")
>       raise e
E       ValueError: This is an error message
E       Note: this error occurred extracting argument 'foo'

So for me, I think the upsides are:

• We can add notes to all errors in a consistent way
• Adding a note to an existing error is probably more efficient than creating a new error object to substitute / wrap it
• In user-facing contexts (full tracebacks, interactive use, pytest output) they will get more consistent feedback than with our current implementation which only enriches TypeError (e.g. ValueError, OverflowError come to mind immediately).
• I might even be tempted to argue that not mangling the orginal exception message might make it easier for users to search for e.g. TypeError: 'str' object cannot be interpreted as an integer

[Icxolu]

I've rebased this, switched to the add_note variant and updates the tests to also validate __notes__ where needed.

[flying-sheep]

I built a little prototype based on this: https://github.com/flying-sheep/pyo3-err-bridge/blob/main/src/lib.rs

It allow to wrap backtrace-capturing errors (currently color-eyre and anyhow) into PyErrs, maintaining the traceback, so it’s visible on the python side.

Once this PR is merged, I’ll convert the prototype into a PR that updates our anyhow feature with this, and add a color-eyre feature.

[davidhewitt]

LGTM with just two tiny nits, thanks!

[Icxolu]

For reference: I've adapted the tests to dynamically check if add_note exists and if so, checks that __notes__ contains the expected values. This skips the checks on older Python versions that don't support add_note, while still checking on newer interpreters in abi3 mode.