Update to pyo3 v0.22

[benbrandt]

Updates to the latest version of py03. They have had quite a few breaking changes, but I attempted to do the minimum amount of changes to get to the latest version

[benbrandt]

@dicej I don't think I fully grasped that the runtime crate was getting embedded. I'll take another closer look at what made the tests fail before you have to review to save you some time

[benbrandt]

@dicej I have gotten it close, but there are still a few failing tests around resources.
Overall, I think I am likely missing some context on how best to represent lifetimes for some of these pointers, but hopefully this is still a helpful starting place?

[benbrandt]

It seems like since this is embedded in the WIT world, we can tie this to the version of cpython we are using?

[benbrandt]

Another thought: If I understand the resolver correctly, this will likely actually be targeting abi for 3.9? Since pyo3 is shared across the crates and it goes to the lowest enabled feature?

[dicej]

Features are additive, so presumably both the abi3-py312 and abi3-py39 features will be enabled. Not sure what that means in practice.

[dicej]

Looks like the resource tests are failing due to reference count issues; finalizers are either being run too soon or not at all. Formerly, we relied on pyo3 v0.20's Py::into_ref, which kept the returned reference alive until the GIL was released, which worked perfectly for our case, but it looks like there's no equivalent in v0.22, so we'll either need to implement our own equivalent or come up with a different strategy. I've got to sign off for the day, but I'll take another look tomorrow.

[benbrandt]

Thanks for looking. If there isn't an immediately obvious solution, I'm also fine putting this on pause for a bit depending on your availability

[benbrandt]

Hmm rereading the migration docs (yet again) I think I missed a few things in there as well. I can take another look as well to try and make some of this closer to the previous implementation

[benbrandt]

@dicej I did another pass and changed all of these C apis to return a Py. On second thought, it felt really strange to be returning something with a lifetime created in the scope of this function over this boundary. Maybe it was technically ok, but this also seems to be closer to what the WASM side expects (a single pointer).

[dicej]

I'm a bit concerned that we've gone from a function that is safe and has no unsafe code, to a safe-to-call function with an unsafe block inside it, to an unsafe-to-call function whose entire body is unsafe.

Granted, the only caller currently is generated Wasm code, which doesn't have any notion of Rust's rules, but now we have to document the requirements to call these functions beyond what's indicated by the types. Speaking of which, what makes this function unsafe to call (versus being safe to call but containing unsafe code in the body)?

Also, more (and larger) unsafe code blocks greatly increase the maintenance burden, so I want to confirm that there's no way to avoid them when upgrading PyO3. In principle, this function has pretty simple requirements: it needs proof that the caller holds the GIL (as indicated by the &Python parameter) and it needs a valid reference to a Python value which lives at least as long as the call (the &PyAny parameter). With both of those things satisfied, it's not clear to me why we'd need unsafe code to examine the value. Was the old PyO3 API unsound (i.e. providing only the illusion of safety), or have we just not hit upon the correct way to use the new API while avoiding unsafe?

[dicej]

Here's another option that doesn't require any unsafe:

#[export_name = "componentize-py#ToCanonBool"]
pub extern "C" fn componentize_py_to_canon_bool(
    py: &Python,
    value: ManuallyDrop<Py<PyAny>>,
) -> u32 {
    if value.is_truthy(*py).unwrap() {
        1
    } else {
        0
    }
}

It's not terribly pretty, and I suspect there are better options, but it works. I'll need to study the docs more.

[dicej]

This might be better:

#[export_name = "componentize-py#ToCanonBool"]
pub extern "C" fn componentize_py_to_canon_bool(_py: &Python, value: Borrowed<PyAny>) -> u32 {
    if value.is_truthy().unwrap() {
        1
    } else {
        0
    }
}

I feel like there's a type missing in PyO3. We have Bound (owned, with proof of GIL), Borrowed (borrowed, with proof of GIL), and Py (owned, no proof of GIL), but there's no BorrowedPy (borrowed, no proof of GIL) -- that's the one we really want here.

[benbrandt]

@dicej fair points. I think the ultimate issue is that we don't currently have a good way to test the changes. And while pyo3 claims to fix some soundness issues, I am worried I've potentially just created a lot more.

I think I'll put a pause on this to give us both a break as there isn't currently a pressing need to upgrade. And I might try and come at this with fresh eyes later

[dicej]

SGTM. For what it's worth, I think that if we make appropriate use of the Py, Bound, and Borrowed types, we'll be able to avoid any new unsafe code and end up with something we have at least the same level of confidence in as we did with v0.20 (i.e. not perfect confidence -- there could still be subtle leaks lurking -- but good enough to move forward).