Skip to content
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

[DRAFT] Support for C++ Exceptions #1426

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

[DRAFT] Support for C++ Exceptions #1426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cb3a9e
Initial exception support -- works for POD but nothing more complex
torrancew Jan 16, 2025
e6904c1
Add more exception test cases, and implement support for non-trivial
torrancew Jan 20, 2025
1172b08
Remove unused function
torrancew Jan 20, 2025
fc95425
Add more test cases for moveit types
torrancew Jan 20, 2025
5d8edee
Add support for constructors
torrancew Jan 20, 2025
d3b7f6a
Add shim traits to mimic WithinUniquePtrTrivial/WithinBoxTrivial for …
torrancew Jan 20, 2025
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
9 changes: 9 additions & 0 deletions engine/src/conversion/analysis/fun/function_wrapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ pub(crate) enum RustConversionType {
FromRValueParamToPtr,
FromReferenceWrapperToPointer, // unwrapped_type is always Type::Ptr
FromPointerToReferenceWrapper, // unwrapped_type is always Type::Ptr
WrapResult(Box<RustConversionType>),
}

impl RustConversionType {
Expand Down Expand Up @@ -159,6 +160,14 @@ impl TypeConversionPolicy {
}
}

pub(crate) fn wrap_result(self) -> Self {
let inner_conversion = Box::new(self.rust_conversion);
TypeConversionPolicy {
rust_conversion: RustConversionType::WrapResult(inner_conversion),
..self
}
}

