Skip to content

Commit 79fcd3b

Browse files
committed
Partial conversion to CppRef<T> everywhere
Background: Rust references have certain rules, most notably that the underlying data cannot be changed while an immutable reference exists. That's essentially impossible to promise for any C++ data; C++ may retain references or pointers to data any modify it at any time. This presents a problem for Rust/C++ interop tooling. Various solutions or workarounds are possible: 1) All C++ data is represented as zero-sized types. This is the approach taken by cxx for opaque types. This sidesteps all of the Rust reference rules, since those rules only apply to areas of memory that are referred to. This doesn't really work well enough for autocxx since we want to be able to keep C++ data on the Rust stack, using all the fancy moveit shenanigans, and that means that Rust must know the true size and alignment of the type. 2) All C++ data is represented as UnsafeCell<MaybeUninit<T>>. This also sidesteps the reference rules. This would be a valid option for autocxx. 3) Have a sufficiently simple language boundary that humans can reasonably guarantee there are no outstanding references on the C++ side which could be used to modify the underlying data. This is the approach taken by cxx for cxx::kind::Trivial types. It's just about possible to cause UB using one of these types, but you really have to work at it. In practice such UB is unlikely. 4) Never allow Rust references to C++ types. Instead use a special smart pointer type in Rust, representing a C++ reference. This is the direction in this PR. More detail on this last approach here: https://medium.com/@adetaylor/are-we-reference-yet-c-references-in-rust-72c1c6c7015a This facility is already in autocxx, by adopting the safety policy "unsafe_references_wrapped". However, it hasn't really been battle tested and has a bunch of deficiencies. It's been awaiting formal Rust support for "arbitrary self types" so that methods can be called on such smart pointers. This is now [fairly close to stabilization](rust-lang/rust#44874 (comment)); this PR is part of the experimentation required to investigate whether that rustc feature should go ahead and get stabilized. This PR essentially converts autocxx to only operate in this mode - there should no longer ever be Rust references to C++ data. This PR is incomplete: * There are still 60 failing integration tests. Mostly these relate to subclass support, which isn't yet converted. * `ValueParam` and `RValueParam` need to support taking `CppPin<T>`, and possibly `CppRef<T: CopyNew>` etc. * Because we can't implement `Deref` for `cxx::UniquePtr<T>` to emit a `CppRef<T>`, unfortunately `cxx::UniquePtr<T>` can't be used in cases where we want to provide a `const T&`. It's necessary to call `.as_cpp_ref()` on the `UniquePtr`. This is sufficiently annoying that it might be necessary to implement a trait `ReferenceParam` like we have for `ValueParam` and `RValueParam`. (Alternatives include upstreaming `CppRef<T>` into cxx, but per reason 4 listed above, the complexity probably isn't merited for statically-declared cxx interfaces; or separating from cxx entirely.) This also shows up a [Rustc problem which is fixed here](rust-lang/rust#135179). Ergonomic findings: * The problem with `cxx::UniquePtr` as noted above. * It's nice that `Deref` coercion allows methods to be called on `CppPin` as well as `CppRef`. * To get the same benefit for parameters being passed in by reference, you need to pass in `&my_cpp_pin_wrapped_thing` which is weird given that the whole point is we're trying to avoid Rust references. Obviously, calling `.as_cpp_ref()` works too, so this weirdness can be avoided. * When creating some C++ data `T`, in Rust, it's annoying to have to decide a-priori whether it'll be Rust or C++ accessing the data. If the former, you just create a new `T`; if the latter you need to wrap it in `CppPin::new`. This is only really a problem when creating a C++ object on which you'll call methods. It feels like it'll be OK in practice. Possibly this can be resolved by making the method receiver some sort of `impl MethodReceiver<T>` generic; an implementation for `T` could be provided which auto-wraps it into a `CppPin` (consuming it at that point). This sounds messy though. A bit more thought required, but even if this isn't possible it doesn't sound like a really serious ergonomics problem, especially if we can use `#[diagnostic::on_unimplemented]` somehow to guide. Next steps here: * Stabilize arbitrary self types. This PR has gone far enough to show that there are no really serious unexpected issues there. * Implement `ValueParam` and `RValueParam` as necessary for `CppRef` and `CppPin` types. * Work on those ergonomic issues to the extent possible. * Make a bold decision about whether autocxx should shift wholesale away from `&` to `CppRef<T>`. If so, this will be a significant breaking change.
1 parent 0988d44 commit 79fcd3b

File tree

15 files changed

+350
-329
lines changed

15 files changed

+350
-329
lines changed

Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ rust-version = "1.77"
2626
resolver = "2"
2727

2828
[features]
29-
arbitrary_self_types_pointers = []
29+
arbitrary_self_types = []
3030

3131
[dependencies]
3232
autocxx-macro = { path="macro", version="0.27.1" }

demo/src/main.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
// option. This file may not be copied, modified, or distributed
77
// except according to those terms.
88

9+
#![feature(arbitrary_self_types)]
10+
911
use autocxx::prelude::*;
1012
include_cpp! {
1113
#include "input.h"
@@ -16,9 +18,9 @@ include_cpp! {
1618

1719
fn main() {
1820
println!("Hello, world! - C++ math should say 12={}", ffi::DoMath(4));
19-
let mut goat = ffi::Goat::new().within_box();
20-
goat.as_mut().add_a_horn();
21-
goat.as_mut().add_a_horn();
21+
let goat = ffi::Goat::new().within_cpp_pin();
22+
goat.add_a_horn();
23+
goat.add_a_horn();
2224
assert_eq!(
2325
goat.describe().as_ref().unwrap().to_string_lossy(),
2426
"This goat has 2 horns."

engine/src/conversion/analysis/fun/mod.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -1530,13 +1530,15 @@ impl<'a> FnAnalyzer<'a> {
15301530
let from_type = self_ty.as_ref().unwrap();
15311531
let from_type_path = from_type.to_type_path();
15321532
let to_type = to_type.to_type_path();
1533-
let (trait_signature, ty, method_name) = match *mutable {
1533+
let (trait_signature, ty, method_name): (_, Type, _) = match *mutable {
15341534
CastMutability::ConstToConst => (
15351535
parse_quote! {
1536-
AsRef < #to_type >
1536+
autocxx::AsCppRef < #to_type >
15371537
},
1538-
Type::Path(from_type_path),
1539-
"as_ref",
1538+
parse_quote! {
1539+
autocxx::CppRef< #from_type_path >
1540+
},
1541+
"as_cpp_ref",
15401542
),
15411543
CastMutability::MutToConst => (
15421544
parse_quote! {

engine/src/conversion/codegen_rs/function_wrapper_rs.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,12 @@ impl TypeConversionPolicy {
172172
_ => panic!("Not a pointer"),
173173
};
174174
let (ty, wrapper_name) = if is_mut {
175-
(parse_quote! { autocxx::CppMutRef<'a, #ty> }, "CppMutRef")
175+
(
176+
parse_quote! { autocxx::CppMutLtRef<'a, #ty> },
177+
"CppMutLtRef",
178+
)
176179
} else {
177-
(parse_quote! { autocxx::CppRef<'a, #ty> }, "CppRef")
180+
(parse_quote! { autocxx::CppLtRef<'a, #ty> }, "CppLtRef")
178181
};
179182
let wrapper_name = make_ident(wrapper_name);
180183
RustParamConversion::Param {
@@ -194,9 +197,9 @@ impl TypeConversionPolicy {
194197
_ => panic!("Not a pointer"),
195198
};
196199
let ty = if is_mut {
197-
parse_quote! { &mut autocxx::CppMutRef<'a, #ty> }
200+
parse_quote! { autocxx::CppMutRef<#ty> }
198201
} else {
199-
parse_quote! { &autocxx::CppRef<'a, #ty> }
202+
parse_quote! { autocxx::CppRef<#ty> }
200203
};
201204
RustParamConversion::Param {
202205
ty,

engine/src/conversion/codegen_rs/mod.rs

+3-9
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ pub(crate) mod unqualify;
1717
use indexmap::map::IndexMap as HashMap;
1818
use indexmap::set::IndexSet as HashSet;
1919

20-
use autocxx_parser::{ExternCppType, IncludeCppConfig, RustFun, UnsafePolicy};
20+
use autocxx_parser::{ExternCppType, IncludeCppConfig, RustFun};
2121

2222
use itertools::Itertools;
2323
use proc_macro2::{Span, TokenStream};
@@ -131,7 +131,6 @@ fn get_string_items() -> Vec<Item> {
131131
/// In practice, much of the "generation" involves connecting together
132132
/// existing lumps of code within the Api structures.
133133
pub(crate) struct RsCodeGenerator<'a> {
134-
unsafe_policy: &'a UnsafePolicy,
135134
include_list: &'a [String],
136135
bindgen_mod: ItemMod,
137136
original_name_map: CppNameMap,
@@ -143,14 +142,12 @@ impl<'a> RsCodeGenerator<'a> {
143142
/// Generate code for a set of APIs that was discovered during parsing.
144143
pub(crate) fn generate_rs_code(
145144
all_apis: ApiVec<FnPhase>,
146-
unsafe_policy: &'a UnsafePolicy,
147145
include_list: &'a [String],
148146
bindgen_mod: ItemMod,
149147
config: &'a IncludeCppConfig,
150148
header_name: Option<String>,
151149
) -> Vec<Item> {
152150
let c = Self {
153-
unsafe_policy,
154151
include_list,
155152
bindgen_mod,
156153
original_name_map: CppNameMap::new_from_apis(&all_apis),
@@ -617,11 +614,8 @@ impl<'a> RsCodeGenerator<'a> {
617614
name, superclass, ..
618615
} => {
619616
let methods = associated_methods.get(&superclass);
620-
let generate_peer_constructor = subclasses_with_a_single_trivial_constructor.contains(&name.0.name) &&
621-
// TODO: Create an UnsafeCppPeerConstructor trait for calling an unsafe
622-
// constructor instead? Need to create unsafe versions of everything that uses
623-
// it too.
624-
matches!(self.unsafe_policy, UnsafePolicy::AllFunctionsSafe);
617+
let generate_peer_constructor =
618+
subclasses_with_a_single_trivial_constructor.contains(&name.0.name);
625619
self.generate_subclass(name, &superclass, methods, generate_peer_constructor)
626620
}
627621
Api::ExternCppType {

engine/src/conversion/conversion_tests.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ fn do_test(input: ItemMod) {
3131
let inclusions = "".into();
3232
bc.convert(
3333
input,
34-
UnsafePolicy::AllFunctionsSafe,
34+
UnsafePolicy::ReferencesWrappedAllFunctionsSafe,
3535
inclusions,
3636
&CodegenOptions::default(),
3737
"",

engine/src/conversion/mod.rs

-1
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,6 @@ impl<'a> BridgeConverter<'a> {
192192
.map_err(ConvertError::Cpp)?;
193193
let rs = RsCodeGenerator::generate_rs_code(
194194
analyzed_apis,
195-
&unsafe_policy,
196195
self.include_list,
197196
bindgen_mod,
198197
self.config,

examples/reference-wrappers/src/main.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
// Necessary to be able to call methods on reference wrappers.
2727
// For that reason, this example only builds on nightly Rust.
28-
#![feature(arbitrary_self_types_pointers)]
28+
#![feature(arbitrary_self_types)]
2929

3030
use autocxx::prelude::*;
3131
use std::pin::Pin;

integration-tests/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,7 @@ pub fn do_run_test(
380380
let safety_policy = format_ident!("{}", safety_policy);
381381
let unexpanded_rust = quote! {
382382
#module_attributes
383+
#![feature(arbitrary_self_types)]
383384

384385
use autocxx::prelude::*;
385386

integration-tests/tests/cpprefs_test.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ fn run_cpprefs_test(
2727
generate_pods: &[&str],
2828
) {
2929
if !arbitrary_self_types_supported() {
30-
// "unsafe_references_wrapped" requires arbitrary_self_types_pointers, which requires nightly.
30+
// "unsafe_references_wrapped" requires arbitrary_self_types, which requires nightly.
3131
return;
3232
}
3333
do_run_test(
@@ -127,7 +127,7 @@ fn test_return_reference_cpprefs() {
127127
let rs = quote! {
128128
let b = CppPin::new(ffi::Bob { a: 3, b: 4 });
129129
let b_ref = b.as_cpp_ref();
130-
let bob = ffi::give_bob(&b_ref);
130+
let bob = ffi::give_bob(b_ref);
131131
let val = unsafe { bob.as_ref() };
132132
assert_eq!(val.b, 4);
133133
};

0 commit comments

Comments
 (0)