Skip to content

Commit

Permalink
fix(neon): Relax lifetime constraints on With and provide helper fo…
Browse files Browse the repository at this point in the history
…r forcing HRTB with JS values
  • Loading branch information
kjvalencik committed Dec 17, 2024
1 parent efba683 commit 30f4ddb
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 25 deletions.
6 changes: 3 additions & 3 deletions crates/neon/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,15 @@ pub use neon_macros::main;
/// ```
/// # #[cfg(all(feature = "napi-6", feature = "futures"))]
/// # {
/// # use neon::types::extract::{TryIntoJs, With};
/// # use neon::types::extract::{self, TryIntoJs};
/// #[neon::export]
/// async fn add(a: f64, b: f64) -> impl for<'cx> TryIntoJs<'cx> {
/// let sum = a + b;
///
/// With(move |_cx| {
/// extract::with(move |cx| {
/// println!("Hello from the JavaScript main thread!");
///
/// sum
/// sum.try_into_js(cx)
/// })
/// }
/// # }
Expand Down
2 changes: 1 addition & 1 deletion crates/neon/src/types_impl/extract/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ use crate::{
pub use self::{
boxed::Boxed,
error::{Error, TypeExpected},
with::With,
with::{with, With},
};

#[cfg(feature = "serde")]
Expand Down
55 changes: 42 additions & 13 deletions crates/neon/src/types_impl/extract/with.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
use crate::{context::Cx, result::JsResult, types::extract::TryIntoJs};
use crate::{
context::Cx,
result::JsResult,
types::{extract::TryIntoJs, Value},
};

/// Wraps a closure that will be lazily evaluated when [`TryIntoJs::try_into_js`] is
/// called.
///
/// Useful for executing arbitrary code on the main thread before returning from a
/// function exported with [`neon::export`](crate::export).
///
/// If you see the following error, due to [incorrect inference][issue], use [`with`]
/// instead.
///
/// [issue]: https://github.com/rust-lang/rust/issues/70263
///
/// ```text
/// error: implementation of `neon::types::extract::TryIntoJs` is not general enough
/// ```
///
/// ## Example
///
/// ```
/// # use neon::{prelude::*, types::extract::{TryIntoJs, With}};
/// # use neon::{prelude::*, types::extract::{self, TryIntoJs}};
/// use std::time::Instant;
///
/// #[neon::export(task)]
Expand All @@ -18,28 +31,44 @@ use crate::{context::Cx, result::JsResult, types::extract::TryIntoJs};
/// let sum = nums.into_iter().sum::<f64>();
/// let log = format!("sum took {} ms", start.elapsed().as_millis());
///
/// With(move |cx| -> NeonResult<_> {
/// extract::with(move |cx| -> NeonResult<_> {
/// cx.global::<JsObject>("console")?
/// .method(cx, "log")?
/// .arg(&log)?
/// .exec()?;
///
/// Ok(sum)
/// sum.try_into_js(cx)
/// })
/// }
/// ```
pub struct With<F, O>(pub F)
pub struct With<F>(pub F);

/// Helper to ensure correct inference of [lifetime bounds][hrtb] on closures
/// provided to [`With<F>`](With) without needing [explicit annotation][binder].
///
/// **Note:** The return type is [`JsResult`]. If you need to return a non-JavaScript type,
/// call [`TryIntoJs::try_into_js`].
///
/// _See [`With`](With#Example) for example usage._
///
/// [hrtb]: https://doc.rust-lang.org/nomicon/hrtb.html
/// [binder]: https://rust-lang.github.io/rfcs/3216-closure-lifetime-binder.html
pub fn with<V, F>(f: F) -> With<F>
where
// N.B.: We include additional required bounds to allow the compiler to infer the
// correct closure argument when using `impl for<'cx> TryIntoJs<'cx>`. Without
// these bounds, it would be necessary to write a more verbose signature:
// `With<impl for<'cx> FnOnce(&mut Cx<'cx>) -> SomeConcreteReturnType>`.
for<'cx> F: FnOnce(&mut Cx<'cx>) -> O;
V: Value,
for<'cx> F: FnOnce(&mut Cx<'cx>) -> JsResult<'cx, V>,

// N.B.: This bound ensures that the return type implements `TryIntoJs<'_>`
// without making it an opaque `impl Trait`.
for<'cx> With<F>: TryIntoJs<'cx, Value = V>,
{
With(f)
}

impl<'cx, F, O> TryIntoJs<'cx> for With<F, O>
impl<'cx, O, F> TryIntoJs<'cx> for With<F>
where
F: FnOnce(&mut Cx) -> O,
O: TryIntoJs<'cx>,
F: FnOnce(&mut Cx<'cx>) -> O,
{
type Value = O::Value;

Expand All @@ -48,4 +77,4 @@ where
}
}

