Repository: dtolnay/cxx (language: rust)

Issue:
clippy::redundant_closure warning on cxx::bridge macro in cxx 1.0.43+
Hi! Starting with cxx 1.0.43, I'm getting this Clippy warning for my project (Rust 1.48 and Rust 1.51):

```
error: redundant closure found
 --> rust/libnewsboat-ffi/src/logger.rs:3:1
  |
3 | #[cxx::bridge(namespace = "newsboat::Logger")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
```

I couldn't narrow this down to anything in particular; the only way to make the warning go away is to make the bridge mod empty. It's also not related to the `(namespace = …)` thing, because removing it doesn't make the warning go away.

The warning advises to "run with -Z macro-backtrace for more info", but doing so makes the warning go away (I tried both `cargo +nightly …` and `rustup override set nightly`).

The result of macro expansion:

```rust
$ cargo +nightly rustc --package libnewsboat-ffi --profile=check -- -Zunstable-options --pretty=expanded
#![feature(prelude_import)]
// Each function in this crate is used in a single place: a corresponding C++ wrapper. It doesn't
// make sense to document these functions because they are basically an internal detail and don't
// stand on their own.
#![allow(clippy :: missing_safety_doc)]
// This lint is nitpicky, I don't think it's really important how the literals are written.
#![allow(clippy :: unreadable_literal)]
#[prelude_import]
use std::prelude::rust_2018::*;
#[macro_use]
extern crate std;

use libc::c_char;
use std::ffi::CString;
use std::panic::{catch_unwind, UnwindSafe};
use std::process::abort;

// SNIP: some unrelated modules

pub mod logger {
    use libnewsboat::logger;
    #[deny(improper_ctypes, improper_ctypes_definitions)]
    #[allow(non_camel_case_types, non_snake_case, clippy ::
            upper_case_acronyms)]
    mod ffi {
        #[repr(transparent)]
        pub struct Level {
            #[allow(missing_docs)]
            pub repr: u8,
        }
        impl ::core::marker::StructuralEq for Level { }
        #[automatically_derived]
        #[allow(unused_qualifications)]
        impl ::core::cmp::Eq for Level {
            #[inline]
            #[doc(hidden)]
            fn assert_receiver_is_total_eq(&self) -> () {
                { let _: ::core::cmp::AssertParamIsEq<u8>; }
            }
        }
        impl ::core::marker::StructuralPartialEq for Level { }
        #[automatically_derived]
        #[allow(unused_qualifications)]
        impl ::core::cmp::PartialEq for Level {
            #[inline]
            fn eq(&self, other: &Level) -> bool {
                match *other {
                    Level { repr: ref __self_1_0 } =>
                    match *self {
                        Level { repr: ref __self_0_0 } =>
                        (*__self_0_0) == (*__self_1_0),
                    },
                }
            }
            #[inline]
            fn ne(&self, other: &Level) -> bool {
                match *other {
                    Level { repr: ref __self_1_0 } =>
                    match *self {
                        Level { repr: ref __self_0_0 } =>
                        (*__self_0_0) != (*__self_1_0),
                    },
                }
            }
        }
        #[allow(non_upper_case_globals)]
        impl Level {
            pub const USERERROR: Self = Level{repr: 1,};
            pub const CRITICAL: Self = Level{repr: 2,};
            pub const ERROR: Self = Level{repr: 3,};
            pub const WARN: Self = Level{repr: 4,};
            pub const INFO: Self = Level{repr: 5,};
            pub const DEBUG: Self = Level{repr: 6,};
        }
        unsafe impl ::cxx::ExternType for Level {
            #[doc(hidden)]
            type Id =
             (::cxx::n, ::cxx::e, ::cxx::w, ::cxx::s, ::cxx::b, ::cxx::o,
              ::cxx::a, ::cxx::t, (), ::cxx::L, ::cxx::o, ::cxx::g, ::cxx::g,
              ::cxx::e, ::cxx::r, (), ::cxx::L, ::cxx::e, ::cxx::v, ::cxx::e,
              ::cxx::l);
            type Kind = ::cxx::kind::Trivial;
        }
        impl ::std::marker::Copy for Level { }
        impl ::std::clone::Clone for Level {
            fn clone(&self) -> Self { *self }
        }
        #[doc(hidden)]
        const _: () =
            {
                #[doc(hidden)]
                #[export_name = "newsboat$Logger$cxxbridge1$unset_loglevel"]
                unsafe extern "C" fn __unset_loglevel() {
                    let __fn = "newsboat::logger::ffi::unset_loglevel";
                    fn __unset_loglevel() { super::unset_loglevel() }
                    ::cxx::private::catch_unwind(__fn,
                                                 move || __unset_loglevel())
                }
                #[doc(hidden)]
                #[export_name = "newsboat$Logger$cxxbridge1$set_logfile"]
                unsafe extern "C" fn __set_logfile(logfile:
                                                       ::cxx::private::RustStr) {
                    let __fn = "newsboat::logger::ffi::set_logfile";
                    fn __set_logfile(logfile: &str) {
                        super::set_logfile(logfile)
                    }
                    ::cxx::private::catch_unwind(__fn,
                                                 move ||
                                                     __set_logfile(logfile.as_str()))
                }
                #[doc(hidden)]
                #[export_name = "newsboat$Logger$cxxbridge1$get_loglevel"]
                unsafe extern "C" fn __get_loglevel() -> i64 {
                    let __fn = "newsboat::logger::ffi::get_loglevel";
                    fn __get_loglevel() -> i64 { super::get_loglevel() }
                    ::cxx::private::catch_unwind(__fn,
                                                 move || __get_loglevel())
                }
                #[doc(hidden)]
                #[export_name = "newsboat$Logger$cxxbridge1$set_loglevel"]
                unsafe extern "C" fn __set_loglevel(level: Level) {
                    let __fn = "newsboat::logger::ffi::set_loglevel";
                    fn __set_loglevel(level: Level) {
                        super::set_loglevel(level)
                    }
                    ::cxx::private::catch_unwind(__fn,
                                                 move ||
                                                     __set_loglevel(level))
                }
                #[doc(hidden)]
                #[export_name = "newsboat$Logger$cxxbridge1$log_internal"]
                unsafe extern "C" fn __log_internal(level: Level,
                                                    message:
                                                        ::cxx::private::RustStr) {
                    let __fn = "newsboat::logger::ffi::log_internal";
                    fn __log_internal(level: Level, message: &str) {
                        super::log_internal(level, message)
                    }
                    ::cxx::private::catch_unwind(__fn,
                                                 move ||
                                                     __log_internal(level,
                                                                    message.as_str()))
                }
                #[doc(hidden)]
                #[export_name =
                  "newsboat$Logger$cxxbridge1$set_user_error_logfile"]
                unsafe extern "C" fn __set_user_error_logfile(user_error_logfile:
                                                                  ::cxx::private::RustStr) {
                    let __fn =
                        "newsboat::logger::ffi::set_user_error_logfile";
                    fn __set_user_error_logfile(user_error_logfile: &str) {
                        super::set_user_error_logfile(user_error_logfile)
                    }
                    ::cxx::private::catch_unwind(__fn,
                                                 move ||
                                                     __set_user_error_logfile(user_error_logfile.as_str()))
                }
            };
    }
    fn ffi_level_to_log_level(level: ffi::Level) -> logger::Level {
        match level {
            ffi::Level::USERERROR => logger::Level::UserError,
            ffi::Level::CRITICAL => logger::Level::Critical,
            ffi::Level::ERROR => logger::Level::Error,
            ffi::Level::WARN => logger::Level::Warn,
            ffi::Level::INFO => logger::Level::Info,
            ffi::Level::DEBUG => logger::Level::Debug,
            _ => { ::std::rt::begin_panic("Unknown log level") }
        }
    }
    fn unset_loglevel() { logger::get_instance().unset_loglevel(); }
    fn set_logfile(logfile: &str) {
        logger::get_instance().set_logfile(logfile);
    }
    fn get_loglevel() -> i64 { logger::get_instance().get_loglevel() as i64 }
    fn set_loglevel(level: ffi::Level) {
        let level = ffi_level_to_log_level(level);
        logger::get_instance().set_loglevel(level);
    }
    fn log_internal(level: ffi::Level, message: &str) {
        let level = ffi_level_to_log_level(level);
        logger::get_instance().log(level, message);
    }
    fn set_user_error_logfile(user_error_logfile: &str) {
        logger::get_instance().set_user_error_logfile(user_error_logfile);
    }
}

// SNIP: a few more modules
```

