Repository: dtolnay/cxx (language: rust)

Issue:
Pass-by-reference of shared struct containing String to C++ FFI gives "not FFI-safe" error

`src/main.rs`:

```
#[cxx::bridge(namespace = org::example)]
mod ffi {
    struct SharedThing {
        x: String,
    }

    extern "C" {
        include!("src/demo.h");
        fn do_thing(state: &SharedThing);
    }
}

fn main() {
    let shared = ffi::SharedThing {
        x: "hello".to_string(),
    };
    ffi::do_thing(&shared);
}
```

`src/demo.h`:

```
#pragma once
#include <string>

#include "rust/cxx.h"

namespace org {
namespace example {

struct SharedThing;

void do_thing(const SharedThing& state);

}  // namespace example
}  // namespace org
```

`src/demo.cc`:

```
#include "src/demo.h"

#include <iostream>

#include "src/main.rs.h"

namespace org {
namespace example {

void do_thing(const SharedThing& state) { std::cout << state.x << std::endl; }

}  // namespace example
}  // namespace org
```

`build.rs`:

```
fn main() {
    cxx_build::bridge("src/main.rs")
        .file("src/demo.cc")
        .flag_if_supported("-std=c++14")
        .compile("cxxbridge-demo");

    println!("cargo:rerun-if-changed=src/main.rs");
    println!("cargo:rerun-if-changed=src/demo.h");
    println!("cargo:rerun-if-changed=src/demo.cc");
}
```

`Cargo.toml`:

```
[package]
name = "cxx_test"
version = "0.1.0"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
cxx = "0.3"


[build-dependencies]
cxx-build = "0.3"
```

Resulting error:
```
error: `extern` block uses type `std::string::String`, which is not FFI-safe
 --> src/main.rs:9:28
  |
9 |         fn do_thing(state: &SharedThing);
  |                            ^^^^^^^^^^^^ not FFI-safe
  |
note: the lint level is defined here
 --> src/main.rs:1:1
  |
1 | #[cxx::bridge(namespace = org::example)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
  = note: this struct has unspecified layout
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

error: could not compile `cxx_test`.
```

Changing it to pass-by-value works fine.

Changing `x` to a non-String type also works.


Discussion / hints:
Can confirm this. Passing `String` on its own (by value or shared-ref) works as expected:
```
#[cxx::bridge]
mod ffi {
	extern "C" {
		fn pass_by_ref(s: &String) -> String;
		fn pass_by_value(s: String) -> String;
	}
}
```

But good to hear that it's still possible to pass the struct by value as a workaround.  
For completeness, there are a few other ways I tried to pass strings to C++:
* `Box<String>` fails with _unsupported target type of Box_
* `Box<str>` fails with _unsupported type_
* `*const char` fails with _unsupported type_
* `Vec<u8>` also fails with _not FFI-safe_, if enclosing struct is passed by reference
* `&str` would need a lifetime annotation, fails with _struct with generic parameters is not supported yet_
* `CxxString` cannot be constructed from Rust

I have been using a `UniquePtr<CxxString>` in structs. As you note you can't construct them from Rust, but you can expose another C++ API which makes one from a `&str`.

