From ca84e689d54b0fb5da30794f334be071291e2584 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sun, 24 Apr 2022 14:52:43 -0700 Subject: [PATCH 01/27] Add basic reference wrapper --- .github/workflows/ci.yml | 3 + Cargo.toml | 2 +- .../analysis/fun/function_wrapper.rs | 16 +++ engine/src/conversion/analysis/fun/mod.rs | 123 +++++++++++++++++- engine/src/conversion/api.rs | 7 + .../codegen_cpp/function_wrapper_cpp.rs | 16 +++ .../src/conversion/codegen_rs/fun_codegen.rs | 42 +++++- .../codegen_rs/function_wrapper_rs.rs | 43 ++++++ engine/src/conversion/codegen_rs/mod.rs | 108 ++++++++++++++- examples/reference-wrappers/Cargo.toml | 21 +++ examples/reference-wrappers/build.rs | 17 +++ examples/reference-wrappers/src/input.h | 35 +++++ examples/reference-wrappers/src/main.rs | 31 +++++ parser/src/config.rs | 10 +- parser/src/directives.rs | 4 +- src/lib.rs | 3 + src/reference_wrapper.rs | 109 ++++++++++++++++ 17 files changed, 569 insertions(+), 21 deletions(-) create mode 100644 examples/reference-wrappers/Cargo.toml create mode 100644 examples/reference-wrappers/build.rs create mode 100644 examples/reference-wrappers/src/input.h create mode 100644 examples/reference-wrappers/src/main.rs create mode 100644 src/reference_wrapper.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3eb54d1db..367b7299a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,6 +139,9 @@ jobs: - name: Build non-trivial-type-on-stack example working-directory: ./examples/non-trivial-type-on-stack run: cargo build + - name: Build reference-wrappers example + working-directory: ./examples/reference-wrappers + run: cargo build # We do not build the LLVM example because even 'apt-get install llvm-13-dev' # does not work to install the LLVM 13 headers. diff --git a/Cargo.toml b/Cargo.toml index a9e64a979..d3756e079 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ moveit = { version = "0.5", features = [ "cxx" ] } [workspace] members = ["parser", "engine", "gen/cmd", "gen/build", "macro", "demo", "tools/reduce", "tools/mdbook-preprocessor", "integration-tests"] -exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/chromium-fake-render-frame-host", "examples/pod", "examples/non-trivial-type-on-stack", "examples/llvm", "tools/stress-test"] +exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/chromium-fake-render-frame-host", "examples/pod", "examples/non-trivial-type-on-stack", "examples/llvm", "examples/reference-wrappers", "tools/stress-test"] #[patch.crates-io] #cxx = { path="../cxx" } diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index f2f6aaafe..56ef537d6 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -23,6 +23,8 @@ pub(crate) enum CppConversionType { /// Ignored in the sense that it isn't passed into the C++ function. IgnoredPlacementPtrParameter, FromReturnValueToPlacementPtr, + FromPointerToReference, + FromReferenceToPointer, } impl CppConversionType { @@ -36,6 +38,8 @@ impl CppConversionType { CppConversionType::FromValueToUniquePtr } CppConversionType::FromValueToUniquePtr => CppConversionType::FromUniquePtrToValue, + CppConversionType::FromPointerToReference => CppConversionType::FromReferenceToPointer, + CppConversionType::FromReferenceToPointer => CppConversionType::FromPointerToReference, _ => panic!("Did not expect to have to invert this conversion"), } } @@ -52,6 +56,8 @@ pub(crate) enum RustConversionType { FromValueParamToPtr, FromPlacementParamToNewReturn, FromRValueParamToPtr, + FromReferenceWrapperToPointer, + FromPointerToReferenceWrapper, } impl RustConversionType { @@ -90,6 +96,14 @@ impl TypeConversionPolicy { } } + pub(crate) fn return_reference_into_wrapper(ty: Type) -> Self { + TypeConversionPolicy { + unwrapped_type: ty, + cpp_conversion: CppConversionType::FromReferenceToPointer, + rust_conversion: RustConversionType::FromPointerToReferenceWrapper, + } + } + pub(crate) fn new_to_unique_ptr(ty: Type) -> Self { TypeConversionPolicy { unwrapped_type: ty, @@ -161,6 +175,8 @@ impl TypeConversionPolicy { RustConversionType::FromValueParamToPtr | RustConversionType::FromRValueParamToPtr | RustConversionType::FromPlacementParamToNewReturn + | RustConversionType::FromPointerToReferenceWrapper + | RustConversionType::FromReferenceWrapperToPointer ) } diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index b196840fa..0a115c96b 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -41,7 +41,7 @@ use proc_macro2::Span; use quote::quote; use syn::{ parse_quote, punctuated::Punctuated, token::Comma, FnArg, Ident, Pat, ReturnType, Type, - TypePtr, Visibility, + TypePath, TypePtr, TypeReference, Visibility, }; use crate::{ @@ -1213,6 +1213,21 @@ impl<'a> FnAnalyzer<'a> { let ret_type_conversion_needed = ret_type_conversion .as_ref() .map_or(false, |x| x.cpp_work_needed()); + + log::info!( + "DDN={}, PTCN={}, RTCN={}", + cxxbridge_name, + param_conversion_needed, + ret_type_conversion_needed + ); + log::info!( + "Param convs = {:?}", + param_details + .iter() + .map(|p| &p.conversion.cpp_conversion) + .cloned() + .collect_vec() + ); // See https://github.com/dtolnay/cxx/issues/878 for the reason for this next line. let effective_cpp_name = cpp_name.as_ref().unwrap_or(&rust_name); let cpp_name_incompatible_with_cxx = @@ -1635,7 +1650,18 @@ impl<'a> FnAnalyzer<'a> { is_placement_return_destination = construct_into_self; if treat_this_as_reference { pp.ident = Ident::new("self", pp.ident.span()); + // if is_placement_return_destination + // // || !matches!( + // // self.config.unsafe_policy, + // // UnsafePolicy::ReferencesWrappedAllFunctionsSafe + // // ) + // || matches!( + // force_rust_conversion, + // Some(RustConversionType::FromPlacementParamToNewReturn) + // ) + // { pointer_treatment = PointerTreatment::Reference; + // } } syn::Pat::Ident(pp) } @@ -1646,6 +1672,7 @@ impl<'a> FnAnalyzer<'a> { } _ => old_pat, }; + let is_placement_return_destination = is_placement_return_destination || matches!( force_rust_conversion, @@ -1657,6 +1684,8 @@ impl<'a> FnAnalyzer<'a> { is_move_constructor, force_rust_conversion, sophistication, + self_type.is_some(), + is_placement_return_destination, ); let new_ty = annotated_type.ty; pt.pat = Box::new(new_pat.clone()); @@ -1698,6 +1727,8 @@ impl<'a> FnAnalyzer<'a> { is_move_constructor: bool, force_rust_conversion: Option, sophistication: TypeConversionSophistication, + is_self: bool, + is_placement_return_destination: bool, ) -> TypeConversionPolicy { let is_subclass_holder = match &annotated_type.kind { type_converter::TypeKind::SubclassHolder(holder) => Some(holder), @@ -1707,6 +1738,9 @@ impl<'a> FnAnalyzer<'a> { annotated_type.kind, type_converter::TypeKind::RValueReference ); + let is_reference = + matches!(annotated_type.kind, type_converter::TypeKind::Reference) || is_self; + let rust_conversion_forced = force_rust_conversion.is_some(); let ty = &*annotated_type.ty; if let Some(holder_id) = is_subclass_holder { let subclass = SubclassName::from_holder_name(holder_id); @@ -1735,7 +1769,20 @@ impl<'a> FnAnalyzer<'a> { Type::Path(p) => { let ty = ty.clone(); let tn = QualifiedName::from_type_path(p); - if self.pod_safe_types.contains(&tn) { + if matches!( + self.config.unsafe_policy, + UnsafePolicy::ReferencesWrappedAllFunctionsSafe + ) && is_reference + && !rust_conversion_forced + // must be std::pin::Pin<&mut T> + { + let inner_ty = extract_type_from_pinned_mut_ref(p); + TypeConversionPolicy { + unwrapped_type: parse_quote! { *mut #inner_ty }, + cpp_conversion: CppConversionType::FromPointerToReference, + rust_conversion: RustConversionType::FromReferenceWrapperToPointer, + } + } else if self.pod_safe_types.contains(&tn) { if known_types().lacks_copy_constructor(&tn) { TypeConversionPolicy { unwrapped_type: ty, @@ -1784,6 +1831,18 @@ impl<'a> FnAnalyzer<'a> { cpp_conversion: CppConversionType::FromPtrToValue, rust_conversion: RustConversionType::FromRValueParamToPtr, } + } else if matches!( + self.config.unsafe_policy, + UnsafePolicy::ReferencesWrappedAllFunctionsSafe + ) && is_reference + && !rust_conversion_forced + && !is_placement_return_destination + { + TypeConversionPolicy { + unwrapped_type: ty.clone(), + cpp_conversion: CppConversionType::FromPointerToReference, + rust_conversion: RustConversionType::FromReferenceWrapperToPointer, + } } else { TypeConversionPolicy { unwrapped_type: ty.clone(), @@ -1792,6 +1851,30 @@ impl<'a> FnAnalyzer<'a> { } } } + // TODO remove this + Type::Reference(TypeReference { + elem, mutability, .. + }) if matches!( + self.config.unsafe_policy, + UnsafePolicy::ReferencesWrappedAllFunctionsSafe + ) && !rust_conversion_forced + && !is_placement_return_destination => + { + let unwrapped_type = if mutability.is_some() { + parse_quote! { + *mut #elem + } + } else { + parse_quote! { + *const #elem + } + }; + TypeConversionPolicy { + unwrapped_type, + cpp_conversion: CppConversionType::FromPointerToReference, + rust_conversion: RustConversionType::FromReferenceWrapperToPointer, + } + } _ => { let rust_conversion = force_rust_conversion.unwrap_or(RustConversionType::None); TypeConversionPolicy { @@ -1874,8 +1957,19 @@ impl<'a> FnAnalyzer<'a> { } } _ => { - let was_reference = matches!(boxed_type.as_ref(), Type::Reference(_)); - let conversion = Some(TypeConversionPolicy::new_unconverted(ty.clone())); + let was_reference = references.ref_return; + let conversion = Some( + if was_reference + && matches!( + self.config.unsafe_policy, + UnsafePolicy::ReferencesWrappedAllFunctionsSafe + ) + { + TypeConversionPolicy::return_reference_into_wrapper(ty.clone()) + } else { + TypeConversionPolicy::new_unconverted(ty.clone()) + }, + ); ReturnTypeAnalysis { rt: ReturnType::Type(*rarrow, boxed_type), conversion, @@ -2142,3 +2236,24 @@ impl Api { } } } + +fn extract_type_from_pinned_mut_ref(ty: &TypePath) -> Type { + match ty + .path + .segments + .last() + .expect("was not std::pin::Pin") + .arguments + { + syn::PathArguments::AngleBracketed(ref ab) => { + match ab.args.first().expect("did not have angle bracketed args") { + syn::GenericArgument::Type(ref ty) => match ty { + Type::Reference(ref tyr) => tyr.elem.as_ref().clone(), + _ => panic!("pin did not contain a reference"), + }, + _ => panic!("argument was not a type"), + } + } + _ => panic!("did not find angle bracketed args"), + } +} diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index 24b6bb7f0..c5a1b607f 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -712,3 +712,10 @@ 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 b2e1ea384..034099c6a 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -6,8 +6,11 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use syn::{Type, TypePtr}; + use crate::conversion::{ analysis::fun::function_wrapper::{CppConversionType, TypeConversionPolicy}, + api::Pointerness, ConvertError, }; @@ -38,6 +41,17 @@ impl TypeConversionPolicy { type_to_cpp(&self.unwrapped_type, cpp_name_map) } + pub(crate) fn is_a_pointer(&self) -> Pointerness { + match &self.unwrapped_type { + Type::Ptr(TypePtr { + mutability: Some(_), + .. + }) => Pointerness::MutPtr, + Type::Ptr(_) => Pointerness::ConstPtr, + _ => Pointerness::Not, + } + } + fn unique_ptr_wrapped_type( &self, original_name_map: &CppNameMap, @@ -60,6 +74,7 @@ impl TypeConversionPolicy { CppConversionType::None | CppConversionType::FromReturnValueToPlacementPtr => { Some(var_name.to_string()) } + CppConversionType::FromPointerToReference => Some(format!("(*{})", var_name)), CppConversionType::Move => Some(format!("std::move({})", var_name)), CppConversionType::FromUniquePtrToValue | CppConversionType::FromPtrToMove => { Some(format!("std::move(*{})", var_name)) @@ -78,6 +93,7 @@ impl TypeConversionPolicy { }) } CppConversionType::IgnoredPlacementPtrParameter => None, + CppConversionType::FromReferenceToPointer => Some(format!("&{}", var_name)), }) } } diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 7c32b1bca..5a709d49c 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -23,7 +23,7 @@ use super::{ function_wrapper_rs::RustParamConversion, maybe_unsafes_to_tokens, unqualify::{unqualify_params, unqualify_ret_type}, - ImplBlockDetails, MaybeUnsafeStmt, RsCodegenResult, TraitImplBlockDetails, Use, + ImplBlockDetails, ImplBlockKey, MaybeUnsafeStmt, RsCodegenResult, TraitImplBlockDetails, Use, }; use crate::{ conversion::{ @@ -31,7 +31,7 @@ use crate::{ ArgumentAnalysis, FnAnalysis, FnKind, MethodKind, RustRenameStrategy, TraitMethodDetails, }, - api::UnsafetyNeeded, + api::{Pointerness, UnsafetyNeeded}, }, types::{Namespace, QualifiedName}, }; @@ -341,6 +341,31 @@ 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 => ImplBlockKey { + ty: parse_quote! { + CppMutRef< 'a, #ty> + }, + lifetime: Some(parse_quote! { 'a }), + }, + Pointerness::ConstPtr => ImplBlockKey { + ty: parse_quote! { + CppRef< 'a, #ty> + }, + lifetime: Some(parse_quote! { 'a }), + }, + Pointerness::Not => ImplBlockKey { + ty: parse_quote! { # ty }, + lifetime: None, + }, + }; Box::new(ImplBlockDetails { item: ImplItem::Method(parse_quote! { #(#doc_attrs)* @@ -348,7 +373,7 @@ impl<'a> FnGenerator<'a> { #call_body } }), - ty: impl_block_type_name.get_final_ident(), + ty, }) } @@ -385,14 +410,17 @@ impl<'a> FnGenerator<'a> { let rust_name = make_ident(&self.rust_name); let doc_attrs = self.doc_attrs; let unsafety = self.unsafety.wrapper_token(); - Box::new(ImplBlockDetails { - item: ImplItem::Method(parse_quote! { + let ty = impl_block_type_name.get_final_ident(); + let ty = parse_quote! { #ty }; + let stuff = quote! { #(#doc_attrs)* pub #unsafety fn #rust_name #lifetime_tokens ( #wrapper_params ) #ret_type { #call_body } - }), - ty: impl_block_type_name.get_final_ident(), + }; + Box::new(ImplBlockDetails { + item: ImplItem::Method(parse_quote! { #stuff }), + ty: ImplBlockKey { ty, lifetime: None }, }) } diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index a3fc71ff7..4c47e749c 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -167,6 +167,49 @@ impl TypeConversionPolicy { }; RustParamConversion::ReturnValue { ty } } + RustConversionType::FromPointerToReferenceWrapper => { + let (ty, is_mut) = match &self.unwrapped_type { + Type::Ptr(TypePtr { + elem, mutability, .. + }) => (&*elem, mutability.is_some()), + _ => panic!("Not a ptr"), + }; + let (ty, wrapper_name) = if is_mut { + (parse_quote! { &mut CppMutRef<'a, #ty> }, "CppMutRef") + } else { + (parse_quote! { &CppRef<'a, #ty> }, "CppRef") + }; + let wrapper_name = make_ident(wrapper_name); + RustParamConversion::Param { + ty, + local_variables: Vec::new(), + conversion: quote! { + #wrapper_name (#var) + }, + conversion_requires_unsafe: false, + } + } + RustConversionType::FromReferenceWrapperToPointer => { + let (ty, is_mut) = match &self.unwrapped_type { + Type::Ptr(TypePtr { + elem, mutability, .. + }) => (&*elem, mutability.is_some()), + _ => panic!("Not a ptr"), + }; + let ty = if is_mut { + parse_quote! { &mut CppMutRef<'a, #ty> } + } else { + parse_quote! { &CppRef<'a, #ty> } + }; + RustParamConversion::Param { + ty, + local_variables: Vec::new(), + conversion: quote! { + #var .0 + }, + conversion_requires_unsafe: false, + } + } } } } diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index b41aca966..885d6c7e3 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -23,7 +23,8 @@ 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, TraitItem, TypePath, + ForeignItemFn, Ident, ImplItem, Item, ItemForeignMod, ItemMod, Lifetime, TraitItem, Type, + TypePath, }; use crate::{ @@ -61,10 +62,16 @@ use super::{ use super::{convert_error::ErrorContext, ConvertError}; 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: Ident, + ty: ImplBlockKey, } struct TraitImplBlockDetails { @@ -126,6 +133,92 @@ fn get_string_items() -> Vec { } } }), + Item::Struct(parse_quote! { + #[repr(transparent)] + pub struct CppRef<'a, T>(pub *const T, pub ::std::marker::PhantomData<&'a T>); + }), + Item::Impl(parse_quote! { + impl<'a, T> autocxx::CppRef<'a, T> for CppRef<'a, T> { + fn as_ptr(&self) -> *const T { + self.0 + } + } + }), + Item::Impl(parse_quote! { + impl<'a, T: ::cxx::private::UniquePtrTarget> CppRef<'a, T> { + /// Create a `CppRef` to an item within a pinned box. + pub fn from_box(boxed_item: &'a ::std::pin::Pin<::std::boxed::Box>) -> Self { + Self(unsafe { ::std::pin::Pin::into_inner_unchecked(boxed_item.as_ref()) as *const T }, ::std::marker::PhantomData) + } + } + }), + Item::Struct(parse_quote! { + #[repr(transparent)] + pub struct CppMutRef<'a, T>(pub *mut T, pub ::std::marker::PhantomData<&'a T>); + }), + Item::Impl(parse_quote! { + impl<'a, T> autocxx::CppRef<'a, T> for CppMutRef<'a, T> { + fn as_ptr(&self) -> *const T { + self.0 + } + } + }), + Item::Impl(parse_quote! { + impl<'a, T> autocxx::CppMutRef<'a, T> for CppMutRef<'a, T> { + fn as_mut_ptr(&self) -> *mut T { + self.0 + } + } + }), + Item::Impl(parse_quote! { + impl<'a, T: ::cxx::private::UniquePtrTarget> CppMutRef<'a, T> { + /// Create a `CppMutRef` to an item within a pinned box. + pub fn from_box(boxed_item: &'a mut ::std::pin::Pin<::std::boxed::Box>) -> Self { + Self(unsafe { ::std::pin::Pin::into_inner_unchecked(boxed_item.as_mut()) as *mut T }, ::std::marker::PhantomData) + } + /// Create a const C++ reference from this mutable C++ reference. + pub fn as_cpp_ref(&self) -> CppRef<'a, T> { + use autocxx::CppRef; + CppRef(self.as_ptr(), ::std::marker::PhantomData) + } + } + }), + Item::Struct(parse_quote! { + /// "Pins" an object so that C++-compatible references can be created. + #[repr(transparent)] + pub struct CppPin(T); + }), + Item::Impl(parse_quote! { + impl<'a, T: 'a> autocxx::CppPin<'a, T> for CppPin + { + type CppRef = CppRef<'a, T>; + type CppMutRef = CppMutRef<'a, T>; + fn as_ptr(&self) -> *const T { + ::std::ptr::addr_of!(self.0) + } + fn as_mut_ptr(&mut self) -> *mut T { + ::std::ptr::addr_of_mut!(self.0) + } + fn as_cpp_ref(&self) -> Self::CppRef { + CppRef(self.as_ptr(), ::std::marker::PhantomData) + } + fn as_cpp_mut_ref(&mut self) -> Self::CppMutRef { + CppMutRef(self.as_mut_ptr(), ::std::marker::PhantomData) + } + } + }), + Item::Impl(parse_quote! { + impl CppPin { + /// Make this object available to C++ by reference. + /// This takes ownership of the object, thus proving you + /// have no existing Rust references. Any reference to this + /// object from Rust after this point is unsafe, since Rust + /// and C++ reference semantics are incompatible. + pub fn new(item: T) -> CppPin { + Self(item) + } + } + }), ] .to_vec() } @@ -374,7 +467,7 @@ impl<'a> RsCodeGenerator<'a> { #[allow(unused_imports)] use self:: #(#supers)::* - ::ToCppString; + ::{ToCppString, CppRef, CppMutRef}; })); } let supers = super_duper.take(ns.depth() + 1); @@ -410,8 +503,10 @@ 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 #ty { + impl #lt #ty { #(#entries)* } })) @@ -1075,7 +1170,10 @@ impl<'a> RsCodeGenerator<'a> { fn #method(_uhoh: autocxx::BindingGenerationFailure) { } }, - ty: self_ty, + ty: ImplBlockKey { + ty: parse_quote! { #self_ty }, + lifetime: None, + }, })), None, None, diff --git a/examples/reference-wrappers/Cargo.toml b/examples/reference-wrappers/Cargo.toml new file mode 100644 index 000000000..c02447caa --- /dev/null +++ b/examples/reference-wrappers/Cargo.toml @@ -0,0 +1,21 @@ +# Copyright 2020 Google LLC +# +# Licensed under the Apache License, Version 2.0 or the MIT license +# , at your +# option. This file may not be copied, modified, or distributed +# except according to those terms. + +[package] +name = "autocxx-reference-wrapper-example" +version = "0.22.1" +authors = ["Adrian Taylor "] +edition = "2021" + +[dependencies] +cxx = "1.0.68" +autocxx = { path = "../..", version="0.22.1" } + +[build-dependencies] +autocxx-build = { path = "../../gen/build", version="0.22.1" } +miette = { version="4.3", features=["fancy"]} diff --git a/examples/reference-wrappers/build.rs b/examples/reference-wrappers/build.rs new file mode 100644 index 000000000..fd8da795f --- /dev/null +++ b/examples/reference-wrappers/build.rs @@ -0,0 +1,17 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() -> miette::Result<()> { + let path = std::path::PathBuf::from("src"); + let mut b = autocxx_build::Builder::new("src/main.rs", &[&path]).build()?; + b.flag_if_supported("-std=c++14").compile("autocxx-reference-wrapper-example"); + + println!("cargo:rerun-if-changed=src/main.rs"); + println!("cargo:rerun-if-changed=src/input.h"); + Ok(()) +} diff --git a/examples/reference-wrappers/src/input.h b/examples/reference-wrappers/src/input.h new file mode 100644 index 000000000..b6cdd874e --- /dev/null +++ b/examples/reference-wrappers/src/input.h @@ -0,0 +1,35 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#pragma once + +#include +#include +#include +#include + +class Goat { +public: + Goat() : horns(0) {} + void add_a_horn(); + std::string describe() const; +private: + uint32_t horns; +}; + +inline uint32_t DoMath(uint32_t a) { + return a * 3; +} + +inline void Goat::add_a_horn() { horns++; } +inline std::string Goat::describe() const { + std::ostringstream oss; + std::string plural = horns == 1 ? "" : "s"; + oss << "This goat has " << horns << " horn" << plural << "."; + return oss.str(); +} diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs new file mode 100644 index 000000000..00a6bec47 --- /dev/null +++ b/examples/reference-wrappers/src/main.rs @@ -0,0 +1,31 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use autocxx::prelude::*; +include_cpp! { + #include "input.h" + safety!(unsafe_references_wrapped) + generate!("DoMath") + generate!("Goat") +} + +fn main() { + println!("Hello, world! - C++ math should say 12={}", ffi::DoMath(4)); + let mut goat = ffi::Goat::new().within_box(); + let mut goat = ffi::CppMutRef::from_box(&mut goat); + goat.add_a_horn(); + goat.add_a_horn(); + assert_eq!( + goat.as_cpp_ref() + .describe() + .as_ref() + .unwrap() + .to_string_lossy(), + "This goat has 2 horns." + ); +} diff --git a/parser/src/config.rs b/parser/src/config.rs index 4204acf33..1a907717e 100644 --- a/parser/src/config.rs +++ b/parser/src/config.rs @@ -33,6 +33,7 @@ use quote::quote; pub enum UnsafePolicy { AllFunctionsSafe, AllFunctionsUnsafe, + ReferencesWrappedAllFunctionsSafe, } impl Default for UnsafePolicy { @@ -50,8 +51,13 @@ impl Parse for UnsafePolicy { Some(id) => { if id == "unsafe_ffi" { Ok(UnsafePolicy::AllFunctionsSafe) + } else if id == "unsafe_references_wrapped" { + Ok(UnsafePolicy::ReferencesWrappedAllFunctionsSafe) } else { - Err(syn::Error::new(id.span(), "expected unsafe_ffi")) + Err(syn::Error::new( + id.span(), + "expected unsafe_ffi or unsafe_references_wrapped", + )) } } None => Ok(UnsafePolicy::AllFunctionsUnsafe), @@ -70,6 +76,8 @@ impl ToTokens for UnsafePolicy { fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) { if *self == UnsafePolicy::AllFunctionsSafe { tokens.extend(quote! { unsafe }) + } else if *self == UnsafePolicy::ReferencesWrappedAllFunctionsSafe { + tokens.extend(quote! { unsafe_references_wrapped }) } } } diff --git a/parser/src/directives.rs b/parser/src/directives.rs index 160829ec2..70c88fc7a 100644 --- a/parser/src/directives.rs +++ b/parser/src/directives.rs @@ -268,10 +268,8 @@ impl Directive for Safety { ) -> Box + 'a> { let policy = &config.unsafe_policy; match config.unsafe_policy { - crate::UnsafePolicy::AllFunctionsSafe => { - Box::new(std::iter::once(policy.to_token_stream())) - } crate::UnsafePolicy::AllFunctionsUnsafe => Box::new(std::iter::empty()), + _ => Box::new(std::iter::once(policy.to_token_stream())), } } } diff --git a/src/lib.rs b/src/lib.rs index 659201129..37c0f7de4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,10 +14,13 @@ // do anything - all the magic is handled entirely by // autocxx_macro::include_cpp_impl. +mod reference_wrapper; mod rvalue_param; pub mod subclass; mod value_param; +pub use reference_wrapper::{CppMutRef, CppPin, CppRef}; + #[cfg_attr(doc, aquamarine::aquamarine)] /// Include some C++ headers in your Rust project. /// diff --git a/src/reference_wrapper.rs b/src/reference_wrapper.rs new file mode 100644 index 000000000..fe05a87f2 --- /dev/null +++ b/src/reference_wrapper.rs @@ -0,0 +1,109 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +/// A C++ const reference. These are different from Rust's `&T` in that +/// these may exist even while the object is nutated elsewhere. +/// +/// This is a trait not a struct due to the nuances of Rust's orphan rule +/// - implemntations of this trait are found in each set of generated bindings +/// but they are essentially the same. +pub trait CppRef<'a, T> { + /// Retrieve the underlying C++ pointer. + fn as_ptr(&self) -> *const T; + + /// Get a regular Rust reference out of this C++ reference. + /// + /// # Safety + /// + /// Callers must guarantee that the referent is not modified by any other + /// C++ or Rust code while the returned reference exists. Callers must + /// also guarantee that no mutable Rust reference is created to the + /// referent while the returned reference exists. + unsafe fn as_ref(&self) -> &T { + &*self.as_ptr() + } +} + +/// A C++ non-const reference. These are different from Rust's `&mut T` in that +/// several C++ references can exist to the same underlying data ("aliasing") +/// and that's not permitted in Rust. +/// +/// This is a trait not a struct due to the nuances of Rust's orphan rule +/// - implemntations of this trait are found in each set of generated bindings +/// but they are essentially the same. +pub trait CppMutRef<'a, T>: CppRef<'a, T> { + /// Retrieve the underlying C++ pointer. + fn as_mut_ptr(&self) -> *mut T; + + /// Get a regular Rust mutable reference out of this C++ reference. + /// + /// # Safety + /// + /// Callers must guarantee that the referent is not modified by any other + /// C++ or Rust code while the returned reference exists. Callers must + /// also guarantee that no other Rust reference is created to the referent + /// while the returned reference exists. + unsafe fn as_mut(&mut self) -> &mut T { + &mut *self.as_mut_ptr() + } +} + +/// Any newtype wrapper which causes the contained object to obey C++ reference +/// semantics rather than Rust reference semantics. +/// +/// The complex generics here are working around the orphan rule - the only +/// important generic is `T` which is the underlying stored type. +/// +/// C++ references are permitted to alias one another, and commonly do. +/// Rust references must alias according only to the narrow rules of the +/// borrow checker. +/// +/// If you need C++ to access your Rust object, first imprison it in one of these +/// objects, then use [`Self::as_cpp_ref`] to obtain C++ references to it. +pub trait CppPin<'a, T: 'a> { + /// The type of C++ reference created to the contained object. + type CppRef: CppRef<'a, T>; + + /// The type of C++ mutable reference created to the contained object.. + type CppMutRef: CppMutRef<'a, T>; + + /// Get an immutable pointer to the underlying object. + fn as_ptr(&self) -> *const T; + + /// Get a mutable pointer to the underlying object. + fn as_mut_ptr(&mut self) -> *mut T; + + /// Returns a reference which obeys C++ reference semantics + fn as_cpp_ref(&self) -> Self::CppRef; + + /// Returns a mutable reference which obeys C++ reference semantics. + /// + /// Note that this requires unique ownership of `self`, but this is + /// advisory since the resulting reference can be cloned. + fn as_cpp_mut_ref(&mut self) -> Self::CppMutRef; + + /// Get a normal Rust reference to the underlying object. This is unsafe. + /// + /// # Safety + /// + /// You must guarantee that C++ will not mutate the object while the + /// reference exists. + unsafe fn as_ref(&self) -> &T { + &*self.as_ptr() + } + + /// Get a normal Rust mutable reference to the underlying object. This is unsafe. + /// + /// # Safety + /// + /// You must guarantee that C++ will not mutate the object while the + /// reference exists. + unsafe fn as_mut(&mut self) -> &mut T { + &mut *self.as_mut_ptr() + } +} From b2ca2eec08fd7d2694be101107bb544d43dc23f3 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 17 Jun 2022 12:46:02 -0700 Subject: [PATCH 02/27] Remove some nonsense --- engine/src/conversion/analysis/fun/mod.rs | 25 ----------------------- 1 file changed, 25 deletions(-) diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 0a115c96b..e1faf9799 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -1214,20 +1214,6 @@ impl<'a> FnAnalyzer<'a> { .as_ref() .map_or(false, |x| x.cpp_work_needed()); - log::info!( - "DDN={}, PTCN={}, RTCN={}", - cxxbridge_name, - param_conversion_needed, - ret_type_conversion_needed - ); - log::info!( - "Param convs = {:?}", - param_details - .iter() - .map(|p| &p.conversion.cpp_conversion) - .cloned() - .collect_vec() - ); // See https://github.com/dtolnay/cxx/issues/878 for the reason for this next line. let effective_cpp_name = cpp_name.as_ref().unwrap_or(&rust_name); let cpp_name_incompatible_with_cxx = @@ -1650,18 +1636,7 @@ impl<'a> FnAnalyzer<'a> { is_placement_return_destination = construct_into_self; if treat_this_as_reference { pp.ident = Ident::new("self", pp.ident.span()); - // if is_placement_return_destination - // // || !matches!( - // // self.config.unsafe_policy, - // // UnsafePolicy::ReferencesWrappedAllFunctionsSafe - // // ) - // || matches!( - // force_rust_conversion, - // Some(RustConversionType::FromPlacementParamToNewReturn) - // ) - // { pointer_treatment = PointerTreatment::Reference; - // } } syn::Pat::Ident(pp) } From 38a4b7874a4216991db413da6a26aa3d91a1de3d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 17 Jun 2022 13:09:41 -0700 Subject: [PATCH 03/27] Import items only if CppRefs in use --- engine/src/conversion/codegen_rs/mod.rs | 30 ++++++++++++++++--------- parser/src/config.rs | 8 +++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 885d6c7e3..e59ca7ecc 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -133,6 +133,12 @@ fn get_string_items() -> Vec { } } }), + ] + .to_vec() +} + +fn get_cppref_items() -> Vec { + [ Item::Struct(parse_quote! { #[repr(transparent)] pub struct CppRef<'a, T>(pub *const T, pub ::std::marker::PhantomData<&'a T>); @@ -315,6 +321,9 @@ impl<'a> RsCodeGenerator<'a> { let mut extern_rust_mod_items = extern_rust_mod_items.into_iter().flatten().collect(); // And a list of global items to include at the top level. let mut all_items: Vec = all_items.into_iter().flatten().collect(); + if self.config.unsafe_policy.requires_cpprefs() { + all_items.append(&mut get_cppref_items()) + } // And finally any C++ we need to generate. And by "we" I mean autocxx not cxx. let has_additional_cpp_needs = additional_cpp_needs.into_iter().any(std::convert::identity); extern_c_mod_items.extend(self.build_include_foreign_items(has_additional_cpp_needs)); @@ -453,23 +462,24 @@ impl<'a> RsCodeGenerator<'a> { } fn append_uses_for_ns(&mut self, items: &mut Vec, ns: &Namespace) { + let mut imports_from_super = vec!["cxxbridge"]; + if !self.config.exclude_utilities() { + imports_from_super.push("ToCppString"); + } + if self.config.unsafe_policy.requires_cpprefs() { + imports_from_super.extend(["CppRef", "CppMutRef"]); + } + let imports_from_super = imports_from_super.into_iter().map(make_ident); let super_duper = std::iter::repeat(make_ident("super")); // I'll get my coat let supers = super_duper.clone().take(ns.depth() + 2); items.push(Item::Use(parse_quote! { #[allow(unused_imports)] use self:: #(#supers)::* - ::cxxbridge; + ::{ + #(#imports_from_super),* + }; })); - if !self.config.exclude_utilities() { - let supers = super_duper.clone().take(ns.depth() + 2); - items.push(Item::Use(parse_quote! { - #[allow(unused_imports)] - use self:: - #(#supers)::* - ::{ToCppString, CppRef, CppMutRef}; - })); - } let supers = super_duper.take(ns.depth() + 1); items.push(Item::Use(parse_quote! { #[allow(unused_imports)] diff --git a/parser/src/config.rs b/parser/src/config.rs index 1a907717e..8d302493e 100644 --- a/parser/src/config.rs +++ b/parser/src/config.rs @@ -82,6 +82,14 @@ impl ToTokens for UnsafePolicy { } } +impl UnsafePolicy { + /// Whether we are treating C++ references as a different thing from Rust + /// references and therefore have to generate lots of code for a CppRef type + pub fn requires_cpprefs(&self) -> bool { + matches!(self, Self::ReferencesWrappedAllFunctionsSafe) + } +} + /// An entry in the allowlist. #[derive(Hash, Debug)] pub enum AllowlistEntry { From 60f48ae18c9ae864b3dbe2024f3aca87d24325c8 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 17 Jun 2022 13:27:58 -0700 Subject: [PATCH 04/27] Start to add tests for CppRefs stuff --- integration-tests/src/lib.rs | 10 ++- integration-tests/tests/cpprefs_test.rs | 70 +++++++++++++++++++++ integration-tests/tests/integration_test.rs | 1 + integration-tests/tests/lib.rs | 1 + 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 integration-tests/tests/cpprefs_test.rs diff --git a/integration-tests/src/lib.rs b/integration-tests/src/lib.rs index 17dbbf626..352c8ab5b 100644 --- a/integration-tests/src/lib.rs +++ b/integration-tests/src/lib.rs @@ -21,7 +21,7 @@ use autocxx_engine::{ use log::info; use once_cell::sync::OnceCell; use proc_macro2::{Span, TokenStream}; -use quote::{quote, TokenStreamExt}; +use quote::{format_ident, quote, TokenStreamExt}; use syn::Token; use tempfile::{tempdir, TempDir}; @@ -205,6 +205,7 @@ pub fn run_test( None, None, None, + "unsafe_ffi", ) .unwrap() } @@ -259,6 +260,7 @@ pub fn run_test_ex( builder_modifier, code_checker, extra_rust, + "unsafe_ffi", ) .unwrap() } @@ -290,6 +292,7 @@ pub fn run_test_expect_fail( None, None, None, + "unsafe_ffi", ) .expect_err("Unexpected success"); } @@ -311,6 +314,7 @@ pub fn run_test_expect_fail_ex( builder_modifier, code_checker, extra_rust, + "unsafe_ffi", ) .expect_err("Unexpected success"); } @@ -360,14 +364,16 @@ pub fn do_run_test( builder_modifier: Option, rust_code_checker: Option, extra_rust: Option, + safety_policy: &str, ) -> Result<(), TestError> { let hexathorpe = Token![#](Span::call_site()); + let safety_policy = format_ident!("{}", safety_policy); let unexpanded_rust = quote! { use autocxx::prelude::*; include_cpp!( #hexathorpe include "input.h" - safety!(unsafe_ffi) + safety!(#safety_policy) #directives ); diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs new file mode 100644 index 000000000..ad1b79b0a --- /dev/null +++ b/integration-tests/tests/cpprefs_test.rs @@ -0,0 +1,70 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +//! Tests specific to reference wrappers. + +use autocxx_integration_tests::{directives_from_lists, do_run_test}; +use indoc::indoc; +use proc_macro2::TokenStream; +use quote::quote; + +/// A positive test, we expect to pass. +fn run_cpprefs_test( + cxx_code: &str, + header_code: &str, + rust_code: TokenStream, + generate: &[&str], + generate_pods: &[&str], +) { + do_run_test( + cxx_code, + header_code, + rust_code, + directives_from_lists(generate, generate_pods, None), + None, + None, + None, + "unsafe_references_wrapped", + ) + .unwrap() +} + +#[test] +fn test_method_call_const() { + run_cpprefs_test( + "", + indoc! {" + #include + #include + + class Goat { + public: + Goat() : horns(0) {} + void add_a_horn(); + std::string describe() const; + private: + uint32_t horns; + }; + + inline void Goat::add_a_horn() { horns++; } + inline std::string Goat::describe() const { + std::ostringstream oss; + std::string plural = horns == 1 ? \"\" : \"s\"; + oss << \"This goat has \" << horns << \" horn\" << plural << \".\"; + return oss.str(); + } + "}, + quote! { + let mut goat = ffi::Goat::new().within_box(); + let mut goat = ffi::CppMutRef::from_box(&mut goat); + goat.add_a_horn(); + }, + &["Goat"], + &[], + ) +} diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 82144e1a6..00a5203bf 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -5920,6 +5920,7 @@ fn test_double_destruction() { None, None, None, + "unsafe_ffi", ) { Err(TestError::CppBuild(_)) => {} // be sure this fails due to a static_assert // rather than some runtime problem diff --git a/integration-tests/tests/lib.rs b/integration-tests/tests/lib.rs index 17f076a0a..8d9eba929 100644 --- a/integration-tests/tests/lib.rs +++ b/integration-tests/tests/lib.rs @@ -8,4 +8,5 @@ mod builder_modifiers; mod code_checkers; +mod cpprefs_test; mod integration_test; From c40832b72e347dd648fcfdc3f0e23eb141ef6f72 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 17 Jun 2022 14:38:16 -0700 Subject: [PATCH 05/27] Add tests --- integration-tests/tests/cpprefs_test.rs | 34 ++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index ad1b79b0a..87be7656c 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -35,7 +35,7 @@ fn run_cpprefs_test( } #[test] -fn test_method_call_const() { +fn test_method_call_mut() { run_cpprefs_test( "", indoc! {" @@ -46,12 +46,38 @@ fn test_method_call_const() { public: Goat() : horns(0) {} void add_a_horn(); - std::string describe() const; private: uint32_t horns; }; inline void Goat::add_a_horn() { horns++; } + "}, + quote! { + let mut goat = ffi::Goat::new().within_box(); + let mut goat = ffi::CppMutRef::from_box(&mut goat); + goat.add_a_horn(); + }, + &["Goat"], + &[], + ) +} + +#[test] +fn test_method_call_const() { + run_cpprefs_test( + "", + indoc! {" + #include + #include + + class Goat { + public: + Goat() : horns(0) {} + std::string describe() const; + private: + uint32_t horns; + }; + inline std::string Goat::describe() const { std::ostringstream oss; std::string plural = horns == 1 ? \"\" : \"s\"; @@ -61,8 +87,8 @@ fn test_method_call_const() { "}, quote! { let mut goat = ffi::Goat::new().within_box(); - let mut goat = ffi::CppMutRef::from_box(&mut goat); - goat.add_a_horn(); + let goat = ffi::CppMutRef::from_box(&mut goat); + goat.as_cpp_ref().describe(); }, &["Goat"], &[], From da141a5d6363fe0b7335a052a377cfa0ea4558de Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 17 Jun 2022 14:39:16 -0700 Subject: [PATCH 06/27] Simplify example --- examples/reference-wrappers/src/input.h | 4 ---- examples/reference-wrappers/src/main.rs | 2 -- 2 files changed, 6 deletions(-) diff --git a/examples/reference-wrappers/src/input.h b/examples/reference-wrappers/src/input.h index b6cdd874e..853fc0984 100644 --- a/examples/reference-wrappers/src/input.h +++ b/examples/reference-wrappers/src/input.h @@ -22,10 +22,6 @@ class Goat { uint32_t horns; }; -inline uint32_t DoMath(uint32_t a) { - return a * 3; -} - inline void Goat::add_a_horn() { horns++; } inline std::string Goat::describe() const { std::ostringstream oss; diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index 00a6bec47..27aa18f33 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -10,12 +10,10 @@ use autocxx::prelude::*; include_cpp! { #include "input.h" safety!(unsafe_references_wrapped) - generate!("DoMath") generate!("Goat") } fn main() { - println!("Hello, world! - C++ math should say 12={}", ffi::DoMath(4)); let mut goat = ffi::Goat::new().within_box(); let mut goat = ffi::CppMutRef::from_box(&mut goat); goat.add_a_horn(); From 3fb0ec7427a4776875e736da779e254c95d4daaa Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jun 2022 22:37:49 -0700 Subject: [PATCH 07/27] Return types now pointy --- .../codegen_cpp/function_wrapper_cpp.rs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs index 034099c6a..342fbfbb3 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use syn::{Type, TypePtr}; +use syn::{Type, TypePtr, TypeReference}; use crate::conversion::{ analysis::fun::function_wrapper::{CppConversionType, TypeConversionPolicy}, @@ -33,6 +33,15 @@ impl TypeConversionPolicy { pub(super) fn converted_type(&self, cpp_name_map: &CppNameMap) -> Result { match self.cpp_conversion { CppConversionType::FromValueToUniquePtr => self.unique_ptr_wrapped_type(cpp_name_map), + CppConversionType::FromReferenceToPointer => { + let (elem, is_mut) = self.unwrapped_type_referent(); + let const_string = if is_mut { + "" + } else { + "const " + }; + Ok(format!("{}{}*", const_string, type_to_cpp(elem, cpp_name_map)?)) + }, _ => self.unwrapped_type_as_string(cpp_name_map), } } @@ -41,6 +50,17 @@ impl TypeConversionPolicy { type_to_cpp(&self.unwrapped_type, cpp_name_map) } + fn unwrapped_type_referent(&self) -> (&Type, bool) { + match &self.unwrapped_type { + Type::Reference(TypeReference { + elem, + mutability, + .. + }) => (&elem, mutability.is_some()), + _ => panic!("Unexpectedly trying to convert non-reference") + } + } + pub(crate) fn is_a_pointer(&self) -> Pointerness { match &self.unwrapped_type { Type::Ptr(TypePtr { From 2b9b3c4830836bd30904555029a8f3a8f30fdd78 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jun 2022 22:38:03 -0700 Subject: [PATCH 08/27] Add more example --- examples/reference-wrappers/build.rs | 3 ++- examples/reference-wrappers/src/input.cc | 11 +++++++++++ examples/reference-wrappers/src/input.h | 11 +++++++++++ examples/reference-wrappers/src/main.rs | 3 +++ 4 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 examples/reference-wrappers/src/input.cc diff --git a/examples/reference-wrappers/build.rs b/examples/reference-wrappers/build.rs index fd8da795f..64c573d44 100644 --- a/examples/reference-wrappers/build.rs +++ b/examples/reference-wrappers/build.rs @@ -9,7 +9,8 @@ fn main() -> miette::Result<()> { let path = std::path::PathBuf::from("src"); let mut b = autocxx_build::Builder::new("src/main.rs", &[&path]).build()?; - b.flag_if_supported("-std=c++14").compile("autocxx-reference-wrapper-example"); + b.flag_if_supported("-std=c++14") + .file("src/input.cc").compile("autocxx-reference-wrapper-example"); println!("cargo:rerun-if-changed=src/main.rs"); println!("cargo:rerun-if-changed=src/input.h"); diff --git a/examples/reference-wrappers/src/input.cc b/examples/reference-wrappers/src/input.cc new file mode 100644 index 000000000..32bf76d1b --- /dev/null +++ b/examples/reference-wrappers/src/input.cc @@ -0,0 +1,11 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#include "input.h" + +Goat the_goat; \ No newline at end of file diff --git a/examples/reference-wrappers/src/input.h b/examples/reference-wrappers/src/input.h index 853fc0984..ce8abc0c7 100644 --- a/examples/reference-wrappers/src/input.h +++ b/examples/reference-wrappers/src/input.h @@ -22,6 +22,7 @@ class Goat { uint32_t horns; }; + inline void Goat::add_a_horn() { horns++; } inline std::string Goat::describe() const { std::ostringstream oss; @@ -29,3 +30,13 @@ inline std::string Goat::describe() const { oss << "This goat has " << horns << " horn" << plural << "."; return oss.str(); } + +class Field { +public: + const Goat& get_goat() const { + return the_goat; + } + +private: + Goat the_goat; +}; \ No newline at end of file diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index 27aa18f33..803dd5bde 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -11,6 +11,7 @@ include_cpp! { #include "input.h" safety!(unsafe_references_wrapped) generate!("Goat") + generate!("Field") } fn main() { @@ -26,4 +27,6 @@ fn main() { .to_string_lossy(), "This goat has 2 horns." ); + let mut field = ffi::Field::new().within_box(); + let another_goat = field.get_goat(); } From d869b45821d77889e1baa8a520afb0905d5d8107 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 21 Jun 2022 23:31:19 -0700 Subject: [PATCH 09/27] Partial work towards returning CppRefs --- .../analysis/fun/function_wrapper.rs | 27 ++++++++++----- engine/src/conversion/analysis/fun/mod.rs | 20 +++++------ .../codegen_cpp/function_wrapper_cpp.rs | 34 ++++++++----------- .../codegen_rs/function_wrapper_rs.rs | 20 ++++++----- examples/reference-wrappers/src/main.rs | 11 +++++- 5 files changed, 63 insertions(+), 49 deletions(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index 56ef537d6..4354b0fd5 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -10,7 +10,8 @@ use crate::{ conversion::api::SubclassName, types::{Namespace, QualifiedName}, }; -use syn::{parse_quote, Ident, Type}; +use quote::ToTokens; +use syn::{parse_quote, Ident, Type, TypeReference}; #[derive(Clone, Debug)] pub(crate) enum CppConversionType { @@ -23,8 +24,8 @@ pub(crate) enum CppConversionType { /// Ignored in the sense that it isn't passed into the C++ function. IgnoredPlacementPtrParameter, FromReturnValueToPlacementPtr, - FromPointerToReference, - FromReferenceToPointer, + FromPointerToReference, // unwrapped_type is always Type::Ptr + FromReferenceToPointer, // unwrapped_type is always Type::Ptr } impl CppConversionType { @@ -56,8 +57,8 @@ pub(crate) enum RustConversionType { FromValueParamToPtr, FromPlacementParamToNewReturn, FromRValueParamToPtr, - FromReferenceWrapperToPointer, - FromPointerToReferenceWrapper, + FromReferenceWrapperToPointer, // unwrapped_type is always Type::Ptr + FromPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr } impl RustConversionType { @@ -97,8 +98,18 @@ impl TypeConversionPolicy { } pub(crate) fn return_reference_into_wrapper(ty: Type) -> Self { + let (unwrapped_type, is_mut) = match ty { + Type::Reference(TypeReference { + elem, mutability, .. + }) => (*elem, mutability.is_some()), + _ => panic!("Not a ptr: {}", ty.to_token_stream().to_string()), + }; TypeConversionPolicy { - unwrapped_type: ty, + unwrapped_type: if is_mut { + parse_quote! { *mut #unwrapped_type } + } else { + parse_quote! { *const #unwrapped_type } + }, cpp_conversion: CppConversionType::FromReferenceToPointer, rust_conversion: RustConversionType::FromPointerToReferenceWrapper, } @@ -175,8 +186,8 @@ impl TypeConversionPolicy { RustConversionType::FromValueParamToPtr | RustConversionType::FromRValueParamToPtr | RustConversionType::FromPlacementParamToNewReturn - | RustConversionType::FromPointerToReferenceWrapper - | RustConversionType::FromReferenceWrapperToPointer + | RustConversionType::FromPointerToReferenceWrapper { .. } + | RustConversionType::FromReferenceWrapperToPointer { .. } ) } diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index e1faf9799..b6707899a 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -1751,9 +1751,9 @@ impl<'a> FnAnalyzer<'a> { && !rust_conversion_forced // must be std::pin::Pin<&mut T> { - let inner_ty = extract_type_from_pinned_mut_ref(p); + let unwrapped_type = extract_type_from_pinned_mut_ref(p); TypeConversionPolicy { - unwrapped_type: parse_quote! { *mut #inner_ty }, + unwrapped_type: parse_quote! { *mut #unwrapped_type }, cpp_conversion: CppConversionType::FromPointerToReference, rust_conversion: RustConversionType::FromReferenceWrapperToPointer, } @@ -1835,17 +1835,13 @@ impl<'a> FnAnalyzer<'a> { ) && !rust_conversion_forced && !is_placement_return_destination => { - let unwrapped_type = if mutability.is_some() { - parse_quote! { - *mut #elem - } - } else { - parse_quote! { - *const #elem - } - }; + let is_mut = mutability.is_some(); TypeConversionPolicy { - unwrapped_type, + unwrapped_type: if is_mut { + panic!("Never expected to find &mut T at this point, we should be Pin<&mut T> by now") + } else { + parse_quote! { *const #elem } + }, cpp_conversion: CppConversionType::FromPointerToReference, rust_conversion: RustConversionType::FromReferenceWrapperToPointer, } diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs index 342fbfbb3..470cb240e 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use syn::{Type, TypePtr, TypeReference}; +use syn::{Type, TypePtr}; use crate::conversion::{ analysis::fun::function_wrapper::{CppConversionType, TypeConversionPolicy}, @@ -34,13 +34,18 @@ impl TypeConversionPolicy { match self.cpp_conversion { CppConversionType::FromValueToUniquePtr => self.unique_ptr_wrapped_type(cpp_name_map), CppConversionType::FromReferenceToPointer => { - let (elem, is_mut) = self.unwrapped_type_referent(); - let const_string = if is_mut { - "" - } else { - "const " + let (const_string, ty) = match &self.unwrapped_type { + Type::Ptr(TypePtr { + mutability: Some(_), + elem, + .. + }) => ("", elem.as_ref()), + Type::Ptr(TypePtr { + elem, .. + }) => ("const ", elem.as_ref()), + _ => panic!("Not a pointer") }; - Ok(format!("{}{}*", const_string, type_to_cpp(elem, cpp_name_map)?)) + Ok(format!("{}{}*", const_string, type_to_cpp(ty, cpp_name_map)?)) }, _ => self.unwrapped_type_as_string(cpp_name_map), } @@ -50,17 +55,6 @@ impl TypeConversionPolicy { type_to_cpp(&self.unwrapped_type, cpp_name_map) } - fn unwrapped_type_referent(&self) -> (&Type, bool) { - match &self.unwrapped_type { - Type::Reference(TypeReference { - elem, - mutability, - .. - }) => (&elem, mutability.is_some()), - _ => panic!("Unexpectedly trying to convert non-reference") - } - } - pub(crate) fn is_a_pointer(&self) -> Pointerness { match &self.unwrapped_type { Type::Ptr(TypePtr { @@ -94,7 +88,7 @@ impl TypeConversionPolicy { CppConversionType::None | CppConversionType::FromReturnValueToPlacementPtr => { Some(var_name.to_string()) } - CppConversionType::FromPointerToReference => Some(format!("(*{})", var_name)), + CppConversionType::FromPointerToReference { .. } => Some(format!("(*{})", var_name)), CppConversionType::Move => Some(format!("std::move({})", var_name)), CppConversionType::FromUniquePtrToValue | CppConversionType::FromPtrToMove => { Some(format!("std::move(*{})", var_name)) @@ -113,7 +107,7 @@ impl TypeConversionPolicy { }) } CppConversionType::IgnoredPlacementPtrParameter => None, - CppConversionType::FromReferenceToPointer => Some(format!("&{}", var_name)), + CppConversionType::FromReferenceToPointer { .. } => Some(format!("&{}", var_name)), }) } } diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 4c47e749c..1ea7ecaac 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -168,11 +168,13 @@ impl TypeConversionPolicy { RustParamConversion::ReturnValue { ty } } RustConversionType::FromPointerToReferenceWrapper => { - let (ty, is_mut) = match &self.unwrapped_type { + let (is_mut, ty) = match &self.unwrapped_type { Type::Ptr(TypePtr { - elem, mutability, .. - }) => (&*elem, mutability.is_some()), - _ => panic!("Not a ptr"), + mutability, + elem, + .. + }) => (mutability.is_some(), elem.as_ref()), + _ => panic!("Not a pointer") }; let (ty, wrapper_name) = if is_mut { (parse_quote! { &mut CppMutRef<'a, #ty> }, "CppMutRef") @@ -190,11 +192,13 @@ impl TypeConversionPolicy { } } RustConversionType::FromReferenceWrapperToPointer => { - let (ty, is_mut) = match &self.unwrapped_type { + let (is_mut, ty) = match &self.unwrapped_type { Type::Ptr(TypePtr { - elem, mutability, .. - }) => (&*elem, mutability.is_some()), - _ => panic!("Not a ptr"), + mutability, + elem, + .. + }) => (mutability.is_some(), elem.as_ref()), + _ => panic!("Not a pointer") }; let ty = if is_mut { parse_quote! { &mut CppMutRef<'a, #ty> } diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index 803dd5bde..ece515d0b 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -28,5 +28,14 @@ fn main() { "This goat has 2 horns." ); let mut field = ffi::Field::new().within_box(); - let another_goat = field.get_goat(); + let mut field = ffi::CppMutRef::from_box(&mut field); + let another_goat = field.as_cpp_ref().get_goat(); + assert_eq!( + another_goat.as_cpp_ref() + .describe() + .as_ref() + .unwrap() + .to_string_lossy(), + "This goat has 0 horns." + ); } From 66de39c3f83dddd4a415c7e0f9b86c1d797f4508 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Wed, 22 Jun 2022 19:58:31 -0700 Subject: [PATCH 10/27] Return values work --- engine/src/conversion/analysis/fun/mod.rs | 10 ++-- .../src/conversion/codegen_rs/fun_codegen.rs | 54 +++++++++++++------ .../codegen_rs/function_wrapper_rs.rs | 20 +++---- examples/reference-wrappers/src/main.rs | 4 +- 4 files changed, 53 insertions(+), 35 deletions(-) diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index b6707899a..17c471fa7 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -183,7 +183,7 @@ pub(crate) struct ArgumentAnalysis { pub(crate) is_placement_return_destination: bool, } -struct ReturnTypeAnalysis { +pub(crate) struct ReturnTypeAnalysis { rt: ReturnType, conversion: Option, was_reference: bool, @@ -1213,6 +1213,7 @@ impl<'a> FnAnalyzer<'a> { let ret_type_conversion_needed = ret_type_conversion .as_ref() .map_or(false, |x| x.cpp_work_needed()); + let return_needs_rust_conversion = ret_type_conversion.as_ref().map(|ra| ra.rust_work_needed()).unwrap_or_default(); // See https://github.com/dtolnay/cxx/issues/878 for the reason for this next line. let effective_cpp_name = cpp_name.as_ref().unwrap_or(&rust_name); @@ -1351,9 +1352,10 @@ impl<'a> FnAnalyzer<'a> { .any(|pd| pd.conversion.rust_work_needed()); let rust_wrapper_needed = match kind { + _ if any_param_needs_rust_conversion || return_needs_rust_conversion => true, FnKind::TraitMethod { .. } => true, - FnKind::Method { .. } => any_param_needs_rust_conversion || cxxbridge_name != rust_name, - _ => any_param_needs_rust_conversion, + FnKind::Method { .. } => cxxbridge_name != rust_name, + _ => false, }; // Naming, part two. @@ -1929,6 +1931,7 @@ impl<'a> FnAnalyzer<'a> { } _ => { let was_reference = references.ref_return; + log::info!("ADE FOO wr={}", was_reference); let conversion = Some( if was_reference && matches!( @@ -1936,6 +1939,7 @@ impl<'a> FnAnalyzer<'a> { UnsafePolicy::ReferencesWrappedAllFunctionsSafe ) { + log::info!("ADE FOO@"); TypeConversionPolicy::return_reference_into_wrapper(ty.clone()) } else { TypeConversionPolicy::new_unconverted(ty.clone()) diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 5a709d49c..a72946e32 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -29,7 +29,7 @@ use crate::{ conversion::{ analysis::fun::{ ArgumentAnalysis, FnAnalysis, FnKind, MethodKind, RustRenameStrategy, - TraitMethodDetails, + TraitMethodDetails, function_wrapper::TypeConversionPolicy, }, api::{Pointerness, UnsafetyNeeded}, }, @@ -96,6 +96,7 @@ pub(super) fn gen_function( let cxxbridge_name = analysis.cxxbridge_name; let rust_name = &analysis.rust_name; let ret_type = analysis.ret_type; + let ret_conversion = analysis.ret_conversion; let param_details = analysis.param_details; let wrapper_function_needed = analysis.cpp_wrapper.is_some(); let params = analysis.params; @@ -119,6 +120,8 @@ pub(super) fn gen_function( always_unsafe_due_to_trait_definition, doc_attrs: &doc_attrs, non_pod_types, + ret_type: &ret_type, + ret_conversion: &ret_conversion, }; // In rare occasions, we might need to give an explicit lifetime. let (lifetime_tokens, params, ret_type) = add_explicit_lifetime_if_necessary( @@ -148,15 +151,14 @@ pub(super) fn gen_function( impl_entry = Some(fn_generator.generate_method_impl( matches!(method_kind, MethodKind::Constructor { .. }), impl_for, - &ret_type, )); } FnKind::TraitMethod { ref details, .. } => { - trait_impl_entry = Some(fn_generator.generate_trait_impl(details, &ret_type)); + trait_impl_entry = Some(fn_generator.generate_trait_impl(details)); } _ => { // Generate plain old function - bindgen_mod_items.push(fn_generator.generate_function_impl(&ret_type)); + bindgen_mod_items.push(fn_generator.generate_function_impl()); } } } @@ -225,6 +227,8 @@ pub(super) fn gen_function( #[derive(Clone)] struct FnGenerator<'a> { param_details: &'a [ArgumentAnalysis], + ret_conversion: &'a Option, + ret_type: &'a ReturnType, cxxbridge_name: &'a Ident, rust_name: &'a str, unsafety: &'a UnsafetyNeeded, @@ -235,10 +239,10 @@ struct FnGenerator<'a> { impl<'a> FnGenerator<'a> { fn common_parts<'b>( - &self, + &'b self, avoid_self: bool, parameter_reordering: &Option>, - ret_type: &'b ReturnType, + ret_type: Option, ) -> ( Option, Punctuated, @@ -249,7 +253,7 @@ impl<'a> FnGenerator<'a> { let mut local_variables = Vec::new(); let mut arg_list = Vec::new(); let mut ptr_arg_name = None; - let mut ret_type = Cow::Borrowed(ret_type); + let mut ret_type: Cow<'a,_> = ret_type.map(Cow::Owned).unwrap_or(Cow::Borrowed(self.ret_type)); let mut any_conversion_requires_unsafe = false; for pd in self.param_details { let wrapper_arg_name = if pd.self_type.is_some() && !avoid_self { @@ -257,7 +261,7 @@ impl<'a> FnGenerator<'a> { } else { pd.name.clone() }; - let rust_for_param = pd.conversion.rust_conversion(wrapper_arg_name.clone()); + let rust_for_param = pd.conversion.rust_conversion(parse_quote! { #wrapper_arg_name }); match rust_for_param { RustParamConversion::Param { ty, @@ -305,6 +309,26 @@ impl<'a> FnGenerator<'a> { }, any_conversion_requires_unsafe || matches!(self.unsafety, UnsafetyNeeded::JustBridge), ); + let context_is_unsafe = matches!(self.unsafety, UnsafetyNeeded::Always) + || self.always_unsafe_due_to_trait_definition; + let (call_body, ret_type) = match self.ret_conversion { + Some(ret_conversion) if ret_conversion.rust_work_needed() => { + let expr = maybe_unsafes_to_tokens(vec![call_body], context_is_unsafe); + let conv = ret_conversion.rust_conversion(parse_quote! { #expr }); + let (conversion, requires_unsafe, ty) = match conv { + RustParamConversion::Param { local_variables, .. } if !local_variables.is_empty() => panic!("return type required variables"), + RustParamConversion::Param { conversion, conversion_requires_unsafe, ty, .. } => (conversion, conversion_requires_unsafe, ty), + _ => panic!("Unexpected - return type is supposed to be converted to a return type") + }; + (if requires_unsafe { + MaybeUnsafeStmt::NeedsUnsafe(conversion) + } else { + MaybeUnsafeStmt::Normal(conversion) + }, Cow::Owned(parse_quote! { -> #ty })) + }, + _ => (call_body, ret_type), + }; + let call_stmts = if let Some(ptr_arg_name) = ptr_arg_name { let mut closure_stmts = local_variables; closure_stmts.push(MaybeUnsafeStmt::binary( @@ -323,8 +347,6 @@ impl<'a> FnGenerator<'a> { call_stmts.push(call_body); call_stmts }; - let context_is_unsafe = matches!(self.unsafety, UnsafetyNeeded::Always) - || self.always_unsafe_due_to_trait_definition; let call_body = maybe_unsafes_to_tokens(call_stmts, context_is_unsafe); (lifetime_tokens, wrapper_params, ret_type, call_body) } @@ -334,10 +356,9 @@ impl<'a> FnGenerator<'a> { &self, avoid_self: bool, impl_block_type_name: &QualifiedName, - ret_type: &ReturnType, ) -> Box { let (lifetime_tokens, wrapper_params, ret_type, call_body) = - self.common_parts(avoid_self, &None, ret_type); + self.common_parts(avoid_self, &None, None); let rust_name = make_ident(self.rust_name); let unsafety = self.unsafety.wrapper_token(); let doc_attrs = self.doc_attrs; @@ -381,10 +402,9 @@ impl<'a> FnGenerator<'a> { fn generate_trait_impl( &self, details: &TraitMethodDetails, - ret_type: &ReturnType, ) -> Box { let (lifetime_tokens, wrapper_params, ret_type, call_body) = - self.common_parts(details.avoid_self, &details.parameter_reordering, ret_type); + self.common_parts(details.avoid_self, &details.parameter_reordering, None); let doc_attrs = self.doc_attrs; let unsafety = self.unsafety.wrapper_token(); let key = details.trt.clone(); @@ -406,7 +426,7 @@ impl<'a> FnGenerator<'a> { ) -> Box { let ret_type: ReturnType = parse_quote! { -> impl autocxx::moveit::new::New }; let (lifetime_tokens, wrapper_params, ret_type, call_body) = - self.common_parts(true, &None, &ret_type); + self.common_parts(true, &None, Some(ret_type)); let rust_name = make_ident(&self.rust_name); let doc_attrs = self.doc_attrs; let unsafety = self.unsafety.wrapper_token(); @@ -425,9 +445,9 @@ impl<'a> FnGenerator<'a> { } /// Generate a function call wrapper - fn generate_function_impl(&self, ret_type: &ReturnType) -> Item { + fn generate_function_impl(&self) -> Item { let (lifetime_tokens, wrapper_params, ret_type, call_body) = - self.common_parts(false, &None, ret_type); + self.common_parts(false, &None, None); let rust_name = make_ident(self.rust_name); let doc_attrs = self.doc_attrs; let unsafety = self.unsafety.wrapper_token(); diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 1ea7ecaac..239151aca 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -7,7 +7,7 @@ // except according to those terms. use proc_macro2::TokenStream; -use syn::{Pat, Type, TypePtr}; +use syn::{Type, TypePtr, Expr}; use crate::{ conversion::analysis::fun::function_wrapper::{RustConversionType, TypeConversionPolicy}, @@ -32,8 +32,7 @@ pub(super) enum RustParamConversion { } impl TypeConversionPolicy { - /// If returns `None` then this parameter should be omitted entirely. - pub(super) fn rust_conversion(&self, var: Pat) -> RustParamConversion { + pub(super) fn rust_conversion(&self, var: Expr) -> RustParamConversion { match self.rust_conversion { RustConversionType::None => RustParamConversion::Param { ty: self.converted_rust_type(), @@ -123,12 +122,7 @@ impl TypeConversionPolicy { }; let handler_type = make_ident(handler_type); let param_trait = make_ident(param_trait); - let var_name = if let Pat::Ident(pti) = &var { - &pti.ident - } else { - panic!("Unexpected non-ident parameter name"); - }; - let space_var_name = make_ident(format!("{}_space", var_name)); + let space_var_name = make_ident("foo_space"); // TODO let ty = &self.unwrapped_type; let ty = parse_quote! { impl autocxx::#param_trait<#ty> }; // This is the usual trick to put something on the stack, then @@ -148,7 +142,7 @@ impl TypeConversionPolicy { }, ), MaybeUnsafeStmt::needs_unsafe( - quote! { #space_var_name.as_mut().populate(#var_name); }, + quote! { #space_var_name.as_mut().populate(#var); }, ), ], conversion: quote! { @@ -177,16 +171,16 @@ impl TypeConversionPolicy { _ => panic!("Not a pointer") }; let (ty, wrapper_name) = if is_mut { - (parse_quote! { &mut CppMutRef<'a, #ty> }, "CppMutRef") + (parse_quote! { CppMutRef<'a, #ty> }, "CppMutRef") } else { - (parse_quote! { &CppRef<'a, #ty> }, "CppRef") + (parse_quote! { CppRef<'a, #ty> }, "CppRef") }; let wrapper_name = make_ident(wrapper_name); RustParamConversion::Param { ty, local_variables: Vec::new(), conversion: quote! { - #wrapper_name (#var) + #wrapper_name (#var, std::marker::PhantomData) }, conversion_requires_unsafe: false, } diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index ece515d0b..f339bdee0 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -28,10 +28,10 @@ fn main() { "This goat has 2 horns." ); let mut field = ffi::Field::new().within_box(); - let mut field = ffi::CppMutRef::from_box(&mut field); + let field = ffi::CppMutRef::from_box(&mut field); let another_goat = field.as_cpp_ref().get_goat(); assert_eq!( - another_goat.as_cpp_ref() + another_goat .describe() .as_ref() .unwrap() From e6fd2bdd53fc69003a1a92f567d63c0aaa99715d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Wed, 22 Jun 2022 19:58:43 -0700 Subject: [PATCH 11/27] Fmt --- engine/src/conversion/analysis/fun/mod.rs | 5 +- .../codegen_cpp/function_wrapper_cpp.rs | 14 +++--- .../src/conversion/codegen_rs/fun_codegen.rs | 47 ++++++++++++------- .../codegen_rs/function_wrapper_rs.rs | 14 ++---- 4 files changed, 47 insertions(+), 33 deletions(-) diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 17c471fa7..af520aae3 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -1213,7 +1213,10 @@ impl<'a> FnAnalyzer<'a> { let ret_type_conversion_needed = ret_type_conversion .as_ref() .map_or(false, |x| x.cpp_work_needed()); - let return_needs_rust_conversion = ret_type_conversion.as_ref().map(|ra| ra.rust_work_needed()).unwrap_or_default(); + let return_needs_rust_conversion = ret_type_conversion + .as_ref() + .map(|ra| ra.rust_work_needed()) + .unwrap_or_default(); // See https://github.com/dtolnay/cxx/issues/878 for the reason for this next line. let effective_cpp_name = cpp_name.as_ref().unwrap_or(&rust_name); diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs index 470cb240e..5f91392f8 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -40,13 +40,15 @@ impl TypeConversionPolicy { elem, .. }) => ("", elem.as_ref()), - Type::Ptr(TypePtr { - elem, .. - }) => ("const ", elem.as_ref()), - _ => panic!("Not a pointer") + Type::Ptr(TypePtr { elem, .. }) => ("const ", elem.as_ref()), + _ => panic!("Not a pointer"), }; - Ok(format!("{}{}*", const_string, type_to_cpp(ty, cpp_name_map)?)) - }, + Ok(format!( + "{}{}*", + const_string, + type_to_cpp(ty, cpp_name_map)? + )) + } _ => self.unwrapped_type_as_string(cpp_name_map), } } diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index a72946e32..823548ed1 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -28,8 +28,8 @@ use super::{ use crate::{ conversion::{ analysis::fun::{ - ArgumentAnalysis, FnAnalysis, FnKind, MethodKind, RustRenameStrategy, - TraitMethodDetails, function_wrapper::TypeConversionPolicy, + function_wrapper::TypeConversionPolicy, ArgumentAnalysis, FnAnalysis, FnKind, + MethodKind, RustRenameStrategy, TraitMethodDetails, }, api::{Pointerness, UnsafetyNeeded}, }, @@ -253,7 +253,9 @@ impl<'a> FnGenerator<'a> { let mut local_variables = Vec::new(); let mut arg_list = Vec::new(); let mut ptr_arg_name = None; - let mut ret_type: Cow<'a,_> = ret_type.map(Cow::Owned).unwrap_or(Cow::Borrowed(self.ret_type)); + let mut ret_type: Cow<'a, _> = ret_type + .map(Cow::Owned) + .unwrap_or(Cow::Borrowed(self.ret_type)); let mut any_conversion_requires_unsafe = false; for pd in self.param_details { let wrapper_arg_name = if pd.self_type.is_some() && !avoid_self { @@ -261,7 +263,9 @@ impl<'a> FnGenerator<'a> { } else { pd.name.clone() }; - let rust_for_param = pd.conversion.rust_conversion(parse_quote! { #wrapper_arg_name }); + let rust_for_param = pd + .conversion + .rust_conversion(parse_quote! { #wrapper_arg_name }); match rust_for_param { RustParamConversion::Param { ty, @@ -316,16 +320,28 @@ impl<'a> FnGenerator<'a> { let expr = maybe_unsafes_to_tokens(vec![call_body], context_is_unsafe); let conv = ret_conversion.rust_conversion(parse_quote! { #expr }); let (conversion, requires_unsafe, ty) = match conv { - RustParamConversion::Param { local_variables, .. } if !local_variables.is_empty() => panic!("return type required variables"), - RustParamConversion::Param { conversion, conversion_requires_unsafe, ty, .. } => (conversion, conversion_requires_unsafe, ty), - _ => panic!("Unexpected - return type is supposed to be converted to a return type") + RustParamConversion::Param { + local_variables, .. + } if !local_variables.is_empty() => panic!("return type required variables"), + RustParamConversion::Param { + conversion, + conversion_requires_unsafe, + ty, + .. + } => (conversion, conversion_requires_unsafe, ty), + _ => panic!( + "Unexpected - return type is supposed to be converted to a return type" + ), }; - (if requires_unsafe { - MaybeUnsafeStmt::NeedsUnsafe(conversion) - } else { - MaybeUnsafeStmt::Normal(conversion) - }, Cow::Owned(parse_quote! { -> #ty })) - }, + ( + if requires_unsafe { + MaybeUnsafeStmt::NeedsUnsafe(conversion) + } else { + MaybeUnsafeStmt::Normal(conversion) + }, + Cow::Owned(parse_quote! { -> #ty }), + ) + } _ => (call_body, ret_type), }; @@ -399,10 +415,7 @@ impl<'a> FnGenerator<'a> { } /// Generate an 'impl Trait for Type { methods-go-here }' in its entrety. - fn generate_trait_impl( - &self, - details: &TraitMethodDetails, - ) -> Box { + fn generate_trait_impl(&self, details: &TraitMethodDetails) -> Box { let (lifetime_tokens, wrapper_params, ret_type, call_body) = self.common_parts(details.avoid_self, &details.parameter_reordering, None); let doc_attrs = self.doc_attrs; diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 239151aca..18483ed3b 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -7,7 +7,7 @@ // except according to those terms. use proc_macro2::TokenStream; -use syn::{Type, TypePtr, Expr}; +use syn::{Expr, Type, TypePtr}; use crate::{ conversion::analysis::fun::function_wrapper::{RustConversionType, TypeConversionPolicy}, @@ -164,11 +164,9 @@ impl TypeConversionPolicy { RustConversionType::FromPointerToReferenceWrapper => { let (is_mut, ty) = match &self.unwrapped_type { Type::Ptr(TypePtr { - mutability, - elem, - .. + mutability, elem, .. }) => (mutability.is_some(), elem.as_ref()), - _ => panic!("Not a pointer") + _ => panic!("Not a pointer"), }; let (ty, wrapper_name) = if is_mut { (parse_quote! { CppMutRef<'a, #ty> }, "CppMutRef") @@ -188,11 +186,9 @@ impl TypeConversionPolicy { RustConversionType::FromReferenceWrapperToPointer => { let (is_mut, ty) = match &self.unwrapped_type { Type::Ptr(TypePtr { - mutability, - elem, - .. + mutability, elem, .. }) => (mutability.is_some(), elem.as_ref()), - _ => panic!("Not a pointer") + _ => panic!("Not a pointer"), }; let ty = if is_mut { parse_quote! { &mut CppMutRef<'a, #ty> } From c3172ad033c9d600312b99a73a44d6c375d3318f Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Wed, 22 Jun 2022 20:00:22 -0700 Subject: [PATCH 12/27] Clippy --- engine/src/conversion/analysis/fun/function_wrapper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index 4354b0fd5..a5bf88e39 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -102,7 +102,7 @@ impl TypeConversionPolicy { Type::Reference(TypeReference { elem, mutability, .. }) => (*elem, mutability.is_some()), - _ => panic!("Not a ptr: {}", ty.to_token_stream().to_string()), + _ => panic!("Not a ptr: {}", ty.to_token_stream()), }; TypeConversionPolicy { unwrapped_type: if is_mut { From dad7dbcc0784d37809a7ff4b25b1fec5f38938f3 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Wed, 22 Jun 2022 20:01:09 -0700 Subject: [PATCH 13/27] Add comment --- engine/src/conversion/analysis/fun/function_wrapper.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index a5bf88e39..89a53481b 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -81,6 +81,10 @@ impl RustConversionType { /// * Finally, the actual C++ API receives a `std::string` by value. /// The implementation here is distributed across this file, and /// `function_wrapper_rs` and `function_wrapper_cpp`. +/// TODO: we should make this into a single enum, with the Type as enum +/// variant params. That would remove the possibility of various runtime +/// panics by enforcing (for example) that conversion from a pointer always +/// has a Type::Ptr. #[derive(Clone)] pub(crate) struct TypeConversionPolicy { pub(crate) unwrapped_type: Type, From 1b4be714a6a822c0a6270dd0d82b167c8eb0b26e Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Wed, 22 Jun 2022 22:32:29 -0700 Subject: [PATCH 14/27] Cease to rely on CppRefs for methods if not enabled --- .../src/conversion/codegen_rs/fun_codegen.rs | 37 ++++++++++++------- engine/src/conversion/codegen_rs/mod.rs | 1 + 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 823548ed1..7128bb341 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -6,6 +6,7 @@ // 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; @@ -89,6 +90,7 @@ 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(); @@ -122,6 +124,7 @@ 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( @@ -235,6 +238,7 @@ 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> { @@ -385,23 +389,30 @@ impl<'a> FnGenerator<'a> { .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 => ImplBlockKey { - ty: parse_quote! { - CppMutRef< 'a, #ty> + let ty = if self.reference_wrappers { + match receiver_pointerness { + Pointerness::MutPtr => ImplBlockKey { + ty: parse_quote! { + CppMutRef< 'a, #ty> + }, + lifetime: Some(parse_quote! { 'a }), }, - lifetime: Some(parse_quote! { 'a }), - }, - Pointerness::ConstPtr => ImplBlockKey { - ty: parse_quote! { - CppRef< 'a, #ty> + Pointerness::ConstPtr => ImplBlockKey { + ty: parse_quote! { + CppRef< 'a, #ty> + }, + lifetime: Some(parse_quote! { 'a }), }, - lifetime: Some(parse_quote! { 'a }), - }, - Pointerness::Not => ImplBlockKey { + Pointerness::Not => ImplBlockKey { + ty: parse_quote! { # ty }, + lifetime: None, + }, + } + } else { + ImplBlockKey { ty: parse_quote! { # ty }, lifetime: None, - }, + } }; Box::new(ImplBlockDetails { item: ImplItem::Method(parse_quote! { diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index e59ca7ecc..3a6145403 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -595,6 +595,7 @@ 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)], From ab430cf1e8efabb0577117f79e807edd5a02a1ca Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 24 Jun 2022 15:25:24 -0700 Subject: [PATCH 15/27] Progress --- examples/reference-wrappers/src/main.rs | 14 ++++++++++++++ src/lib.rs | 3 +++ 2 files changed, 17 insertions(+) diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index f339bdee0..faf5a5885 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -7,6 +7,7 @@ // except according to those terms. use autocxx::prelude::*; + include_cpp! { #include "input.h" safety!(unsafe_references_wrapped) @@ -38,4 +39,17 @@ fn main() { .to_string_lossy(), "This goat has 0 horns." ); + + + let field = ffi::Field::new().within_unique_ptr(); + let field = ffi::CppPin::new(field); + let another_goat = field.as_cpp_ref().get_goat(); + assert_eq!( + another_goat + .describe() + .as_ref() + .unwrap() + .to_string_lossy(), + "This goat has 0 horns." + ); } diff --git a/src/lib.rs b/src/lib.rs index 37c0f7de4..453c9f6da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -623,6 +623,9 @@ pub mod prelude { pub use crate::WithinBoxTrivial; pub use crate::WithinUniquePtr; pub use crate::WithinUniquePtrTrivial; + pub use crate::CppMutRef; + pub use crate::CppRef; + pub use crate::CppPin; pub use cxx::UniquePtr; pub use moveit::moveit; pub use moveit::new::New; From cc3d4f1cc62958701b7571f0b6ba3bccf13da10c Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 24 Jun 2022 15:47:11 -0700 Subject: [PATCH 16/27] Get the Pin thing working --- engine/src/conversion/codegen_rs/mod.rs | 15 +++++++------- examples/reference-wrappers/src/main.rs | 27 +------------------------ src/lib.rs | 6 +++--- 3 files changed, 12 insertions(+), 36 deletions(-) diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 3a6145403..122083131 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -190,20 +190,21 @@ fn get_cppref_items() -> Vec { } }), Item::Struct(parse_quote! { - /// "Pins" an object so that C++-compatible references can be created. + /// "Pins" a `UniquePtr` to an object, so that C++-compatible references can be created. #[repr(transparent)] - pub struct CppPin(T); + pub struct CppUniquePtrPin(::cxx::UniquePtr); }), Item::Impl(parse_quote! { - impl<'a, T: 'a> autocxx::CppPin<'a, T> for CppPin + impl<'a, T: 'a + ::cxx::private::UniquePtrTarget> autocxx::CppPin<'a, T> for CppUniquePtrPin { type CppRef = CppRef<'a, T>; type CppMutRef = CppMutRef<'a, T>; fn as_ptr(&self) -> *const T { - ::std::ptr::addr_of!(self.0) + // TODO add as_ptr to cxx to avoid the ephemeral reference + self.0.as_ref().unwrap() as *const T } fn as_mut_ptr(&mut self) -> *mut T { - ::std::ptr::addr_of_mut!(self.0) + unsafe { ::std::pin::Pin::into_inner_unchecked(self.0.as_mut().unwrap()) as *mut T } } fn as_cpp_ref(&self) -> Self::CppRef { CppRef(self.as_ptr(), ::std::marker::PhantomData) @@ -214,13 +215,13 @@ fn get_cppref_items() -> Vec { } }), Item::Impl(parse_quote! { - impl CppPin { + impl CppUniquePtrPin { /// Make this object available to C++ by reference. /// This takes ownership of the object, thus proving you /// have no existing Rust references. Any reference to this /// object from Rust after this point is unsafe, since Rust /// and C++ reference semantics are incompatible. - pub fn new(item: T) -> CppPin { + pub fn new(item: ::cxx::UniquePtr) -> Self { Self(item) } } diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index faf5a5885..ab8fdf61b 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -16,33 +16,8 @@ include_cpp! { } fn main() { - let mut goat = ffi::Goat::new().within_box(); - let mut goat = ffi::CppMutRef::from_box(&mut goat); - goat.add_a_horn(); - goat.add_a_horn(); - assert_eq!( - goat.as_cpp_ref() - .describe() - .as_ref() - .unwrap() - .to_string_lossy(), - "This goat has 2 horns." - ); - let mut field = ffi::Field::new().within_box(); - let field = ffi::CppMutRef::from_box(&mut field); - let another_goat = field.as_cpp_ref().get_goat(); - assert_eq!( - another_goat - .describe() - .as_ref() - .unwrap() - .to_string_lossy(), - "This goat has 0 horns." - ); - - let field = ffi::Field::new().within_unique_ptr(); - let field = ffi::CppPin::new(field); + let field = ffi::CppUniquePtrPin::new(field); let another_goat = field.as_cpp_ref().get_goat(); assert_eq!( another_goat diff --git a/src/lib.rs b/src/lib.rs index 453c9f6da..935d77775 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -616,6 +616,9 @@ pub mod prelude { pub use crate::c_void; pub use crate::cpp_semantics; pub use crate::include_cpp; + pub use crate::CppMutRef; + pub use crate::CppPin; + pub use crate::CppRef; pub use crate::PinMut; pub use crate::RValueParam; pub use crate::ValueParam; @@ -623,9 +626,6 @@ pub mod prelude { pub use crate::WithinBoxTrivial; pub use crate::WithinUniquePtr; pub use crate::WithinUniquePtrTrivial; - pub use crate::CppMutRef; - pub use crate::CppRef; - pub use crate::CppPin; pub use cxx::UniquePtr; pub use moveit::moveit; pub use moveit::new::New; From dfb60cc11c43cfb1b90c2d5f619e719cdb8223b9 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 24 Jun 2022 15:48:05 -0700 Subject: [PATCH 17/27] Remove old from_box stuff --- engine/src/conversion/codegen_rs/mod.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 122083131..3983fa2c8 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -150,14 +150,6 @@ fn get_cppref_items() -> Vec { } } }), - Item::Impl(parse_quote! { - impl<'a, T: ::cxx::private::UniquePtrTarget> CppRef<'a, T> { - /// Create a `CppRef` to an item within a pinned box. - pub fn from_box(boxed_item: &'a ::std::pin::Pin<::std::boxed::Box>) -> Self { - Self(unsafe { ::std::pin::Pin::into_inner_unchecked(boxed_item.as_ref()) as *const T }, ::std::marker::PhantomData) - } - } - }), Item::Struct(parse_quote! { #[repr(transparent)] pub struct CppMutRef<'a, T>(pub *mut T, pub ::std::marker::PhantomData<&'a T>); @@ -178,10 +170,6 @@ fn get_cppref_items() -> Vec { }), Item::Impl(parse_quote! { impl<'a, T: ::cxx::private::UniquePtrTarget> CppMutRef<'a, T> { - /// Create a `CppMutRef` to an item within a pinned box. - pub fn from_box(boxed_item: &'a mut ::std::pin::Pin<::std::boxed::Box>) -> Self { - Self(unsafe { ::std::pin::Pin::into_inner_unchecked(boxed_item.as_mut()) as *mut T }, ::std::marker::PhantomData) - } /// Create a const C++ reference from this mutable C++ reference. pub fn as_cpp_ref(&self) -> CppRef<'a, T> { use autocxx::CppRef; From 632663f1676647e3513713f6682011780e4327d7 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 24 Jun 2022 15:52:35 -0700 Subject: [PATCH 18/27] Fix test --- integration-tests/tests/cpprefs_test.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index 87be7656c..f5dd42222 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -77,7 +77,7 @@ fn test_method_call_const() { private: uint32_t horns; }; - + inline std::string Goat::describe() const { std::ostringstream oss; std::string plural = horns == 1 ? \"\" : \"s\"; @@ -86,8 +86,8 @@ fn test_method_call_const() { } "}, quote! { - let mut goat = ffi::Goat::new().within_box(); - let goat = ffi::CppMutRef::from_box(&mut goat); + let goat = ffi::Goat::new().within_unique_ptr(); + let goat = ffi::CppUniquePtrPin::new(goat); goat.as_cpp_ref().describe(); }, &["Goat"], From 5fe7d993867c37a57c7589bab73cdfa86f2d332e Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sat, 25 Jun 2022 08:30:17 -0700 Subject: [PATCH 19/27] Remove some logs --- engine/src/conversion/analysis/fun/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index af520aae3..b1395eff3 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -1831,7 +1831,6 @@ impl<'a> FnAnalyzer<'a> { } } } - // TODO remove this Type::Reference(TypeReference { elem, mutability, .. }) if matches!( @@ -1934,7 +1933,6 @@ impl<'a> FnAnalyzer<'a> { } _ => { let was_reference = references.ref_return; - log::info!("ADE FOO wr={}", was_reference); let conversion = Some( if was_reference && matches!( @@ -1942,7 +1940,6 @@ impl<'a> FnAnalyzer<'a> { UnsafePolicy::ReferencesWrappedAllFunctionsSafe ) { - log::info!("ADE FOO@"); TypeConversionPolicy::return_reference_into_wrapper(ty.clone()) } else { TypeConversionPolicy::new_unconverted(ty.clone()) From b5894f6a70be2d64512d2bffca3720fb132d00e9 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sat, 25 Jun 2022 08:34:23 -0700 Subject: [PATCH 20/27] Remove whitespace --- integration-tests/tests/cpprefs_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index f5dd42222..43c16c5bb 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -49,7 +49,7 @@ fn test_method_call_mut() { private: uint32_t horns; }; - + inline void Goat::add_a_horn() { horns++; } "}, quote! { From 4d7b64134e3c19549d09ebf768f69e432840d0c0 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sat, 25 Jun 2022 08:39:01 -0700 Subject: [PATCH 21/27] Fix a test --- integration-tests/tests/cpprefs_test.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index 43c16c5bb..d0e848c73 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -53,9 +53,9 @@ fn test_method_call_mut() { inline void Goat::add_a_horn() { horns++; } "}, quote! { - let mut goat = ffi::Goat::new().within_box(); - let mut goat = ffi::CppMutRef::from_box(&mut goat); - goat.add_a_horn(); + let goat = ffi::Goat::new().within_unique_ptr(); + let mut goat = ffi::CppUniquePtrPin::new(goat); + goat.as_cpp_mut_ref().add_a_horn(); }, &["Goat"], &[], From 1b039853885a429ed64d3913c539db8034740157 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sat, 25 Jun 2022 08:43:29 -0700 Subject: [PATCH 22/27] Fix another test --- engine/src/conversion/codegen_rs/fun_codegen.rs | 6 ++++-- engine/src/conversion/codegen_rs/function_wrapper_rs.rs | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/engine/src/conversion/codegen_rs/fun_codegen.rs b/engine/src/conversion/codegen_rs/fun_codegen.rs index 7128bb341..db222a643 100644 --- a/engine/src/conversion/codegen_rs/fun_codegen.rs +++ b/engine/src/conversion/codegen_rs/fun_codegen.rs @@ -261,6 +261,7 @@ impl<'a> FnGenerator<'a> { .map(Cow::Owned) .unwrap_or(Cow::Borrowed(self.ret_type)); let mut any_conversion_requires_unsafe = false; + let mut variable_counter = 0usize; for pd in self.param_details { let wrapper_arg_name = if pd.self_type.is_some() && !avoid_self { parse_quote!(self) @@ -269,7 +270,7 @@ impl<'a> FnGenerator<'a> { }; let rust_for_param = pd .conversion - .rust_conversion(parse_quote! { #wrapper_arg_name }); + .rust_conversion(parse_quote! { #wrapper_arg_name }, &mut variable_counter); match rust_for_param { RustParamConversion::Param { ty, @@ -322,7 +323,8 @@ impl<'a> FnGenerator<'a> { let (call_body, ret_type) = match self.ret_conversion { Some(ret_conversion) if ret_conversion.rust_work_needed() => { let expr = maybe_unsafes_to_tokens(vec![call_body], context_is_unsafe); - let conv = ret_conversion.rust_conversion(parse_quote! { #expr }); + let conv = + ret_conversion.rust_conversion(parse_quote! { #expr }, &mut variable_counter); let (conversion, requires_unsafe, ty) = match conv { RustParamConversion::Param { local_variables, .. diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 18483ed3b..48e223825 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -32,7 +32,7 @@ pub(super) enum RustParamConversion { } impl TypeConversionPolicy { - pub(super) fn rust_conversion(&self, var: Expr) -> RustParamConversion { + pub(super) fn rust_conversion(&self, var: Expr, counter: &mut usize) -> RustParamConversion { match self.rust_conversion { RustConversionType::None => RustParamConversion::Param { ty: self.converted_rust_type(), @@ -122,7 +122,10 @@ impl TypeConversionPolicy { }; let handler_type = make_ident(handler_type); let param_trait = make_ident(param_trait); - let space_var_name = make_ident("foo_space"); // TODO + let var_counter = *counter; + *counter += 1; + let space_var_name = format!("space{}", var_counter); + let space_var_name = make_ident(space_var_name); let ty = &self.unwrapped_type; let ty = parse_quote! { impl autocxx::#param_trait<#ty> }; // This is the usual trick to put something on the stack, then From 60417503c4db7d8035e4bf9af548d1d38fc64c7d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sat, 25 Jun 2022 09:01:23 -0700 Subject: [PATCH 23/27] Introduce cpp_pin_uniqueptr to allow swapping impls later --- engine/src/conversion/codegen_rs/mod.rs | 13 ++++++++----- examples/reference-wrappers/src/main.rs | 2 +- integration-tests/tests/cpprefs_test.rs | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 3983fa2c8..25517d180 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -204,16 +204,19 @@ fn get_cppref_items() -> Vec { }), Item::Impl(parse_quote! { impl CppUniquePtrPin { - /// Make this object available to C++ by reference. - /// This takes ownership of the object, thus proving you - /// have no existing Rust references. Any reference to this - /// object from Rust after this point is unsafe, since Rust - /// and C++ reference semantics are incompatible. pub fn new(item: ::cxx::UniquePtr) -> Self { Self(item) } } }), + Item::Fn(parse_quote! { + /// Pin this item so that we can create C++ references to it. + /// This makes it impossible to hold Rust references because Rust + /// references are fundamentally incompatible with C++ references. + pub fn cpp_pin_uniqueptr (item: ::cxx::UniquePtr) -> CppUniquePtrPin { + CppUniquePtrPin::new(item) + } + }) ] .to_vec() } diff --git a/examples/reference-wrappers/src/main.rs b/examples/reference-wrappers/src/main.rs index ab8fdf61b..097016e56 100644 --- a/examples/reference-wrappers/src/main.rs +++ b/examples/reference-wrappers/src/main.rs @@ -17,7 +17,7 @@ include_cpp! { fn main() { let field = ffi::Field::new().within_unique_ptr(); - let field = ffi::CppUniquePtrPin::new(field); + let field = ffi::cpp_pin_uniqueptr(field); let another_goat = field.as_cpp_ref().get_goat(); assert_eq!( another_goat diff --git a/integration-tests/tests/cpprefs_test.rs b/integration-tests/tests/cpprefs_test.rs index d0e848c73..4241decc2 100644 --- a/integration-tests/tests/cpprefs_test.rs +++ b/integration-tests/tests/cpprefs_test.rs @@ -87,7 +87,7 @@ fn test_method_call_const() { "}, quote! { let goat = ffi::Goat::new().within_unique_ptr(); - let goat = ffi::CppUniquePtrPin::new(goat); + let goat = ffi::cpp_pin_uniqueptr(goat); goat.as_cpp_ref().describe(); }, &["Goat"], From 47430a789cb46c6937bf0d2db37e0847423822a5 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sun, 26 Jun 2022 22:19:21 -0700 Subject: [PATCH 24/27] Better encapsulate TypeConversionPolicy. No functional changes. --- .../analysis/fun/function_wrapper.rs | 20 ++- engine/src/conversion/analysis/fun/mod.rs | 122 ++++++++---------- .../codegen_cpp/function_wrapper_cpp.rs | 6 +- engine/src/conversion/codegen_cpp/mod.rs | 2 +- .../codegen_rs/function_wrapper_rs.rs | 14 +- 5 files changed, 84 insertions(+), 80 deletions(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index 89a53481b..ab3b7d9e7 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -87,20 +87,32 @@ impl RustConversionType { /// has a Type::Ptr. #[derive(Clone)] pub(crate) struct TypeConversionPolicy { - pub(crate) unwrapped_type: Type, + unwrapped_type: Type, pub(crate) cpp_conversion: CppConversionType, pub(crate) rust_conversion: RustConversionType, } impl TypeConversionPolicy { pub(crate) fn new_unconverted(ty: Type) -> Self { - TypeConversionPolicy { + Self::new(ty, CppConversionType::None, RustConversionType::None) + } + + pub(crate) fn new( + ty: Type, + cpp_conversion: CppConversionType, + rust_conversion: RustConversionType, + ) -> Self { + Self { unwrapped_type: ty, - cpp_conversion: CppConversionType::None, - rust_conversion: RustConversionType::None, + cpp_conversion, + rust_conversion, } } + pub(crate) fn cxxbridge_type(&self) -> &Type { + &self.unwrapped_type + } + pub(crate) fn return_reference_into_wrapper(ty: Type) -> Self { let (unwrapped_type, is_mut) = match ty { Type::Reference(TypeReference { diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index b1395eff3..7194746aa 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -840,7 +840,7 @@ impl<'a> FnAnalyzer<'a> { let arg_is_reference = matches!( param_details .get(1) - .map(|param| ¶m.conversion.unwrapped_type), + .map(|param| param.conversion.cxxbridge_type()), Some(Type::Reference(_)) ); // Some exotic forms of copy constructor have const and/or volatile qualifiers. @@ -1728,22 +1728,22 @@ impl<'a> FnAnalyzer<'a> { let ty = parse_quote! { rust::Box<#holder_id> }; - TypeConversionPolicy { - unwrapped_type: ty, - cpp_conversion: CppConversionType::Move, - rust_conversion: RustConversionType::ToBoxedUpHolder(subclass), - } + TypeConversionPolicy::new( + ty, + CppConversionType::Move, + RustConversionType::ToBoxedUpHolder(subclass), + ) }; } else if matches!( force_rust_conversion, Some(RustConversionType::FromPlacementParamToNewReturn) ) && matches!(sophistication, TypeConversionSophistication::Regular) { - return TypeConversionPolicy { - unwrapped_type: ty.clone(), - cpp_conversion: CppConversionType::IgnoredPlacementPtrParameter, - rust_conversion: RustConversionType::FromPlacementParamToNewReturn, - }; + return TypeConversionPolicy::new( + ty.clone(), + CppConversionType::IgnoredPlacementPtrParameter, + RustConversionType::FromPlacementParamToNewReturn, + ); } match ty { Type::Path(p) => { @@ -1757,60 +1757,60 @@ impl<'a> FnAnalyzer<'a> { // must be std::pin::Pin<&mut T> { let unwrapped_type = extract_type_from_pinned_mut_ref(p); - TypeConversionPolicy { - unwrapped_type: parse_quote! { *mut #unwrapped_type }, - cpp_conversion: CppConversionType::FromPointerToReference, - rust_conversion: RustConversionType::FromReferenceWrapperToPointer, - } + TypeConversionPolicy::new( + parse_quote! { *mut #unwrapped_type }, + CppConversionType::FromPointerToReference, + RustConversionType::FromReferenceWrapperToPointer, + ) } else if self.pod_safe_types.contains(&tn) { if known_types().lacks_copy_constructor(&tn) { - TypeConversionPolicy { - unwrapped_type: ty, - cpp_conversion: CppConversionType::Move, - rust_conversion: RustConversionType::None, - } + TypeConversionPolicy::new( + ty, + CppConversionType::Move, + RustConversionType::None, + ) } else { TypeConversionPolicy::new_unconverted(ty) } } else if known_types().convertible_from_strs(&tn) && !self.config.exclude_utilities() { - TypeConversionPolicy { - unwrapped_type: ty, - cpp_conversion: CppConversionType::FromUniquePtrToValue, - rust_conversion: RustConversionType::FromStr, - } + TypeConversionPolicy::new( + ty, + CppConversionType::FromUniquePtrToValue, + RustConversionType::FromStr, + ) } else if matches!( sophistication, TypeConversionSophistication::SimpleForSubclasses ) { - TypeConversionPolicy { - unwrapped_type: ty, - cpp_conversion: CppConversionType::FromUniquePtrToValue, - rust_conversion: RustConversionType::None, - } + TypeConversionPolicy::new( + ty, + CppConversionType::FromUniquePtrToValue, + RustConversionType::None, + ) } else { - TypeConversionPolicy { - unwrapped_type: ty, - cpp_conversion: CppConversionType::FromPtrToValue, - rust_conversion: RustConversionType::FromValueParamToPtr, - } + TypeConversionPolicy::new( + ty, + CppConversionType::FromPtrToValue, + RustConversionType::FromValueParamToPtr, + ) } } Type::Ptr(tp) => { let rust_conversion = force_rust_conversion.unwrap_or(RustConversionType::None); if is_move_constructor { - TypeConversionPolicy { - unwrapped_type: ty.clone(), - cpp_conversion: CppConversionType::FromPtrToMove, + TypeConversionPolicy::new( + ty.clone(), + CppConversionType::FromPtrToMove, rust_conversion, - } + ) } else if is_rvalue_ref { - TypeConversionPolicy { - unwrapped_type: *tp.elem.clone(), - cpp_conversion: CppConversionType::FromPtrToValue, - rust_conversion: RustConversionType::FromRValueParamToPtr, - } + TypeConversionPolicy::new( + *tp.elem.clone(), + CppConversionType::FromPtrToValue, + RustConversionType::FromRValueParamToPtr, + ) } else if matches!( self.config.unsafe_policy, UnsafePolicy::ReferencesWrappedAllFunctionsSafe @@ -1818,17 +1818,13 @@ impl<'a> FnAnalyzer<'a> { && !rust_conversion_forced && !is_placement_return_destination { - TypeConversionPolicy { - unwrapped_type: ty.clone(), - cpp_conversion: CppConversionType::FromPointerToReference, - rust_conversion: RustConversionType::FromReferenceWrapperToPointer, - } + TypeConversionPolicy::new( + ty.clone(), + CppConversionType::FromPointerToReference, + RustConversionType::FromReferenceWrapperToPointer, + ) } else { - TypeConversionPolicy { - unwrapped_type: ty.clone(), - cpp_conversion: CppConversionType::None, - rust_conversion, - } + TypeConversionPolicy::new(ty.clone(), CppConversionType::None, rust_conversion) } } Type::Reference(TypeReference { @@ -1840,23 +1836,19 @@ impl<'a> FnAnalyzer<'a> { && !is_placement_return_destination => { let is_mut = mutability.is_some(); - TypeConversionPolicy { - unwrapped_type: if is_mut { + TypeConversionPolicy::new( + if is_mut { panic!("Never expected to find &mut T at this point, we should be Pin<&mut T> by now") } else { parse_quote! { *const #elem } }, - cpp_conversion: CppConversionType::FromPointerToReference, - rust_conversion: RustConversionType::FromReferenceWrapperToPointer, - } + CppConversionType::FromPointerToReference, + RustConversionType::FromReferenceWrapperToPointer, + ) } _ => { let rust_conversion = force_rust_conversion.unwrap_or(RustConversionType::None); - TypeConversionPolicy { - unwrapped_type: ty.clone(), - cpp_conversion: CppConversionType::None, - rust_conversion, - } + TypeConversionPolicy::new(ty.clone(), CppConversionType::None, rust_conversion) } } } diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs index 5f91392f8..536762623 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -34,7 +34,7 @@ impl TypeConversionPolicy { match self.cpp_conversion { CppConversionType::FromValueToUniquePtr => self.unique_ptr_wrapped_type(cpp_name_map), CppConversionType::FromReferenceToPointer => { - let (const_string, ty) = match &self.unwrapped_type { + let (const_string, ty) = match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability: Some(_), elem, @@ -54,11 +54,11 @@ impl TypeConversionPolicy { } fn unwrapped_type_as_string(&self, cpp_name_map: &CppNameMap) -> Result { - type_to_cpp(&self.unwrapped_type, cpp_name_map) + type_to_cpp(self.cxxbridge_type(), cpp_name_map) } pub(crate) fn is_a_pointer(&self) -> Pointerness { - match &self.unwrapped_type { + match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability: Some(_), .. diff --git a/engine/src/conversion/codegen_cpp/mod.rs b/engine/src/conversion/codegen_cpp/mod.rs index 0e4975c81..0e6dccedb 100644 --- a/engine/src/conversion/codegen_cpp/mod.rs +++ b/engine/src/conversion/codegen_cpp/mod.rs @@ -559,7 +559,7 @@ impl<'a> CppCodeGenerator<'a> { underlying_function_call = match placement_param { Some(placement_param) => { - let tyname = type_to_cpp(&ret.unwrapped_type, &self.original_name_map)?; + let tyname = type_to_cpp(ret.cxxbridge_type(), &self.original_name_map)?; format!("new({}) {}({})", placement_param, tyname, call_itself) } None => format!("return {}", call_itself), diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 48e223825..708d41c02 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -62,7 +62,7 @@ impl TypeConversionPolicy { } } RustConversionType::FromPinMaybeUninitToPtr => { - let ty = match &self.unwrapped_type { + let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), }; @@ -79,7 +79,7 @@ impl TypeConversionPolicy { } } RustConversionType::FromPinMoveRefToPtr => { - let ty = match &self.unwrapped_type { + let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), }; @@ -98,7 +98,7 @@ impl TypeConversionPolicy { } } RustConversionType::FromTypeToPtr => { - let ty = match &self.unwrapped_type { + let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), }; @@ -126,7 +126,7 @@ impl TypeConversionPolicy { *counter += 1; let space_var_name = format!("space{}", var_counter); let space_var_name = make_ident(space_var_name); - let ty = &self.unwrapped_type; + let ty = self.cxxbridge_type(); let ty = parse_quote! { impl autocxx::#param_trait<#ty> }; // This is the usual trick to put something on the stack, then // immediately shadow the variable name so it can't be accessed or moved. @@ -158,14 +158,14 @@ impl TypeConversionPolicy { // but not in the arguments for the wrapper function, because instead we return an // impl New which uses the cxx::bridge function's pointer parameter. RustConversionType::FromPlacementParamToNewReturn => { - let ty = match &self.unwrapped_type { + let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => *(*elem).clone(), _ => panic!("Not a ptr"), }; RustParamConversion::ReturnValue { ty } } RustConversionType::FromPointerToReferenceWrapper => { - let (is_mut, ty) = match &self.unwrapped_type { + let (is_mut, ty) = match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability, elem, .. }) => (mutability.is_some(), elem.as_ref()), @@ -187,7 +187,7 @@ impl TypeConversionPolicy { } } RustConversionType::FromReferenceWrapperToPointer => { - let (is_mut, ty) = match &self.unwrapped_type { + let (is_mut, ty) = match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability, elem, .. }) => (mutability.is_some(), elem.as_ref()), From c868f6983f96546c621c8339b35181199b616433 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 27 Jun 2022 08:21:15 -0700 Subject: [PATCH 25/27] Move --- .../analysis/fun/function_wrapper.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index ab3b7d9e7..e20b5dd6c 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -28,6 +28,21 @@ pub(crate) enum CppConversionType { FromReferenceToPointer, // unwrapped_type is always Type::Ptr } +#[derive(Clone, Debug)] +pub(crate) enum RustConversionType { + None, + FromStr, + ToBoxedUpHolder(SubclassName), + FromPinMaybeUninitToPtr, + FromPinMoveRefToPtr, + FromTypeToPtr, + FromValueParamToPtr, + FromPlacementParamToNewReturn, + FromRValueParamToPtr, + FromReferenceWrapperToPointer, // unwrapped_type is always Type::Ptr + FromPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr +} + impl CppConversionType { /// If we've found a function which does X to its parameter, what /// is the opposite of X? This is used for subclasses where calls @@ -46,21 +61,6 @@ impl CppConversionType { } } -#[derive(Clone, Debug)] -pub(crate) enum RustConversionType { - None, - FromStr, - ToBoxedUpHolder(SubclassName), - FromPinMaybeUninitToPtr, - FromPinMoveRefToPtr, - FromTypeToPtr, - FromValueParamToPtr, - FromPlacementParamToNewReturn, - FromRValueParamToPtr, - FromReferenceWrapperToPointer, // unwrapped_type is always Type::Ptr - FromPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr -} - impl RustConversionType { pub(crate) fn requires_mutability(&self) -> Option { match self { From be42a056654853d27e87e0e61066a1d1b516c02b Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 27 Jun 2022 09:54:48 -0700 Subject: [PATCH 26/27] Introduce comprehensive Rust conversion types --- .../analysis/fun/function_wrapper.rs | 41 ++++++++++--------- engine/src/conversion/analysis/fun/mod.rs | 34 +++++++-------- .../codegen_rs/function_wrapper_rs.rs | 24 +++++------ engine/src/conversion/codegen_rs/lifetime.rs | 2 +- 4 files changed, 52 insertions(+), 49 deletions(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index e20b5dd6c..3ee9dd3e7 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -31,16 +31,19 @@ pub(crate) enum CppConversionType { #[derive(Clone, Debug)] pub(crate) enum RustConversionType { None, - FromStr, - ToBoxedUpHolder(SubclassName), - FromPinMaybeUninitToPtr, - FromPinMoveRefToPtr, - FromTypeToPtr, - FromValueParamToPtr, - FromPlacementParamToNewReturn, - FromRValueParamToPtr, - FromReferenceWrapperToPointer, // unwrapped_type is always Type::Ptr - FromPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr + FromReturnValueToPlacementPtr, // like None + FromValueToValueToMove, + FromUniquePtrToUniquePtrToValue, + FromStrToUniquePtrToValue, + ToBoxedUpHolderToMove(SubclassName), + FromPinMaybeUninitToPtrToPtr, + FromPinMoveRefToPtrToPtr, + FromValueToPtrToPtr, + FromValueParamToPtrToValue, + FromPlacementParamToNewReturnToNone, + FromRValueParamToPtrToValue, + FromReferenceWrapperToPointerToReference, // unwrapped_type is always Type::Ptr + FromReferenceToPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr } impl CppConversionType { @@ -64,7 +67,7 @@ impl CppConversionType { impl RustConversionType { pub(crate) fn requires_mutability(&self) -> Option { match self { - Self::FromPinMoveRefToPtr => Some(parse_quote! { mut }), + Self::FromPinMoveRefToPtrToPtr => Some(parse_quote! { mut }), _ => None, } } @@ -127,7 +130,7 @@ impl TypeConversionPolicy { parse_quote! { *const #unwrapped_type } }, cpp_conversion: CppConversionType::FromReferenceToPointer, - rust_conversion: RustConversionType::FromPointerToReferenceWrapper, + rust_conversion: RustConversionType::FromReferenceToPointerToReferenceWrapper, } } @@ -135,7 +138,7 @@ impl TypeConversionPolicy { TypeConversionPolicy { unwrapped_type: ty, cpp_conversion: CppConversionType::FromValueToUniquePtr, - rust_conversion: RustConversionType::None, + rust_conversion: RustConversionType::FromUniquePtrToUniquePtrToValue, } } @@ -146,7 +149,7 @@ impl TypeConversionPolicy { // Rust conversion is marked as none here, since this policy // will be applied to the return value, and the Rust-side // shenanigans applies to the placement new *parameter* - rust_conversion: RustConversionType::None, + rust_conversion: RustConversionType::FromReturnValueToPlacementPtr, } } @@ -199,11 +202,11 @@ impl TypeConversionPolicy { pub(crate) fn bridge_unsafe_needed(&self) -> bool { matches!( self.rust_conversion, - RustConversionType::FromValueParamToPtr - | RustConversionType::FromRValueParamToPtr - | RustConversionType::FromPlacementParamToNewReturn - | RustConversionType::FromPointerToReferenceWrapper { .. } - | RustConversionType::FromReferenceWrapperToPointer { .. } + RustConversionType::FromValueParamToPtrToValue + | RustConversionType::FromRValueParamToPtrToValue + | RustConversionType::FromPlacementParamToNewReturnToNone + | RustConversionType::FromReferenceToPointerToReferenceWrapper { .. } + | RustConversionType::FromReferenceWrapperToPointerToReference { .. } ) } diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 7194746aa..93417e926 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -1021,7 +1021,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromTypeToPtr), + Some(RustConversionType::FromValueToPtrToPtr), sophistication, false, false, @@ -1045,7 +1045,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMaybeUninitToPtr), + Some(RustConversionType::FromPinMaybeUninitToPtrToPtr), sophistication, false, false, @@ -1070,7 +1070,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMaybeUninitToPtr), + Some(RustConversionType::FromPinMaybeUninitToPtrToPtr), sophistication, false, false, @@ -1083,7 +1083,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMoveRefToPtr), + Some(RustConversionType::FromPinMoveRefToPtrToPtr), sophistication, false, true, @@ -1656,7 +1656,7 @@ impl<'a> FnAnalyzer<'a> { let is_placement_return_destination = is_placement_return_destination || matches!( force_rust_conversion, - Some(RustConversionType::FromPlacementParamToNewReturn) + Some(RustConversionType::FromPlacementParamToNewReturnToNone) ); let annotated_type = self.convert_boxed_type(pt.ty, ns, pointer_treatment)?; let conversion = self.argument_conversion_details( @@ -1731,18 +1731,18 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( ty, CppConversionType::Move, - RustConversionType::ToBoxedUpHolder(subclass), + RustConversionType::ToBoxedUpHolderToMove(subclass), ) }; } else if matches!( force_rust_conversion, - Some(RustConversionType::FromPlacementParamToNewReturn) + Some(RustConversionType::FromPlacementParamToNewReturnToNone) ) && matches!(sophistication, TypeConversionSophistication::Regular) { return TypeConversionPolicy::new( ty.clone(), CppConversionType::IgnoredPlacementPtrParameter, - RustConversionType::FromPlacementParamToNewReturn, + RustConversionType::FromPlacementParamToNewReturnToNone, ); } match ty { @@ -1760,14 +1760,14 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( parse_quote! { *mut #unwrapped_type }, CppConversionType::FromPointerToReference, - RustConversionType::FromReferenceWrapperToPointer, + RustConversionType::FromReferenceWrapperToPointerToReference, ) } else if self.pod_safe_types.contains(&tn) { if known_types().lacks_copy_constructor(&tn) { TypeConversionPolicy::new( ty, CppConversionType::Move, - RustConversionType::None, + RustConversionType::FromValueToValueToMove, ) } else { TypeConversionPolicy::new_unconverted(ty) @@ -1778,7 +1778,7 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( ty, CppConversionType::FromUniquePtrToValue, - RustConversionType::FromStr, + RustConversionType::FromStrToUniquePtrToValue, ) } else if matches!( sophistication, @@ -1787,13 +1787,13 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( ty, CppConversionType::FromUniquePtrToValue, - RustConversionType::None, + RustConversionType::FromUniquePtrToUniquePtrToValue, ) } else { TypeConversionPolicy::new( ty, CppConversionType::FromPtrToValue, - RustConversionType::FromValueParamToPtr, + RustConversionType::FromValueParamToPtrToValue, ) } } @@ -1809,7 +1809,7 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( *tp.elem.clone(), CppConversionType::FromPtrToValue, - RustConversionType::FromRValueParamToPtr, + RustConversionType::FromRValueParamToPtrToValue, ) } else if matches!( self.config.unsafe_policy, @@ -1821,7 +1821,7 @@ impl<'a> FnAnalyzer<'a> { TypeConversionPolicy::new( ty.clone(), CppConversionType::FromPointerToReference, - RustConversionType::FromReferenceWrapperToPointer, + RustConversionType::FromReferenceWrapperToPointerToReference, ) } else { TypeConversionPolicy::new(ty.clone(), CppConversionType::None, rust_conversion) @@ -1843,7 +1843,7 @@ impl<'a> FnAnalyzer<'a> { parse_quote! { *const #elem } }, CppConversionType::FromPointerToReference, - RustConversionType::FromReferenceWrapperToPointer, + RustConversionType::FromReferenceWrapperToPointerToReference, ) } _ => { @@ -1893,7 +1893,7 @@ impl<'a> FnAnalyzer<'a> { &References::default(), false, false, - Some(RustConversionType::FromPlacementParamToNewReturn), + Some(RustConversionType::FromPlacementParamToNewReturnToNone), TypeConversionSophistication::Regular, false, )?; diff --git a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs index 708d41c02..6a09cc359 100644 --- a/engine/src/conversion/codegen_rs/function_wrapper_rs.rs +++ b/engine/src/conversion/codegen_rs/function_wrapper_rs.rs @@ -34,19 +34,19 @@ pub(super) enum RustParamConversion { impl TypeConversionPolicy { pub(super) fn rust_conversion(&self, var: Expr, counter: &mut usize) -> RustParamConversion { match self.rust_conversion { - RustConversionType::None => RustParamConversion::Param { + RustConversionType::None | RustConversionType::FromUniquePtrToUniquePtrToValue | RustConversionType::FromReturnValueToPlacementPtr | RustConversionType::FromValueToValueToMove => RustParamConversion::Param { ty: self.converted_rust_type(), local_variables: Vec::new(), conversion: quote! { #var }, conversion_requires_unsafe: false, }, - RustConversionType::FromStr => RustParamConversion::Param { + RustConversionType::FromStrToUniquePtrToValue => RustParamConversion::Param { ty: parse_quote! { impl ToCppString }, local_variables: Vec::new(), conversion: quote! ( #var .into_cpp() ), conversion_requires_unsafe: false, }, - RustConversionType::ToBoxedUpHolder(ref sub) => { + RustConversionType::ToBoxedUpHolderToMove(ref sub) => { let holder_type = sub.holder(); let id = sub.id(); let ty = parse_quote! { autocxx::subclass::CppSubclassRustPeerHolder< @@ -61,7 +61,7 @@ impl TypeConversionPolicy { conversion_requires_unsafe: false, } } - RustConversionType::FromPinMaybeUninitToPtr => { + RustConversionType::FromPinMaybeUninitToPtrToPtr => { let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), @@ -78,7 +78,7 @@ impl TypeConversionPolicy { conversion_requires_unsafe: true, } } - RustConversionType::FromPinMoveRefToPtr => { + RustConversionType::FromPinMoveRefToPtrToPtr => { let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), @@ -97,7 +97,7 @@ impl TypeConversionPolicy { conversion_requires_unsafe: true, } } - RustConversionType::FromTypeToPtr => { + RustConversionType::FromValueToPtrToPtr => { let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => &*elem, _ => panic!("Not a ptr"), @@ -112,10 +112,10 @@ impl TypeConversionPolicy { conversion_requires_unsafe: false, } } - RustConversionType::FromValueParamToPtr | RustConversionType::FromRValueParamToPtr => { + RustConversionType::FromValueParamToPtrToValue | RustConversionType::FromRValueParamToPtrToValue => { let (handler_type, param_trait) = match self.rust_conversion { - RustConversionType::FromValueParamToPtr => ("ValueParamHandler", "ValueParam"), - RustConversionType::FromRValueParamToPtr => { + RustConversionType::FromValueParamToPtrToValue => ("ValueParamHandler", "ValueParam"), + RustConversionType::FromRValueParamToPtrToValue => { ("RValueParamHandler", "RValueParam") } _ => unreachable!(), @@ -157,14 +157,14 @@ impl TypeConversionPolicy { // This type of conversion means that this function parameter appears in the cxx::bridge // but not in the arguments for the wrapper function, because instead we return an // impl New which uses the cxx::bridge function's pointer parameter. - RustConversionType::FromPlacementParamToNewReturn => { + RustConversionType::FromPlacementParamToNewReturnToNone => { let ty = match self.cxxbridge_type() { Type::Ptr(TypePtr { elem, .. }) => *(*elem).clone(), _ => panic!("Not a ptr"), }; RustParamConversion::ReturnValue { ty } } - RustConversionType::FromPointerToReferenceWrapper => { + RustConversionType::FromReferenceToPointerToReferenceWrapper => { let (is_mut, ty) = match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability, elem, .. @@ -186,7 +186,7 @@ impl TypeConversionPolicy { conversion_requires_unsafe: false, } } - RustConversionType::FromReferenceWrapperToPointer => { + RustConversionType::FromReferenceWrapperToPointerToReference => { let (is_mut, ty) = match self.cxxbridge_type() { Type::Ptr(TypePtr { mutability, elem, .. diff --git a/engine/src/conversion/codegen_rs/lifetime.rs b/engine/src/conversion/codegen_rs/lifetime.rs index ea2c782b5..f0d0343ee 100644 --- a/engine/src/conversion/codegen_rs/lifetime.rs +++ b/engine/src/conversion/codegen_rs/lifetime.rs @@ -51,7 +51,7 @@ pub(crate) fn add_explicit_lifetime_if_necessary<'r>( pd.has_lifetime || matches!( pd.conversion.rust_conversion, - RustConversionType::FromValueParamToPtr + RustConversionType::FromValueParamToPtrToValue ) }); let return_type_is_impl = return_type_is_impl(&ret_type); From 1381a96c823df828fc3afd86a9cdcfc5bc502686 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 27 Jun 2022 12:03:11 -0700 Subject: [PATCH 27/27] Partial work --- .../analysis/fun/function_wrapper.rs | 8 ++++++ engine/src/conversion/analysis/fun/mod.rs | 28 +++++++++---------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/engine/src/conversion/analysis/fun/function_wrapper.rs b/engine/src/conversion/analysis/fun/function_wrapper.rs index 3ee9dd3e7..01c319a5b 100644 --- a/engine/src/conversion/analysis/fun/function_wrapper.rs +++ b/engine/src/conversion/analysis/fun/function_wrapper.rs @@ -46,6 +46,14 @@ pub(crate) enum RustConversionType { FromReferenceToPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr } +pub(crate) enum ConversionHint { + None, + FromValueToPtrToPtr, + FromPlacementParamToNewReturnToNone, + FromPinMaybeUninitToPtrToPtr, + FromPinMoveRefToPtrToPtr, +} + impl CppConversionType { /// If we've found a function which does X to its parameter, what /// is the opposite of X? This is used for subclasses where calls diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 93417e926..6598e3d2f 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -54,7 +54,7 @@ use crate::{ use self::{ bridge_name_tracker::BridgeNameTracker, - function_wrapper::RustConversionType, + function_wrapper::{RustConversionType, ConversionHint}, implicit_constructors::{find_constructors_present, ItemsFound}, overload_tracker::OverloadTracker, subclass::{ @@ -737,7 +737,7 @@ impl<'a> FnAnalyzer<'a> { &fun.references, true, false, - None, + ConversionHint::None, sophistication, false, ) @@ -1002,7 +1002,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - None, + ConversionHint::None, sophistication, true, false, @@ -1021,7 +1021,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromValueToPtrToPtr), + ConversionHint::FromValueToPtrToPtr, sophistication, false, false, @@ -1045,7 +1045,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMaybeUninitToPtrToPtr), + ConversionHint::FromPinMaybeUninitToPtrToPtr, sophistication, false, false, @@ -1070,7 +1070,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMaybeUninitToPtrToPtr), + ConversionHint::FromPinMaybeUninitToPtrToPtr, sophistication, false, false, @@ -1083,7 +1083,7 @@ impl<'a> FnAnalyzer<'a> { &rust_name, &mut params, &mut param_details, - Some(RustConversionType::FromPinMoveRefToPtrToPtr), + ConversionHint::FromPinMoveRefToPtrToPtr, sophistication, false, true, @@ -1418,7 +1418,7 @@ impl<'a> FnAnalyzer<'a> { rust_name: &str, params: &mut Punctuated, param_details: &mut [ArgumentAnalysis], - force_rust_conversion: Option, + force_rust_conversion: ConversionHint, sophistication: TypeConversionSophistication, construct_into_self: bool, is_move_constructor: bool, @@ -1587,7 +1587,7 @@ impl<'a> FnAnalyzer<'a> { references: &References, treat_this_as_reference: bool, is_move_constructor: bool, - force_rust_conversion: Option, + force_rust_conversion: ConversionHint, sophistication: TypeConversionSophistication, construct_into_self: bool, ) -> Result<(FnArg, ArgumentAnalysis), ConvertError> { @@ -1656,7 +1656,7 @@ impl<'a> FnAnalyzer<'a> { let is_placement_return_destination = is_placement_return_destination || matches!( force_rust_conversion, - Some(RustConversionType::FromPlacementParamToNewReturnToNone) + ConversionHint::FromPlacementParamToNewReturnToNone ); let annotated_type = self.convert_boxed_type(pt.ty, ns, pointer_treatment)?; let conversion = self.argument_conversion_details( @@ -1705,7 +1705,7 @@ impl<'a> FnAnalyzer<'a> { &self, annotated_type: &Annotated>, is_move_constructor: bool, - force_rust_conversion: Option, + force_rust_conversion: ConversionHint, sophistication: TypeConversionSophistication, is_self: bool, is_placement_return_destination: bool, @@ -1720,7 +1720,7 @@ impl<'a> FnAnalyzer<'a> { ); let is_reference = matches!(annotated_type.kind, type_converter::TypeKind::Reference) || is_self; - let rust_conversion_forced = force_rust_conversion.is_some(); + let rust_conversion_forced = !matches!(force_rust_conversion, ConversionHint::None); let ty = &*annotated_type.ty; if let Some(holder_id) = is_subclass_holder { let subclass = SubclassName::from_holder_name(holder_id); @@ -1736,7 +1736,7 @@ impl<'a> FnAnalyzer<'a> { }; } else if matches!( force_rust_conversion, - Some(RustConversionType::FromPlacementParamToNewReturnToNone) + ConversionHint::FromPlacementParamToNewReturnToNone ) && matches!(sophistication, TypeConversionSophistication::Regular) { return TypeConversionPolicy::new( @@ -1893,7 +1893,7 @@ impl<'a> FnAnalyzer<'a> { &References::default(), false, false, - Some(RustConversionType::FromPlacementParamToNewReturnToNone), + ConversionHint::FromPlacementParamToNewReturnToNone, TypeConversionSophistication::Regular, false, )?;