pub(crate) fn cpp_work_needed(&self) -> bool {
!matches!(self.cpp_conversion, CppConversionType::None)
}
Expand Down
37 changes: 28 additions & 9 deletions engine/src/conversion/analysis/fun/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ pub(crate) struct FnAnalysis {
/// subclass entries for them. But we do not want to have them
/// be externally callable.
pub(crate) ignore_reason: Result<(), ConvertErrorWithContext>,
/// Whether this method can throw an exception
pub(crate) throwable: bool,
/// Whether this can be called by external code. Not so for
/// protected methods.
pub(crate) externally_callable: bool,
Expand Down Expand Up @@ -825,7 +827,7 @@ impl<'a> FnAnalyzer<'a> {
);
let (kind, error_context, rust_name) = if let Some(trait_details) = trait_details {
trait_details
} else if let Some(self_ty) = self_ty {
} else if let Some(ref self_ty) = self_ty {
// Some kind of method or static method.
let type_ident = self_ty.get_final_item();
// bindgen generates methods with the name:
Expand All @@ -837,7 +839,7 @@ impl<'a> FnAnalyzer<'a> {
let mut rust_name = ideal_rust_name;
let nested_type_ident = self
.nested_type_name_map
.get(&self_ty)
.get(self_ty)
.map(|s| s.as_str())
.unwrap_or_else(|| self_ty.get_final_item());
if matches!(
Expand All @@ -851,7 +853,7 @@ impl<'a> FnAnalyzer<'a> {
}
rust_name = predetermined_rust_name
.unwrap_or_else(|| self.get_overload_name(ns, type_ident, rust_name));
let error_context = self.error_context_for_method(&self_ty, &rust_name);
let error_context = self.error_context_for_method(self_ty, &rust_name);

// If this is 'None', then something weird is going on. We'll check for that
// later when we have enough context to generate useful errors.
Expand Down Expand Up @@ -883,7 +885,7 @@ impl<'a> FnAnalyzer<'a> {
(
FnKind::TraitMethod {
kind,
impl_for: self_ty,
impl_for: self_ty.clone(),
details: Box::new(TraitMethodDetails {
trt: TraitImplSignature {
ty: ty.into(),
Expand All @@ -904,7 +906,7 @@ impl<'a> FnAnalyzer<'a> {
} else {
(
FnKind::Method {
impl_for: self_ty,
impl_for: self_ty.clone(),
method_kind: MethodKind::Constructor { is_default: false },
},
error_context,
Expand All @@ -914,12 +916,12 @@ impl<'a> FnAnalyzer<'a> {
} else if matches!(fun.special_member, Some(SpecialMemberKind::Destructor)) {
rust_name = predetermined_rust_name
.unwrap_or_else(|| self.get_overload_name(ns, type_ident, rust_name));
let error_context = self.error_context_for_method(&self_ty, &rust_name);
let error_context = self.error_context_for_method(self_ty, &rust_name);
let ty = Type::Path(self_ty.to_type_path());
(
FnKind::TraitMethod {
kind: TraitMethodKind::Destructor,
impl_for: self_ty,
impl_for: self_ty.clone(),
details: Box::new(TraitMethodDetails {
trt: TraitImplSignature {
ty: ty.into(),
Expand Down Expand Up @@ -973,10 +975,10 @@ impl<'a> FnAnalyzer<'a> {
// Disambiguate overloads.
let rust_name = predetermined_rust_name
.unwrap_or_else(|| self.get_overload_name(ns, type_ident, rust_name));
let error_context = self.error_context_for_method(&self_ty, &rust_name);
let error_context = self.error_context_for_method(self_ty, &rust_name);
(
FnKind::Method {
impl_for: self_ty,
impl_for: self_ty.clone(),
method_kind,
},
error_context,
Expand Down Expand Up @@ -1196,6 +1198,16 @@ impl<'a> FnAnalyzer<'a> {
}
let mut cxxbridge_name = make_ident(&cxxbridge_name);

let friendly_name = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that much of the naming stuff is a bit of a mess, but please do your best to be consistent here with the way other configuration directives are approached.

In byvalue_checker.rs we have:

        let pod_requests = config
            .get_pod_requests()
            .iter()
            .map(|ty| QualifiedName::new_from_cpp_name(ty))
            .collect();

and the comment in new_from_cpp_name suggests this is what you should be using.

let ns_pfx = (!ns.is_empty())
.then_some(format!("{ns}::"))
.unwrap_or_default();
let ty_pfx = self_ty.map_or(String::default(), |ty| format!("{ty}::"));

format!("{ns_pfx}{ty_pfx}{}", name.cpp_name())
};
let throwable = self.config.is_throwable(&friendly_name);

// Analyze the return type, just as we previously did for the
// parameters.
let mut return_analysis = self
Expand Down Expand Up @@ -1432,6 +1444,12 @@ impl<'a> FnAnalyzer<'a> {
_ => RustRenameStrategy::None,
};

let ret_type_conversion = if throwable {
ret_type_conversion.map(|c| c.wrap_result())
} else {
ret_type_conversion
};

let analysis = FnAnalysis {
cxxbridge_name: cxxbridge_name.clone(),
rust_name: rust_name.clone(),
Expand All @@ -1444,6 +1462,7 @@ impl<'a> FnAnalyzer<'a> {
requires_unsafe,
vis: vis.into(),
cpp_wrapper,
throwable,
deps,
ignore_reason,
externally_callable,
Expand Down
48 changes: 40 additions & 8 deletions engine/src/conversion/codegen_rs/fun_codegen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pub(super) fn gen_function(
param_details: &param_details,
cxxbridge_name: &cxxbridge_name,
rust_name,
throwable: analysis.throwable,
unsafety: &analysis.requires_unsafe,
always_unsafe_due_to_trait_definition,
doc_attrs: &doc_attrs,
Expand All @@ -143,7 +144,8 @@ pub(super) fn gen_function(
..
} => {
// Constructor.
impl_entry = Some(fn_generator.generate_constructor_impl(impl_for));
impl_entry =
Some(fn_generator.generate_constructor_impl(impl_for, analysis.throwable));
}
FnKind::Method {
ref impl_for,
Expand Down Expand Up @@ -196,6 +198,16 @@ pub(super) fn gen_function(
// which the user has declared.
let params = unqualify_params(params);
let ret_type = unqualify_ret_type(ret_type.into_owned());

let ret_type = if analysis.throwable {
Copy link
Collaborator

@adetaylor adetaylor Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight preference for a single match statement with an arm something like _ if !analysis.throwable => ret_type. (Here and elsewhere). Just to reduce depth of nested statements.

match ret_type {
ReturnType::Default => parse_quote!(-> Result<()>),
ReturnType::Type(_, ty) => parse_quote!(-> Result< #ty >),
}
} else {
ret_type
};

// And we need to make an attribute for the namespace that the function
// itself is in.
let namespace_attr = if ns.is_empty() || wrapper_function_needed {
Expand Down Expand Up @@ -232,6 +244,7 @@ struct FnGenerator<'a> {
param_details: &'a [ArgumentAnalysis],
ret_conversion: &'a Option<TypeConversionPolicy>,
ret_type: &'a ReturnType,
throwable: bool,
cxxbridge_name: &'a Ident,
rust_name: &'a str,
unsafety: &'a UnsafetyNeeded,
Expand Down Expand Up @@ -293,8 +306,14 @@ impl<'a> FnGenerator<'a> {
}
RustParamConversion::ReturnValue { ty } => {
ptr_arg_name = Some(pd.name.to_token_stream());
ret_type = Cow::Owned(parse_quote! {
-> impl autocxx::moveit::new::New<Output = #ty>
ret_type = Cow::Owned(if self.throwable {
parse_quote! {
-> impl autocxx::moveit::new::TryNew<Output = #ty, Error = cxx::Exception>
}
} else {
parse_quote! {
-> impl autocxx::moveit::new::New<Output = #ty>
}
});
arg_list.push(pd.name.to_token_stream());
}
Expand Down Expand Up @@ -369,10 +388,18 @@ impl<'a> FnGenerator<'a> {
));
closure_stmts.push(call_body);
let closure_stmts = maybe_unsafes_to_tokens(closure_stmts, true);
vec![MaybeUnsafeStmt::needs_unsafe(parse_quote! {
autocxx::moveit::new::by_raw(move |#ptr_arg_name| {
#closure_stmts
})
vec![MaybeUnsafeStmt::needs_unsafe(if self.throwable {
parse_quote! {
autocxx::moveit::new::try_by_raw(move |#ptr_arg_name| {
#closure_stmts
})
}
} else {
parse_quote! {
autocxx::moveit::new::by_raw(move |#ptr_arg_name| {
#closure_stmts
})
}
})]
} else {
let mut call_stmts = local_variables;
Expand Down Expand Up @@ -452,8 +479,13 @@ impl<'a> FnGenerator<'a> {
fn generate_constructor_impl(
&self,
impl_block_type_name: &QualifiedName,
throwable: bool,
) -> Box<ImplBlockDetails> {
let ret_type: ReturnType = parse_quote! { -> impl autocxx::moveit::new::New<Output=Self> };
let ret_type: ReturnType = if throwable {
parse_quote! { -> impl autocxx::moveit::new::TryNew<Output=Self, Error = cxx::Exception> }
} else {
parse_quote! { -> impl autocxx::moveit::new::New<Output=Self> }
};
let (lifetime_tokens, wrapper_params, ret_type, call_body) =
self.common_parts(true, &None, Some(ret_type));
let rust_name = make_ident(self.rust_name);
Expand Down
36 changes: 35 additions & 1 deletion engine/src/conversion/codegen_rs/function_wrapper_rs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,17 @@ pub(super) enum RustParamConversion {

impl TypeConversionPolicy {
pub(super) fn rust_conversion(&self, var: Expr, counter: &mut usize) -> RustParamConversion {
match self.rust_conversion {
let mut wrap_result = false;
let rust_conversion =
if let RustConversionType::WrapResult(ref inner) = self.rust_conversion {
wrap_result = true;
inner.as_ref()
} else {
&self.rust_conversion
};

let base_conversion = match rust_conversion {
RustConversionType::WrapResult(_) => panic!("Nested Results are not supported!"),
RustConversionType::None => RustParamConversion::Param {
ty: self.converted_rust_type(),
local_variables: Vec::new(),
Expand Down Expand Up @@ -213,6 +223,30 @@ impl TypeConversionPolicy {
conversion_requires_unsafe: false,
}
}
};

if wrap_result {
match base_conversion {
RustParamConversion::ReturnValue { ty } => RustParamConversion::ReturnValue {
ty: parse_quote!( ::std::result::Result< #ty, ::cxx::Exception> ),
},
RustParamConversion::Param {
ty,
local_variables,
conversion,
conversion_requires_unsafe,
} => {
let ty = parse_quote!( impl autocxx::moveit::new::TryNew<Output = #ty, Error =::cxx::Exception> );
RustParamConversion::Param {
ty,
local_variables,
conversion,
conversion_requires_unsafe,
}
}
}
} else {
base_conversion
}
}
}
Loading
Loading