From 3c678e275c800b900e18b8840b9d24f0fe148f52 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jan 2025 10:26:22 +0000 Subject: [PATCH 1/5] Add failing test for lifetime problem. --- integration-tests/tests/cpprefs_test.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index a42d93186..b5ab3a920 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -107,3 +107,25 @@ 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 + struct Bob { + uint32_t a; + uint32_t b; + }; + const Bob& give_bob(const Bob& input_bob); + "}; + let rs = quote! { + let b = ffi::Bob { a: 3, b: 4 }; + assert_eq!(ffi::give_bob(&b).b, 4); + }; + run_cpprefs_test(cxx, hdr, rs, &["give_bob"], &["Bob"]); +} \ No newline at end of file From 4996f971b9b141f3560a1fdeda85cf0fbdc5b6fc Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jan 2025 11:13:41 +0000 Subject: [PATCH 2/5] Fix lifetimes in CppRef<'a, T> return types. Previously these types did contain a lifetime, but we didn't parameterize the function by a lifetime. --- .../src/conversion/codegen_rs/fun_codegen.rs | 2 ++ engine/src/conversion/codegen_rs/lifetime.rs | 26 ++++++++++++++++--- integration-tests/tests/cpprefs_test.rs | 9 ++++--- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 1b68e8c2a..c7b3f39ab 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -133,6 +133,7 @@ pub(super) fn gen_function( params, Cow::Borrowed(&ret_type), non_pod_types, + &ret_conversion, ); if analysis.rust_wrapper_needed { @@ -308,6 +309,7 @@ impl<'a> FnGenerator<'a> { wrapper_params, ret_type, self.non_pod_types, + self.ret_conversion, ); let cxxbridge_name = self.cxxbridge_name; diff --git a/engine/src/conversion/codegen_rs/lifetime.rs b/engine/src/conversion/codegen_rs/lifetime.rs index 556015854..355a8a2e4 100644 --- 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 return type pub(crate) fn add_explicit_lifetime_if_necessary<'r>( param_details: &[ArgumentAnalysis], mut params: Punctuated, ret_type: Cow<'r, ReturnType>, non_pod_types: &HashSet, + ret_conversion: &Option, ) -> ( Option, Punctuated, @@ -54,11 +57,22 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>( ) }); 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(¶ms, 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) + { return (None, params, ret_type); } let new_return_type = match ret_type.as_ref() { @@ -83,6 +97,12 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>( #rarrow #old_tyit + 'a }) } + Type::Ptr(_) if return_type_is_cppref => { + // The ptr will be converted to CppRef<'a, T> elsewhere, so we + // just need to return the return type as-is such that the + // next match statement adds <'a> to the function. + Some(ret_type.clone().into_owned()) + } _ => None, }, _ => None, diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index b5ab3a920..72bd4adf4 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -124,8 +124,11 @@ fn test_return_reference_cpprefs() { const Bob& give_bob(const Bob& input_bob); "}; let rs = quote! { - let b = ffi::Bob { a: 3, b: 4 }; - assert_eq!(ffi::give_bob(&b).b, 4); + 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"]); -} \ No newline at end of file +} From 356c442a34f01e3beb55a4af6e16d0a6dce90a29 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jan 2025 11:16:42 +0000 Subject: [PATCH 3/5] Allow logging from cpprefs_tests. --- integration-tests/tests/cpprefs_test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index 72bd4adf4..46349ed7b 100644 --- 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) From ee104e6eb17a13f5e97f17a54bd12490d3e2149d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jan 2025 12:18:32 +0000 Subject: [PATCH 4/5] Remove lifetime from impl blocks; add to fns. Previously we parameterized some impl blocks by <'a>. Instead, parameterize each function individually. --- engine/src/conversion/api.rs | 7 ---- .../codegen_cpp/function_wrapper_cpp.rs | 12 ------ .../src/conversion/codegen_rs/fun_codegen.rs | 37 +++---------------- engine/src/conversion/codegen_rs/lifetime.rs | 21 +++++++---- engine/src/conversion/codegen_rs/mod.rs | 21 ++--------- 5 files changed, 22 insertions(+), 76 deletions(-) diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index d5da41c52..f0d1ee6af 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -719,10 +719,3 @@ impl Api { 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 index 661a5b3a3..2a19f3965 100644 --- 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 index c7b3f39ab..7c7f486a6 100644 --- 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,7 +23,7 @@ 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::{ @@ -32,7 +31,7 @@ use crate::{ function_wrapper::TypeConversionPolicy, ArgumentAnalysis, FnAnalysis, FnKind, MethodKind, RustRenameStrategy, TraitMethodDetails, }, - api::{Pointerness, UnsafetyNeeded}, + api::UnsafetyNeeded, }, minisyn::minisynize_vec, types::{Namespace, QualifiedName}, @@ -91,13 +90,13 @@ pub(super) fn gen_function( analysis: FnAnalysis, cpp_call_name: String, non_pod_types: &HashSet, - config: &IncludeCppConfig, ) -> RsCodegenResult { if analysis.ignore_reason.is_err() || !analysis.externally_callable { return RsCodegenResult::default(); } let cxxbridge_name = analysis.cxxbridge_name; let rust_name = &analysis.rust_name; + eprintln!("GENERATING {rust_name}"); let ret_type = analysis.ret_type; let ret_conversion = analysis.ret_conversion; let param_details = analysis.param_details; @@ -125,7 +124,6 @@ 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( @@ -239,7 +237,6 @@ struct FnGenerator<'a> { always_unsafe_due_to_trait_definition: bool, doc_attrs: &'a Vec, non_pod_types: &'a HashSet, - reference_wrappers: bool, } impl<'a> FnGenerator<'a> { @@ -396,31 +393,7 @@ 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)* @@ -428,7 +401,7 @@ impl<'a> FnGenerator<'a> { #call_body } }), - ty, + ty: parse_quote! { # ty }, }) } @@ -471,7 +444,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 index 355a8a2e4..46683f202 100644 --- a/engine/src/conversion/codegen_rs/lifetime.rs +++ b/engine/src/conversion/codegen_rs/lifetime.rs @@ -32,7 +32,7 @@ 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 return type +/// 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, @@ -56,6 +56,13 @@ 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, @@ -71,7 +78,8 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>( if !(has_mutable_receiver || hits_1024_bug || returning_impl_with_a_reference_param - || return_type_is_cppref) + || return_type_is_cppref + || any_param_is_cppref) { return (None, params, ret_type); } @@ -97,18 +105,15 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>( #rarrow #old_tyit + 'a }) } - Type::Ptr(_) if return_type_is_cppref => { - // The ptr will be converted to CppRef<'a, T> elsewhere, so we - // just need to return the return type as-is such that the - // next match statement adds <'a> to the function. - Some(ret_type.clone().into_owned()) - } _ => None, }, _ => None, }; 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 index 98991c67a..9185f7fef 100644 --- 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, -} - /// 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, From 0df10dc1d7e51db2d74b4785d966ebcd610b7d0c Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jan 2025 12:29:16 +0000 Subject: [PATCH 5/5] Remove logging. --- engine/src/conversion/codegen_rs/fun_codegen.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 7c7f486a6..624301e8d 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -96,7 +96,6 @@ pub(super) fn gen_function( } let cxxbridge_name = analysis.cxxbridge_name; let rust_name = &analysis.rust_name; - eprintln!("GENERATING {rust_name}"); let ret_type = analysis.ret_type; let ret_conversion = analysis.ret_conversion; let param_details = analysis.param_details;