Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: try_catch #29

Merged
merged 4 commits into from
Jul 2, 2021
Merged

RFC: try_catch #29

merged 4 commits into from
Jul 2, 2021

Conversation

dherman
Copy link
Contributor

@dherman dherman commented May 30, 2020

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:

match cx.try_catch(|cx| { cx.throw(42) }) {
    Ok(v) => { /* ... computation produced a normal result ... */ }
    Err(v) => { /* ... computation threw an exception ... */ }
}

Rendered

kjvalencik
kjvalencik previously approved these changes Jun 18, 2020
@kjvalencik kjvalencik dismissed their stale review June 18, 2020 22:06

Requesting updates

text/0000-try-catch.md Outdated Show resolved Hide resolved
text/0000-try-catch.md Outdated Show resolved Hide resolved
text/0000-try-catch.md Outdated Show resolved Hide resolved
@kjvalencik
Copy link
Member

A user may choose not to operate on the Result of a try_catch. With the current design, a small amount of boilerplate to transform the Result<Handle<'a, T>, JsValue<'a>> into a JsResult<'a, T> for returning from a method:

let value = match result {
    Ok(v) => Ok(v),
    Err(err) => cx.throw(err)?,
};

I propose adding to this RFC an implementation of JsResultExt to remove ease this boilerplate:

impl <'a, V, E> JsResultExt<'a, V> for Result<Handle<'a, V>, Handle<'a, E>>
where
    V: Value,
    E: Value,
{
    fn or_throw<'b, C: Context<'b>>(self, cx: &mut C) -> JsResult<'a, V> {
        match self {
            Ok(v) => Ok(v),
            Err(err) => cx.throw(err),
        }
    }
}

This would simplify to:

let value = result.or_throw(value)?;

@dherman dherman mentioned this pull request Jul 7, 2020
10 tasks
@kjvalencik
Copy link
Member

Notes from feature flagged implementation:

  • There are some additional trait bounds required (notably UnwindSafe for legacy)
  • Lifetimes are slightly different (requires 'b: 'a)
  • Documentation should include the panic behavior on a manually constructed Err(Throw)

@kjvalencik kjvalencik added the final comment period last opportunity to discuss the proposed conclusion label Sep 18, 2020
@goto-bus-stop goto-bus-stop changed the base branch from master to main October 8, 2020 13:45
text/0000-try-catch.md Outdated Show resolved Hide resolved
- 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`
@dherman dherman merged commit 340b75f into neon-bindings:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final comment period last opportunity to discuss the proposed conclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants