Repository: dtolnay/cxx (language: rust)

Issue:
ExternTypes may be safely passed and returned by value
```rust
        unsafe impl cxx::ExternType for bindgen::Bob {
            type Id = cxx::type_id!("Bob");
        }
        mod bindgen {
            #[repr(C)]
            pub struct Bob {
                pub a: u32,
                pub b: u32,
            }
        }
        #[cxx::bridge]
        pub mod cxxbridge {
            extern "C" {
                include!("input.h");
                pub fn give_bob() -> Bob;
                type Bob = super::bindgen::Bob;
            }
        }
```

doesn't work because we can't return `ExternType`s by value.

Each extern type may be one of three fundamental categories of type:

* An alias to another `cxx::bridge` type (in which case it's a simple alias and we don't do anything in the current instantiation)
* A type which `is_trivially_move_constructible && is_trivially_destructible`. If so, we allow allow it to be passed by value into/our of cxx types, and we check this with `static_asserts` as discussed in #280. ("trivial" types for short henceforth)
* A type which isn't trivially move constructable/destructable. In these cases we don't allow use by value but we do allow generation of `UniquePtr` traits etc. as per #312.

This distinction is implied by #280 but this could be done as a pre-requisite step first. I also feel it would be desirable to pin this down before finalizing design for #312.

The question is, for a given `ExternType`, how does cxx know which of these three categories of type it is? What's your plan there? Is it to require the developer to implement an extra trait for trivial types which is then checked for safety using `static_assert`? Or did you plan to do something funky to auto-detect trivial types at build time?

