JNIEnv::with_local_frame should be able to return any value, not just jni::results::Result<JObject>

[argv-minus-one]

This is to continue the conversation begun in #398 (comment).

In brief, we want to change JNIEnv::with_local_frame to be able to return any result type, not just jni::results::Result<JObject>.

---

@rib, I'm mostly okay with your proposed change, but I have one issue with it. Because with_local_frame returns jni::results::Result<T>, and because the closure will always perform JNI calls and therefore potentially fail with jni::Results::Error, with_local_frame will always return nested Results. This has already given you some trouble.

There is a method Result::flatten to unnest Results, but:

1. It's unstable.
2. It doesn't work if the error types are different, even if one error type can be converted to the other (although that has been proposed).

The nested Results can easily be unnested with the ? operator in some situations:

let () = env.with_local_frame(1, |env| {
    some_fallible_function()?;
    Ok(())
})??;

But that's only true if with_local_frame is called in a function that returns Result and the error type implements both From<jni::errors::Error> and From<E> (where E is the error type returned by the closure). Otherwise, the Results have to be unnested explicitly:

let result: Result<(), E> = env.with_local_frame(1, |env| {
    some_fallible_function()?;
    Ok(())
}).map_err(Into::into).and_then(std::convert::identity);

---

I'm not sure if this is actually better or not, but here's another idea: have the with_local_frame closure return a Result, but with any error type that implements From<jni::errors::Error>. That way, a different error type can be used without nesting Results. If there is a problem pushing or popping the local frame, the error is converted into the closure's error type.

The catch is the compiler won't be able to infer the closure's error type unless it's spelled out somewhere, like with an explicit return type on the closure, with a turbofish on with_local_frame, or by directly returning the result of a function call without wrapping it in Ok. (A default for the type parameter E would solve this problem, but unfortunately Rust doesn't support that.)

Neither approach could be truly called ergonomic, but it seems easier to write out an explicit error type on the closure than to unnest Results and convert errors explicitly. At least, I think so. Let me know what you think of this.

If we do go with this approach, I'll add documentation about writing out the error type explicitly.

[rib]

Yeah, overall it seems like there are quite a few trade offs and it's tricky to find something that's ergonomic for the largest number of use cases.

I think we can probably also keep in mind the possibility for applications/crates to create slightly more ergonomic wrappers in situations where they use the API frequently and they can be more explicit about the Result/Error type they want to use in their crate.

Although the nesting of Results hasn't seemed too bad to me so far, I like the sound of your idea to allow any error type that implements From<jni::errors::Error> and would like to try your branch out and see how that feels to use.

With the separate Results it does also seem like a reasonable separation of concerns, even though it's extra work to unwrap. The outer Result conceptually relates to the Push/PopLocalFrame and it makes sense that it's a jni::errors::Result. For the closure it's possible that there may be relatively little JNI code that actually runs in the local frame - and it's even possible that any errors from JNI will be handled explicitly / internally before leaving the local frame. I quite like that the closure has complete freedom to return whatever it likes - which may not even be a Result if it's handling JNI errors internally.

[rib]

To give an update here @argv-minus-one - I tried out your iteration of this which has with_local_frame constrain the Result error to implement From<jni::errors::Error>.

At least for how I was starting to use with_local_frame this works fine for me and I'd be happy to go with that approach if we figure it's slightly less complex to avoid nesting Results here.

It'll probably also make sense to update the Executor APIs that wrap with_local_frame to be consistent with this too - though we can do that afterwards.