Merge pull request #1464 from google/opaque-directive

Introduce 'opaque' directive.

Author: adetaylor

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

@@ -12,12 +12,11 @@ use indexmap::{map::Entry, set::IndexSet as HashSet};
 
 use syn::{PatType, Type, TypeArray};
 
+use crate::conversion::analysis::type_converter::TypeKind;
 use crate::conversion::type_helpers::type_is_reference;
 use crate::{
     conversion::{
-        analysis::{
-            depth_first::fields_and_bases_first, pod::PodAnalysis, type_converter::TypeKind,
-        },
+        analysis::{depth_first::fields_and_bases_first, pod::PodAnalysis},
         api::{Api, ApiName, FuncToConvert},
         apivec::ApiVec,
         convert_error::ConvertErrorWithContext,
@@ -205,6 +204,11 @@ pub(super) fn find_constructors_present(
             name,
             analysis:
                 PodAnalysis {
+                    // Do not include TypeKind::Opaque here
+                    kind:
+                        crate::conversion::api::TypeKind::Abstract
+                        | crate::conversion::api::TypeKind::Pod
+                        | crate::conversion::api::TypeKind::NonPod,
                     bases,
                     field_info,
                     num_generics: 0usize,

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

@@ -36,6 +36,7 @@ use super::tdef::{TypedefAnalysis, TypedefPhase};
 pub(crate) struct FieldInfo {
     pub(crate) ty: Type,
     pub(crate) type_kind: type_converter::TypeKind,
+    pub(crate) bindgen_opaque_data: bool,
 }
 
 #[derive(std::fmt::Debug)]
@@ -162,7 +163,9 @@ fn analyze_struct(
         &mut field_info,
         extra_apis,
     );
-    let type_kind = if byvalue_checker.is_pod(&name.name) {
+    let type_kind = if field_info.iter().any(|fi| fi.bindgen_opaque_data) {
+        TypeKind::Opaque
+    } else if byvalue_checker.is_pod(&name.name) {
         // It's POD so any errors encountered parsing its fields are important.
         // Let's not allow anything to be POD if it's got rvalue reference fields.
         if details.has_rvalue_reference_fields {
@@ -253,6 +256,11 @@ fn get_struct_field_types(
                     field_info.push(FieldInfo {
                         ty: r.ty,
                         type_kind: r.kind,
+                        bindgen_opaque_data: f
+                            .ident
+                            .as_ref()
+                            .map(|id| id == "_bindgen_opaque_blob")
+                            .unwrap_or_default(),
                     });
                 }
             }

engine/src/conversion/api.rs

@@ -40,11 +40,15 @@ use super::{
 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
+    Opaque, // bindgen opaque data. We can't generate any constructors as we don't
+    // know what fields it has.
+    // NB you might not find references to this in the codebase - that's
+    // because `implicit_constructor.rs` acts on all the other types of TypeKind
     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.
 }
 
 /// Details about a C++ struct.

engine/src/conversion/codegen_rs/mod.rs

@@ -780,7 +780,7 @@ impl<'a> RsCodeGenerator<'a> {
         // b) generate a concrete type definition, e.g. by using bindgen's
         //    or doing our own, and then telling cxx 'type A = bindgen::A';'
         match type_kind {
-            TypeKind::Pod | TypeKind::NonPod => {
+            TypeKind::Pod | TypeKind::NonPod | TypeKind::Opaque => {
                 // Feed cxx "type T = root::bindgen::T"
                 // For non-POD types, there might be the option of simply giving
                 // cxx a "type T;" as we do for abstract types below. There's

engine/src/lib.rs

@@ -382,6 +382,10 @@ impl IncludeCppEngine {
             }
         }
 
+        for item in &self.config.opaquelist {
+            builder = builder.opaque_type(item);
+        }
+
         // At this point it woul be great to use `Builder::opaque_type` for
         // everything which is on the allowlist but not on the POD list.
         // This would free us from a large proportion of bindgen bugs which

integration-tests/tests/integration_test.rs

@@ -12495,6 +12495,38 @@ fn test_class_named_string() {
     run_test("", hdr, quote! {}, &["a::String"], &[]);
 }
 
+#[test]
+fn test_opaque_directive() {
+    let hdr = indoc! {"
+        #include <memory>
+        class Foo {
+        public:
+            int a;
+        };
+        Foo global_foo;
+        class Bar {
+        public:
+            const Foo& get_foo() const { return global_foo; }
+        };
+    "};
+    let rs = quote! {
+        use autocxx::prelude::*;
+        let _ = ffi::Bar::new().within_unique_ptr().get_foo();
+    };
+    run_test_ex(
+        "",
+        hdr,
+        rs,
+        quote! {
+            generate!("Bar")
+            opaque!("Foo")
+        },
+        None,
+        None,
+        None,
+    );
+}
+
 // Yet to test:
 // - Ifdef
 // - Out param pointers

parser/src/config.rs

@@ -227,6 +227,7 @@ pub struct IncludeCppConfig {
     pub extern_rust_funs: Vec<RustFun>,
     pub concretes: ConcretesMap,
     pub externs: ExternCppTypeMap,
+    pub opaquelist: Vec<String>,
 }
 
 impl Parse for IncludeCppConfig {
@@ -387,6 +388,10 @@ impl IncludeCppConfig {
         self.blocklist.iter()
     }
 
+    pub fn get_opaquelist(&self) -> impl Iterator<Item = &String> {
+        self.opaquelist.iter()
+    }
+
     fn is_concrete_type(&self, cpp_name: &str) -> bool {
         self.concretes.0.values().any(|val| *val == cpp_name)
     }

parser/src/directives.rs

@@ -58,6 +58,13 @@ pub(crate) fn get_directives() -> &'static DirectivesMap {
                 |config| &config.blocklist,
             )),
         );
+        need_exclamation.insert(
+            "opaque".into(),
+            Box::new(StringList(
+                |config| &mut config.opaquelist,
+                |config| &config.opaquelist,
+            )),
+        );
         need_exclamation.insert(
             "block_constructors".into(),
             Box::new(StringList(

src/lib.rs

@@ -186,6 +186,7 @@ macro_rules! exclude_utilities {
 /// otherwise generated.
 /// This is 'greedy' in the sense that any functions/methods
 /// which take or return such a type will _also_ be blocked.
+/// See also [`opaque`].
 ///
 /// A directive to be included inside
 /// [include_cpp] - see [include_cpp] for general information.
@@ -194,6 +195,35 @@ macro_rules! block {
     ($($tt:tt)*) => { $crate::usage!{$($tt)*} };
 }
 
+/// Instruct `bindgen` to generate a type as an opaque type -
+/// that is, without fields inside it. This should be used
+/// only when there's a need to workaround some `bindgen`
+/// issue where it's incorrectly generating the type.
+/// See the [bindgen documentation](https://rust-lang.github.io/rust-bindgen/opaque.html)
+/// for what exactly this means.
+///
+/// At first glance, this might seem to have no effect for
+/// types which are marked non-POD. However, it prevents
+/// autocxx from inferring whether the type has implicit
+/// constructors, and thus limits the options for even
+/// allocating the type. This should therefore only be used
+/// when trying to work around a bug.
+///
+/// The types which are generated when you use this are
+/// rather useless - it's hard to interact with them at all
+/// from within the generated Rust. Worse still, if they're
+/// included as members of any other type, those types are
+/// "infected" and also become useless (in the sense that
+/// we can't figure out what constructors those types might
+/// have). Use with caution and only when really needed.
+///
+/// A directive to be included inside
+/// [include_cpp] - see [include_cpp] for general information.
+#[macro_export]
+macro_rules! opaque {
+    ($($tt:tt)*) => { $crate::usage!{$($tt)*} };
+}
+
 /// Avoid generating implicit constructors for this type.
 /// The rules for when to generate C++ implicit constructors
 /// are complex, and if autocxx gets it wrong, you can block