From cdbf5fa4f8210a059d339912156b0b40d89dee50 Mon Sep 17 00:00:00 2001 From: Thomas Hebb Date: Mon, 7 Apr 2025 23:00:11 -0400 Subject: [PATCH 1/4] Synthesize subclass constructor when superclass destructor is protected Currently, we only synthesize a default constructor when the superclass destructor is public. This is to comply with 11.4.5.2 of the C++ standard[1], which states that a default constructor is deleted if "any potentially constructed subobject [e.g. the virtual base class] has a type with a destructor that is deleted or inaccessible from the defaulted default constructor". We correctly exclude private base class destructors, which are not accessible from the subclass's constructor. But we also exclude protected base class destructors, which are! Fix that by matching protected superclass destructors too. [1] https://github.com/cplusplus/draft/releases/tag/n4917 --- engine/src/conversion/analysis/fun/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 6ad07ef2c..e70400a0d 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -523,7 +523,9 @@ impl<'a> FnAnalyzer<'a> { FuncToConvert { special_member: Some(SpecialMemberKind::Destructor), is_deleted: None | Some(Explicitness::Defaulted), - cpp_vis: CppVisibility::Public, + // Both public and protected destructors are accessible + // from a subclass's default constructor. + cpp_vis: CppVisibility::Public | CppVisibility::Protected, .. } ) => From 28bdbbba6f2c0e1be15bd4d3465cea22594679a2 Mon Sep 17 00:00:00 2001 From: Thomas Hebb Date: Mon, 7 Apr 2025 23:13:38 -0400 Subject: [PATCH 2/4] Add analysis field to Api::Subclass It's exactly like what we already have for Enum, Typedef, Function, and Struct. This commit just makes the boring structural changes: the field itself is still empty. A future commit will use it to track superclass destructor visibility. --- .../src/conversion/analysis/abstract_types.rs | 1 + .../conversion/analysis/constructor_deps.rs | 1 + engine/src/conversion/analysis/deps.rs | 2 ++ engine/src/conversion/analysis/fun/mod.rs | 14 ++++++++++++++ .../src/conversion/analysis/fun/subclass.rs | 5 ++++- engine/src/conversion/analysis/name_check.rs | 1 + engine/src/conversion/analysis/pod/mod.rs | 3 +++ engine/src/conversion/analysis/tdef.rs | 2 ++ engine/src/conversion/api.rs | 18 ++++++++++++++++++ engine/src/conversion/codegen_cpp/mod.rs | 4 +++- engine/src/conversion/error_reporter.rs | 19 +++++++++++++------ engine/src/conversion/parse/parse_bindgen.rs | 1 + 12 files changed, 63 insertions(+), 8 deletions(-) diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index 7a9986f38..b0b81b18c 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -234,6 +234,7 @@ pub(crate) fn discard_ignored_functions(apis: ApiVec) -> ApiVec) -> }, Api::enum_unchanged, Api::typedef_unchanged, + Api::subclass_unchanged, ); results } diff --git a/engine/src/conversion/analysis/deps.rs b/engine/src/conversion/analysis/deps.rs index 3fb00eede..6ce048ee6 100644 --- a/engine/src/conversion/analysis/deps.rs +++ b/engine/src/conversion/analysis/deps.rs @@ -39,6 +39,7 @@ impl HasDependencies for Api { Api::Subclass { name: _, superclass, + .. } => Box::new(std::iter::once(superclass)), Api::RustSubclassFn { details, .. } => Box::new(details.dependencies.iter()), Api::RustFn { deps, .. } => Box::new(deps.iter()), @@ -88,6 +89,7 @@ impl HasDependencies for Api { Api::Subclass { name: _, superclass, + .. } => Box::new(std::iter::once(superclass)), Api::RustSubclassFn { details, .. } => Box::new(details.dependencies.iter()), Api::RustFn { deps, .. } => Box::new(deps.iter()), diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index e70400a0d..aec80027d 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -219,6 +219,9 @@ pub(crate) struct PodAndConstructorAnalysis { pub(crate) constructors: PublicConstructors, } +#[derive(std::fmt::Debug)] +pub(crate) struct SubclassAnalysis {} + /// An analysis phase where we've analyzed each function, but /// haven't yet determined which constructors/etc. belong to each type. #[derive(std::fmt::Debug)] @@ -228,6 +231,7 @@ impl AnalysisPhase for FnPrePhase1 { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = PodAnalysis; type FunAnalysis = FnAnalysis; + type SubclassAnalysis = (); } /// An analysis phase where we've analyzed each function, and identified @@ -239,6 +243,7 @@ impl AnalysisPhase for FnPrePhase2 { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = PodAndConstructorAnalysis; type FunAnalysis = FnAnalysis; + type SubclassAnalysis = SubclassAnalysis; } #[derive(Debug)] @@ -273,6 +278,7 @@ impl AnalysisPhase for FnPhase { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = PodAndDepAnalysis; type FunAnalysis = FnAnalysis; + type SubclassAnalysis = SubclassAnalysis; } /// Whether to allow highly optimized calls because this is a simple Rust->C++ call, @@ -332,6 +338,7 @@ impl<'a> FnAnalyzer<'a> { Api::struct_unchanged, Api::enum_unchanged, Api::typedef_unchanged, + Api::subclass_unchanged, ); let mut results = me.add_constructors_present(results); me.add_subclass_constructors(&mut results); @@ -2127,6 +2134,13 @@ impl<'a> FnAnalyzer<'a> { }, Api::enum_unchanged, Api::typedef_unchanged, + |name, superclass, analysis| { + Ok(Box::new(std::iter::once(Api::Subclass { + name, + superclass, + analysis: SubclassAnalysis {}, + }))) + }, ); results } diff --git a/engine/src/conversion/analysis/fun/subclass.rs b/engine/src/conversion/analysis/fun/subclass.rs index 1e253357d..692b62ee9 100644 --- a/engine/src/conversion/analysis/fun/subclass.rs +++ b/engine/src/conversion/analysis/fun/subclass.rs @@ -40,7 +40,10 @@ pub(super) fn subclasses_by_superclass( let mut subclasses_per_superclass: HashMap> = HashMap::new(); for api in apis.iter() { - if let Api::Subclass { name, superclass } = api { + if let Api::Subclass { + name, superclass, .. + } = api + { subclasses_per_superclass .entry(superclass.clone()) .or_default() diff --git a/engine/src/conversion/analysis/name_check.rs b/engine/src/conversion/analysis/name_check.rs index 966647909..3f7326732 100644 --- a/engine/src/conversion/analysis/name_check.rs +++ b/engine/src/conversion/analysis/name_check.rs @@ -50,6 +50,7 @@ pub(crate) fn check_names(apis: ApiVec) -> ApiVec { Api::Subclass { name: SubclassName(ref name), ref superclass, + .. } => { validate_all_segments_ok_for_cxx(name.name.segment_iter())?; validate_all_segments_ok_for_cxx(superclass.segment_iter())?; diff --git a/engine/src/conversion/analysis/pod/mod.rs b/engine/src/conversion/analysis/pod/mod.rs index 47baade93..594bcd863 100644 --- a/engine/src/conversion/analysis/pod/mod.rs +++ b/engine/src/conversion/analysis/pod/mod.rs @@ -66,6 +66,7 @@ impl AnalysisPhase for PodPhase { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = PodAnalysis; type FunAnalysis = (); + type SubclassAnalysis = (); } /// In our set of APIs, work out which ones are safe to represent @@ -103,6 +104,7 @@ pub(crate) fn analyze_pod_apis( }, |name, item| analyze_enum(name, item, parse_callback_results), Api::typedef_unchanged, + Api::subclass_unchanged, ); // Conceivably, the process of POD-analysing the first set of APIs could result // in us creating new APIs to concretize generic types. @@ -125,6 +127,7 @@ pub(crate) fn analyze_pod_apis( }, |name, item| analyze_enum(name, item, parse_callback_results), Api::typedef_unchanged, + Api::subclass_unchanged, ); assert!(more_extra_apis.is_empty()); Ok(results) diff --git a/engine/src/conversion/analysis/tdef.rs b/engine/src/conversion/analysis/tdef.rs index b639263b3..18cefcc4a 100644 --- a/engine/src/conversion/analysis/tdef.rs +++ b/engine/src/conversion/analysis/tdef.rs @@ -40,6 +40,7 @@ impl AnalysisPhase for TypedefPhase { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = (); type FunAnalysis = (); + type SubclassAnalysis = (); } #[allow(clippy::needless_collect)] // we need the extra collect because the closure borrows extra_apis @@ -78,6 +79,7 @@ pub(crate) fn convert_typedef_targets( }, }))) }, + Api::subclass_unchanged, ); results.extend(extra_apis.into_iter().map(add_analysis)); results diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index 80c8b203c..0464375ea 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -193,6 +193,7 @@ pub(crate) trait AnalysisPhase: std::fmt::Debug { type TypedefAnalysis: std::fmt::Debug; type StructAnalysis: std::fmt::Debug; type FunAnalysis: std::fmt::Debug; + type SubclassAnalysis: std::fmt::Debug; } /// No analysis has been applied to this API. @@ -203,6 +204,7 @@ impl AnalysisPhase for NullPhase { type TypedefAnalysis = (); type StructAnalysis = (); type FunAnalysis = (); + type SubclassAnalysis = (); } #[derive(Clone, Debug)] @@ -449,6 +451,7 @@ pub(crate) enum Api { Subclass { name: SubclassName, superclass: QualifiedName, + analysis: T::SubclassAnalysis, }, /// Contributions to the traits representing superclass methods that we might /// subclass in Rust. @@ -610,4 +613,19 @@ impl Api { { Ok(Box::new(std::iter::once(Api::Enum { name, item }))) } + + pub(crate) fn subclass_unchanged( + name: SubclassName, + superclass: QualifiedName, + analysis: T::SubclassAnalysis, + ) -> Result>>, ConvertErrorWithContext> + where + T: 'static, + { + Ok(Box::new(std::iter::once(Api::Subclass { + name, + superclass, + analysis, + }))) + } } diff --git a/engine/src/conversion/codegen_cpp/mod.rs b/engine/src/conversion/codegen_cpp/mod.rs index 02c494f95..51865185c 100644 --- a/engine/src/conversion/codegen_cpp/mod.rs +++ b/engine/src/conversion/codegen_cpp/mod.rs @@ -212,7 +212,9 @@ impl<'a> CppCodeGenerator<'a> { for api in deferred_apis.into_iter() { match api { - Api::Subclass { name, superclass } => self.generate_subclass( + Api::Subclass { + name, superclass, .. + } => self.generate_subclass( superclass, name, constructors_by_subclass.remove(name).unwrap_or_default(), diff --git a/engine/src/conversion/error_reporter.rs b/engine/src/conversion/error_reporter.rs index c5555a475..89dc5214f 100644 --- a/engine/src/conversion/error_reporter.rs +++ b/engine/src/conversion/error_reporter.rs @@ -9,7 +9,7 @@ use crate::minisyn::ItemEnum; use super::{ - api::{AnalysisPhase, Api, ApiName, FuncToConvert, StructDetails, TypedefKind}, + api::{AnalysisPhase, Api, ApiName, FuncToConvert, StructDetails, SubclassName, TypedefKind}, apivec::ApiVec, convert_error::{ConvertErrorWithContext, ErrorContext}, ConvertErrorFromCpp, @@ -55,13 +55,14 @@ where /// Run some code which generates an API. Add that API, or if /// anything goes wrong, instead add a note of the problem in our /// output API such that users will see documentation for the problem. -pub(crate) fn convert_apis( +pub(crate) fn convert_apis( in_apis: ApiVec, out_apis: &mut ApiVec, mut func_conversion: FF, mut struct_conversion: SF, mut enum_conversion: EF, mut typedef_conversion: TF, + mut subclass_conversion: SCF, ) where A: AnalysisPhase, B: AnalysisPhase + 'static, @@ -85,6 +86,11 @@ pub(crate) fn convert_apis( Option, A::TypedefAnalysis, ) -> Result>>, ConvertErrorWithContext>, + SCF: FnMut( + SubclassName, + QualifiedName, + A::SubclassAnalysis, + ) -> Result>>, ConvertErrorWithContext>, { out_apis.extend(in_apis.into_iter().flat_map(|api| { let fullname = api.name_info().clone(); @@ -142,10 +148,6 @@ pub(crate) fn convert_apis( subclass, details, }))), - Api::Subclass { name, superclass } => Ok(Box::new(std::iter::once(Api::Subclass { - name, - superclass, - }))), Api::IgnoredItem { name, err, ctx } => { Ok(Box::new(std::iter::once(Api::IgnoredItem { name, @@ -184,6 +186,11 @@ pub(crate) fn convert_apis( details, analysis, } => struct_conversion(name, details, analysis), + Api::Subclass { + name, + superclass, + analysis, + } => subclass_conversion(name, superclass, analysis), }; api_or_error(fullname, result) })) diff --git a/engine/src/conversion/parse/parse_bindgen.rs b/engine/src/conversion/parse/parse_bindgen.rs index 513278a18..8e217abd0 100644 --- a/engine/src/conversion/parse/parse_bindgen.rs +++ b/engine/src/conversion/parse/parse_bindgen.rs @@ -108,6 +108,7 @@ impl<'a> ParseBindgen<'a> { .extend(self.config.subclasses.iter().map(|sc| Api::Subclass { name: SubclassName::new(sc.subclass.clone().into()), superclass: QualifiedName::new_from_cpp_name(&sc.superclass), + analysis: (), })); for fun in &self.config.extern_rust_funs { let id = fun.sig.ident.clone(); From c649e4ca25a1176010f43393f1d69a1f8a6103e8 Mon Sep 17 00:00:00 2001 From: Thomas Hebb Date: Tue, 8 Apr 2025 00:20:27 -0400 Subject: [PATCH 3/4] Omit unique_ptr upcasts when superclass has inaccessible destructor Currently, we emit as__unique_ptr helpers for all Rust subclasses. But this breaks subclasses derived from classes with private or protected destructors, since unique_ptr needs to be able to destruct the object it references. Fix that by omitting those helpers when we know the superclass destructor is inaccessible. The core changes are in codegen_rs and codegen_cpp; everything else is just plumbing to get information about superclass destructor visibility where it needs to be. --- .../src/conversion/analysis/abstract_types.rs | 4 +- .../conversion/analysis/constructor_deps.rs | 6 +- engine/src/conversion/analysis/fun/mod.rs | 100 ++++++++++++++---- engine/src/conversion/codegen_cpp/mod.rs | 28 +++-- engine/src/conversion/codegen_rs/mod.rs | 43 +++++--- 5 files changed, 130 insertions(+), 51 deletions(-) diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index b0b81b18c..2f6401d20 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -11,7 +11,7 @@ use syn::{punctuated::Punctuated, token::Comma}; use super::{ fun::{ - FnAnalysis, FnKind, FnPhase, FnPrePhase2, MethodKind, PodAndConstructorAnalysis, + FnAnalysis, FnKind, FnPhase, FnPrePhase3, MethodKind, PodAndConstructorAnalysis, TraitMethodKind, }, pod::PodAnalysis, @@ -63,7 +63,7 @@ impl Signature { } /// Spot types with pure virtual functions and mark them abstract. -pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec { +pub(crate) fn mark_types_abstract(apis: ApiVec) -> ApiVec { #[derive(Default, Debug, Clone)] struct ClassAbstractState { undefined: HashSet, diff --git a/engine/src/conversion/analysis/constructor_deps.rs b/engine/src/conversion/analysis/constructor_deps.rs index 7063ceaaa..c11eeaa7c 100644 --- a/engine/src/conversion/analysis/constructor_deps.rs +++ b/engine/src/conversion/analysis/constructor_deps.rs @@ -19,7 +19,7 @@ use crate::{ }; use super::fun::{ - FnAnalysis, FnKind, FnPhase, FnPrePhase2, PodAndConstructorAnalysis, PodAndDepAnalysis, + FnAnalysis, FnKind, FnPhase, FnPrePhase3, PodAndConstructorAnalysis, PodAndDepAnalysis, TraitMethodKind, }; @@ -28,7 +28,7 @@ use super::fun::{ /// which will later be used as edges in the garbage collection, because /// typically any use of a type will require us to call its copy or move /// constructor. The same applies to its alloc/free functions. -pub(crate) fn decorate_types_with_constructor_deps(apis: ApiVec) -> ApiVec { +pub(crate) fn decorate_types_with_constructor_deps(apis: ApiVec) -> ApiVec { let mut constructors_and_allocators_by_type = find_important_constructors(&apis); let mut results = ApiVec::new(); convert_apis( @@ -72,7 +72,7 @@ fn decorate_struct( } fn find_important_constructors( - apis: &ApiVec, + apis: &ApiVec, ) -> HashMap> { let mut results: HashMap> = HashMap::new(); for api in apis.iter() { diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index aec80027d..d082d8d99 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -220,7 +220,9 @@ pub(crate) struct PodAndConstructorAnalysis { } #[derive(std::fmt::Debug)] -pub(crate) struct SubclassAnalysis {} +pub(crate) struct SubclassAnalysis { + pub(crate) superclass_destructor_visibility: Option, +} /// An analysis phase where we've analyzed each function, but /// haven't yet determined which constructors/etc. belong to each type. @@ -240,6 +242,18 @@ impl AnalysisPhase for FnPrePhase1 { pub(crate) struct FnPrePhase2; impl AnalysisPhase for FnPrePhase2 { + type TypedefAnalysis = TypedefAnalysis; + type StructAnalysis = PodAndConstructorAnalysis; + type FunAnalysis = FnAnalysis; + type SubclassAnalysis = (); +} + +/// An analysis phase where we've annotated each subclass with its superclass's +/// destructor visibility. +#[derive(std::fmt::Debug)] +pub(crate) struct FnPrePhase3; + +impl AnalysisPhase for FnPrePhase3 { type TypedefAnalysis = TypedefAnalysis; type StructAnalysis = PodAndConstructorAnalysis; type FunAnalysis = FnAnalysis; @@ -313,7 +327,7 @@ impl<'a> FnAnalyzer<'a> { unsafe_policy: &'a UnsafePolicy, config: &'a IncludeCppConfig, force_wrapper_generation: bool, - ) -> ApiVec { + ) -> ApiVec { let mut me = Self { unsafe_policy, extra_apis: ApiVec::new(), @@ -340,8 +354,8 @@ impl<'a> FnAnalyzer<'a> { Api::typedef_unchanged, Api::subclass_unchanged, ); - let mut results = me.add_constructors_present(results); - me.add_subclass_constructors(&mut results); + let results = me.add_constructors_present(results); + let mut results = me.add_subclass_constructors(results); results.extend(me.extra_apis.into_iter().map(add_analysis)); results } @@ -509,12 +523,12 @@ impl<'a> FnAnalyzer<'a> { } } - fn add_subclass_constructors(&mut self, apis: &mut ApiVec) { + fn add_subclass_constructors(&mut self, apis: ApiVec) -> ApiVec { let mut results = ApiVec::new(); - // Pre-assemble a list of types with known destructors, to avoid having to - // do a O(n^2) nested loop. - let types_with_destructors: HashSet<_> = apis + // Pre-assemble a list of superclass destructor visibility, to avoid + // having to do a O(n^2) nested loop. + let destructor_visibility_by_class: HashMap<_, _> = apis .iter() .filter_map(|api| match api { Api::Function { @@ -530,18 +544,14 @@ impl<'a> FnAnalyzer<'a> { FuncToConvert { special_member: Some(SpecialMemberKind::Destructor), is_deleted: None | Some(Explicitness::Defaulted), - // Both public and protected destructors are accessible - // from a subclass's default constructor. - cpp_vis: CppVisibility::Public | CppVisibility::Protected, .. } ) => { - Some(impl_for) + Some((impl_for.clone(), fun.cpp_vis)) } _ => None, }) - .cloned() .collect(); for api in apis.iter() { @@ -561,8 +571,12 @@ impl<'a> FnAnalyzer<'a> { } = api { // If we don't have an accessible destructor, then std::unique_ptr cannot be - // instantiated for this C++ type. - if !types_with_destructors.contains(sup) { + // instantiated for this C++ type. Both public and protected destructors are + // accessible from a subclass's default constructor. + if !matches!( + destructor_visibility_by_class.get(sup), + Some(CppVisibility::Public | CppVisibility::Protected) + ) { continue; } @@ -580,7 +594,28 @@ impl<'a> FnAnalyzer<'a> { } } } - apis.extend(results.into_iter()); + + convert_apis( + apis, + &mut results, + Api::fun_unchanged, + Api::struct_unchanged, + Api::enum_unchanged, + Api::typedef_unchanged, + |name, superclass, _| { + Ok(Box::new(std::iter::once(Api::Subclass { + name, + analysis: SubclassAnalysis { + superclass_destructor_visibility: destructor_visibility_by_class + .get(&superclass) + .copied(), + }, + superclass, + }))) + }, + ); + + results } /// Analyze a given function, and any permutations of that function which @@ -2134,13 +2169,7 @@ impl<'a> FnAnalyzer<'a> { }, Api::enum_unchanged, Api::typedef_unchanged, - |name, superclass, analysis| { - Ok(Box::new(std::iter::once(Api::Subclass { - name, - superclass, - analysis: SubclassAnalysis {}, - }))) - }, + Api::subclass_unchanged, ); results } @@ -2383,6 +2412,31 @@ impl HasFieldsAndBases for Api { } } +impl HasFieldsAndBases for Api { + fn name(&self) -> &QualifiedName { + self.name() + } + + fn field_and_base_deps(&self) -> Box + '_> { + match self { + Api::Struct { + analysis: + PodAndConstructorAnalysis { + pod: + PodAnalysis { + field_definition_deps, + bases, + .. + }, + .. + }, + .. + } => Box::new(field_definition_deps.iter().chain(bases.iter())), + _ => Box::new(std::iter::empty()), + } + } +} + /// Stringify a function argument for diagnostics fn describe_arg(arg: &syn::FnArg) -> String { match arg { diff --git a/engine/src/conversion/codegen_cpp/mod.rs b/engine/src/conversion/codegen_cpp/mod.rs index 51865185c..a99b12e9c 100644 --- a/engine/src/conversion/codegen_cpp/mod.rs +++ b/engine/src/conversion/codegen_cpp/mod.rs @@ -26,7 +26,7 @@ use super::{ analysis::{ fun::{ function_wrapper::{CppFunction, CppFunctionBody}, - FnPhase, PodAndDepAnalysis, + FnPhase, PodAndDepAnalysis, SubclassAnalysis, }, pod::PodAnalysis, }, @@ -34,6 +34,7 @@ use super::{ apivec::ApiVec, ConvertErrorFromCpp, CppEffectiveName, }; +use autocxx_bindgen::callbacks::Visibility as CppVisibility; static GENERATED_FILE_HEADER: &str = "// Generated using autocxx - do not edit directly.\n// @generated.\n\n"; @@ -213,9 +214,15 @@ impl<'a> CppCodeGenerator<'a> { for api in deferred_apis.into_iter() { match api { Api::Subclass { - name, superclass, .. + name, + superclass, + analysis: + SubclassAnalysis { + superclass_destructor_visibility, + }, } => self.generate_subclass( superclass, + superclass_destructor_visibility, name, constructors_by_subclass.remove(name).unwrap_or_default(), methods_by_subclass.remove(name).unwrap_or_default(), @@ -657,6 +664,7 @@ impl<'a> CppCodeGenerator<'a> { fn generate_subclass( &mut self, superclass: &QualifiedName, + superclass_destructor_visibility: &Option, subclass: &SubclassName, constructors: Vec<&CppFunction>, methods: Vec, @@ -711,13 +719,15 @@ impl<'a> CppCodeGenerator<'a> { method_decls.push(format!( "{super_name}& As_{super_name}_mut() {{ return *this; }}" )); - self.additional_functions.push(ExtraCpp { - declaration: Some(format!( - "inline std::unique_ptr<{}> {}_As_{}_UniquePtr(std::unique_ptr<{}> u) {{ return std::unique_ptr<{}>(u.release()); }}", - superclass.to_cpp_name(), subclass.cpp(), super_name, subclass.cpp(), superclass.to_cpp_name(), - )), - ..Default::default() - }); + if let Some(CppVisibility::Public) = superclass_destructor_visibility { + self.additional_functions.push(ExtraCpp { + declaration: Some(format!( + "inline std::unique_ptr<{}> {}_As_{}_UniquePtr(std::unique_ptr<{}> u) {{ return std::unique_ptr<{}>(u.release()); }}", + superclass.to_cpp_name(), subclass.cpp(), super_name, subclass.cpp(), superclass.to_cpp_name(), + )), + ..Default::default() + }); + } // And now constructors let mut constructor_decls: Vec = Vec::new(); for constructor in constructors { diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index dc169ee3d..c2741463a 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -42,7 +42,7 @@ use self::{ use super::{ analysis::{ - fun::{FnPhase, PodAndDepAnalysis, ReceiverMutability}, + fun::{FnPhase, PodAndDepAnalysis, ReceiverMutability, SubclassAnalysis}, pod::PodAnalysis, }, api::{AnalysisPhase, Api, SubclassName, TypeKind}, @@ -55,6 +55,7 @@ use super::{ codegen_cpp::type_to_cpp::CppNameMap, }; use super::{convert_error::ErrorContext, ConvertErrorFromCpp}; +use autocxx_bindgen::callbacks::Visibility as CppVisibility; use quote::quote; /// An entry which needs to go into an `impl` block for a given type. @@ -512,7 +513,12 @@ impl<'a> RsCodeGenerator<'a> { details, subclass, .. } => Self::generate_subclass_fn(id.into(), *details, subclass), Api::Subclass { - name, superclass, .. + name, + superclass, + analysis: + SubclassAnalysis { + superclass_destructor_visibility, + }, } => { let methods = associated_methods.get(&superclass); let generate_peer_constructor = subclasses_with_a_single_trivial_constructor.contains(&name.0.name) && @@ -520,7 +526,13 @@ impl<'a> RsCodeGenerator<'a> { // constructor instead? Need to create unsafe versions of everything that uses // it too. matches!(self.unsafe_policy, UnsafePolicy::AllFunctionsSafe); - self.generate_subclass(name, &superclass, methods, generate_peer_constructor) + self.generate_subclass( + name, + &superclass, + superclass_destructor_visibility, + methods, + generate_peer_constructor, + ) } Api::ExternCppType { details: ExternCppType { rust_path, .. }, @@ -539,6 +551,7 @@ impl<'a> RsCodeGenerator<'a> { &self, sub: SubclassName, superclass: &QualifiedName, + superclass_destructor_visibility: Option, methods: Option<&Vec>, generate_peer_constructor: bool, ) -> RsCodegenResult { @@ -629,10 +642,6 @@ impl<'a> RsCodeGenerator<'a> { extern_c_mod_items.push(parse_quote! { fn #as_mut_id(self: Pin<&mut #cpp_id>) -> Pin<&mut #super_cxxxbridge_id>; }); - let as_unique_ptr_id = make_ident(format!("{cpp_id}_As_{super_name}_UniquePtr")); - extern_c_mod_items.push(parse_quote! { - fn #as_unique_ptr_id(u: UniquePtr<#cpp_id>) -> UniquePtr<#super_cxxxbridge_id>; - }); output_mod_items.push(parse_quote! { impl AsRef<#super_path> for super::#id { fn as_ref(&self) -> &cxxbridge::#super_cxxxbridge_id { @@ -650,14 +659,20 @@ impl<'a> RsCodeGenerator<'a> { } } }); - let rs_as_unique_ptr_id = make_ident(format!("as_{super_name}_unique_ptr")); - output_mod_items.push(parse_quote! { - impl super::#id { - pub fn #rs_as_unique_ptr_id(u: cxx::UniquePtr<#cpp_id>) -> cxx::UniquePtr { - cxxbridge::#as_unique_ptr_id(u) + if let Some(CppVisibility::Public) = superclass_destructor_visibility { + let as_unique_ptr_id = make_ident(format!("{cpp_id}_As_{super_name}_UniquePtr")); + extern_c_mod_items.push(parse_quote! { + fn #as_unique_ptr_id(u: UniquePtr<#cpp_id>) -> UniquePtr<#super_cxxxbridge_id>; + }); + let rs_as_unique_ptr_id = make_ident(format!("as_{super_name}_unique_ptr")); + output_mod_items.push(parse_quote! { + impl super::#id { + pub fn #rs_as_unique_ptr_id(u: cxx::UniquePtr<#cpp_id>) -> cxx::UniquePtr { + cxxbridge::#as_unique_ptr_id(u) + } } - } - }); + }); + } let remove_ownership = sub.remove_ownership(); global_items.push(parse_quote! { #[allow(non_snake_case)] From b1d91c1350407df0b5b073c4ad58c993b3edd386 Mon Sep 17 00:00:00 2001 From: Thomas Hebb Date: Tue, 8 Apr 2025 01:24:44 -0400 Subject: [PATCH 4/4] Add protected superclass destructor test --- integration-tests/tests/integration_test.rs | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 9d6590023..12b8c119e 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -8281,6 +8281,46 @@ fn test_pv_protected_constructor() { ); } +#[test] +fn test_pv_protected_destructor() { + let hdr = indoc! {" + #include + + class Observer { + public: + Observer() {} + virtual void foo() const {} + protected: + virtual ~Observer() {} + }; + inline void bar() {} + "}; + run_test_ex( + "", + hdr, + quote! { + let obs = MyObserver::new_rust_owned(MyObserver { a: 3, cpp_peer: Default::default() }); + obs.borrow().foo(); + }, + quote! { + generate!("bar") + subclass!("Observer",MyObserver) + }, + None, + None, + Some(quote! { + use autocxx::subclass::CppSubclass; + use ffi::Observer_methods; + #[autocxx::subclass::subclass] + pub struct MyObserver { + a: u32 + } + impl Observer_methods for MyObserver { + } + }), + ); +} + #[test] fn test_pv_protected_method() { let hdr = indoc! {"