Repository: dtolnay/cxx (language: rust)

Issue:
Error message not quite correct for Trivial/Opaque mistakes in extern types
If I modify the demo example along these lines:

```rust
unsafe impl ExternType for bindgen::BlobstoreClient {
    type Id = type_id!("org::blobstore::BlobstoreClient");
    type Kind = cxx::kind::Opaque; // no good!
}

mod bindgen {
    #[repr(C)]
    pub struct BlobstoreClient {
        a: u32,
    }
}

#[cxx::bridge(namespace = "org::blobstore")]
mod ffi {
    impl UniquePtr<BlobstoreClient> {}

    // C++ types and signatures exposed to Rust.
    unsafe extern "C++" {
        include!("demo/include/blobstore.h");

        type BlobstoreClient = super::bindgen::BlobstoreClient;

        fn update(blob: BlobstoreClient); // reason why the type needs to be Trivial
    }
}
```

Expected behavior:
* Rust error message that `cxx::Kind::Opaque` is insufficient and the kind needs to be `cxx::Kind::Trivial`

Ideal behavior:
* Rust error message that `cxx::Kind::Opaque` is insufficient, _with details from the `TrivialReason` saying _why_ it needs to be `Trivial`

Actual behavior:
* When building the demo, the C++ is built first, giving an error: `error: static_assert failed due to requirement '::rust::IsRelocatable<org::blobstore::BlobstoreClient>::value' "type ::org::blobstore::BlobstoreClient marked as Trivial in Rust is not trivially move constructible and trivially destructible in C++"`
* We never see any Rust compilation or errors.

At the very least, this sentence is factually incorrect, because the type is not _marked_ as Trivial. It _needs to be_ trivial and that's what the error message should be saying.

At least in the `cargo` case the C++ will always be built first and I can't think of any way to get a nice error message pertaining to the `Kind` of the `ExternType` since that's entirely invisible to `cxx::bridge` and therefore to the generated C++. For other build systems, I guess it's 50/50 whether the user sees the Rust or C++ error message first.

Before I raise a small PR to fix the wording of the C++ error message, I wondered whether you had any cunning plans to squeeze out a better error message in these cases.

NB I have not tried the _exact_ code I state above. I am encountering these symptoms for my WIP in #546.


Discussion / hints:

Fix patch:
diff --git a/gen/src/write.rs b/gen/src/write.rs
index 2770a053c..6d8322666 100644
--- a/gen/src/write.rs
+++ b/gen/src/write.rs
@@ -1,4 +1,3 @@
-use crate::gen::block::Block;
 use crate::gen::nested::NamespaceEntries;
 use crate::gen::out::OutFile;
 use crate::gen::{builtin, include, Opt};
@@ -8,6 +7,7 @@ use crate::syntax::{
     derive, mangle, Api, Enum, ExternFn, ExternType, Pair, RustName, Signature, Struct, Trait,
     Type, Types, Var,
 };
+use crate::{gen::block::Block, syntax::types::TrivialReason};
 use proc_macro2::Ident;
 use std::collections::{HashMap, HashSet};
 
@@ -117,8 +117,8 @@ fn write_data_structures<'a>(out: &mut OutFile<'a>, apis: &'a [Api]) {
     out.next_section();
     for api in apis {
         if let Api::TypeAlias(ety) = api {
-            if out.types.required_trivial.contains_key(&ety.name.rust) {
-                check_trivial_extern_type(out, &ety.name)
+            if let Some(reasons) = out.types.required_trivial.get(&ety.name.rust) {
+                check_trivial_extern_type(out, &ety.name, reasons)
             }
         }
     }
@@ -366,7 +366,7 @@ fn check_enum<'a>(out: &mut OutFile<'a>, enm: &'a Enum) {
     }
 }
 