Fix patch:
diff --git a/macro/src/expand.rs b/macro/src/expand.rs
index c86dc91cc..b5d3067b0 100644
--- a/macro/src/expand.rs
+++ b/macro/src/expand.rs
@@ -208,7 +208,7 @@ fn expand_cxx_function_decl(efn: &ExternFn, types: &Types) -> TokenStream {
     });
     let args = efn.args.iter().map(|arg| {
         let ident = &arg.ident;
-        let ty = expand_extern_type(&arg.ty, types);
+        let ty = expand_extern_type(&arg.ty, types, true);
         if arg.ty == RustString {
             quote!(#ident: *const #ty)
         } else if let Type::RustVec(_) = arg.ty {
@@ -225,11 +225,11 @@ fn expand_cxx_function_decl(efn: &ExternFn, types: &Types) -> TokenStream {
     let ret = if efn.throws {
         quote!(-> ::cxx::private::Result)
     } else {
-        expand_extern_return_type(&efn.ret, types)
+        expand_extern_return_type(&efn.ret, types, true)
     };
     let mut outparam = None;
     if indirect_return(efn, types) {
-        let ret = expand_extern_type(efn.ret.as_ref().unwrap(), types);
+        let ret = expand_extern_type(efn.ret.as_ref().unwrap(), types, true);
         outparam = Some(quote!(__return: *mut #ret));
     }
     let link_name = mangle::extern_fn(efn, types);
@@ -287,6 +287,10 @@ fn expand_cxx_function_shim(efn: &ExternFn, types: &Types) -> TokenStream {
                     None => quote!(::cxx::private::RustVec::from_ref(#var)),
                     Some(_) => quote!(::cxx::private::RustVec::from_mut(#var)),
                 },
+                inner if types.is_considered_improper_ctype(inner) => match ty.mutability {
+                    None => quote!(#var as *const #inner as *const ::std::ffi::c_void),
+                    Some(_) => quote!(#var as *mut #inner as *mut ::std::ffi::c_void),
+                },
                 _ => quote!(#var),
             },
             Type::Str(_) => quote!(::cxx::private::RustStr::from(#var)),
@@ -323,7 +327,7 @@ fn expand_cxx_function_shim(efn: &ExternFn, types: &Types) -> TokenStream {
         .collect::<TokenStream>();
     let local_name = format_ident!("__{}", efn.name.rust);
     let call = if indirect_return {
-        let ret = expand_extern_type(efn.ret.as_ref().unwrap(), types);
+        let ret = expand_extern_type(efn.ret.as_ref().unwrap(), types, true);
         setup.extend(quote! {
             let mut __return = ::std::mem::MaybeUninit::<#ret>::uninit();
         });
@@ -376,6 +380,10 @@ fn expand_cxx_function_shim(efn: &ExternFn, types: &Types) -> TokenStream {
                         None => quote!(#call.as_vec()),
                         Some(_) => quote!(#call.as_mut_vec()),
                     },
+                    inner if types.is_considered_improper_ctype(inner) => {
+                        let mutability = ty.mutability;
+                        quote!(&#mutability *#call.cast())
+                    }
                     _ => call,
                 },
                 Type::Str(_) => quote!(#call.as_str()),
@@ -508,7 +516,7 @@ fn expand_rust_function_shim_impl(
     });
     let args = sig.args.iter().map(|arg| {
         let ident = &arg.ident;
-        let ty = expand_extern_type(&arg.ty, types);
+        let ty = expand_extern_type(&arg.ty, types, false);
         if types.needs_indirect_abi(&arg.ty) {
             quote!(#ident: *mut #ty)
         } else {
@@ -609,7 +617,7 @@ fn expand_rust_function_shim_impl(
     let mut outparam = None;
     let indirect_return = indirect_return(sig, types);
     if indirect_return {
-        let ret = expand_extern_type(sig.ret.as_ref().unwrap(), types);
+        let ret = expand_extern_type(sig.ret.as_ref().unwrap(), types, false);
         outparam = Some(quote!(__return: *mut #ret,));
     }
     if sig.throws {
@@ -627,7 +635,7 @@ fn expand_rust_function_shim_impl(
     let ret = if sig.throws {
         quote!(-> ::cxx::private::Result)
     } else {
-        expand_extern_return_type(&sig.ret, types)
+        expand_extern_return_type(&sig.ret, types, false)
     };
 
     let pointer = match invoke {
@@ -939,15 +947,15 @@ fn indirect_return(sig: &Signature, types: &Types) -> bool {
         .map_or(false, |ret| sig.throws || types.needs_indirect_abi(ret))
 }
 
-fn expand_extern_type(ty: &Type, types: &Types) -> TokenStream {
+fn expand_extern_type(ty: &Type, types: &Types, proper: bool) -> TokenStream {
     match ty {
         Type::Ident(ident) if ident.rust == RustString => quote!(::cxx::private::RustString),
         Type::RustBox(ty) | Type::UniquePtr(ty) => {
-            let inner = expand_extern_type(&ty.inner, types);
+            let inner = expand_extern_type(&ty.inner, types, proper);
             quote!(*mut #inner)
         }
         Type::RustVec(ty) => {
-            let elem = expand_extern_type(&ty.inner, types);
+            let elem = expand_extern_type(&ty.inner, types, proper);
             quote!(::cxx::private::RustVec<#elem>)
         }
         Type::Ref(ty) => {
@@ -957,9 +965,13 @@ fn expand_extern_type(ty: &Type, types: &Types) -> TokenStream {
                     quote!(&#mutability ::cxx::private::RustString)
                 }
                 Type::RustVec(ty) => {
-                    let inner = expand_extern_type(&ty.inner, types);
+                    let inner = expand_extern_type(&ty.inner, types, proper);
                     quote!(&#mutability ::cxx::private::RustVec<#inner>)
                 }
+                inner if proper && types.is_considered_improper_ctype(inner) => match mutability {
+                    None => quote!(*const ::std::ffi::c_void),
+                    Some(_) => quote!(*#mutability ::std::ffi::c_void),
+                },
                 _ => quote!(#ty),
             }
         }
@@ -969,11 +981,11 @@ fn expand_extern_type(ty: &Type, types: &Types) -> TokenStream {
     }
 }
 
-fn expand_extern_return_type(ret: &Option<Type>, types: &Types) -> TokenStream {
+fn expand_extern_return_type(ret: &Option<Type>, types: &Types, proper: bool) -> TokenStream {
     let ret = match ret {
         Some(ret) if !types.needs_indirect_abi(ret) => ret,
         _ => return TokenStream::new(),
     };
-    let ty = expand_extern_type(ret, types);
+    let ty = expand_extern_type(ret, types, proper);
     quote!(-> #ty)
 }
diff --git a/syntax/improper.rs b/syntax/improper.rs
new file mode 100644
index 000000000..6fd31629a
--- /dev/null
+++ b/syntax/improper.rs
@@ -0,0 +1,36 @@
+use self::ImproperCtype::*;
+use crate::syntax::atom::Atom::{self, *};
+use crate::syntax::{Type, Types};
+use proc_macro2::Ident;
+
+pub enum ImproperCtype<'a> {
+    Definite(bool),
+    Depends(&'a Ident),
+}
+
+impl<'a> Types<'a> {
+    // yes, no, maybe
+    pub fn determine_improper_ctype(&self, ty: &Type) -> ImproperCtype<'a> {
+        match ty {
+            Type::Ident(ident) => {
+                let ident = &ident.rust;
+                if let Some(atom) = Atom::from(ident) {
+                    Definite(atom == RustString)
+                } else if let Some(strct) = self.structs.get(ident) {
+                    Depends(&strct.name.rust) // iterate to fixed-point
+                } else {
+                    Definite(self.rust.contains(ident))
+                }
+            }
+            Type::RustBox(_)
+            | Type::RustVec(_)
+            | Type::Str(_)
+            | Type::Fn(_)
+            | Type::Void(_)
+            | Type::Slice(_)
+            | Type::SliceRefU8(_) => Definite(true),
+            Type::UniquePtr(_) | Type::CxxVector(_) => Definite(false),
+            Type::Ref(ty) => self.determine_improper_ctype(&ty.inner),
+        }
+    }
+}
diff --git a/syntax/mod.rs b/syntax/mod.rs
index ac6350a2a..b742e37a9 100644
--- a/syntax/mod.rs
+++ b/syntax/mod.rs
@@ -10,6 +10,7 @@ pub mod error;
 pub mod file;
 pub mod ident;
 mod impls;
+mod improper;
 pub mod mangle;
 mod names;
 pub mod namespace;
diff --git a/syntax/types.rs b/syntax/types.rs
index 67b041bfc..5d6d58259 100644
--- a/syntax/types.rs
+++ b/syntax/types.rs
@@ -1,4 +1,5 @@
 use crate::syntax::atom::Atom::{self, *};
+use crate::syntax::improper::ImproperCtype;
 use crate::syntax::report::Errors;
 use crate::syntax::set::OrderedSet as Set;
 use crate::syntax::{
@@ -19,6 +20,7 @@ pub struct Types<'a> {
     pub required_trivial: Map<&'a Ident, TrivialReason<'a>>,
     pub explicit_impls: Set<&'a Impl>,
     pub resolutions: Map<&'a Ident, &'a Pair>,
+    pub struct_improper_ctypes: UnorderedSet<&'a Ident>,
 }
 
 impl<'a> Types<'a> {
@@ -32,6 +34,7 @@ impl<'a> Types<'a> {
         let mut untrusted = Map::new();
         let mut explicit_impls = Set::new();
         let mut resolutions = Map::new();
+        let struct_improper_ctypes = UnorderedSet::new();
 
         fn visit<'a>(all: &mut Set<&'a Type>, ty: &'a Type) {
             all.insert(ty);
@@ -190,7 +193,7 @@ impl<'a> Types<'a> {
             }
         }
 
-        Types {
+        let mut types = Types {
             all,
             structs,
             enums,
@@ -201,7 +204,34 @@ impl<'a> Types<'a> {
             required_trivial,
             explicit_impls,
             resolutions,
+            struct_improper_ctypes,
+        };
+
+        let mut unresolved_structs: Vec<&Ident> = types.structs.keys().copied().collect();
+        let mut new_information = true;
+        while new_information {
+            new_information = false;
+            unresolved_structs.retain(|ident| {
+                let mut retain = false;
+                for var in &types.structs[ident].fields {
+                    if match types.determine_improper_ctype(&var.ty) {
+                        ImproperCtype::Depends(inner) => {
+                            retain = true;
+                            types.struct_improper_ctypes.contains(inner)
+                        }
+                        ImproperCtype::Definite(improper) => improper,
+                    } {
+                        types.struct_improper_ctypes.insert(ident);
+                        new_information = true;
+                        return false;
+                    }
+                }
+                // If all fields definite false, remove from unresolved_structs.
+                retain
+            });
         }
+
+        types
     }
 
     pub fn needs_indirect_abi(&self, ty: &Type) -> bool {
@@ -227,6 +257,19 @@ impl<'a> Types<'a> {
         false
     }
 
+    // Types that trigger rustc's default #[warn(improper_ctypes)] lint, even if
+    // they may be otherwise unproblematic to mention in an extern signature.
+    // For example in a signature like `extern "C" fn(*const String)`, rustc
+    // refuses to believe that C could know how to supply us with a pointer to a
+    // Rust String, even though C could easily have obtained that pointer
+    // legitimately from a Rust call.
+    pub fn is_considered_improper_ctype(&self, ty: &Type) -> bool {
+        match self.determine_improper_ctype(ty) {
+            ImproperCtype::Definite(improper) => improper,
+            ImproperCtype::Depends(ident) => self.struct_improper_ctypes.contains(ident),
+        }
+    }
+
     pub fn resolve(&self, ident: &ResolvableName) -> &Pair {
         self.resolutions
             .get(&ident.rust)


Test patch:
diff --git a/tests/ffi/lib.rs b/tests/ffi/lib.rs
index f40ec6ccc..af8a9f540 100644
--- a/tests/ffi/lib.rs
+++ b/tests/ffi/lib.rs
@@ -69,6 +69,10 @@ pub mod ffi {
         z: usize,
     }
 
+    struct SharedString {
+        msg: String,
+    }
+
     enum Enum {
         AVal,
         BVal = 2020,
@@ -169,6 +173,7 @@ pub mod ffi {
         fn c_take_ref_rust_vec_string(v: &Vec<String>);
         fn c_take_ref_rust_vec_index(v: &Vec<u8>);
         fn c_take_ref_rust_vec_copy(v: &Vec<u8>);
+        fn c_take_ref_shared_string(s: &SharedString) -> &SharedString;
         fn c_take_callback(callback: fn(String) -> usize);
         fn c_take_enum(e: Enum);
         fn c_take_ns_enum(e: AEnum);
diff --git a/tests/ffi/tests.cc b/tests/ffi/tests.cc
index a03ec2b75..747788f2e 100644
--- a/tests/ffi/tests.cc
+++ b/tests/ffi/tests.cc
@@ -385,6 +385,13 @@ void c_take_ref_rust_vec_copy(const rust::Vec<uint8_t> &v) {
   }
 }
 
+const SharedString &c_take_ref_shared_string(const SharedString &s) {
+  if (std::string(s.msg) == "2020") {
+    cxx_test_suite_set_correct();
+  }
+  return s;
+}
+
 void c_take_callback(rust::Fn<size_t(rust::String)> callback) {
   callback("2020");
 }
diff --git a/tests/ffi/tests.h b/tests/ffi/tests.h
index e5510485c..7fc563327 100644
--- a/tests/ffi/tests.h
+++ b/tests/ffi/tests.h
@@ -36,6 +36,7 @@ namespace tests {
 
 struct R;
 struct Shared;
+struct SharedString;
 enum class Enum : uint16_t;
 
 class C {
@@ -134,6 +135,7 @@ void c_take_ref_rust_vec(const rust::Vec<uint8_t> &v);
 void c_take_ref_rust_vec_string(const rust::Vec<rust::String> &v);
 void c_take_ref_rust_vec_index(const rust::Vec<uint8_t> &v);
 void c_take_ref_rust_vec_copy(const rust::Vec<uint8_t> &v);
+const SharedString &c_take_ref_shared_string(const SharedString &s);
 void c_take_callback(rust::Fn<size_t(rust::String)> callback);
 void c_take_enum(Enum e);
 void c_take_ns_enum(::A::AEnum e);
diff --git a/tests/test.rs b/tests/test.rs
index a90b8fc34..2c6f062f2 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -137,6 +137,9 @@ fn test_c_take() {
     check!(ffi::c_take_ref_rust_vec(&test_vec));
     check!(ffi::c_take_ref_rust_vec_index(&test_vec));
     check!(ffi::c_take_ref_rust_vec_copy(&test_vec));
+    check!(ffi::c_take_ref_shared_string(&ffi::SharedString {
+        msg: "2020".to_owned()
+    }));
     let ns_shared_test_vec = vec![ffi::AShared { z: 1010 }, ffi::AShared { z: 1011 }];
     check!(ffi::c_take_rust_vec_ns_shared(ns_shared_test_vec));
     let nested_ns_shared_test_vec = vec![ffi::ABShared { z: 1010 }, ffi::ABShared { z: 1011 }];