Add PyCapsule API

[milesgranger]

Will close #1193

Ref: https://docs.python.org/3/c-api/capsule.html#capsules

TODO:

• an entry in CHANGELOG.md
• docs to all new functions and / or detail in the guide
• tests for all new or changed functions

[davidhewitt]

Just wanted to say thank-you for working on this - it's a really nice feature to add to PyO3. I've been a bit busy recently, however trying to find a moment to review when I can...

[milesgranger]

No worries; thank you! I'm still chipping away as I get some time here and there. I'm sure there are some glaring errors I've missed, so put it here anyway if someone wanted to take a peak.

[mejrs]

Thanks! Sorry for not getting to this sooner.

Unfortunately casting away lifetimes and types like this is really, really unsafe.

For example, this segfaults:

fn main() {
    let cap: Py<PyCapsule> = Python::with_gil(|py| {
        let name = CString::new("foo").unwrap();
        let cap = PyCapsule::new(py, (), &name, None).unwrap();

        let stuff: Vec<u8> = vec![1, 2, 3, 4];

        cap.set_context(py, &stuff).unwrap();

        cap.into()
    });

    Python::with_gil(|py| {
        let ctx: Option<&&Vec<u8>> = cap.as_ref(py).get_context(py).unwrap();
        dbg!(ctx);
    })
}

Or, you can implement transmute:

fn transmute<'p, T, U>(py: Python<'p>, t: T) -> &'p U {
    let name = CString::new("foo").unwrap();
    let cap = PyCapsule::new(py, (), &name, None).unwrap();
    cap.set_context(py, &t).unwrap();
    let ctx: Option<&U> = cap.get_context(py).unwrap();
    ctx.unwrap()
}

fn main() {
    Python::with_gil(|py| {
        let foo = 32u8;
        let boom: &Vec<u32> = transmute(py, &foo);
        dbg!(boom);
    });
}

And you can also use Py<PyCapsule> to smuggle things like Rc to other threads.

A good way to fix some of this is to introduce some variance - i.e., make the type PyCapsule<T>, so that type information is not lost. However this may make a safe api of PyCapsule almost useless.

Maybe you can use dyn Any for this? You can do some runtime reflection with it.

[milesgranger]

I suspected I'd fouled that up a bit. Thanks for the thorough explanation, the failing example is nice to have. I'll work on it; suspect this PR will be "slow to update". Working on it as time presents itself. :-)

[milesgranger]

My current understanding is capsules are used to store by value and not by reference (unless static reference). If I modify your example to pass by value, it works fine. Thus, if I update the signatures of new and set_context to have T: 'static then at least your example would no longer compile. Then current tests and the updated example here all pass:

pub fn new<'py, T: 'static>(
        py: Python<'py>,
        value: T,
        name: &CStr,
        destructor: Option<ffi::PyCapsule_Destructor>,
    ) -> PyResult<&'py Self>;

pub fn set_context<'py, T: 'static>(&self, py: Python<'py>, context: T) -> PyResult<()>;

I'm still pretty confident the value of Box::into_raw(T) as *mut c_void leaked. Which I suppose is another issue.

Edit:
Nvm, using heaptrack and some bloated test Vecs, it doesn't seem like it's leaked. Assume Python ends up owning the pointer/data?

[davidhewitt]

Thanks, in addition to discussion above I finally had some time to give this a read today. I think there's a few things which need to be solved yet.

[davidhewitt]

The pointer value of the capsule is pointing to a struct for T and F, and somehow get_reference(&self) -> &T still works, even though it's really just getting the pointer to this CapsuleContents struct that holds the value and destructor.

Because the first field of the CapsuleContents struct is the T, through blind luck the compiler's put that at the front of the struct and so the cast works. This is UB because the default struct layout is not guaranteed. If you add #[repr(C)] to CapsuleContents (maybe with a comment why that repr is used), it's sound to rely on first field T always being at zero offset inside the struct.

https://doc.rust-lang.org/reference/type-layout.html#the-c-representation

Do you think having a macro of some sort for a destructor would be helpful for the user?

It's not clear to me what the use case for this would be. Also, macros are hard for users to figure out how to use without lots of good documentation, so without a strong motivation I'm unconvinced it's worth the maintenance cost.

[davidhewitt]

Thanks for the continued progress on this, I think it's getting there and will be a nice feature to add to PyO3!

[milesgranger]

Thank you for the thorough reviews here, a lot of new stuff here for me, so appreciate it. I know you're busy! 🙏

[davidhewitt]

👍 This is looking great to me! Just needs an Added CHANGELOG entry and then a couple of final suggestions. Also it might be nice to squash the commits, though I can also do that on merge.

Thanks for implementing this.

[davidhewitt]

👍 this is shaping up really well, just wanted to say thanks again for working on this. It's a tricky one with a few design gotchas which are taking a bit of effort to hash out, so the patience with repeated iterations is appreciated. I think with luck we're close now 🤞

[birkenfeld]

Only a few nits left :)

[davidhewitt]

@birkenfeld as I think you're very much on top of this PR, do you want to approve, squash-and-merge it once you're happy? Thanks for comprehensive reviewing, and thanks @milesgranger for all the iterations put in! 35 commits 😮

[milesgranger]

It's been a pleasure, learned a lot, and really appreciate the repeated and helpful reviews! 🙏

[birkenfeld]

@davidhewitt will do - should the module be renamed to capsule though (to be consistent with buffer which is also a CPython type, as opposed to pycell)?

@milesgranger I'll do this change if @davidhewitt agrees and then merge it, everything else LGTM!

[davidhewitt]

Ah yes good spot, it should probably even live in src/types/capsule.rs and be exposed as pyo3::types::PyCapsule ?

[birkenfeld]

That's probably even better. Not sure why buffer is so lonely at toplevel :D

[davidhewitt]

PyBuffer is this weird thing that isn't really a Python object, so it's kind of lost wandering the wasteland of murky design 😅

[birkenfeld]

Thanks again @milesgranger!