-fn check_trivial_extern_type(out: &mut OutFile, id: &Pair) {
+fn check_trivial_extern_type(out: &mut OutFile, id: &Pair, reasons: &[TrivialReason]) {
     // NOTE: The following static assertion is just nice-to-have and not
     // necessary for soundness. That's because triviality is always declared by
     // the user in the form of an unsafe impl of cxx::ExternType:
@@ -402,13 +402,16 @@ fn check_trivial_extern_type(out: &mut OutFile, id: &Pair) {
 
     let id = id.to_fully_qualified();
     out.builtin.relocatable = true;
-    writeln!(out, "static_assert(");
-    writeln!(out, "    ::rust::IsRelocatable<{}>::value,", id);
-    writeln!(
-        out,
-        "    \"type {} marked as Trivial in Rust is not trivially move constructible and trivially destructible in C++\");",
-        id,
-    );
+    for reason in reasons {
+        writeln!(out, "static_assert(");
+        writeln!(out, "    ::rust::IsRelocatable<{}>::value,", id);
+        writeln!(
+            out,
+            "    \"type {} is not move constructible and trivially destructible in C++ yet is used as a trivial type in Rust ({})\");",
+            id,
+            reason
+        );
+    }
 }
 
 fn write_struct_operator_decls<'a>(out: &mut OutFile<'a>, strct: &'a Struct) {
diff --git a/syntax/check.rs b/syntax/check.rs
index 3b7b51365..6acaa0f8f 100644
--- a/syntax/check.rs
+++ b/syntax/check.rs
@@ -1,6 +1,5 @@
 use crate::syntax::atom::Atom::{self, *};
 use crate::syntax::report::Errors;
-use crate::syntax::types::TrivialReason;
 use crate::syntax::{
     error, ident, Api, Array, Enum, ExternFn, ExternType, Impl, Lang, Receiver, Ref, Signature,
     SliceRef, Struct, Trait, Ty1, Type, TypeAlias, Types,
@@ -189,7 +188,7 @@ fn check_type_ref(cx: &mut Check, ty: &Ref) {
             cx.error(
                 ty,
                 format!(
-                    "mutable reference to C++ type requires a pin -- use Pin<&mut {}>",
+                    "mutable reference to C++ type requires a pin -- use Pin<&mut {}> or declare the type Trivial in a cxx::ExternType impl",
                     requires_pin,
                 ),
             );
@@ -325,19 +324,15 @@ fn check_api_type(cx: &mut Check, ety: &ExternType) {
         cx.error(span, "extern type bounds are not implemented yet");
     }
 
-    if let Some(reason) = cx.types.required_trivial.get(&ety.name.rust) {
-        let what = match reason {
-            TrivialReason::StructField(strct) => format!("a field of `{}`", strct.name.rust),
-            TrivialReason::FunctionArgument(efn) => format!("an argument of `{}`", efn.name.rust),
-            TrivialReason::FunctionReturn(efn) => format!("a return value of `{}`", efn.name.rust),
-            TrivialReason::BoxTarget => format!("Box<{}>", ety.name.rust),
-            TrivialReason::VecElement => format!("a vector element in Vec<{}>", ety.name.rust),
-        };
-        let msg = format!(
-            "needs a cxx::ExternType impl in order to be used as {}",
-            what,
-        );
-        cx.error(ety, msg);
+    if let Some(reasons) = cx.types.required_trivial.get(&ety.name.rust) {
+        for reason in reasons {
+            let what = reason.describe_in_context(&ety);
+            let msg = format!(
+                "needs a cxx::ExternType impl in order to be used as {}",
+                what,
+            );
+            cx.error(ety, msg);
+        }
     }
 }
 
@@ -385,7 +380,7 @@ fn check_api_fn(cx: &mut Check, efn: &ExternFn) {
             cx.error(
                 span,
                 format!(
-                    "mutable reference to C++ type requires a pin -- use `self: Pin<&mut {}>`",
+                    "mutable reference to opaque C++ type requires a pin -- use `self: Pin<&mut {}>` or declare the type Trivial in a cxx::ExternType impl",
                     receiver.ty.rust,
                 ),
             );
diff --git a/syntax/types.rs b/syntax/types.rs
index 72216b3a8..eb3393a4c 100644
--- a/syntax/types.rs
+++ b/syntax/types.rs
@@ -8,7 +8,7 @@ use crate::syntax::{
 };
 use proc_macro2::Ident;
 use quote::ToTokens;
-use std::collections::BTreeMap as Map;
+use std::{collections::BTreeMap as Map, fmt::Display};
 
 pub struct Types<'a> {
     pub all: Set<&'a Type>,
@@ -18,7 +18,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: Map<&'a Ident, TrivialReason<'a>>,
+    pub required_trivial: Map<&'a Ident, Vec<TrivialReason<'a>>>,
     pub explicit_impls: Set<&'a Impl>,
     pub resolutions: Map<&'a RustName, &'a Pair>,
     pub struct_improper_ctypes: UnorderedSet<&'a Ident>,
@@ -169,15 +169,17 @@ impl<'a> Types<'a> {
         // 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 = Map::new();
-        let mut insist_alias_types_are_trivial = |ty: &'a Type, reason| {
-            if let Type::Ident(ident) = ty {
-                if cxx.contains(&ident.rust)
-                    && !structs.contains_key(&ident.rust)
-                    && !enums.contains_key(&ident.rust)
-                {
-                    required_trivial.entry(&ident.rust).or_insert(reason);
-                }
+        let mut required_trivial: Map<_, Vec<_>> = Map::new();
+
+        let mut insist_extern_types_are_trivial = |ident: &'a RustName, reason| {
+            if cxx.contains(&ident.rust)
+                && !structs.contains_key(&ident.rust)
+                && !enums.contains_key(&ident.rust)
+            {
+                required_trivial
+                    .entry(&ident.rust)
+                    .or_default()
+                    .push(reason);
             }
         };
         for api in apis {
@@ -185,17 +187,23 @@ impl<'a> Types<'a> {
                 Api::Struct(strct) => {
                     let reason = TrivialReason::StructField(strct);
                     for field in &strct.fields {
-                        insist_alias_types_are_trivial(&field.ty, reason);
+                        if let Type::Ident(ident) = &field.ty {
+                            insist_extern_types_are_trivial(&ident, reason);
+                        }
                     }
                 }
                 Api::CxxFunction(efn) | Api::RustFunction(efn) => {
                     let reason = TrivialReason::FunctionArgument(efn);
                     for arg in &efn.args {
-                        insist_alias_types_are_trivial(&arg.ty, reason);
+                        if let Type::Ident(ident) = &arg.ty {
+                            insist_extern_types_are_trivial(&ident, reason);
+                        }
                     }
                     if let Some(ret) = &efn.ret {
                         let reason = TrivialReason::FunctionReturn(efn);
-                        insist_alias_types_are_trivial(&ret, reason);
+                        if let Type::Ident(ident) = &ret {
+                            insist_extern_types_are_trivial(&ident, reason);
+                        }
                     }
                 }
                 _ => {}
@@ -205,11 +213,37 @@ impl<'a> Types<'a> {
             match ty {
                 Type::RustBox(ty) => {
                     let reason = TrivialReason::BoxTarget;
-                    insist_alias_types_are_trivial(&ty.inner, reason);
+                    if let Type::Ident(ident) = &ty.inner {
+                        insist_extern_types_are_trivial(&ident, reason);
+                    }
                 }
                 Type::RustVec(ty) => {
                     let reason = TrivialReason::VecElement;
-                    insist_alias_types_are_trivial(&ty.inner, reason);
+                    if let Type::Ident(ident) = &ty.inner {
+                        insist_extern_types_are_trivial(&ident, reason);
+                    }
+                }
+                _ => {}
+            }
+        }
+        for api in apis {
+            match api {
+                Api::CxxFunction(efn) | Api::RustFunction(efn) => {
+                    let reason = TrivialReason::UnpinnedMutableReferenceFunctionArgument(efn);
+                    if let Some(receiver) = &efn.receiver {
+                        if receiver.mutable && !receiver.pinned {
+                            insist_extern_types_are_trivial(&receiver.ty, reason);
+                        }
+                    }
+                    for arg in &efn.args {
+                        if let Type::Ref(reff) = &arg.ty {
+                            if reff.mutable && !reff.pinned {
+                                if let Type::Ident(ident) = &reff.inner {
+                                    insist_extern_types_are_trivial(&ident, reason);
+                                }
+                            }
+                        }
+                    }
                 }
                 _ => {}
             }
@@ -310,6 +344,36 @@ pub enum TrivialReason<'a> {
     FunctionReturn(&'a ExternFn),
     BoxTarget,
     VecElement,
+    UnpinnedMutableReferenceFunctionArgument(&'a ExternFn),
+}
+
+impl<'a> TrivialReason<'a> {
+    pub fn describe_in_context(&self, ety: &ExternType) -> String {
+        match self {
+            TrivialReason::BoxTarget => format!("Box<{}>", ety.name.rust),
+            TrivialReason::VecElement => format!("a vector element in Vec<{}>", ety.name.rust),
+            _ => self.to_string(),
+        }
+    }
+}
+
+impl<'a> Display for TrivialReason<'a> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            TrivialReason::StructField(strct) => write!(f, "a field of `{}`", strct.name.rust),
+            TrivialReason::FunctionArgument(efn) => write!(f, "an argument of `{}`", efn.name.rust),
+            TrivialReason::FunctionReturn(efn) => {
+                write!(f, "a return value of `{}`", efn.name.rust)
+            }
+            TrivialReason::BoxTarget => write!(f, "in a Box<...>"),
+            TrivialReason::VecElement => write!(f, "a Vec<...> element"),
+            TrivialReason::UnpinnedMutableReferenceFunctionArgument(efn) => write!(
+                f,
+                "a non-pinned mutable reference argument of {}",
+                efn.name.rust
+            ),
+        }
+    }
 }
 
 fn duplicate_name(cx: &mut Errors, sp: impl ToTokens, ident: &Ident) {


Test patch:
diff --git a/tests/ffi/module.rs b/tests/ffi/module.rs
index a2c13d3b4..cd9fc2b73 100644
--- a/tests/ffi/module.rs
+++ b/tests/ffi/module.rs
@@ -32,12 +32,16 @@ pub mod ffi2 {
 
         fn c_take_trivial_ptr(d: UniquePtr<D>);
         fn c_take_trivial_ref(d: &D);
+        fn c_take_trivial_ref_method(self: &D);
+        fn c_take_trivial_mut_ref_method(self: &mut D);
         fn c_take_trivial(d: D);
         fn c_take_trivial_ns_ptr(g: UniquePtr<G>);
         fn c_take_trivial_ns_ref(g: &G);
         fn c_take_trivial_ns(g: G);
         fn c_take_opaque_ptr(e: UniquePtr<E>);
         fn c_take_opaque_ref(e: &E);
+        fn c_take_opaque_ref_method(self: &E);
+        fn c_take_opaque_mut_ref_method(self: Pin<&mut E>);
         fn c_take_opaque_ns_ptr(e: UniquePtr<F>);
         fn c_take_opaque_ns_ref(e: &F);
         fn c_return_trivial_ptr() -> UniquePtr<D>;
diff --git a/tests/ffi/tests.cc b/tests/ffi/tests.cc
index 2be1e0614..1c8a122f7 100644
--- a/tests/ffi/tests.cc
+++ b/tests/ffi/tests.cc
@@ -529,6 +529,18 @@ void c_take_trivial_ref(const D &d) {
   }
 }
 
+void D::c_take_trivial_ref_method() const {
+  if (d == 30) {
+    cxx_test_suite_set_correct();
+  }
+}
+
+void D::c_take_trivial_mut_ref_method() {
+  if (d == 30) {
+    cxx_test_suite_set_correct();
+  }
+}
+
 void c_take_trivial(D d) {
   if (d.d == 30) {
     cxx_test_suite_set_correct();
@@ -571,6 +583,18 @@ void c_take_opaque_ref(const E &e) {
   }
 }
 
+void E::c_take_opaque_ref_method() const {
+  if (e == 40 && e_str == "hello") {
+    cxx_test_suite_set_correct();
+  }
+}
+
+void E::c_take_opaque_mut_ref_method() {
+  if (e == 40 && e_str == "hello") {
+    cxx_test_suite_set_correct();
+  }
+}
+
 void c_take_opaque_ns_ref(const ::F::F &f) {
   if (f.f == 40 && f.f_str == "hello") {
     cxx_test_suite_set_correct();
diff --git a/tests/ffi/tests.h b/tests/ffi/tests.h
index 024ecead4..f8d983b9c 100644
--- a/tests/ffi/tests.h
+++ b/tests/ffi/tests.h
@@ -59,11 +59,15 @@ class C {
 
 struct D {
   uint64_t d;
+  void c_take_trivial_ref_method() const;
+  void c_take_trivial_mut_ref_method();
 };
 
 struct E {
   uint64_t e;
   std::string e_str;
+  void c_take_opaque_ref_method() const;
+  void c_take_opaque_mut_ref_method();
 };
 
 enum COwnedEnum {
diff --git a/tests/test.rs b/tests/test.rs
index 00a7228ba..b8edf3709 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -263,10 +263,14 @@ fn test_rust_name_attribute() {
 
 #[test]
 fn test_extern_trivial() {
-    let d = ffi2::c_return_trivial();
+    let mut d = ffi2::c_return_trivial();
     check!(ffi2::c_take_trivial_ref(&d));
+    check!(d.c_take_trivial_ref_method());
+    check!(d.c_take_trivial_mut_ref_method());
     check!(ffi2::c_take_trivial(d));
-    let d = ffi2::c_return_trivial_ptr();
+    let mut d = ffi2::c_return_trivial_ptr();
+    check!(d.c_take_trivial_ref_method());
+    check!(d.c_take_trivial_mut_ref_method());
     check!(ffi2::c_take_trivial_ptr(d));
     cxx::UniquePtr::new(ffi2::D { d: 42 });
     let d = ffi2::ns_c_return_trivial();
@@ -282,8 +286,10 @@ fn test_extern_trivial() {
 
 #[test]
 fn test_extern_opaque() {
-    let e = ffi2::c_return_opaque_ptr();
+    let mut e = ffi2::c_return_opaque_ptr();
     check!(ffi2::c_take_opaque_ref(e.as_ref().unwrap()));
+    check!(e.c_take_opaque_ref_method());
+    check!(e.pin_mut().c_take_opaque_mut_ref_method());
     check!(ffi2::c_take_opaque_ptr(e));
 
     let f = ffi2::c_return_ns_opaque_ptr();
diff --git a/tests/ui/by_value_not_supported.stderr b/tests/ui/by_value_not_supported.stderr
index 0a56dd48f..4bcce1b3e 100644
--- a/tests/ui/by_value_not_supported.stderr
+++ b/tests/ui/by_value_not_supported.stderr
@@ -22,6 +22,18 @@ error: needs a cxx::ExternType impl in order to be used as a field of `S`
 10 |         type C;
    |         ^^^^^^
 
+error: needs a cxx::ExternType impl in order to be used as an argument of `f`
+  --> $DIR/by_value_not_supported.rs:10:9
+   |
+10 |         type C;
+   |         ^^^^^^
+
+error: needs a cxx::ExternType impl in order to be used as a return value of `f`
+  --> $DIR/by_value_not_supported.rs:10:9
+   |
+10 |         type C;
+   |         ^^^^^^
+
 error: passing opaque C++ type by value is not supported
   --> $DIR/by_value_not_supported.rs:16:14
    |
diff --git a/tests/ui/pin_mut_opaque.stderr b/tests/ui/pin_mut_opaque.stderr
index 32d5cfcbc..91a7caaf3 100644
--- a/tests/ui/pin_mut_opaque.stderr
+++ b/tests/ui/pin_mut_opaque.stderr
@@ -1,28 +1,46 @@
-error: mutable reference to C++ type requires a pin -- use Pin<&mut Opaque>
+error: mutable reference to C++ type requires a pin -- use Pin<&mut Opaque> or declare the type Trivial in a cxx::ExternType impl
  --> $DIR/pin_mut_opaque.rs:5:19
   |
 5 |         fn f(arg: &mut Opaque);
   |                   ^^^^^^^^^^^
 
-error: mutable reference to C++ type requires a pin -- use Pin<&mut CxxString>
+error: mutable reference to C++ type requires a pin -- use Pin<&mut CxxString> or declare the type Trivial in a cxx::ExternType impl
  --> $DIR/pin_mut_opaque.rs:8:17
   |
 8 |         fn s(s: &mut CxxString);
   |                 ^^^^^^^^^^^^^^
 
-error: mutable reference to C++ type requires a pin -- use Pin<&mut CxxVector<...>>
+error: mutable reference to C++ type requires a pin -- use Pin<&mut CxxVector<...>> or declare the type Trivial in a cxx::ExternType impl
  --> $DIR/pin_mut_opaque.rs:9:17
   |
 9 |         fn v(v: &mut CxxVector<u8>);
   |                 ^^^^^^^^^^^^^^^^^^
 
-error: mutable reference to C++ type requires a pin -- use `self: Pin<&mut Opaque>`
+error: needs a cxx::ExternType impl in order to be used as a non-pinned mutable reference argument of f
+ --> $DIR/pin_mut_opaque.rs:4:9
+  |
+4 |         type Opaque;
+  |         ^^^^^^^^^^^
+
+error: needs a cxx::ExternType impl in order to be used as a non-pinned mutable reference argument of g
+ --> $DIR/pin_mut_opaque.rs:4:9
+  |
+4 |         type Opaque;
+  |         ^^^^^^^^^^^
+
+error: needs a cxx::ExternType impl in order to be used as a non-pinned mutable reference argument of h
+ --> $DIR/pin_mut_opaque.rs:4:9
+  |
+4 |         type Opaque;
+  |         ^^^^^^^^^^^
+
+error: mutable reference to opaque C++ type requires a pin -- use `self: Pin<&mut Opaque>` or declare the type Trivial in a cxx::ExternType impl
  --> $DIR/pin_mut_opaque.rs:6:14
   |
 6 |         fn g(&mut self);
   |              ^^^^^^^^^
 
-error: mutable reference to C++ type requires a pin -- use `self: Pin<&mut Opaque>`
+error: mutable reference to opaque C++ type requires a pin -- use `self: Pin<&mut Opaque>` or declare the type Trivial in a cxx::ExternType impl
  --> $DIR/pin_mut_opaque.rs:7:20
   |
 7 |         fn h(self: &mut Opaque);