impl<F, O> super::private::Sealed for With<F, O> where for<'cx> F: FnOnce(&mut Cx<'cx>) -> O {}
impl<F> super::private::Sealed for With<F> {}
8 changes: 4 additions & 4 deletions crates/neon/src/types_impl/function/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,19 +217,19 @@ impl<'cx> private::TryIntoArgumentsInternal<'cx> for () {
}
}

impl<'cx, F, O> private::TryIntoArgumentsInternal<'cx> for With<F, O>
impl<'cx, F, O> private::TryIntoArgumentsInternal<'cx> for With<F>
where
F: FnOnce(&mut Cx) -> O,
F: FnOnce(&mut Cx<'cx>) -> O,
O: private::TryIntoArgumentsInternal<'cx>,
{
fn try_into_args_vec(self, cx: &mut Cx<'cx>) -> NeonResult<private::ArgsVec<'cx>> {
(self.0)(cx).try_into_args_vec(cx)
}
}

impl<'cx, F, O> TryIntoArguments<'cx> for With<F, O>
impl<'cx, F, O> TryIntoArguments<'cx> for With<F>
where
F: FnOnce(&mut Cx) -> O,
F: FnOnce(&mut Cx<'cx>) -> O,
O: TryIntoArguments<'cx>,
{
}
Expand Down
7 changes: 7 additions & 0 deletions test/napi/lib/extract.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,11 @@ describe("Extractors", () => {
}
);
});

it("With", async () => {
assert.strictEqual(await addon.sleepWithJs(1.5), 1.5);
assert.strictEqual(await addon.sleepWithJsSync(1.5), 1.5);
assert.strictEqual(await addon.sleepWith(1.5), 1.5);
assert.strictEqual(await addon.sleepWithSync(1.5), 1.5);
});
});
42 changes: 42 additions & 0 deletions test/napi/src/js/extract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,45 @@ pub fn extract_either(either: Either<String, f64>) -> String {
Either::Right(n) => format!("Number: {n}"),
}
}

#[neon::export(task)]
// Ensure that `With` produces a closure that can be moved across thread boundaries
// and can return a JavaScript value.
fn sleep_with_js(n: f64) -> impl for<'cx> TryIntoJs<'cx> {
use std::{thread, time::Duration};

thread::sleep(Duration::from_millis(n as u64));

with(move |cx| Ok(cx.number(n)))
}

#[neon::export]
// Ensure that `With` can be used synchronously
fn sleep_with_js_sync(n: f64) -> impl for<'cx> TryIntoJs<'cx> {
sleep_with_js(n)
}

#[neon::export(task)]
// Ensure that `With` can be used Rust data
fn sleep_with(n: f64) -> impl for<'cx> TryIntoJs<'cx> {
use std::{thread, time::Duration};

// HACK: Force HRTB on the closure. Can be replaced with `closure_lifetime_binder`
// https://rust-lang.github.io/rfcs/3216-closure-lifetime-binder.html
fn bind<O, F>(f: F) -> F
where
for<'cx> F: FnOnce(&mut Cx<'cx>) -> O,
{
f
}

thread::sleep(Duration::from_millis(n as u64));

With(bind(move |_| n))
}

#[neon::export]
// Ensure that `With` can be used Rust data synchronously
fn sleep_with_sync(n: f64) -> impl for<'cx> TryIntoJs<'cx> {
sleep_with(n)
}
11 changes: 10 additions & 1 deletion test/napi/src/js/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,19 @@ pub fn call_js_function_with_bind_and_args_with(mut cx: FunctionContext) -> JsRe
}

pub fn call_js_function_with_bind_and_args_and_with(mut cx: FunctionContext) -> JsResult<JsNumber> {
// HACK: Force HRTB on the closure. Can be replaced with `closure_lifetime_binder`
// https://rust-lang.github.io/rfcs/3216-closure-lifetime-binder.html
fn bind<O, F>(f: F) -> F
where
for<'cx> F: FnOnce(&mut Cx<'cx>) -> O,
{
f
}

let n: f64 = cx
.argument::<JsFunction>(0)?
.bind(&mut cx)
.args(With(|_| (1, 2, 3)))?
.args(With(bind(|_| (1, 2, 3))))?
.call()?;
Ok(cx.number(n))
}
Expand Down
6 changes: 3 additions & 3 deletions test/napi/src/js/futures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use neon::{
prelude::*,
types::{
buffer::TypedArray,
extract::{Error, Json, TryIntoJs, With},
extract::{self, Error, Json, TryIntoJs},
},
};

Expand Down Expand Up @@ -129,9 +129,9 @@ fn async_with_events(
Ok(async move {
let res = data.into_iter().map(|(a, b)| a * b).collect::<Vec<_>>();

With(move |cx| -> NeonResult<_> {
extract::with(move |cx| -> NeonResult<_> {
emit(cx, "end")?;
Ok(res)
res.try_into_js(cx)
})
})
}
Expand Down

0 comments on commit 30f4ddb

Please sign in to comment.