The only closures that I can see are the ones passed to `::cxx::private::catch_unwind`, but these were present even in 1.0.41 (which didn't produce the warning).

I'm okay with suppressing this warning on my end, but I thought I'd let you know about this and see if you have some insights.

If you'd like to experiment locally:
- the repo is https://github.com/newsboat/newsboat.git
- the crate in question resides under rust/libnewsboat-ffi
- the command to run is `cargo clippy` (my CI adds `-D warnings` and more flags, but none of it is required to get the warning)


Discussion / hints:

Fix patch:
diff --git a/macro/src/expand.rs b/macro/src/expand.rs
index ff19bc611..5685abeee 100644
--- a/macro/src/expand.rs
+++ b/macro/src/expand.rs
@@ -983,14 +983,22 @@ fn expand_rust_function_shim_impl(
             _ => quote!(#var),
         }
     });
-    let vars = receiver_var.into_iter().chain(arg_vars);
+    let vars: Vec<_> = receiver_var.into_iter().chain(arg_vars).collect();
 
     let wrap_super = invoke.map(|invoke| expand_rust_function_shim_super(sig, &local_name, invoke));
 
+    let mut requires_closure;
     let mut call = match invoke {
-        Some(_) => quote!(#local_name),
-        None => quote!(::std::mem::transmute::<*const (), #sig>(__extern)),
+        Some(_) => {
+            requires_closure = false;
+            quote!(#local_name)
+        }
+        None => {
+            requires_closure = true;
+            quote!(::std::mem::transmute::<*const (), #sig>(__extern))
+        }
     };
+    requires_closure |= !vars.is_empty();
     call.extend(quote! { (#(#vars),*) });
 
     let span = body_span;
@@ -1032,8 +1040,14 @@ fn expand_rust_function_shim_impl(
 
     let mut expr = match conversion {
         None => call,
-        Some(conversion) if !sig.throws => quote_spanned!(span=> #conversion(#call)),
-        Some(conversion) => quote_spanned!(span=> ::std::result::Result::map(#call, #conversion)),
+        Some(conversion) if !sig.throws => {
+            requires_closure = true;
+            quote_spanned!(span=> #conversion(#call))
+        }
+        Some(conversion) => {
+            requires_closure = true;
+            quote_spanned!(span=> ::std::result::Result::map(#call, #conversion))
+        }
     };
 
     let mut outparam = None;
@@ -1047,12 +1061,20 @@ fn expand_rust_function_shim_impl(
             Some(_) => quote_spanned!(span=> __return),
             None => quote_spanned!(span=> &mut ()),
         };
+        requires_closure = true;
         expr = quote_spanned!(span=> ::cxx::private::r#try(#out, #expr));
     } else if indirect_return {
+        requires_closure = true;
         expr = quote_spanned!(span=> ::std::ptr::write(__return, #expr));
     }
 
-    expr = quote_spanned!(span=> ::cxx::private::catch_unwind(__fn, move || #expr));
+    let closure = if requires_closure {
+        quote_spanned!(span=> move || #expr)
+    } else {
+        quote!(#local_name)
+    };
+
+    expr = quote_spanned!(span=> ::cxx::private::catch_unwind(__fn, #closure));
 
     let ret = if sig.throws {
         quote!(-> ::cxx::private::Result)


Test patch: