Proto #1: Exception instances as Py<BaseException> #686

nagisa · 2019-12-09T22:36:24Z

This prototype implements use of Py<BaseException> as the instance to
use for exception instances. These instances integrate reasonably well
with the Rust’s standard error mechanisms by implementing the Display
and Error traits. These error types can also be stored into e.g.
failure::Fails or other error types as a cause of some greater error.

There are a couple of sketchy points in this PR most notable of which is
the implementation of std::error::Error::source. See the included FIXME.

This is also somewhat of an experiment around the ownership-based Py<>
that I have outlined in #679 and the issues
implementing source appear to stem from exactly the fact that Python’s GIL
and Rust’s ownership are fairly incompatible.

FWIW: I think this might be revealing a soundness issue in our current setup too.

This prototype implements use of Py<BaseException> as the instance to use for exception instances. These instances integrate reasonably well with the Rust’s standard error mechanisms by implementing the `Display` and `Error` traits. These error types can also be stored into e.g. `failure::Fail`s or other error types as a cause of some greater error.

nagisa · 2019-12-09T22:36:44Z

cc @kngwyu

nagisa · 2019-12-09T23:15:08Z

src/exceptions.rs

+impl std::error::Error for BaseException {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        unsafe {
+            // Returns either `None` or an instance of an exception.


This needs to obtain the GIL.

kngwyu · 2019-12-13T03:21:01Z

src/exceptions.rs

        unsafe impl PyTypeObject for $name {
            fn init_type() -> std::ptr::NonNull<$crate::ffi::PyTypeObject> {
                unsafe { std::ptr::NonNull::new_unchecked(ffi::$exc_name as *mut _) }
            }
        }
+
+        impl<'v> PyTryFrom<'v> for $name {


Upcasting to PyAny is indeed a missing part 👍

kngwyu · 2019-12-13T03:27:54Z

src/exceptions.rs

+impl std::fmt::Debug for BaseException {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        // Sneaky: we don’t really need a GIL lock here as nothing should be able to just mutate
+        // the "type" of an object, right? RIGHT???


Sorry I also am not sure 😓
We need tests.

kngwyu · 2019-12-13T03:30:43Z

src/exceptions.rs

+        let py_type_name = unsafe { CStr::from_ptr((*(*self.as_ptr()).ob_type).tp_name) };
+        let type_name = py_type_name.to_string_lossy();
+        f.debug_struct(&*type_name)
+            // TODO: print out actual fields!


We can access actual field only by type name?
Or we need to get the global PyExc_~s here?

By "actual fields" I meant the fields of the PyException_HEAD structure component, which is defined as such:

#define PyException_HEAD PyObject_HEAD PyObject *dict;\ PyObject *args; PyObject *traceback;\ PyObject *context; PyObject *cause;\ char suppress_context;

At very least values of args, traceback, context, cause and suppress_context are interesting, but I can imagine it being useful to also print out at least the refcount from PyObject_HEAD too.

f.debug_struct(&*type_name) is just a builder for debug output. See https://doc.rust-lang.org/stable/std/fmt/struct.Formatter.html#method.debug_struct. The final result should look something like

f.debug_struct(type_name).field("args", &self.args).field("traceback", &self.traceback)/* etc... */.finish()

kngwyu · 2019-12-13T03:36:52Z

src/exceptions.rs

+                // must return a `&Py<BaseException>` instead… but we cannot do that because
+                // nothing is storing such a thing anywhere and thus we cannot take a reference to
+                // that…


Sorry I feel it difficult to understand why we cannot use Py<Exception> here... 🤔

We cannot return a Py<Exception> because the trait contract requires:

fn source(&self) -> Option< & (dyn std::error::Error + 'static)>

The most important detail is that this method is returning a reference with a lifetime tied to &self (I tried to make it more visible by adding whitespace around it). In other words, it requires that you return a reference to something in Self. Trying to return a Py<BaseException> would result in the typical "value does not live long enough" error, very much like the snippet below would:

fn bogus(&self) -> &str { &String::from("banana") // cannot compile because `String` does not live long enough... }

I see, thanks.

kngwyu · 2019-12-13T03:45:54Z

Thank you, this would be a big step for us.

nagisa · 2019-12-15T00:33:51Z

src/exceptions.rs

+        let gil = Python::acquire_gil();
+        let py = gil.python();
+        if let Ok(s) = crate::ObjectProtocol::str(&*py_self.as_ref(py)) {
+            write!(f, ": {}", &s.to_string_lossy())


FIXME: if the resulting string is empty we want to not add the : .

davidhewitt · 2020-04-11T08:35:35Z

@nagisa I'm curious if you plan to continue with this PR? If not, I'd be happy to take it on, rebase it on master and address the changes reviewed here.

nagisa · 2020-04-11T13:18:17Z

@davidhewitt you’re welcome to take over this. I still think it is very worthwhile to do this but I also no longer have much drive to work on this feature as I implemented something simpler, although less elegant, directly in my code.

kngwyu · 2020-06-02T07:00:11Z

I noticed we lack the link to the most relevant issue #682

davidhewitt · 2020-06-02T20:44:32Z

I have a PR which is almost ready to open as a draft to replace this one.

I've stuck to the original idea here but gone a bit further: &BaseException and Py<BaseException> implement std::error::Error.

&BaseException turns out to be pretty nice here - if we bound &BaseException by the GIL lifetime in the normal way, then the lifetime issue above related to Error::cause goes away. (Because the returned reference can be an owned &BaseException reference tied to the GIL lifetime.)

davidhewitt · 2020-07-06T06:46:22Z

Superseded by #1024

nagisa commented Dec 9, 2019

View reviewed changes

kngwyu reviewed Dec 13, 2019

View reviewed changes

nagisa commented Dec 15, 2019

View reviewed changes

kngwyu mentioned this pull request Apr 11, 2020

Add catch_unwind! macro to prevent panics crossing ffi boundaries #797

Merged

4 tasks

davidhewitt mentioned this pull request Jul 4, 2020

Rework exceptions to be native types #1024

Merged

1 task

davidhewitt closed this Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proto #1: Exception instances as Py<BaseException> #686

Proto #1: Exception instances as Py<BaseException> #686

nagisa commented Dec 9, 2019 •

edited

nagisa commented Dec 9, 2019

nagisa Dec 9, 2019

kngwyu Dec 13, 2019

kngwyu Dec 13, 2019

kngwyu Dec 13, 2019

nagisa Dec 15, 2019 •

edited

kngwyu Dec 13, 2019

nagisa Dec 15, 2019 •

edited

kngwyu Dec 18, 2019

kngwyu commented Dec 13, 2019

nagisa Dec 15, 2019

davidhewitt commented Apr 11, 2020

nagisa commented Apr 11, 2020

kngwyu commented Jun 2, 2020 •

edited

davidhewitt commented Jun 2, 2020

davidhewitt commented Jul 6, 2020

Proto #1: Exception instances as Py<BaseException> #686

Proto #1: Exception instances as Py<BaseException> #686

Conversation

nagisa commented Dec 9, 2019 • edited

nagisa commented Dec 9, 2019

nagisa Dec 9, 2019

Choose a reason for hiding this comment

kngwyu Dec 13, 2019

Choose a reason for hiding this comment

kngwyu Dec 13, 2019

Choose a reason for hiding this comment

kngwyu Dec 13, 2019

Choose a reason for hiding this comment

nagisa Dec 15, 2019 • edited

Choose a reason for hiding this comment

kngwyu Dec 13, 2019

Choose a reason for hiding this comment

nagisa Dec 15, 2019 • edited

Choose a reason for hiding this comment

kngwyu Dec 18, 2019

Choose a reason for hiding this comment

kngwyu commented Dec 13, 2019

nagisa Dec 15, 2019

Choose a reason for hiding this comment

davidhewitt commented Apr 11, 2020

nagisa commented Apr 11, 2020

kngwyu commented Jun 2, 2020 • edited

davidhewitt commented Jun 2, 2020

davidhewitt commented Jul 6, 2020

nagisa commented Dec 9, 2019 •

edited

nagisa Dec 15, 2019 •

edited

nagisa Dec 15, 2019 •

edited

kngwyu commented Jun 2, 2020 •

edited