From 7131ec056af31012e519f988f9db5dbc221c504b Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 11 Sep 2023 18:40:22 -0700 Subject: [PATCH 1/4] Fix deleted destructors. Fixes #829. --- Cargo.toml | 2 +- .../src/conversion/analysis/abstract_types.rs | 59 ++++++++++++++++++- .../conversion/analysis/constructor_deps.rs | 5 +- engine/src/conversion/api.rs | 20 +++++-- engine/src/conversion/codegen_rs/mod.rs | 2 +- engine/src/conversion/convert_error.rs | 2 + integration-tests/tests/integration_test.rs | 53 +++++++++++++++++ 7 files changed, 130 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b256777cc..c401cb686 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,5 +37,5 @@ exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/ #[patch.crates-io] #cxx = { path="../cxx" } #cxx-gen = { path="../cxx/gen/lib" } -#autocxx-bindgen = { path="../bindgen" } +#autocxx-bindgen = { path="../bindgen/bindgen" } #moveit = { path="../moveit" } diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index 884cbd015..0909d1f81 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -18,7 +18,7 @@ use super::{ }; use crate::conversion::{ analysis::{depth_first::fields_and_bases_first, fun::ReceiverMutability}, - api::{ApiName, TypeKind}, + api::{ApiName, CppVisibility, DeletedOrDefaulted, FuncToConvert, TypeKind}, error_reporter::{convert_apis, convert_item_apis}, ConvertErrorFromCpp, }; @@ -147,6 +147,40 @@ pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec = apis + .iter() + .filter_map(|api| match api { + Api::Function { + fun, + analysis: + FnAnalysis { + kind: + FnKind::TraitMethod { + kind: TraitMethodKind::Destructor, + impl_for, + .. + }, + .. + }, + .. + } if matches!( + fun.as_ref(), + FuncToConvert { + cpp_vis: CppVisibility::Private, + .. + } | FuncToConvert { + is_deleted: DeletedOrDefaulted::Deleted, + .. + } + ) => + { + Some(impl_for.clone()) + } + _ => None, + }) + .collect(); + // mark abstract types as abstract let mut apis: ApiVec<_> = apis .into_iter() @@ -154,6 +188,8 @@ pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec) -> ApiVec) -> ApiVec + { + Err(ConvertErrorFromCpp::NonDestructibleNestedType) + } _ => Ok(Box::new(std::iter::once(api))), }); diff --git a/engine/src/conversion/analysis/constructor_deps.rs b/engine/src/conversion/analysis/constructor_deps.rs index 63b6dbe44..b83e26b00 100644 --- a/engine/src/conversion/analysis/constructor_deps.rs +++ b/engine/src/conversion/analysis/constructor_deps.rs @@ -10,7 +10,7 @@ use indexmap::map::IndexMap as HashMap; use crate::{ conversion::{ - api::{Api, ApiName, StructDetails, TypeKind}, + api::{Api, ApiName, StructDetails}, apivec::ApiVec, convert_error::ConvertErrorWithContext, error_reporter::convert_apis, @@ -51,8 +51,7 @@ fn decorate_struct( constructors_and_allocators_by_type: &mut HashMap>, ) -> Result>>, ConvertErrorWithContext> { let pod = fn_struct.pod; - let is_abstract = matches!(pod.kind, TypeKind::Abstract); - let constructor_and_allocator_deps = if is_abstract || pod.is_generic { + let constructor_and_allocator_deps = if !pod.kind.is_instantiable() || pod.is_generic { Vec::new() } else { constructors_and_allocators_by_type diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index d5da41c52..f79247d4c 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -38,13 +38,21 @@ use super::{ #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub(crate) enum TypeKind { - Pod, // trivial. Can be moved and copied in Rust. - NonPod, // has destructor or non-trivial move constructors. Can only hold by UniquePtr + Pod, // trivial. Can be moved and copied in Rust. + NonPod, // has destructor or non-trivial move constructors. Can only hold by UniquePtr Abstract, // has pure virtual members - can't even generate UniquePtr. - // It's possible that the type itself isn't pure virtual, but it inherits from - // some other type which is pure virtual. Alternatively, maybe we just don't - // know if the base class is pure virtual because it wasn't on the allowlist, - // in which case we'll err on the side of caution. + // It's possible that the type itself isn't pure virtual, but it inherits from + // some other type which is pure virtual. Alternatively, maybe we just don't + // know if the base class is pure virtual because it wasn't on the allowlist, + // in which case we'll err on the side of caution. + NotDestructible, // private or deleted destructor. We treat it the same as Abstract. +} + +impl TypeKind { + /// Whether this TypeKind can be instantiated. + pub(crate) fn is_instantiable(&self) -> bool { + !matches!(self, Self::Abstract | Self::NotDestructible) + } } /// C++ visibility. diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index abab612da..df183bf17 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -946,7 +946,7 @@ impl<'a> RsCodeGenerator<'a> { } } } - TypeKind::Abstract => { + TypeKind::Abstract | TypeKind::NotDestructible => { if is_generic { RsCodegenResult::default() } else { diff --git a/engine/src/conversion/convert_error.rs b/engine/src/conversion/convert_error.rs index 855b2359d..50a21493a 100644 --- a/engine/src/conversion/convert_error.rs +++ b/engine/src/conversion/convert_error.rs @@ -105,6 +105,8 @@ pub enum ConvertErrorFromCpp { RustTypeWithAPath(QualifiedName), #[error("This type is nested within another struct/class, yet is abstract (or is not on the allowlist so we can't be sure). This is not yet supported by autocxx. If you don't believe this type is abstract, add it to the allowlist.")] AbstractNestedType, + #[error("This type is nested within another struct/class, yet is not destructible (private or deleted destructor). This is not yet supported by autocxx.")] + NonDestructibleNestedType, #[error("This typedef was nested within another struct/class. autocxx is unable to represent inner types if they might be abstract. Unfortunately, autocxx couldn't prove that this type isn't abstract, so it can't represent it.")] NestedOpaqueTypedef, #[error( diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 7c2cccfe2..749a0ef76 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -12293,6 +12293,59 @@ fn test_cpp_union_pod() { run_test_expect_fail("", hdr, quote! {}, &[], &["CorrelationId_t_"]); } +#[test] +fn test_deleted_destructor() { + let hdr = indoc! {" + #include + class A { + public: + void foo(); + ~A() = delete; + }; + inline A* get_a() { + static A* a = new A(); + return a; + } + "}; + run_test("", hdr, quote! {}, &["A", "get_a"], &[]); + run_test_expect_fail( + "", + hdr, + quote! { + let _ = ffi::A::within_unique_ptr(); + }, + &["A", "get_a"], + &[], + ); +} + +#[test] +fn test_private_destructor() { + let hdr = indoc! {" + #include + class A { + public: + void foo(); + private: + ~A() {}; + }; + inline A* get_a() { + static A* a = new A(); + return a; + } + "}; + run_test("", hdr, quote! {}, &["A", "get_a"], &[]); + run_test_expect_fail( + "", + hdr, + quote! { + let _ = ffi::A::within_unique_ptr(); + }, + &["A", "get_a"], + &[], + ); +} + // Yet to test: // - Ifdef // - Out param pointers From bbdfb26eada7fe440e88ad31d93f95d929bfd3c8 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 28 Feb 2025 15:42:19 +0000 Subject: [PATCH 2/4] Fix merge error. --- integration-tests/tests/integration_test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 707ca3638..29e197b97 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -12437,6 +12437,7 @@ fn test_private_destructor() { &["A", "get_a"], &[], ); +} #[test] fn test_using_string_function() { From 9bf41a8b500408cfc93f6c589bc8a7d31e2de340 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 28 Feb 2025 15:45:58 +0000 Subject: [PATCH 3/4] Merge fixes. --- engine/src/conversion/analysis/abstract_types.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index ba83a27e4..45099b340 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -6,6 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use autocxx_bindgen::callbacks::Explicitness; use indexmap::map::IndexMap as HashMap; use syn::{punctuated::Punctuated, token::Comma}; @@ -16,12 +17,12 @@ use super::{ }, pod::PodAnalysis, }; -use crate::conversion::{ +use crate::{conversion::{ analysis::{depth_first::fields_and_bases_first, fun::ReceiverMutability}, - api::{ApiName, CppVisibility, DeletedOrDefaulted, FuncToConvert, TypeKind}, + api::{ApiName, CppVisibility, FuncToConvert, TypeKind}, error_reporter::{convert_apis, convert_item_apis}, - ConvertErrorFromCpp, -}; + ConvertErrorFromCpp, CppEffectiveName, +}, minisyn::FnArg}; use crate::{ conversion::{api::Api, apivec::ApiVec}, types::QualifiedName, @@ -170,7 +171,7 @@ pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec @@ -256,7 +257,7 @@ pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec { Err(ConvertErrorFromCpp::NonDestructibleNestedType) From 64a5c19d4dc0daa341e5387fb8cc0b2a494684c2 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 28 Feb 2025 15:46:07 +0000 Subject: [PATCH 4/4] Cargo fmt. --- engine/src/conversion/analysis/abstract_types.rs | 15 +++++++++------ integration-tests/tests/integration_test.rs | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index 45099b340..59cc4a0cb 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -17,12 +17,15 @@ use super::{ }, pod::PodAnalysis, }; -use crate::{conversion::{ - analysis::{depth_first::fields_and_bases_first, fun::ReceiverMutability}, - api::{ApiName, CppVisibility, FuncToConvert, TypeKind}, - error_reporter::{convert_apis, convert_item_apis}, - ConvertErrorFromCpp, CppEffectiveName, -}, minisyn::FnArg}; +use crate::{ + conversion::{ + analysis::{depth_first::fields_and_bases_first, fun::ReceiverMutability}, + api::{ApiName, CppVisibility, FuncToConvert, TypeKind}, + error_reporter::{convert_apis, convert_item_apis}, + ConvertErrorFromCpp, CppEffectiveName, + }, + minisyn::FnArg, +}; use crate::{ conversion::{api::Api, apivec::ApiVec}, types::QualifiedName, diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 5ab74eec0..856cd33de 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -12465,7 +12465,7 @@ fn test_private_destructor() { &[], ); } - + #[test] fn test_using_string_function() { let hdr = indoc! {"