simplify public API, only allowing global JVM references #147

nikomatsakis · 2024-05-04T01:27:57Z

This makes a number of simplifications to the public API for duchess. The idea is that we focus on "global" references by default. The duchess::Result<T> (for example) embeds a global reference to an exception, and the new default handle is duchess::Java<J>, which is a global reference. The prelude has been extended to include java and Java<T> as well, for convenience.

The to_rust, global, and execute combinators have all been merged. You just invoke execute and then the result depends on the return value: returning a Java<T> will create a global reference (like global used to do), returning a Rust value will invoke the "to rust" conversion (like to_rust used to do).

You can no longer obtain explicit handles to the JVM, preventing panics due to nested Jvm::with invocations (and hence fixing #142). Jvm::with is in fact crate local. This also avoids some potential unsoundness results that would have arisen from nested JVM usage.

Fixes #142

You now just do `execute::<R>` and it produces Rust values.

Don't need it anymore.

But they really need deeper rework. We should figure out how to enable testing.

The "execute" name will be reserved for things that convert to Rust. This is the underlying JNI operation without any conversion of the result.

Still pretty bad.

Local did so, tests rely on it. We should probably write a better impl though, one that invokes `toString` perhaps?

Rewrite tests a bit to have same effect without using crate-private method.

rcoh

looks good, had some naming comments on the internal APIs which can defer.

Probably will also need a changelog / update guidance for the next major release.

rcoh · 2024-05-06T14:51:12Z

test-crates/duchess-java-tests/tests/ui/exceptions.rs

- .unwrap()
- .expect("returns ok!");
-
- // This errors out because `try_extract_exception` calls `Jvm::with`.


can delete this comment

Suggested change

// This errors out because `try_extract_exception` calls `Jvm::with`.

rcoh · 2024-05-06T15:00:58Z

book/src/threat_model.md

@@ -39,7 +39,7 @@ This section introduces invariants maintained by Duchess using Rust's type syste

 ### The `'jvm` lifetime `&mut Jvm<'jvm>` is the innermost scope for local variables

-References to Java objects of type `J` are stored in a `Local<'jvm, J>` holder. Local references can come from the arguments to native functions or from `JvmOp::execute_with` calls. `execute_with` calls use the `'jvm`' lifetime found on the `Jvm<'jvm>` argument. This allows the `Local` to be used freely within that scope. It is therefore important that `'jvm` be constrained to the **innermost** valid scope.
+References to Java objects of type `J` are stored in a `Local<'jvm, J>` holder. Local references can come from the arguments to native functions or from `JvmOp::do_jni` calls. `do_jni` calls use the `'jvm`' lifetime found on the `Jvm<'jvm>` argument. This allows the `Local` to be used freely within that scope. It is therefore important that `'jvm` be constrained to the **innermost** valid scope.


I think we can delete the below section about Jvm::with now since Jvm::with is pub(crate)

rcoh · 2024-05-06T15:06:38Z

src/jvm.rs

 }
-
- fn execute_with<'jvm>(self, jvm: &mut Jvm<'jvm>) -> crate::Result<'jvm, Self::Output<'jvm>>;
+ /// Internal method


a slightly more detailed comment would probably be useful for folks working on the project since this is now the fundamental "do a thing" message

since execute_with converts into Rust vs. do_jni which keeps it in Java maybe we could rename execute_with something that mentioned the conversion into Rust

rcoh · 2024-05-06T15:08:03Z

macro/src/derive.rs

@@ -313,7 +310,7 @@ impl Driver<'_> {
 let pattern = variant.pat();
 return Ok(quote_spanned!(self.span() =>
 #pattern => {
- #binding .jderef().upcast().execute_with(jvm)


when would one call execute_with vs. do_jni? Both are on the JvmOp trait AFAICT and it seems like sometimes we call one or the other?

rcoh · 2024-05-06T15:09:45Z

src/jvm/test.rs


 #[test]
 fn nested_jvm_with() {
 Jvm::with(|_jvm| {
 let err = Jvm::with(|_jvm| Ok(())).expect_err("nested JVMs are illegal");
- assert!(matches!(err, duchess::Error::NestedUsage));
+ assert!(matches!(err, crate::Error::NestedUsage));


is this still a useful test since Jvm::with is pub_crate now? I guess for our own internal sake we want to avoid nesting it?

rcoh · 2024-05-06T15:12:38Z

src/error.rs

@@ -66,7 +60,7 @@ where
 }

 impl<'jvm> Error<Local<'jvm, Throwable>> {
- pub fn into_global(self, jvm: &mut Jvm<'jvm>) -> Error<Global<Throwable>> {


should this still be called into_global? It seems we're mostly erradicating the word from the codebase

rcoh · 2024-05-06T15:16:39Z

src/thread.rs

@@ -101,7 +101,7 @@ pub fn attach_permanently(jvm: JvmPtr) -> GlobalResult<AttachGuard> {
 })
 }

-pub unsafe fn attach<'jvm>(jvm: JvmPtr) -> GlobalResult<AttachGuard> {


while we're here we should add a safety section to the docs or make this not pub

nikomatsakis added 12 commits May 3, 2024 20:46

make "to_rust" an end-combinator

8159dd2

allow to_rust to create Global objects too

d8831fd

merge to_rust and execute

2b385bb

You now just do `execute::<R>` and it produces Rust values.

rename duchess::Result to duchess::LocalResult

67b18e4

rename GlobalResult to just Result

b29584c

remove global() combinator

7d6e7c7

Don't need it anymore.

rename Global to Java, add to prelude

328d399

update docs

77cd000

But they really need deeper rework. We should figure out how to enable testing.

rename execute_with to do_jni

af655e6

The "execute" name will be reserved for things that convert to Rust. This is the underlying JNI operation without any conversion of the result.

rename to_rust_with to execute_with

3034e6d

improve docs mildly

8e19fae

Still pretty bad.

Java needs to implement Debug

8b5d816

Local did so, tests rely on it. We should probably write a better impl though, one that invokes `toString` perhaps?

nikomatsakis requested a review from rcoh May 4, 2024 01:27

nikomatsakis force-pushed the global-by-default branch from 0511228 to 1237b91 Compare May 4, 2024 01:28

make Jvm::with crate private

53eb112

Rewrite tests a bit to have same effect without using crate-private method.

nikomatsakis force-pushed the global-by-default branch from 1237b91 to 53eb112 Compare May 4, 2024 01:39

rcoh approved these changes May 6, 2024

View reviewed changes

rcoh added this pull request to the merge queue May 11, 2024

Merged via the queue into duchess-rs:main with commit 424b146 May 11, 2024
7 checks passed

rcoh mentioned this pull request May 20, 2024

global combinator seems redundant? #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify public API, only allowing global JVM references #147

simplify public API, only allowing global JVM references #147

nikomatsakis commented May 4, 2024 •

edited

Loading

rcoh left a comment

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

rcoh May 6, 2024

simplify public API, only allowing global JVM references #147

simplify public API, only allowing global JVM references #147

Conversation

nikomatsakis commented May 4, 2024 • edited Loading

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 4, 2024 •

edited

Loading