(NB for this last category of types, my higher-level code generator plans to create shims which allow us to call pre-existing C++ APIs that take a `T` by instead passing a `UniquePtr<T>`. I have yet to do this bit. It makes superficial sense that "complicated" types need to be entirely allocated, accessed, and managed within C++ but can still be owned by Rust. Figuring out whether this is ergonomic in practice is arguably the entire point of my https://github.com/google/autocxx experiments... if this doesn't work out it's back to the drawing board!)


Discussion / hints:
Hypothetical design:

* `ExternType` gains a new method along these lines:
```rust
pub enum TypeCategory { // better name needed
   #[doc(hidden)]
   Alias,
   Trivial,
   NonTrivial
}

pub unsafe trait ExternType {
    //...
    type Id;

    fn get_category() -> TypeCategory {
       TypeCategory::NonTrivial // default implementation
    }
}
```

Where we expand `ExternType` ourselves, we return `Alias`. Users' pre-existing implementations of `ExternType` will (obviously) get `NonTrivial` and thus get current behavior.

But folk can override `get_category` if they need to state that their type is trivial.

Then:
* For `Alias` types, we do nothing.
* For `Trivial` or `NonTrivial` types, we _may_ generate `UniquePtrTarget` and other traits depending on the decisions in #312.
* For `Trivial` types, we allow them to be passed by value into and out of functions just as if the type were declared in the `cxx::bridge` block. We also generate some `static_asserts` to verify that they really are trivial.
(though maybe if the `ExternType` is never actually instantiated, we may need to do this with an associated type like for `Id`. But something along these lines...)
I'm fiddling around here. I so far have concluded:
* I probably can't use the existing `ExternType` trait derivation, since it's probably not accessible at the point of macro expansion or C++ expansion. As I understand it, it's only used to generate something which can be used for a compile-time assertion check when building the resultant Rust code.
* Instead, it's probably necessary for this "trivial" annotation to be somewhere inside the `#[cxx::bridge]` mod. Right now I'm imagining that a type can `impl` a new trait here to indicate that it's trivial. Fiddling in that direction.

I probably won't have another chance to touch this till next week.
That approach looks promising to me.

As you found, the design of cxx is such that we can't *know* most things during macro expansion, but we can generate static assertions to *verify* things post macro expansion.

Looking at your categorization of Alias, Trivial, NonTrivial --- is there any need to distinguish Alias vs NonTrivial? I hope we can treat those exactly the same.

At that point the only relevant distinction would be Trivial (obtainable by value in Rust) vs NonTrivial (not obtainable by value in Rust). And the only *verification* we would need to emit is that every type alias which is exposed by value to Rust (whether in an extern function signature or as a field of a shared struct) has an ExternType impl which indicates it is Trivial.

Our current scheme for verifying (though this is certainly not the only possible one) is based on:

https://github.com/dtolnay/cxx/blob/acc7fb05f9bbf07e7d6e1accce9cdbdf26b91411/src/extern_type.rs#L109-L110

which we instantiate as follows to verify post macro expansion that a particular type has a specific expected Id in its ExternType impl.

```rust
const _: fn() = verify_extern_type::<YourType, TheExpectedId>;
```

I imagine a verify similar mechanism for verifying the Trivial vs NonTrivial distinction too.

```rust
// if your type alias was exposed by value to Rust, then we generate:
const _: fn() = verify_triviality::<YourType, Trivial>;
```

Beyond that, I believe the existing Rust orphan rules are good enough that we should be able to let you put #312-style "request for an impl" wherever the user wants. (But I need to revisit this more carefully to confirm that I am sure about this.)
Aha, I think you're saying that there should be _both_ something in the `ExternType` impl (presumably outside the `#[cxx::bridge]` block) _and_ some syntax inside the `#[cxx::bridge]` block. I was now thinking we only need the latter? Then there should be no need for the Rust-side verification that you outline, and just C++-side `static_assert`s?

Is there some need to do something in the `ExternType` impl as well as inside the `#[cxx::bridge]`?

As for your question:
> Looking at your categorization of Alias, Trivial, NonTrivial --- is there any need to distinguish Alias vs NonTrivial?

First, an assumption: if there's an alias, the _target_ `cxx::bridge` actually generates all the code for the type, e.g. `UniquePtrTarget` impls, plus all the Rust-side and C++-side wrappers, and the alias therefore means there is no codegen beyond just a simple Rust `type`.

I haven't yet checked that assumption, but that motivated the distinction I made.

I was therefore thinking:

* Alias: we need to look up the Trivial-ness by examining the target type over in its distant `#[cxx::bridge]` section. (Which we can't, of course, and I don't really have a plan there. As it affects how _we_ do codegen based on _our_ `#[cxx::bridge]` section, we really do _need_ to know. But it would be a shame if we had to specify the trivial-ness in both `#[cxx::bridge]` blocks). Also, for an `Alias`, we will _never_ generate `UniquePtrTarget` or other traits because that will be done by the target `#[cxx::bridge]`.
* NonTrivial: may generate `UniquePtrTarget` etc. but can't pass by value.
* Trivial: may generate `UniquePtrTarget` etc. and can pass by value.

I'm also trying to plan ahead for all the different properties we might ever want to attach to types. If we're designing syntax for trivial/non-trivial maybe we should be thinking of a more general type traits arrangement.

Possible properties (and I _know_ many of these will never happen, and may be more appropriate for a higher level code generator; my point is really just to indicate that we might want some general syntax for specifying what the user wants of a type):

Property | Description | Applies to extern types | Applies to opaque C++ types | C++ codegen or static assertions needed | Rust codegen needed
--------- | ----------- | ------------------------ | ------------------------ | ------------------------ | ----------
Trivial | Whether this can be passed by value in Rust, as discussed | Yes | No | Yes | No except in APIs
Unique pointer target | Whether we should allow this too be stored in `UniquePtrs` per #236  | Yes | Yes | No | Yes
Vector target, etc. | Other traits as per the previous line | Yes | Yes | No | Yes
Generate field accessors | Generate accessor _functions_ to get and set C++ fields | Yes (probably non-trivial only) | Yes | Yes | Yes
Allow pass UniquePtr by value | Allow this type to be passed into C++ functions "by value" when all we have is a UniquePtr, for compatibility with existing functions that take the type by value | Yes (non-trivial only) | Yes | Yes | Modifies existing codegen

Each of those _could_ be its own `impl` block within the `#[cxx::bridge]` (as you propose in #236 and I'm starting to hack about with in 84744b7) but I do wonder if we need something nicer. e.g.

```rust
#[cxx::bridge]
mod ffi {
  // ...
  impl TypeTraits for MyRustType {
    const TRAITS = TypeTraits {
       trivial = true;
       generate_accessors = true;
    }
  }
}
```

but it's not very Rusty syntax.
Focusing just on Trivial/NonTrivial for now:

> I think you're saying that there should be _both_ something in the `ExternType` impl (presumably outside the `#[cxx::bridge]` block) _and_ some syntax inside the `#[cxx::bridge]` block. I was now thinking we only need the latter? Then there should be no need for the Rust-side verification that you outline, and just C++-side `static_assert`s?
>
> Is there some need to do something in the `ExternType` impl as well as inside the `#[cxx::bridge]`?

I think my plan is the opposite of your plan. You're thinking about what syntax goes in the cxx::bridge module. I am thinking no new syntax or annotation is needed in the cxx::bridge module at all to support this feature.

Here are the expansion rules:

```rust
extern "C++" {
    // If this type is an "original", not an alias:
    //   pub struct ZeusClient(::cxx::private::Opaque);  // same as today
    //   impl ExternType for ZeusClient { type Kind = cxx::kind::Opaque; }  // "Opaque" == NonTrivial
    type ZeusClient;

    // If an alias, only validate the type Id; no new impl:
    //   verify_id::<crate::bindgen::ZeusClient, cxx::type_id!("ZeusClient")>;
    type ZeusClient = crate::bindgen::ZeusClient;

    // If type is ever mentioned by value, verify trivial:
    //   verify_kind::<ZeusClient, cxx::kind::Trivial>;
    fn new(arg: &str) -> ZeusClient;

    // As per #312. Allowed wherever the user feels like, subject to orphan rules.
    // Optional if T is an "original" and has some signature involving UniquePtr<T> in the same cxx::bridge.
    impl UniquePtr<ZeusClient> {}
}
```

The user handwrites or uses a bindgen to bring Rust definitions of trivial extern types.

```rust
mod bindgen {
    #[repr(C)]
    pub struct ZeusClient {
        pub port: u16,
    }
}

unsafe impl cxx::ExternType for bindgen::ZeusClient {
    type Id = cxx::type_id!("ZeusClient");
    type Kind = cxx::kind::Trivial;
}
```

Next to `verify_kind::<ZeusClient, cxx::kind::Trivial>` we can also verify is_trivially_move_constructible && is_trivially_destructible on the C++ side, but actually this is just nice-to-have since the user went on the record with their `unsafe impl` to unsafely claim it is trivial, so it's on them if that were wrong.

I read your last two comments again and don't spot anything that would require more ceremony/new syntax in the cxx::bridge module, but I could just be missing it.
Ah! I hadn't thought of making this determination based on how the type is _used_. OK! I will think on that, thanks.

Fix patch:
diff --git a/gen/src/write.rs b/gen/src/write.rs
index 862be814f..5ea84fc7c 100644
--- a/gen/src/write.rs
+++ b/gen/src/write.rs
@@ -80,6 +80,11 @@ pub(super) fn gen(
                     write_struct_with_methods(out, ety, methods);
                 }
             }
+            Api::TypeAlias(ety) => {
+                if types.required_trivial_aliases.contains(&ety.ident) {
+                    check_trivial_extern_type(out, &ety.ident)
+                }
+            }
             _ => {}
         }
     }
