Repository: dtolnay/cxx (rust)

Issue:
Bug when defining methods on shared structs
```rust
// str_test.rs
#[cxx::bridge]
mod ffi {
    struct A {
        a: f32,
    }

    extern "Rust"{
        fn hello(self: &A) -> String;
    }
}
```

Then run 
```bash
cxxbridge str_test.rs
```

We get:
```cpp
...

::rust::String A::hello() const noexcept {
  ::rust::MaybeUninit<::rust::String> return$;
  cxxbridge05$A$hello(*this&return$.value);
  return ::std::move(return$.value);
}
```

Note that there's a missing "," in hello invocation.
It should be `cxxbridge05$A$hello(*this, &return$.value);` instead of `cxxbridge05$A$hello(*this&return$.value);`


<!--  ❤️  -->

Pull request description:
Fix placement of commas in C++ member functions that call Rust methods
Fixes #442.

Patch:
diff --git a/gen/src/write.rs b/gen/src/write.rs
index 39053a12..69b9f2eb 100644
--- a/gen/src/write.rs
+++ b/gen/src/write.rs
@@ -698,11 +698,13 @@ fn write_rust_function_shim_impl(
         write!(out, "::rust::repr::PtrLen error$ = ");
     }
     write!(out, "{}(", invoke);
+    let mut needs_comma = false;
     if sig.receiver.is_some() {
         write!(out, "*this");
+        needs_comma = true;
     }
