`needless_pass_by_value` for the extern functions

[stanislav-tkach]

Probably, needless_pass_by_value can be disabled for the extern functions. I'm not sure that it will be correct for all situations, so I'm going to describe my problem.

I'm using jni-rs crate that provides a thin wrapper around Java JNI interface pointer:

#[repr(C)]
pub struct JNIEnv<'a> {
    internal: *mut sys::JNIEnv,
    lifetime: PhantomData<&'a ()>,
}

It is passed by value to all functions that should be accessed from the Java side. For example:

#[no_mangle]
#[allow(non_snake_case)]
#[cfg_attr(feature = "cargo-clippy", allow(needless_pass_by_value))]
pub extern "C" fn Java_com_..._foo(
    env: JNIEnv,
    _: JClass,
    ...
) {
    ...
}

I don't like to suppress this warning for the entire project, but adding allow(needless_pass_by_value) to all extern functions isn't nice, either. Perhaps, jni-rs can pass such parameters by reference, but it will be just reference to pointer. What is the best approach here? 🤔

[oli-obk]

Have you found a solution?

Reference wrappers do sound like a problem, even outside extern functions.

[stanislav-tkach]

@oli-obk
In my case there is "core" written in "normal rust" (with all clippy warnings enabled) and java-bindings that gives me a problem. I have moved all extern functions (bindings) into the separate crate with #![cfg_attr(feature = "cargo-clippy", deny(needless_pass_by_value))] in the root lib.rs.

Actually I also have some utility functions in the bindings crate, for them I have locally enabled warnings with #![cfg_attr(feature = "cargo-clippy", deny(needless_pass_by_value))].

I'm not sure that this is the best approach, but not sure that Clippy should treat extern functions differently either.

[oli-obk]

I'm not 100% convinced that we're doing anything wrong here, but I haven't looked at the code of needless_pass_by_value very closely either. Have you tried changing the lifetime: PhantomData<&'a ()>, to lifetime: PhantomData<&'a mut ()>, (notice the mut)? I think that matches your semantics better, but I don't know much about the rest of your code. Alternatively, if you may really have multiple copies of the same pointer, you could derive(Copy) for JNIEnv which would completely eliminate the lint.

[stanislav-tkach]

Implementing Copy seems inappropriate for me, because it violates borrow checker rules: there can be several instances that share one mutable state.

I'm going to try PhantomData<&'a mut ()> nevertheless.

[oli-obk]

Any updates?

[stanislav-tkach]

Sorry, I have postponed this for too long. 😅 As far as I can see, PhantomData<&'a mut ()> changes nothing here.

Yet again, I'm not sure if there is a problem at all, at least from the Clippy side. I have updated our code in that way, so the problem is minimized. I don't like the idea of disabling warning even for the part of code, but the solution that I have in my mind (disable lint for extern functions in the Clippy) doesn't seem appropriate either.

I suppose the issue can be closed, because I don't see a clear solution. Perhaps @jechas01 (jni-rs crate author and maintainer) can add something here.

[oli-obk]

I think the correct solution would be to scrap the JNIEnv type and replace it with

struct JNIEnv {
    _dummy: (),
}
impl JNIEnv {
    fn as_sys(&mut self) -> *mut JNIEnv { self as *mut Self as _ }
}

And then use &mut JNIEnv everywhere and call as_sys on it whenever you call a jni-sys function.

That way you get all the semantics of references for free.