@@ -124,13 +129,18 @@ pub(super) fn gen(
 fn write_includes(out: &mut OutFile, types: &Types) {
     for ty in types {
         match ty {
-            Type::Ident(ident) => match Atom::from(ident) {
-                Some(U8) | Some(U16) | Some(U32) | Some(U64) | Some(I8) | Some(I16) | Some(I32)
-                | Some(I64) => out.include.cstdint = true,
-                Some(Usize) => out.include.cstddef = true,
-                Some(CxxString) => out.include.string = true,
-                Some(Bool) | Some(Isize) | Some(F32) | Some(F64) | Some(RustString) | None => {}
-            },
+            Type::Ident(ident) => {
+                match Atom::from(ident) {
+                    Some(U8) | Some(U16) | Some(U32) | Some(U64) | Some(I8) | Some(I16)
+                    | Some(I32) | Some(I64) => out.include.cstdint = true,
+                    Some(Usize) => out.include.cstddef = true,
+                    Some(CxxString) => out.include.string = true,
+                    Some(Bool) | Some(Isize) | Some(F32) | Some(F64) | Some(RustString) | None => {}
+                };
+                if types.required_trivial_aliases.contains(&ident) {
+                    out.include.type_traits = true;
+                };
+            }
             Type::RustBox(_) => out.include.type_traits = true,
             Type::UniquePtr(_) => out.include.memory = true,
             Type::CxxVector(_) => out.include.vector = true,
@@ -401,6 +411,11 @@ fn check_enum(out: &mut OutFile, enm: &Enum) {
     }
 }
 
+fn check_trivial_extern_type(out: &mut OutFile, id: &Ident) {
+    writeln!(out, "static_assert(std::is_trivially_move_constructible<{}>::value,\"type {} marked as Trivial in Rust is not trivially move constructible in C++\");", id, id);
+    writeln!(out, "static_assert(std::is_trivially_destructible<{}>::value,\"type {} marked as Trivial in Rust is not trivially destructible in C++\");", id, id);
+}
+
 fn write_exception_glue(out: &mut OutFile, apis: &[Api]) {
     let mut has_cxx_throws = false;
     for api in apis {
diff --git a/macro/src/expand.rs b/macro/src/expand.rs
index 879431e03..fc7c69239 100644
--- a/macro/src/expand.rs
+++ b/macro/src/expand.rs
@@ -57,6 +57,10 @@ fn expand(ffi: Module, apis: &[Api], types: &Types) -> TokenStream {
             Api::TypeAlias(alias) => {
                 expanded.extend(expand_type_alias(alias));
                 hidden.extend(expand_type_alias_verify(namespace, alias));
+                let ident = &alias.ident;
+                if types.required_trivial_aliases.contains(ident) {
+                    hidden.extend(expand_type_alias_kind_trivial_verify(alias));
+                }
             }
         }
     }
@@ -179,6 +183,7 @@ fn expand_cxx_type(namespace: &Namespace, ety: &ExternType) -> TokenStream {
 
         unsafe impl ::cxx::ExternType for #ident {
             type Id = #type_id;
+            type Kind = ::cxx::Opaque;
         }
     }
 }
@@ -677,6 +682,18 @@ fn expand_type_alias_verify(namespace: &Namespace, alias: &TypeAlias) -> TokenSt
     }
 }
 
