Skip to content

Add support for unsafe constructor bindings in Crubit. #910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add support for unsafe constructor bindings in Crubit. #910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/cpp/virtual/example_generated.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ impl<'__unelided> ::ctor::CtorNew<::ctor::RvalueReference<'__unelided, Self>> fo
fn ctor_new(args: ::ctor::RvalueReference<'__unelided, Self>) -> Self::CtorType {
let mut __param_0 = args;
unsafe {
::ctor::FnCtor::new(move |dest: *mut Self| {
::ctor::FnCtor::new(move |__crubit_dest: *mut Self| {
crate::detail::__rust_thunk___ZN11RustDerivedC1EOS_(
dest as *mut ::core::ffi::c_void,
__crubit_dest as *mut ::core::ffi::c_void,
__param_0,
);
})
Expand Down Expand Up @@ -107,9 +107,9 @@ impl ::ctor::CtorNew<::definition::RustDerived> for RustDerived {
fn ctor_new(args: ::definition::RustDerived) -> Self::CtorType {
let mut rust = args;
unsafe {
::ctor::FnCtor::new(move |dest: *mut Self| {
::ctor::FnCtor::new(move |__crubit_dest: *mut Self| {
crate::detail::__rust_thunk___ZN11RustDerivedC1EN10definition11RustDerivedE(
dest as *mut ::core::ffi::c_void,
__crubit_dest as *mut ::core::ffi::c_void,
&mut rust,
);
})
Expand Down
14 changes: 13 additions & 1 deletion rs_bindings_from_cc/generate_bindings/database/function_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ pub enum TraitName {
CtorNew(Rc<[RsTypeKind]>),
Default,
From(Rc<[RsTypeKind]>),
/// The constructor trait for !Unpin types that are considered unsafe.
UnsafeCtorNew(Rc<[RsTypeKind]>),
/// The conversion trait for Unpin types that are considered unsafe.
UnsafeFrom(Rc<[RsTypeKind]>),
/// The PartialEq trait.
PartialEq {
param: Rc<RsTypeKind>,
Expand Down Expand Up @@ -88,6 +92,8 @@ impl TraitName {
TraitName::CtorNew { .. } => "CtorNew",
TraitName::Default => "Default",
TraitName::From { .. } => "From",
TraitName::UnsafeCtorNew { .. } => "UnsafeCtorNew",
TraitName::UnsafeFrom { .. } => "UnsafeFrom",
TraitName::PartialEq { .. } => "PartialEq",
TraitName::PartialOrd { .. } => "PartialOrd",
TraitName::CcIndex { .. } => "CcIndex",
Expand All @@ -101,7 +107,11 @@ impl TraitName {
fn params(&self) -> &[RsTypeKind] {
match self {
Self::Clone | Self::Default | Self::Delete => &[],
Self::CtorNew(params) | Self::From(params) | Self::Other { params, .. } => params,
Self::CtorNew(params)
| Self::From(params)
| Self::UnsafeCtorNew(params)
| Self::UnsafeFrom(params)
| Self::Other { params, .. } => params,
Self::PartialEq { param, .. } | Self::PartialOrd { param } => {
core::slice::from_ref(param)
}
Expand Down Expand Up @@ -212,6 +222,8 @@ impl ImplKind {
Self::None { is_unsafe: true, .. }
| Self::Struct { is_unsafe: true, .. }
| Self::Trait { trait_name: TraitName::Other { is_unsafe_fn: true, .. }, .. }
| Self::Trait { trait_name: TraitName::UnsafeCtorNew(_), .. }
| Self::Trait { trait_name: TraitName::UnsafeFrom(_), .. }
)
}
}
Expand Down
128 changes: 103 additions & 25 deletions rs_bindings_from_cc/generate_bindings/generate_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ fn trait_name_to_token_stream_removing_trait_record(
format_tuple_except_singleton_replacing_by_self(db, arg_types, self_type);
quote! { From < #formatted_arg_types > }
}
UnsafeCtorNew(arg_types) => {
let formatted_arg_types =
format_tuple_except_singleton_replacing_by_self(db, arg_types, self_type);
quote! { ::ctor::UnsafeCtorNew < #formatted_arg_types > }
}
UnsafeFrom(arg_types) => {
let formatted_arg_types =
format_tuple_except_singleton_replacing_by_self(db, arg_types, self_type);
quote! { ::ctor::UnsafeFrom < #formatted_arg_types > }
}
PartialEq { param, .. } => {
if self_type.is_some_and(|self_type| param.as_ref() == self_type) {
quote! {PartialEq}
Expand Down Expand Up @@ -430,7 +440,7 @@ fn generate_cc_operator_index_nonmut_impls(
(*pointer_data.pointee_type).clone()
}

other_variant => {
_ => {
bail_to_errors!(
errors,
"operator[] should return a reference (values are not yet supported), found {}",
Expand Down Expand Up @@ -487,7 +497,7 @@ fn generate_cc_operator_index_mut_impls(
(*pointer_data.pointee_type).clone()
}

other_variant => {
_ => {
bail_to_errors!(
errors,
"(mutable) operator[] should return a reference (values are not yet supported), found {}",
Expand Down Expand Up @@ -931,26 +941,28 @@ fn api_func_shape_for_destructor(
}

/// Issue any errors related to unsafe constructors being unsupported.
/// Returns the unsafe parameters if any, instead of adding errors, when appropriate.
fn issue_unsafe_constructor_errors(
db: &BindingsGenerator,
func: &Func,
record: &Record,
param_types: &[RsTypeKind],
errors: &Errors,
) {
) -> Option<String> {
match func.safety_annotation {
SafetyAnnotation::DisableUnsafe => {}
SafetyAnnotation::DisableUnsafe => None,
SafetyAnnotation::Unsafe => {
errors.add(anyhow!(
"Constructors cannot be `unsafe`, but an explicit unsafe annotation was provided. See b/216648347."));
None
}
SafetyAnnotation::Unannotated => {
// Move and copy constructors are excepted from this check, as Google C++ style
// disallows move and copy constructors which require invariants to hold on public
// fields of the source object.
let is_move_or_copy_ctor = matches!(param_types, [_this, arg] if arg.is_ref_to(record));
if is_move_or_copy_ctor {
return;
return None;
}

// We skip the first parameter because it's the implicit `this` parameter.
Expand All @@ -966,10 +978,9 @@ fn issue_unsafe_constructor_errors(
.collect::<Vec<String>>()
.join("");
if !unsafe_params.is_empty() {
errors.add(anyhow!(
"Constructors cannot be `unsafe`, but this constructor accepts:{unsafe_params}"
));
return Some(unsafe_params);
}
None
}
}
}
Expand All @@ -988,11 +999,9 @@ fn api_func_shape_for_constructor(
errors.add(err);
}
materialize_ctor_in_caller(func, param_types);
issue_unsafe_constructor_errors(db, func, record, param_types, errors);
let unsafe_params = issue_unsafe_constructor_errors(db, func, record, param_types, errors);

if !record.is_unpin() {
let func_name = make_rs_ident("ctor_new");

let [_this, params @ ..] = param_types else {
errors.add(anyhow!(
"This is a bug in Crubit. Could not find `__this` parameter in a constructor: {:?}",
Expand Down Expand Up @@ -1021,9 +1030,16 @@ fn api_func_shape_for_constructor(
}
}
}

let trait_name = if unsafe_params.is_some() {
TraitName::UnsafeCtorNew(params.iter().cloned().collect())
} else {
TraitName::CtorNew(params.iter().cloned().collect())
};

let impl_kind = ImplKind::Trait {
record: record.clone(),
trait_name: TraitName::CtorNew(params.iter().cloned().collect()),
trait_name,
impl_for: ImplFor::T,
format_first_param_as_self: false,
drop_return: false,
Expand All @@ -1033,8 +1049,20 @@ fn api_func_shape_for_constructor(
// meaning it's only usable within the current target anyway.
always_public: true,
};
return Some((make_rs_ident("ctor_new"), impl_kind));
}

if unsafe_params.is_some() {
let func_name = make_rs_ident("unsafe_from");
let impl_kind = ImplKind::new_trait(
TraitName::UnsafeFrom(Rc::from(param_types[1..].to_vec())),
record.clone(),
/* format_first_param_as_self= */ false,
/* force_const_reference_params= */ false,
);
return Some((func_name, impl_kind));
}

match func.params.len() {
0 => {
errors.add(anyhow!(
Expand Down Expand Up @@ -1386,9 +1414,9 @@ fn generate_func_body(
PassingConvention::Ctor => {
quote! {
::ctor::FnCtor::new(
move |dest: *mut #return_type_or_self| {
move |__crubit_dest: *mut #return_type_or_self| {
#crate_root_path::detail::#thunk_ident(
dest as *mut ::core::ffi::c_void
__crubit_dest as *mut ::core::ffi::c_void
#( , #thunk_args )*
);
}
Expand Down Expand Up @@ -1758,14 +1786,17 @@ pub fn generate_function(
quoted_return_type = quote! {};
}

if !errors.is_empty() {
if let ImplKind::Trait { trait_name: TraitName::CtorNew(_), .. } = impl_kind {
// Generated CtorNew functions return an `impl Trait` type which can't use
// the `errors_as_unsatisfied_trait_bound` reporting system because
// the `'error` lifetime causes an error when combined with `impl Trait due to
// https://github.com/rust-lang/rust/issues/134804
errors.consolidate()?;
}
if !errors.is_empty()
&& let ImplKind::Trait {
trait_name: TraitName::CtorNew(_) | TraitName::UnsafeCtorNew(_),
..
} = impl_kind
{
// Generated CtorNew and UnsafeCtorNew functions return an `impl Trait` type which can't use
// the `errors_as_unsatisfied_trait_bound` reporting system because
// the `'error` lifetime causes an error when combined with `impl Trait due to
// https://github.com/rust-lang/rust/issues/134804
errors.consolidate()?;
}

let reportable_status: Result<(), ErrorList> = errors.consolidate();
Expand Down Expand Up @@ -2159,7 +2190,7 @@ pub fn generate_function(
} else {
quote! {}
};
if matches!(trait_name, TraitName::CtorNew(_)) {
if matches!(trait_name, TraitName::CtorNew(_) | TraitName::UnsafeCtorNew(_)) {
extra_body.extend(quote! {
type Error = ::ctor::Infallible;
});
Expand Down Expand Up @@ -2210,6 +2241,24 @@ pub fn generate_function(
}
}
}
TraitName::UnsafeCtorNew(params) if params.len() == 1 => {
let single_param_ = format_tuple_except_singleton_replacing_by_self(
db,
params,
Some(&self_type_for_extra),
);
quote! {
impl #formatted_trait_generic_params ::ctor::UnsafeCtorNew<(#single_param_,)> for #record_name #trait_record_param_tokens #unsatisfied_where_clause {
#extra_body

#[inline (always)]
unsafe fn ctor_new(args: (#single_param_,)) -> Self::CtorType {
let (arg,) = args;
<Self as ::ctor::UnsafeCtorNew<#single_param_>>::ctor_new(arg)
}
}
}
}
TraitName::From(params) if reportable_status.is_ok() => {
let single_param_ = format_tuple_except_singleton_replacing_by_self(
db,
Expand All @@ -2228,6 +2277,24 @@ pub fn generate_function(
}
}
}
TraitName::UnsafeFrom(params) if reportable_status.is_ok() => {
let single_param_ = format_tuple_except_singleton_replacing_by_self(
db,
params,
Some(&self_type_for_extra),
);
quote! {
impl #formatted_trait_generic_params ::ctor::UnsafeCtorNew<#single_param_> for #record_name #trait_record_param_tokens #unsatisfied_where_clause {
type CtorType = Self;
type Error = ::ctor::Infallible;

#[inline (always)]
unsafe fn ctor_new(args: #single_param_) -> Self::CtorType {
<Self as ::ctor::UnsafeFrom<#single_param_>>::unsafe_from(args)
}
}
}
}
TraitName::CcIndex { .. } | TraitName::CcIndexMut { .. } => {
generate_standard_indexing_impl(
db,
Expand Down Expand Up @@ -2614,7 +2681,12 @@ fn function_signature(
}
}
Some(
TraitName::Clone | TraitName::CtorNew(..) | TraitName::Default | TraitName::From(..),
TraitName::Clone
| TraitName::CtorNew(..)
| TraitName::Default
| TraitName::From(..)
| TraitName::UnsafeCtorNew(..)
| TraitName::UnsafeFrom(..),
) => {
move_self_from_out_param_to_return_value(
db,
Expand All @@ -2628,7 +2700,13 @@ fn function_signature(
quoted_return_type = Some(quote! { Self });

// CtorNew and From group parameters into a tuple.
if let Some(TraitName::CtorNew(args_type) | TraitName::From(args_type)) = trait_name {
if let Some(
TraitName::CtorNew(args_type)
| TraitName::From(args_type)
| TraitName::UnsafeCtorNew(args_type)
| TraitName::UnsafeFrom(args_type),
) = trait_name
{
let args_type = if let Some(self_type) = self_type.as_ref() {
format_tuple_except_singleton_replacing_by_self(db, args_type, Some(self_type))
} else {
Expand Down
Loading