Merge pull request #1111 from bsilver8192/subclass-unsafe

Fix and test subclass without `safety!(unsafe)`

Author: adetaylor

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

@@ -270,7 +270,7 @@ enum TypeConversionSophistication {
 }
 
 pub(crate) struct FnAnalyzer<'a> {
-    unsafe_policy: UnsafePolicy,
+    unsafe_policy: &'a UnsafePolicy,
     extra_apis: ApiVec<NullPhase>,
     type_converter: TypeConverter<'a>,
     bridge_name_tracker: BridgeNameTracker,
@@ -288,7 +288,7 @@ pub(crate) struct FnAnalyzer<'a> {
 impl<'a> FnAnalyzer<'a> {
     pub(crate) fn analyze_functions(
         apis: ApiVec<PodPhase>,
-        unsafe_policy: UnsafePolicy,
+        unsafe_policy: &'a UnsafePolicy,
         config: &'a IncludeCppConfig,
     ) -> ApiVec<FnPrePhase2> {
         let mut me = Self {
@@ -476,7 +476,9 @@ impl<'a> FnAnalyzer<'a> {
                 UnsafetyNeeded::Always => UnsafetyNeeded::JustBridge,
                 _ => unsafest_param,
             },
-            _ if self.unsafe_policy == UnsafePolicy::AllFunctionsUnsafe => UnsafetyNeeded::Always,
+            _ if matches!(self.unsafe_policy, UnsafePolicy::AllFunctionsUnsafe) => {
+                UnsafetyNeeded::Always
+            }
             _ => match unsafest_non_placement_param {
                 UnsafetyNeeded::Always => UnsafetyNeeded::Always,
                 UnsafetyNeeded::JustBridge => match unsafest_param {
@@ -638,6 +640,7 @@ impl<'a> FnAnalyzer<'a> {
                     receiver_mutability,
                     sup,
                     subclass_fn_deps,
+                    self.unsafe_policy,
                 ));
 
                 // Create the trait item for the <superclass>_methods and <superclass>_supers
@@ -655,6 +658,7 @@ impl<'a> FnAnalyzer<'a> {
                         receiver_mutability,
                         sup.clone(),
                         is_pure_virtual,
+                        self.unsafe_policy,
                     ));
                 }
             }

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

@@ -10,7 +10,7 @@ use indexmap::map::IndexMap as HashMap;
 
 use syn::{parse_quote, FnArg, PatType, Type, TypePtr};
 
-use crate::conversion::analysis::fun::{FnKind, MethodKind, ReceiverMutability};
+use crate::conversion::analysis::fun::{FnKind, MethodKind, ReceiverMutability, UnsafePolicy};
 use crate::conversion::analysis::pod::PodPhase;
 use crate::conversion::api::{
     CppVisibility, FuncToConvert, Provenance, RustSubclassFnDetails, SubclassConstructorDetails,
@@ -79,12 +79,18 @@ pub(super) fn create_subclass_trait_item(
     receiver_mutability: &ReceiverMutability,
     receiver: QualifiedName,
     is_pure_virtual: bool,
+    unsafe_policy: &UnsafePolicy,
 ) -> Api<FnPrePhase1> {
     let param_names = analysis
         .param_details
         .iter()
         .map(|pd| pd.name.clone())
         .collect();
+    let requires_unsafe = if matches!(unsafe_policy, UnsafePolicy::AllFunctionsUnsafe) {
+        UnsafetyNeeded::Always
+    } else {
+        UnsafetyNeeded::from_param_details(&analysis.param_details, false)
+    };
     Api::SubclassTraitItem {
         name,
         details: SuperclassMethod {
@@ -93,7 +99,7 @@ pub(super) fn create_subclass_trait_item(
             ret_type: analysis.ret_type.clone(),
             param_names,
             receiver_mutability: receiver_mutability.clone(),
-            requires_unsafe: UnsafetyNeeded::from_param_details(&analysis.param_details, false),
+            requires_unsafe,
             is_pure_virtual,
             receiver,
         },
@@ -107,6 +113,7 @@ pub(super) fn create_subclass_function(
     receiver_mutability: &ReceiverMutability,
     superclass: &QualifiedName,
     dependencies: Vec<QualifiedName>,
+    unsafe_policy: &UnsafePolicy,
 ) -> Api<FnPrePhase1> {
     let cpp = sub.cpp();
     let holder_name = sub.holder();
@@ -131,6 +138,11 @@ pub(super) fn create_subclass_function(
         .skip(1)
         .map(|p| p.conversion.clone())
         .collect();
+    let requires_unsafe = if matches!(unsafe_policy, UnsafePolicy::AllFunctionsUnsafe) {
+        UnsafetyNeeded::Always
+    } else {
+        UnsafetyNeeded::from_param_details(&analysis.param_details, false)
+    };
     Api::RustSubclassFn {
         name: ApiName::new_in_root_namespace(rust_call_name.clone()),
         subclass: sub.clone(),
@@ -151,7 +163,7 @@ pub(super) fn create_subclass_function(
             superclass: superclass.clone(),
             receiver_mutability: receiver_mutability.clone(),
             dependencies,
-            requires_unsafe: UnsafetyNeeded::from_param_details(&analysis.param_details, false),
+            requires_unsafe,
             is_pure_virtual: matches!(
                 analysis.kind,
                 FnKind::Method {

engine/src/conversion/codegen_rs/mod.rs

@@ -17,7 +17,7 @@ pub(crate) mod unqualify;
 use indexmap::map::IndexMap as HashMap;
 use indexmap::set::IndexSet as HashSet;
 
-use autocxx_parser::{ExternCppType, IncludeCppConfig, RustFun};
+use autocxx_parser::{ExternCppType, IncludeCppConfig, RustFun, UnsafePolicy};
 
 use itertools::Itertools;
 use proc_macro2::{Span, TokenStream};
@@ -134,6 +134,7 @@ fn get_string_items() -> Vec<Item> {
 /// In practice, much of the "generation" involves connecting together
 /// existing lumps of code within the Api structures.
 pub(crate) struct RsCodeGenerator<'a> {
+    unsafe_policy: &'a UnsafePolicy,
     include_list: &'a [String],
     bindgen_mod: ItemMod,
     original_name_map: CppNameMap,
@@ -145,12 +146,14 @@ impl<'a> RsCodeGenerator<'a> {
     /// Generate code for a set of APIs that was discovered during parsing.
     pub(crate) fn generate_rs_code(
         all_apis: ApiVec<FnPhase>,
+        unsafe_policy: &'a UnsafePolicy,
         include_list: &'a [String],
         bindgen_mod: ItemMod,
         config: &'a IncludeCppConfig,
         header_name: Option<String>,
     ) -> Vec<Item> {
         let c = Self {
+            unsafe_policy,
             include_list,
             bindgen_mod,
             original_name_map: original_name_map_from_apis(&all_apis),
@@ -609,8 +612,11 @@ impl<'a> RsCodeGenerator<'a> {
                 name, superclass, ..
             } => {
                 let methods = associated_methods.get(&superclass);
-                let generate_peer_constructor =
-                    subclasses_with_a_single_trivial_constructor.contains(&name.0.name);
+                let generate_peer_constructor = subclasses_with_a_single_trivial_constructor.contains(&name.0.name) &&
+                    // TODO: Create an UnsafeCppPeerConstructor trait for calling an unsafe
+                    // 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)
             }
             Api::ExternCppType {

engine/src/conversion/mod.rs

@@ -157,7 +157,7 @@ impl<'a> BridgeConverter<'a> {
                 // parameterized by a richer set of metadata.
                 Self::dump_apis("adding casts", &analyzed_apis);
                 let analyzed_apis =
-                    FnAnalyzer::analyze_functions(analyzed_apis, unsafe_policy, self.config);
+                    FnAnalyzer::analyze_functions(analyzed_apis, &unsafe_policy, self.config);
                 // If any of those functions turned out to be pure virtual, don't attempt
                 // to generate UniquePtr implementations for the type, since it can't
                 // be instantiated.
@@ -197,6 +197,7 @@ impl<'a> BridgeConverter<'a> {
                 )?;
                 let rs = RsCodeGenerator::generate_rs_code(
                     analyzed_apis,
+                    &unsafe_policy,
                     self.include_list,
                     bindgen_mod,
                     self.config,

integration-tests/tests/integration_test.rs

@@ -7691,6 +7691,54 @@ fn test_two_superclasses_with_same_name_method() {
     );
 }
 
+#[test]
+fn test_subclass_no_safety() {
+    let hdr = indoc! {"
+    #include <cstdint>
+
+    class Observer {
+    public:
+        Observer() {}
+        virtual void foo() = 0;
+        virtual ~Observer() {}
+    };
+    "};
+    let hexathorpe = Token![#](Span::call_site());
+    let unexpanded_rust = quote! {
+        use autocxx::prelude::*;
+
+        include_cpp!(
+            #hexathorpe include "input.h"
+            subclass!("Observer",MyObserver)
+        );
+
+        use ffi::Observer_methods;
+        #hexathorpe [autocxx::subclass::subclass]
+        pub struct MyObserver;
+        impl Observer_methods for MyObserver {
+            unsafe fn foo(&mut self) {}
+        }
+
+        use autocxx::subclass::{CppSubclass, CppPeerConstructor, CppSubclassRustPeerHolder};
+        use cxx::UniquePtr;
+        impl CppPeerConstructor<ffi::MyObserverCpp> for MyObserver {
+            fn make_peer(
+                &mut self,
+                peer_holder: CppSubclassRustPeerHolder<Self>,
+            ) -> UniquePtr<ffi::MyObserverCpp> {
+                UniquePtr::emplace(unsafe { ffi::MyObserverCpp::new(peer_holder) })
+            }
+        }
+
+        fn main() {
+            let obs = MyObserver::new_rust_owned(MyObserver { cpp_peer: Default::default() });
+            unsafe { obs.borrow_mut().foo() };
+        }
+    };
+
+    do_run_test_manual("", hdr, unexpanded_rust, None, None).unwrap()
+}
+
 #[test]
 fn test_pv_protected_constructor() {
     let hdr = indoc! {"

src/subclass.rs

@@ -207,8 +207,9 @@ pub trait CppPeerConstructor<CppPeer: CppSubclassCppPeer>: Sized {
 /// * You _may_ need to implement [`CppPeerConstructor`] for your subclass,
 ///   but only if autocxx determines that there are multiple possible superclass
 ///   constructors so you need to call one explicitly (or if there's a single
-///   non-trivial superclass constructor.) autocxx will implemente this trait
-///   for you if there's no ambiguity.
+///   non-trivial superclass constructor.) autocxx will implement this trait
+///   for you if there's no ambiguity and FFI functions are safe to call due to
+///   `autocxx::safety!` being used.
 ///
 /// # How to access your Rust structure from outside
 ///