+fn expand_type_alias_kind_trivial_verify(type_alias: &TypeAlias) -> TokenStream {
+    let ident = &type_alias.ident;
+    let begin_span = type_alias.type_token.span;
+    let end_span = type_alias.semi_token.span;
+    let begin = quote_spanned!(begin_span=> ::cxx::private::verify_extern_kind::<);
+    let end = quote_spanned!(end_span=> >);
+
+    quote! {
+        const _: fn() = #begin #ident, ::cxx::Trivial #end;
+    }
+}
+
 fn type_id(namespace: &Namespace, ident: &Ident) -> TokenStream {
     let mut path = String::new();
     for name in namespace {
diff --git a/src/extern_type.rs b/src/extern_type.rs
index 6701ef591..9bc594776 100644
--- a/src/extern_type.rs
+++ b/src/extern_type.rs
@@ -54,7 +54,7 @@
 /// ## Integrating with bindgen-generated types
 ///
 /// Handwritten `ExternType` impls make it possible to plug in a data structure
-/// emitted by bindgen as the definition of an opaque C++ type emitted by CXX.
+/// emitted by bindgen as the definition of a C++ type emitted by CXX.
 ///
 /// By writing the unsafe `ExternType` impl, the programmer asserts that the C++
 /// namespace and type name given in the type id refers to a C++ type that is
@@ -69,10 +69,11 @@
 /// #     pub struct StringPiece([usize; 2]);
 /// # }
 ///
-/// use cxx::{type_id, ExternType};
+/// use cxx::{type_id, ExternType, Opaque};
 ///
 /// unsafe impl ExternType for folly_sys::StringPiece {
 ///     type Id = type_id!("folly::StringPiece");
+///     type Kind = Opaque;
 /// }
 ///
 /// #[cxx::bridge(namespace = folly)]
@@ -92,6 +93,29 @@
 /// #
 /// # fn main() {}
 /// ```
+///
+/// ## Opaque and Trivial types
+///
+/// Some C++ types are safe to hold and pass around in Rust, by value.
+/// Those C++ types must have a trivial move constructor, and must
+/// have no destructor.
+///
+/// If you believe your C++ type is indeed trivial, you can specify
+/// ```
+/// # struct TypeName;
+/// # unsafe impl cxx::ExternType for TypeName {
+/// type Id = cxx::type_id!("name::space::of::TypeName");
+/// type Kind = cxx::Trivial;
+/// # }
+/// ```
+/// which will enable you to pass it into C++ functions by value,
+/// return it by value from such functions, and include it in
+/// `struct`s that you have declared to `cxx::bridge`. Your promises
+/// about the triviality of the C++ type will be checked using
+/// `static_assert`s in the generated C++.
+///
+/// Opaque types can't be passed by value, but can still be held
+/// in `UniquePtr`.
 pub unsafe trait ExternType {
     /// A type-level representation of the type's C++ namespace and type name.
     ///
@@ -101,10 +125,32 @@ pub unsafe trait ExternType {
     /// # struct TypeName;
     /// # unsafe impl cxx::ExternType for TypeName {
     /// type Id = cxx::type_id!("name::space::of::TypeName");
+    /// type Kind = cxx::Opaque;
     /// # }
     /// ```
     type Id;
+
+    /// Either `cxx::Opaque` or `cxx::Trivial`. If in doubt, use
+    /// `cxx::Opaque`.
+    type Kind;
+}
+
+pub(crate) mod kind {
+
+    /// An opaque type which can't be passed or held by value within Rust.
+    /// For example, a C++ type with a destructor, or a non-trivial move
+    /// constructor. Rust's strict move semantics mean that we can't own
+    /// these by value in Rust, but they can still be owned by a
+    /// `UniquePtr`...
+    pub struct Opaque;
+
+    /// A type with trivial move constructors and no destructor, which
+    /// can therefore be owned and moved around in Rust code directly.
+    pub struct Trivial;
 }
 
 #[doc(hidden)]
 pub fn verify_extern_type<T: ExternType<Id = Id>, Id>() {}
+
+#[doc(hidden)]
+pub fn verify_extern_kind<T: ExternType<Kind = Kind>, Kind>() {}
diff --git a/src/lib.rs b/src/lib.rs
index 9efac51e2..2f21de9a1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -395,6 +395,8 @@ mod unwind;
 pub use crate::cxx_string::CxxString;
 pub use crate::cxx_vector::CxxVector;
 pub use crate::exception::Exception;
+pub use crate::extern_type::kind::Opaque;
+pub use crate::extern_type::kind::Trivial;
 pub use crate::extern_type::ExternType;
 pub use crate::unique_ptr::UniquePtr;
 pub use cxxbridge_macro::bridge;
@@ -422,6 +424,7 @@ pub type Vector<T> = CxxVector<T>;
 #[doc(hidden)]
 pub mod private {
     pub use crate::cxx_vector::VectorElement;
+    pub use crate::extern_type::verify_extern_kind;
     pub use crate::extern_type::verify_extern_type;
     pub use crate::function::FatFunction;
     pub use crate::opaque::Opaque;
diff --git a/syntax/check.rs b/syntax/check.rs
index 147984e7e..cfac23647 100644
--- a/syntax/check.rs
+++ b/syntax/check.rs
@@ -338,6 +338,7 @@ fn is_unsized(cx: &mut Check, ty: &Type) -> bool {
         || cx.types.cxx.contains(ident)
             && !cx.types.structs.contains_key(ident)
             && !cx.types.enums.contains_key(ident)
+            && !cx.types.required_trivial_aliases.contains(ident)
         || cx.types.rust.contains(ident)
 }
 
@@ -376,7 +377,11 @@ fn describe(cx: &mut Check, ty: &Type) -> String {
             } else if cx.types.enums.contains_key(ident) {
                 "enum".to_owned()
             } else if cx.types.cxx.contains(ident) {
-                "C++ type".to_owned()
+                if cx.types.required_trivial_aliases.contains(ident) {
+                    "trivial C++ type".to_owned()
+                } else {
+                    "non-trivial C++ type".to_owned()
+                }
             } else if cx.types.rust.contains(ident) {
                 "opaque Rust type".to_owned()
             } else if Atom::from(ident) == Some(CxxString) {
diff --git a/syntax/types.rs b/syntax/types.rs
index 8a86a4645..ff782686a 100644
--- a/syntax/types.rs
+++ b/syntax/types.rs
@@ -14,6 +14,7 @@ pub struct Types<'a> {
     pub rust: Set<&'a Ident>,
     pub aliases: Map<&'a Ident, &'a TypeAlias>,
     pub untrusted: Map<&'a Ident, &'a ExternType>,
+    pub required_trivial_aliases: Set<&'a Ident>,
 }
 
 impl<'a> Types<'a> {
@@ -135,6 +136,55 @@ impl<'a> Types<'a> {
             }
         }
 
+        // All these APIs may contain types passed by value. We need to ensure
+        // we check that this is permissible. We do this _after_ scanning all
+        // the APIs above, in case some function or struct references a type
+        // which is declared subsequently.
+        let mut required_trivial_aliases = Set::new();
+
+        fn insist_alias_types_are_trivial<'c>(
+            required_trivial_aliases: &mut Set<&'c Ident>,
+            aliases: &Map<&'c Ident, &'c TypeAlias>,
+            ty: &'c Type,
+        ) {
+            if let Type::Ident(ident) = ty {
+                if aliases.contains_key(ident) {
+                    required_trivial_aliases.insert(ident);
+                }
+            }
+        }
+
+        for api in apis {
+            match api {
+                Api::Struct(strct) => {
+                    for field in &strct.fields {
+                        insist_alias_types_are_trivial(
+                            &mut required_trivial_aliases,
+                            &aliases,
+                            &field.ty,
+                        );
+                    }
+                }
+                Api::CxxFunction(efn) | Api::RustFunction(efn) => {
+                    for arg in &efn.args {
+                        insist_alias_types_are_trivial(
+                            &mut required_trivial_aliases,
+                            &aliases,
+                            &arg.ty,
+                        );
+                    }
+                    if let Some(ret) = &efn.ret {
+                        insist_alias_types_are_trivial(
+                            &mut required_trivial_aliases,
+                            &aliases,
+                            &ret,
+                        );
+                    }
+                }
+                _ => {}
+            }
+        }
+
         Types {
             all,
             structs,
@@ -143,6 +193,7 @@ impl<'a> Types<'a> {
             rust,
             aliases,
             untrusted,
+            required_trivial_aliases,
         }
     }
 


Test patch:
diff --git a/tests/ui/by_value_not_supported.stderr b/tests/ui/by_value_not_supported.stderr
index 1ff8dbff5..e61ce8427 100644
--- a/tests/ui/by_value_not_supported.stderr
+++ b/tests/ui/by_value_not_supported.stderr
@@ -1,4 +1,4 @@
-error: using C++ type by value is not supported
+error: using non-trivial C++ type by value is not supported
  --> $DIR/by_value_not_supported.rs:4:9
   |
 4 |         c: C,
@@ -16,13 +16,13 @@ error: using C++ string by value is not supported
 6 |         s: CxxString,
   |         ^^^^^^^^^^^^
 
-error: passing C++ type by value is not supported
+error: passing non-trivial C++ type by value is not supported
   --> $DIR/by_value_not_supported.rs:16:14
    |
 16 |         fn f(c: C) -> C;
    |              ^^^^
 
-error: returning C++ type by value is not supported
+error: returning non-trivial C++ type by value is not supported
   --> $DIR/by_value_not_supported.rs:16:23
    |
 16 |         fn f(c: C) -> C;