-    for (i, arg) in sig.args.iter().enumerate() {
-        if i > 0 || sig.receiver.is_some() {
+    for arg in &sig.args {
+        if needs_comma {
             write!(out, ", ");
         }
         match &arg.ty {
@@ -725,15 +727,17 @@ fn write_rust_function_shim_impl(
             ty if ty != RustString && out.types.needs_indirect_abi(ty) => write!(out, "$.value"),
             _ => {}
         }
+        needs_comma = true;
     }
     if indirect_return {
-        if !sig.args.is_empty() {
+        if needs_comma {
             write!(out, ", ");
         }
         write!(out, "&return$.value");
+        needs_comma = true;
     }
     if indirect_call {
-        if !sig.args.is_empty() || indirect_return {
+        if needs_comma {
             write!(out, ", ");
         }
         write!(out, "extern$");
diff --git a/macro/src/expand.rs b/macro/src/expand.rs
index a01c1c64..b5d3067b 100644
--- a/macro/src/expand.rs
+++ b/macro/src/expand.rs
@@ -563,10 +563,14 @@ fn expand_rust_function_shim_impl(
     });
     let vars = receiver_var.into_iter().chain(arg_vars);
 
-    let wrap_super = invoke.map(|invoke| expand_rust_function_shim_super(sig, &local_name, invoke));
-
     let mut call = match invoke {
-        Some(_) => quote!(#local_name),
+        Some(ident) => match &sig.receiver {
+            None => quote!(super::#ident),
+            Some(receiver) => {
+                let receiver_type = &receiver.ty;
+                quote!(#receiver_type::#ident)
+            }
+        },
         None => quote!(::std::mem::transmute::<*const (), #sig>(__extern)),
     };
     call.extend(quote! { (#(#vars),*) });
@@ -644,63 +648,11 @@ fn expand_rust_function_shim_impl(
         #[export_name = #link_name]
         unsafe extern "C" fn #local_name(#(#all_args,)* #outparam #pointer) #ret {
             let __fn = concat!(module_path!(), #catch_unwind_label);
-            #wrap_super
             #expr
         }
     }
 }
 
-// A wrapper like `fn f(x: Arg) { super::f(x) }` just to ensure we have the
-// accurate unsafety declaration and no problematic elided lifetimes.
-fn expand_rust_function_shim_super(
-    sig: &Signature,
-    local_name: &Ident,
-    invoke: &Ident,
-) -> TokenStream {
-    let unsafety = sig.unsafety;
-
-    let receiver_var = sig
-        .receiver
-        .as_ref()
-        .map(|receiver| Ident::new("__self", receiver.var.span));
-    let receiver = sig.receiver.iter().map(|receiver| {
-        let receiver_type = receiver.ty();
-        quote!(#receiver_var: #receiver_type)
-    });
-    let args = sig.args.iter().map(|arg| quote!(#arg));
-    let all_args = receiver.chain(args);
-
-    let ret = if let Some((result, _langle, rangle)) = sig.throws_tokens {
-        let ok = match &sig.ret {
-            Some(ret) => quote!(#ret),
-            None => quote!(()),
-        };
-        let impl_trait = quote_spanned!(result.span=> impl);
-        let display = quote_spanned!(rangle.span=> ::std::fmt::Display);
-        quote!(-> ::std::result::Result<#ok, #impl_trait #display>)
-    } else {
-        expand_return_type(&sig.ret)
-    };
-
-    let arg_vars = sig.args.iter().map(|arg| &arg.ident);
-    let vars = receiver_var.iter().chain(arg_vars);
-
-    let span = invoke.span();
-    let call = match &sig.receiver {
-        None => quote_spanned!(span=> super::#invoke),
-        Some(receiver) => {
-            let receiver_type = &receiver.ty;
-            quote_spanned!(span=> #receiver_type::#invoke)
-        }
-    };
-
-    quote_spanned! {span=>
-        #unsafety fn #local_name(#(#all_args,)*) #ret {
-            #call(#(#vars,)*)
-        }
-    }
-}
-
 fn expand_type_alias(alias: &TypeAlias) -> TokenStream {
     let doc = &alias.doc;
     let ident = &alias.name.rust;


Test patch:
diff --git a/tests/ffi/lib.rs b/tests/ffi/lib.rs
index cf8e876f..7461d66f 100644
--- a/tests/ffi/lib.rs
+++ b/tests/ffi/lib.rs
@@ -1,6 +1,5 @@
 #![allow(
     clippy::boxed_local,
-    clippy::just_underscores_and_digits,
     clippy::ptr_arg,
     clippy::trivially_copy_pass_by_ref
 )]
@@ -275,7 +274,7 @@ pub mod ffi {
         fn r_return_r2(n: usize) -> Box<R2>;
         fn get(self: &R2) -> usize;
         fn set(self: &mut R2, n: usize) -> usize;
-        fn r_method_on_shared(self: &Shared) -> usize;
+        fn r_method_on_shared(self: &Shared) -> String;
 
         #[cxx_name = "rAliasedFunction"]
         fn r_aliased_function(x: i32) -> String;
@@ -319,8 +318,8 @@ impl R2 {
 }
 
 impl ffi::Shared {
-    fn r_method_on_shared(&self) -> usize {
-        2020
+    fn r_method_on_shared(&self) -> String {
+        "2020".to_owned()
     }
 }
 
diff --git a/tests/ffi/tests.cc b/tests/ffi/tests.cc
index fd4751eb..c3c797ad 100644
--- a/tests/ffi/tests.cc
+++ b/tests/ffi/tests.cc
@@ -626,7 +626,7 @@ extern "C" const char *cxx_run_test() noexcept {
   ASSERT(r2->get() == 2021);
   ASSERT(r2->set(2020) == 2020);
   ASSERT(r2->get() == 2020);
-  ASSERT(Shared{0}.r_method_on_shared() == 2020);
+  ASSERT(std::string(Shared{0}.r_method_on_shared()) == "2020");
 
   ASSERT(std::string(rAliasedFunction(2020)) == "2020");
 
diff --git a/tests/ui/missing_unsafe.rs b/tests/ui/missing_unsafe.rs
deleted file mode 100644
index d8c0a23a..00000000
--- a/tests/ui/missing_unsafe.rs
+++ /dev/null
@@ -1,10 +0,0 @@
-#[cxx::bridge]
-mod ffi {
-    extern "Rust" {
-        fn f(x: i32);
-    }
-}
-
-unsafe fn f(_x: i32) {}
-
-fn main() {}
diff --git a/tests/ui/missing_unsafe.stderr b/tests/ui/missing_unsafe.stderr
deleted file mode 100644
index df1bce22..00000000
--- a/tests/ui/missing_unsafe.stderr
+++ /dev/null
@@ -1,7 +0,0 @@
-error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
- --> $DIR/missing_unsafe.rs:4:12
-  |
-4 |         fn f(x: i32);
-  |            ^ call to unsafe function
-  |
-  = note: consult the function's documentation for information on how to avoid undefined behavior
diff --git a/tests/ui/result_no_display.rs b/tests/ui/result_no_display.rs
deleted file mode 100644
index b5356775..00000000
--- a/tests/ui/result_no_display.rs
+++ /dev/null
@@ -1,14 +0,0 @@
-#[cxx::bridge]
-mod ffi {
-    extern "Rust" {
-        fn f() -> Result<()>;
-    }
-}
-
-pub struct NonError;
-
-fn f() -> Result<(), NonError> {
-    Ok(())
-}
-
-fn main() {}
diff --git a/tests/ui/result_no_display.stderr b/tests/ui/result_no_display.stderr
deleted file mode 100644
index d0502f9b..00000000
--- a/tests/ui/result_no_display.stderr
+++ /dev/null
@@ -1,8 +0,0 @@
-error[E0277]: `NonError` doesn't implement `std::fmt::Display`
- --> $DIR/result_no_display.rs:4:19
-  |
-4 |         fn f() -> Result<()>;
-  |                   ^^^^^^^^^^ `NonError` cannot be formatted with the default formatter
-  |
-  = help: the trait `std::fmt::Display` is not implemented for `NonError`
-  = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
diff --git a/tests/ui/unsupported_elided.rs b/tests/ui/unsupported_elided.rs
deleted file mode 100644
index 40333196..00000000
--- a/tests/ui/unsupported_elided.rs
+++ /dev/null
@@ -1,20 +0,0 @@
-use std::marker::PhantomData;
-
-#[cxx::bridge]
-mod ffi {
-    extern "Rust" {
-        type T;
-
-        fn f(t: &T) -> &str;
-    }
-}
-
-pub struct T<'a> {
-    _lifetime: PhantomData<&'a ()>,
-}
-
-fn f<'a>(_t: &T<'a>) -> &'a str {
-    ""
-}
-
-fn main() {}
diff --git a/tests/ui/unsupported_elided.stderr b/tests/ui/unsupported_elided.stderr
deleted file mode 100644
index 5136390c..00000000
--- a/tests/ui/unsupported_elided.stderr
+++ /dev/null
@@ -1,11 +0,0 @@
-error[E0106]: missing lifetime specifier
- --> $DIR/unsupported_elided.rs:8:24
-  |
-8 |         fn f(t: &T) -> &str;
-  |                 --     ^ expected named lifetime parameter
-  |
-  = help: this function's return type contains a borrowed value, but the signature does not say which one of `t`'s 2 lifetimes it is borrowed from
-help: consider introducing a named lifetime parameter
-  |
-8 |         fn f<'a>(t: &'a T) -> &'a str;
-  |             ^^^^    ^^^^^     ^^^