Extend lifetime of holder references

[adamreichold]

Closes #3138

[adamreichold]

@davidhewitt I wanted to push this here to ask your opinion of whether using local variables to extend the holder lifetimes is a suitable approach or whether we should do something else here entirely?

[adamreichold]

I could not produce a test case actually triggering the issue but changed it for completeness. I did put it into a separate commit though if it turns out that we do not need this after all.

[davidhewitt]

Ah, well found!

So this is definitely a viable solution which has the nice property that it allows us to bind local variables inside the generated macro code.

I think the other option is to avoid binding locals at all and just have one giant tail expression, because then the holder references will just be temporaries which will get extended until the end of the expression (by which point the return value has been converted into Python types). Something like this patch seems to fix the bug to me:

diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs
index 01aedf695a..b7f967472d 100644
--- a/pyo3-macros-backend/src/method.rs
+++ b/pyo3-macros-backend/src/method.rs
@@ -425,9 +425,8 @@ impl<'a> FnSpec<'a> {

         let rust_call = |args: Vec<TokenStream>| {
             quote! {
-                let mut ret = function(#self_arg #(#args),*);
-                let owned = _pyo3::impl_::pymethods::OkWrap::wrap(ret, #py);
-                owned.map(|obj| _pyo3::conversion::IntoPyPointer::into_ptr(obj))
+                _pyo3::impl_::pymethods::OkWrap::wrap(function(#self_arg #(#args),*), #py)
+                    .map(|obj| _pyo3::conversion::IntoPyPointer::into_ptr(obj))
                     .map_err(::core::convert::Into::into)
             }
         };

(This same trick might need applying in a few of the other code paths which you've corrected too.)

Overall I'm happy with either approach. Being able to bind local variables makes the generated macro code easier for us to read and debug. On the other hand, the patch I put above keeps the implementation in pyo3-macros-backend simpler. So it's just a case of whatever we like best. I slightly prefer the conciseness of the patch I put above, though I am also fine with the PR as proposed here.

[adamreichold]

So it's just a case of whatever we like best. I slightly prefer the conciseness of the patch I put above, though I am also fine with the PR as proposed here.

I prepared #3142 with the tail-expression-based solution which is certainly a much smaller change and results less generated code which makes it the obvious choice for me.

The only concern I am wrangling with myself is that it does seem like a "trick" to me and I would have a hard time inferring this effect by looking at the code. At least in this regard, I find the solution using local variables more pedestrian and more importantly explicit in that it is easier to understand why this is done.

I am not completely convinced the effect is real though as I have never been on friendly terms with PyO3's proc macro code and this might just be me having a hard time doing anything with it.

[davidhewitt]

Ok - let's go with #3142 for now.

I agree the macro implementations could use some refactoring, they've grown very... organically. 😅

What I'm hoping to move towards with that code is more structs which represent the parsed types, and then potentially more structs / functions to compose the generated code. E.g. this "ok-wrap and convert to PyErr" pattern can probably be its own function (or structure which implements quote::ToTokens, not sure what is more performant).