From ee3caf990b727c60d48618b560dd2b679d94c92b Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 30 May 2020 11:22:20 -0700 Subject: [PATCH 1/4] Initial draft of `try_catch` RFC. --- text/0000-try-catch.md | 104 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 text/0000-try-catch.md diff --git a/text/0000-try-catch.md b/text/0000-try-catch.md new file mode 100644 index 0000000..8ac242a --- /dev/null +++ b/text/0000-try-catch.md @@ -0,0 +1,104 @@ +- Feature Name: try_catch +- Start Date: 2020-05-21 +- RFC PR: (leave this empty) +- Neon Issue: (leave this empty) + +# Summary +[summary]: #summary + +This RFC proposes a new `Context` method for catching any JavaScript exceptions that may be thrown during the execution of a computation. The result of the API uses Rust's standard `Result` enum to distinguish between normal return values and exceptional values: +```rust +match cx.try_catch(|cx| { cx.throw(42) }) { + Ok(v) => { /* ... computation produced a normal result ... */ } + Err(Catch(v)) => { /* ... computation threw an exception ... */ } +} +``` + +# Motivation +[motivation]: #motivation + +Today, Neon APIs that throw JavaScript exceptions product a Rust `Result` with the `Throw` sentinel value, which simply indicates that the JS VM has entered into a throwing state. But they do not offer a way to model the JavaScript `catch` semantics, which restores the VM to non-throwing state and extracts the exceptional value that was being thrown. + +This RFC proposes an idiomatic way to mimic JavaScript's `try`/`catch` syntax with a Neon API, which "tries" a computation encapsulated in a Rust callback and "catches" any exception that gets thrown and reflects that in an idiomatic Rust `Result`. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +## Throwing vs non-throwing state + +The JavaScript VM can be in a _throwing state_ or _non-throwing state_. Any API that can trigger the throwing of a JavaScript exception value sets the VM into throwing state. **When the VM is in a throwing state, most Neon APIs are not allowed to be called and will panic.** + +Every Neon API is documented as either _safe for throwing state_, meaning that the API can safely be called without panicking even when the VM is in a throwing state, or _unsafe for throwing state_, meaning that it will panic if called when the VM is in a throwing state. + +## Catching exceptions + +The `try_catch` method runs a callback and intercepts any thrown exception value and ensures the JavaScript VM is restored to non-throwing state. + +Example: + +```rust +let v = match cx.try_catch(|cx| { cx.throw(v) }) { + Ok(v) => v, + Err(Catch(v)) => v +}; +``` + +## Inspecting the exception state + +At any time, the VM can be inspected to detect whether it is a throwing or non-throwing state: + +```rust +cx.is_throwing() +``` + +The `is_throwing()` method is safe for throwing state. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +The `neon::result` module defines a `Catch` type that represents a caught exception value: + +```rust +pub struct Catch<'a, T>(Handle<'a, T>); +``` + +The `neon::context::Context` trait gets two new methods: + +```rust +pub trait Context<'a> { + fn is_throwing(&self) -> bool; + fn try_catch(&self, f: F) -> Result> + where + T: Value, + U: Value, + F: for<'b> FnOnce(CatchContext<'b>) -> JsResult<'b, T>; +} +``` + +The `is_throwing` method can be implemented with the N-API [`napi_is_exception_pending`](https://nodejs.org/api/n-api.html#n_api_napi_is_exception_pending) method in the upcoming N-API backend, or the underlying V8 API in the legacy backend. + +The `try_catch` method can be implemented with the N-API [`napi_get_and_clear_last_exception`](https://nodejs.org/api/n-api.html#n_api_napi_get_and_clear_last_exception), or with [`Nan::TryCatch`](https://github.com/nodejs/nan/blob/master/doc/errors.md#api_nan_try_catch) in the legacy backend. + +# Drawbacks +[drawbacks]: #drawbacks + +This adds a bit more size to the Neon API but it's a core missing primitive. Note also that it's a primitive in the V8 C++ API, the Nan API, and N-API. It's really just necessary. Otherwise people have to propagate all exceptions back to JS and catch them there. + +# Rationale and alternatives +[alternatives]: #alternatives + +Alternatives: +- A two-callback mirror of `try`/`catch` (e.g., `cx.try_catch(|cx| { cx.throw(42) }, |v, cx| { /* ... */ }))`) +- A builder pattern mirror of `try`/`catch` (e.g., `cx.try_catch(|cx| { cx.throw(42) }).with(|v, cx| { /* ... */ })`) + +I tried these approaches before and they were just awkward and less readable. Note also the more awkward naming and awkward closure arguments. + +Alternative naming: +- `cx.r#try()`: feels more correct but the lexical syntax is super unfortunate. +- `cx.catch()`: prettier than `try_catch` but not very accurate naming. + +# Unresolved questions +[unresolved]: #unresolved-questions + +- We need some implementation experience to get the API signatures exactly right, especially the lifetimes. +- We need some usage experience to see how ergonomic the reflection of exceptions into a `Result` type turns out to be. From b00a1a3dbadb852cc6ccbb149863f2b42728e131 Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 30 May 2020 12:06:10 -0700 Subject: [PATCH 2/4] Remove unnecessary `Catch` wrapper type. --- text/0000-try-catch.md | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/text/0000-try-catch.md b/text/0000-try-catch.md index 8ac242a..b2e9cd6 100644 --- a/text/0000-try-catch.md +++ b/text/0000-try-catch.md @@ -10,7 +10,7 @@ This RFC proposes a new `Context` method for catching any JavaScript exceptions ```rust match cx.try_catch(|cx| { cx.throw(42) }) { Ok(v) => { /* ... computation produced a normal result ... */ } - Err(Catch(v)) => { /* ... computation threw an exception ... */ } + Err(v) => { /* ... computation threw an exception ... */ } } ``` @@ -39,7 +39,7 @@ Example: ```rust let v = match cx.try_catch(|cx| { cx.throw(v) }) { Ok(v) => v, - Err(Catch(v)) => v + Err(v) => v }; ``` @@ -56,18 +56,12 @@ The `is_throwing()` method is safe for throwing state. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -The `neon::result` module defines a `Catch` type that represents a caught exception value: - -```rust -pub struct Catch<'a, T>(Handle<'a, T>); -``` - The `neon::context::Context` trait gets two new methods: ```rust pub trait Context<'a> { fn is_throwing(&self) -> bool; - fn try_catch(&self, f: F) -> Result> + fn try_catch(&self, f: F) -> Result, Handle<'a, U>> where T: Value, U: Value, From 2d4ee792298f1760d5607950ef42b2cd1ca9d482 Mon Sep 17 00:00:00 2001 From: David Herman Date: Tue, 14 Jul 2020 21:32:08 -0700 Subject: [PATCH 3/4] Updates based on feedback in the RFC thread + implementation experience. --- text/0000-try-catch.md | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/text/0000-try-catch.md b/text/0000-try-catch.md index b2e9cd6..cc882c5 100644 --- a/text/0000-try-catch.md +++ b/text/0000-try-catch.md @@ -60,19 +60,30 @@ The `neon::context::Context` trait gets two new methods: ```rust pub trait Context<'a> { + // N-API only fn is_throwing(&self) -> bool; - fn try_catch(&self, f: F) -> Result, Handle<'a, U>> + + fn try_catch<'b: 'a, T, F>(&mut self, f: F) -> Result, Handle<'a, JsValue>> where T: Value, - U: Value, - F: for<'b> FnOnce(CatchContext<'b>) -> JsResult<'b, T>; + F: FnOnce(&mut Self) -> JsResult<'b, T>; } ``` -The `is_throwing` method can be implemented with the N-API [`napi_is_exception_pending`](https://nodejs.org/api/n-api.html#n_api_napi_is_exception_pending) method in the upcoming N-API backend, or the underlying V8 API in the legacy backend. +The `is_throwing` method can be implemented with the N-API [`napi_is_exception_pending`](https://nodejs.org/api/n-api.html#n_api_napi_is_exception_pending) method in the upcoming N-API backend. If the implementation would be too complicated, this method may not be supported with the legacy backend. The `try_catch` method can be implemented with the N-API [`napi_get_and_clear_last_exception`](https://nodejs.org/api/n-api.html#n_api_napi_get_and_clear_last_exception), or with [`Nan::TryCatch`](https://github.com/nodejs/nan/blob/master/doc/errors.md#api_nan_try_catch) in the legacy backend. +For the legacy backend, the type parameter `F` has an additional constraint: + +```rust +F: std::panic::UnwindSafe +``` + +This restriction can be lifted for the N-API backend, which is a backwards-compatible change. + +If the callback to `try_catch` produces `Err(Throw)` but the VM is not actually in a throwing state, the `try_catch` function panics. + # Drawbacks [drawbacks]: #drawbacks @@ -87,6 +98,10 @@ Alternatives: I tried these approaches before and they were just awkward and less readable. Note also the more awkward naming and awkward closure arguments. +We also tried removing the context argument from the callback, since the API simply passes `self` down to the callback, but Rust's type rules for `&mut` references prevent the inner callback from referencing the context since calling `cx.try_catch()` effectively locks `cx`. + +We considered making the type of `try_catch` generic in the return type as well, but this would require doing a downcast, and the goal of `try_catch` is to be infallible so that the `Result` return type only represents a caught JS exception. + Alternative naming: - `cx.r#try()`: feels more correct but the lexical syntax is super unfortunate. - `cx.catch()`: prettier than `try_catch` but not very accurate naming. @@ -94,5 +109,4 @@ Alternative naming: # Unresolved questions [unresolved]: #unresolved-questions -- We need some implementation experience to get the API signatures exactly right, especially the lifetimes. - We need some usage experience to see how ergonomic the reflection of exceptions into a `Result` type turns out to be. From ae555f92bd6022213fb3363857865b38a039640f Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 21 Nov 2020 06:00:38 -0800 Subject: [PATCH 4/4] Signature changes based on suggestions from #630 - Generalize the success result type to a generic `T: Sized` instead of a handle - Simplify the lifetime bound to the context's `'a` instead of a generic `'b: 'a` --- text/0000-try-catch.md | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/text/0000-try-catch.md b/text/0000-try-catch.md index cc882c5..dd59c65 100644 --- a/text/0000-try-catch.md +++ b/text/0000-try-catch.md @@ -63,10 +63,10 @@ pub trait Context<'a> { // N-API only fn is_throwing(&self) -> bool; - fn try_catch<'b: 'a, T, F>(&mut self, f: F) -> Result, Handle<'a, JsValue>> + fn try_catch(&mut self, f: F) -> Result> where - T: Value, - F: FnOnce(&mut Self) -> JsResult<'b, T>; + T: Sized, + F: FnOnce(&mut Self) -> JsResult<'a, T>; } ``` @@ -74,14 +74,6 @@ The `is_throwing` method can be implemented with the N-API [`napi_is_exception_p The `try_catch` method can be implemented with the N-API [`napi_get_and_clear_last_exception`](https://nodejs.org/api/n-api.html#n_api_napi_get_and_clear_last_exception), or with [`Nan::TryCatch`](https://github.com/nodejs/nan/blob/master/doc/errors.md#api_nan_try_catch) in the legacy backend. -For the legacy backend, the type parameter `F` has an additional constraint: - -```rust -F: std::panic::UnwindSafe -``` - -This restriction can be lifted for the N-API backend, which is a backwards-compatible change. - If the callback to `try_catch` produces `Err(Throw)` but the VM is not actually in a throwing state, the `try_catch` function panics. # Drawbacks