Skip to content

add PyErr::set_traceback #5349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add PyErr::set_traceback #5349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions src/err/err_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use std::{
thread::ThreadId,
};

#[cfg(not(Py_3_12))]
use crate::sync::MutexExt;
use crate::{
exceptions::{PyBaseException, PyTypeError},
ffi,
Expand Down Expand Up @@ -137,7 +139,7 @@ pub(crate) struct PyErrStateNormalized {
ptype: Py<PyType>,
pub pvalue: Py<PyBaseException>,
#[cfg(not(Py_3_12))]
ptraceback: Option<Py<PyTraceback>>,
ptraceback: std::sync::Mutex<Option<Py<PyTraceback>>>,
Copy link
Member

@davidhewitt davidhewitt Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

@Icxolu Icxolu Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@davidhewitt davidhewitt Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@davidhewitt davidhewitt Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Icxolu Icxolu Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl PyErrStateNormalized {
Expand All @@ -147,10 +149,10 @@ impl PyErrStateNormalized {
ptype: pvalue.get_type().into(),
#[cfg(not(Py_3_12))]
ptraceback: unsafe {
Py::from_owned_ptr_or_opt(
Mutex::new(Py::from_owned_ptr_or_opt(
pvalue.py(),
ffi::PyException_GetTraceback(pvalue.as_ptr()),
)
))
},
pvalue: pvalue.into(),
}
Expand All @@ -169,6 +171,8 @@ impl PyErrStateNormalized {
#[cfg(not(Py_3_12))]
pub(crate) fn ptraceback<'py>(&self, py: Python<'py>) -> Option<Bound<'py, PyTraceback>> {
self.ptraceback
.lock_py_attached(py)
.unwrap()
.as_ref()
.map(|traceback| traceback.bind(py).clone())
}
Expand All @@ -182,6 +186,21 @@ impl PyErrStateNormalized {
}
}

#[cfg(not(Py_3_12))]
pub(crate) fn set_ptraceback<'py>(&self, py: Python<'py>, tb: Option<Bound<'py, PyTraceback>>) {
*self.ptraceback.lock_py_attached(py).unwrap() = tb.map(Bound::unbind);
}

#[cfg(Py_3_12)]
pub(crate) fn set_ptraceback<'py>(&self, py: Python<'py>, tb: Option<Bound<'py, PyTraceback>>) {
let tb = tb
.as_ref()
.map(Bound::as_ptr)
.unwrap_or_else(|| crate::types::PyNone::get(py).as_ptr());

unsafe { ffi::PyException_SetTraceback(self.pvalue.as_ptr(), tb) };
}

pub(crate) fn take(py: Python<'_>) -> Option<PyErrStateNormalized> {
#[cfg(Py_3_12)]
{
Expand Down Expand Up @@ -227,7 +246,7 @@ impl PyErrStateNormalized {
ptype.map(|ptype| PyErrStateNormalized {
ptype: ptype.unbind(),
pvalue: pvalue.expect("normalized exception value missing").unbind(),
ptraceback: ptraceback.map(Bound::unbind),
ptraceback: std::sync::Mutex::new(ptraceback.map(Bound::unbind)),
})
}
}
Expand All @@ -244,7 +263,7 @@ impl PyErrStateNormalized {
pvalue: unsafe {
Py::from_owned_ptr_or_opt(py, pvalue).expect("Exception value missing")
},
ptraceback: unsafe { Py::from_owned_ptr_or_opt(py, ptraceback) },
ptraceback: unsafe { std::sync::Mutex::new(Py::from_owned_ptr_or_opt(py, ptraceback)) },
}
}

Expand All @@ -254,10 +273,13 @@ impl PyErrStateNormalized {
ptype: self.ptype.clone_ref(py),
pvalue: self.pvalue.clone_ref(py),
#[cfg(not(Py_3_12))]
ptraceback: self
.ptraceback
.as_ref()
.map(|ptraceback| ptraceback.clone_ref(py)),
ptraceback: std::sync::Mutex::new(
self.ptraceback
.lock_py_attached(py)
.unwrap()
.as_ref()
.map(|ptraceback| ptraceback.clone_ref(py)),
),
}
}
}
Expand Down Expand Up @@ -308,7 +330,10 @@ impl PyErrStateInner {
}) => (
ptype.into_ptr(),
pvalue.into_ptr(),
ptraceback.map_or(std::ptr::null_mut(), Py::into_ptr),
ptraceback
.into_inner()
.unwrap()
.map_or(std::ptr::null_mut(), Py::into_ptr),
),
};
unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) }
Expand Down
5 changes: 5 additions & 0 deletions src/err/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ impl PyErr {
self.normalized(py).ptraceback(py)
}

/// Set the traceback associated with the exception, pass `None` to clear it.
pub fn set_traceback<'py>(&self, py: Python<'_>, tb: Option<Bound<'py, PyTraceback>>) {
self.normalized(py).set_ptraceback(py, tb)
}

/// Gets whether an error is present in the Python interpreter's global state.
#[inline]
pub fn occurred(_: Python<'_>) -> bool {
Expand Down
1 change: 1 addition & 0 deletions src/impl_/extract_argument.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ pub fn argument_extraction_error(py: Python<'_>, arg_name: &str, error: PyErr) -
let remapped_error =
PyTypeError::new_err(format!("argument '{}': {}", arg_name, error.value(py)));
remapped_error.set_cause(py, error.cause(py));
remapped_error.set_traceback(py, error.traceback(py));
Copy link
Member

@davidhewitt davidhewitt Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Icxolu Icxolu Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

Copy link
Member

@davidhewitt davidhewitt Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Icxolu Icxolu Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@davidhewitt davidhewitt Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor Author

@Icxolu Icxolu Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@davidhewitt davidhewitt Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@davidhewitt davidhewitt Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Xref #5457 (comment)

remapped_error
} else {
error
Expand Down
Loading