Merge pull request #1428 from google/lifetime-prob

adetaylor · web-flow · commit 0988d44d482c · 2025-01-21T12:53:14.000Z
Add lifetimes to functions returning `CppRef&lt;'a, T&gt;`
diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs
@@ -719,10 +719,3 @@ impl<T: AnalysisPhase> Api<T> {
         Ok(Box::new(std::iter::once(Api::Enum { name, item })))
     }
 }
-
-/// Whether a type is a pointer of some kind.
-pub(crate) enum Pointerness {
-    Not,
-    ConstPtr,
-    MutPtr,
-}
diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs
@@ -10,7 +10,6 @@ use syn::{Type, TypePtr};
 
 use crate::conversion::{
     analysis::fun::function_wrapper::{CppConversionType, TypeConversionPolicy},
-    api::Pointerness,
     ConvertErrorFromCpp,
 };
 
@@ -63,17 +62,6 @@ impl TypeConversionPolicy {
         cpp_name_map.type_to_cpp(self.cxxbridge_type())
     }
 
-    pub(crate) fn is_a_pointer(&self) -> Pointerness {
-        match self.cxxbridge_type() {
-            Type::Ptr(TypePtr {
-                mutability: Some(_),
-                ..
-            }) => Pointerness::MutPtr,
-            Type::Ptr(_) => Pointerness::ConstPtr,
-            _ => Pointerness::Not,
-        }
-    }
-
     fn unique_ptr_wrapped_type(
         &self,
         original_name_map: &CppNameMap,
diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs
@@ -6,7 +6,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use autocxx_parser::IncludeCppConfig;
 use indexmap::set::IndexSet as HashSet;
 use std::borrow::Cow;
 
@@ -24,15 +23,15 @@ use super::{
     function_wrapper_rs::RustParamConversion,
     maybe_unsafes_to_tokens,
     unqualify::{unqualify_params, unqualify_ret_type},
-    ImplBlockDetails, ImplBlockKey, MaybeUnsafeStmt, RsCodegenResult, TraitImplBlockDetails, Use,
+    ImplBlockDetails, MaybeUnsafeStmt, RsCodegenResult, TraitImplBlockDetails, Use,
 };
 use crate::{
     conversion::{
         analysis::fun::{
             function_wrapper::TypeConversionPolicy, ArgumentAnalysis, FnAnalysis, FnKind,
             MethodKind, RustRenameStrategy, TraitMethodDetails,
         },
-        api::{Pointerness, UnsafetyNeeded},
+        api::UnsafetyNeeded,
     },
     minisyn::minisynize_vec,
     types::{Namespace, QualifiedName},
@@ -91,7 +90,6 @@ pub(super) fn gen_function(
     analysis: FnAnalysis,
     cpp_call_name: String,
     non_pod_types: &HashSet<QualifiedName>,
-    config: &IncludeCppConfig,
 ) -> RsCodegenResult {
     if analysis.ignore_reason.is_err() || !analysis.externally_callable {
         return RsCodegenResult::default();
@@ -125,14 +123,14 @@ pub(super) fn gen_function(
         non_pod_types,
         ret_type: &ret_type,
         ret_conversion: &ret_conversion,
-        reference_wrappers: config.unsafe_policy.requires_cpprefs(),
     };
     // In rare occasions, we might need to give an explicit lifetime.
     let (lifetime_tokens, params, ret_type) = add_explicit_lifetime_if_necessary(
         &param_details,
         params,
         Cow::Borrowed(&ret_type),
         non_pod_types,
+        &ret_conversion,
     );
 
     if analysis.rust_wrapper_needed {
@@ -238,7 +236,6 @@ struct FnGenerator<'a> {
     always_unsafe_due_to_trait_definition: bool,
     doc_attrs: &'a Vec<Attribute>,
     non_pod_types: &'a HashSet<QualifiedName>,
-    reference_wrappers: bool,
 }
 
 impl<'a> FnGenerator<'a> {
@@ -308,6 +305,7 @@ impl<'a> FnGenerator<'a> {
             wrapper_params,
             ret_type,
             self.non_pod_types,
+            self.ret_conversion,
         );
 
         let cxxbridge_name = self.cxxbridge_name;
@@ -394,39 +392,15 @@ impl<'a> FnGenerator<'a> {
         let rust_name = make_ident(self.rust_name);
         let unsafety = self.unsafety.wrapper_token();
         let doc_attrs = self.doc_attrs;
-        let receiver_pointerness = self
-            .param_details
-            .iter()
-            .next()
-            .map(|pd| pd.conversion.is_a_pointer())
-            .unwrap_or(Pointerness::Not);
         let ty = impl_block_type_name.get_final_ident();
-        let ty = match receiver_pointerness {
-            Pointerness::MutPtr if self.reference_wrappers => ImplBlockKey {
-                ty: parse_quote! {
-                    #ty
-                },
-                lifetime: Some(parse_quote! { 'a }),
-            },
-            Pointerness::ConstPtr if self.reference_wrappers => ImplBlockKey {
-                ty: parse_quote! {
-                    #ty
-                },
-                lifetime: Some(parse_quote! { 'a }),
-            },
-            _ => ImplBlockKey {
-                ty: parse_quote! { # ty },
-                lifetime: None,
-            },
-        };
         Box::new(ImplBlockDetails {
             item: ImplItem::Fn(parse_quote! {
                 #(#doc_attrs)*
                 pub #unsafety fn #rust_name #lifetime_tokens ( #wrapper_params ) #ret_type {
                     #call_body
                 }
             }),
-            ty,
+            ty: parse_quote! { # ty },
         })
     }
 
@@ -469,7 +443,7 @@ impl<'a> FnGenerator<'a> {
         };
         Box::new(ImplBlockDetails {
             item: ImplItem::Fn(parse_quote! { #stuff }),
-            ty: ImplBlockKey { ty, lifetime: None },
+            ty,
         })
     }
 
diff --git a/engine/src/conversion/codegen_rs/lifetime.rs b/engine/src/conversion/codegen_rs/lifetime.rs
@@ -7,7 +7,8 @@
 // except according to those terms.
 use crate::{
     conversion::analysis::fun::{
-        function_wrapper::RustConversionType, ArgumentAnalysis, ReceiverMutability,
+        function_wrapper::{RustConversionType, TypeConversionPolicy},
+        ArgumentAnalysis, ReceiverMutability,
     },
     types::QualifiedName,
 };
@@ -22,7 +23,7 @@ use syn::{
 
 /// Function which can add explicit lifetime parameters to function signatures
 /// where necessary, based on analysis of parameters and return types.
-/// This is necessary in three cases:
+/// This is necessary in four cases:
 /// 1) where the parameter is a Pin<&mut T>
 ///    and the return type is some kind of reference - because lifetime elision
 ///    is not smart enough to see inside a Pin.
@@ -31,11 +32,13 @@ use syn::{
 ///    built-in type
 /// 3) Any parameter is any form of reference, and we're returning an `impl New`
 ///    3a) an 'impl ValueParam' counts as a reference.
+/// 4) If we're using CppRef<'a, T> as a param or return type
 pub(crate) fn add_explicit_lifetime_if_necessary<'r>(
     param_details: &[ArgumentAnalysis],
     mut params: Punctuated<FnArg, Comma>,
     ret_type: Cow<'r, ReturnType>,
     non_pod_types: &HashSet<QualifiedName>,
+    ret_conversion: &Option<TypeConversionPolicy>,
 ) -> (
     Option<TokenStream>,
     Punctuated<FnArg, Comma>,
@@ -53,12 +56,31 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>(
                 RustConversionType::FromValueParamToPtr
             )
     });
+
+    let any_param_is_cppref = param_details.iter().any(|pd| {
+        matches!(
+            pd.conversion.rust_conversion,
+            RustConversionType::FromReferenceWrapperToPointer
+        )
+    });
     let return_type_is_impl = return_type_is_impl(&ret_type);
+    let return_type_is_cppref = matches!(
+        ret_conversion,
+        Some(TypeConversionPolicy {
+            rust_conversion: RustConversionType::FromPointerToReferenceWrapper,
+            ..
+        })
+    );
     let non_pod_ref_param = reference_parameter_is_non_pod_reference(&params, non_pod_types);
     let ret_type_pod = return_type_is_pod_or_known_type_reference(&ret_type, non_pod_types);
     let returning_impl_with_a_reference_param = return_type_is_impl && any_param_is_reference;
     let hits_1024_bug = non_pod_ref_param && ret_type_pod;
-    if !(has_mutable_receiver || hits_1024_bug || returning_impl_with_a_reference_param) {
+    if !(has_mutable_receiver
+        || hits_1024_bug
+        || returning_impl_with_a_reference_param
+        || return_type_is_cppref
+        || any_param_is_cppref)
+    {
         return (None, params, ret_type);
     }
     let new_return_type = match ret_type.as_ref() {
@@ -89,6 +111,9 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>(
     };
 
     match new_return_type {
+        None if return_type_is_cppref || any_param_is_cppref => {
+            (Some(quote! { <'a> }), params, ret_type)
+        }
         None => (None, params, ret_type),
         Some(new_return_type) => {
             for FnArg::Typed(PatType { ty, .. }) | FnArg::Receiver(syn::Receiver { ty, .. }) in
diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs
@@ -23,8 +23,7 @@ use itertools::Itertools;
 use proc_macro2::{Span, TokenStream};
 use syn::{
     parse_quote, punctuated::Punctuated, token::Comma, Attribute, Expr, FnArg, ForeignItem,
-    ForeignItemFn, Ident, ImplItem, Item, ItemForeignMod, ItemMod, Lifetime, TraitItem, Type,
-    TypePath,
+    ForeignItemFn, Ident, ImplItem, Item, ItemForeignMod, ItemMod, TraitItem, Type, TypePath,
 };
 
 use crate::{
@@ -59,16 +58,10 @@ use super::{
 use super::{convert_error::ErrorContext, ConvertErrorFromCpp};
 use quote::quote;
 
-#[derive(Clone, Hash, PartialEq, Eq)]
-struct ImplBlockKey {
-    ty: Type,
-    lifetime: Option<Lifetime>,
-}
-
 /// An entry which needs to go into an `impl` block for a given type.
 struct ImplBlockDetails {
     item: ImplItem,
-    ty: ImplBlockKey,
+    ty: Type,
 }
 
 struct TraitImplBlockDetails {
@@ -412,10 +405,8 @@ impl<'a> RsCodeGenerator<'a> {
             }
         }
         for (ty, entries) in impl_entries_by_type.into_iter() {
-            let lt = ty.lifetime.map(|lt| quote! { < #lt > });
-            let ty = ty.ty;
             output_items.push(Item::Impl(parse_quote! {
-                impl #lt #ty {
+                impl #ty {
                     #(#entries)*
                 }
             }))
@@ -494,7 +485,6 @@ impl<'a> RsCodeGenerator<'a> {
                 analysis,
                 cpp_call_name,
                 non_pod_types,
-                self.config,
             ),
             Api::Const { const_item, .. } => RsCodegenResult {
                 bindgen_mod_items: vec![Item::Const(const_item.into())],
@@ -1095,10 +1085,7 @@ impl<'a> RsCodeGenerator<'a> {
                         fn #method(_uhoh: autocxx::BindingGenerationFailure) {
                         }
                     },
-                    ty: ImplBlockKey {
-                        ty: parse_quote! { #self_ty },
-                        lifetime: None,
-                    },
+                    ty: parse_quote! { #self_ty },
                 })),
                 None,
                 None,
diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs
@@ -12,6 +12,7 @@ use autocxx_integration_tests::{directives_from_lists, do_run_test};
 use indoc::indoc;
 use proc_macro2::TokenStream;
 use quote::quote;
+use test_log::test;
 
 const fn arbitrary_self_types_supported() -> bool {
     rustversion::cfg!(nightly)
@@ -107,3 +108,28 @@ fn test_method_call_const() {
         &[],
     )
 }
+
+#[test]
+fn test_return_reference_cpprefs() {
+    let cxx = indoc! {"
+        const Bob& give_bob(const Bob& input_bob) {
+            return input_bob;
+        }
+    "};
+    let hdr = indoc! {"
+        #include <cstdint>
+        struct Bob {
+            uint32_t a;
+            uint32_t b;
+        };
+        const Bob& give_bob(const Bob& input_bob);
+    "};
+    let rs = quote! {
+        let b = CppPin::new(ffi::Bob { a: 3, b: 4 });
+        let b_ref = b.as_cpp_ref();
+        let bob = ffi::give_bob(&b_ref);
+        let val = unsafe { bob.as_ref() };
+        assert_eq!(val.b, 4);
+    };
+    run_cpprefs_test(cxx, hdr, rs, &["give_bob"], &["Bob"]);
+}

Original file line number	Diff line number	Diff line change
`@@ -719,10 +719,3 @@ impl<T: AnalysisPhase> Api<T> {`
`719`	`719`	`Ok(Box::new(std::iter::once(Api::Enum { name, item })))`
`720`	`720`	`}`
`721`	`721`	`}`
`722`		`-`
`723`		`-/// Whether a type is a pointer of some kind.`
`724`		`-pub(crate) enum Pointerness {`
`725`		`- Not,`
`726`		`- ConstPtr,`
`727`		`- MutPtr,`
`728`		`-}`