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

Wrap all Node-API functions in neon-runtime to check status #802

Open
kjvalencik opened this issue Sep 24, 2021 · 4 comments
Open

Wrap all Node-API functions in neon-runtime to check status #802

kjvalencik opened this issue Sep 24, 2021 · 4 comments

Comments

@kjvalencik
Copy link
Member

Currently every usage of a Node-API function checks the status. This is almost always required for safety, but it's easy to fix.

Since all Node-API FFI bindings are defined in a macro and all functions return napi_status, we could wrap them to return Result<(), Status> instead and use the linter to ensure the Result is checked.

mod node_api {
    extern "C" {
        fn get_undefined(env: Env, result: *mut Value) -> Status;
    }    
}

pub unsafe fn get_undefined(env: Env, result: *mut Value) -> Result<(), Status> {
    match node_api::get_undefined(env, result) {
        Status::Ok => Ok(()),
        status => Err(status),
    }
}
@kjvalencik
Copy link
Member Author

@kmescharikov The wrappers are defined by this macro here:
https://github.com/neon-bindings/neon/blob/main/crates/neon-runtime/src/napi/bindings/mod.rs#L127-L129

Then all the usages of these functions in neon-runtime would need to be updated to handle the new return type. In most cases, it will be removing an assert and replacing it with an unwrap.

@kmescharikov
Copy link

Thank you! I'll try

@kmescharikov
Copy link

kmescharikov commented Feb 19, 2022

Good evening! I apologize for the long absence, I have a lot of work to do. I don't quite understand the following points:
Many functions already use napi_status, and there is no way to replace assert_eq with unwrap , since status does not have such a method.

  1. Here is an example of a function where I was able to make changes successfully. Are this correct? Tests are passing

image

2.Here is an example of a function where I can't use unwrap so I don't understand what I should do and should I?
image

  1. Also I have an errors about unresolved imports, should I fixed this by importing Env/Local/Value from super::raw or simple super:: ?

image

  1. Also should I replace using of Local to Value ?

image

image

@akonradi-signal
Copy link
Contributor

A suggestion: Result<(), Status> is useful but has the unfortunate ability to hold a Err(napi::Status::Ok) which is rather ambiguous. This project would yield a lot more mileage if the error type for the result couldn't hold a success value. I'd suggest defining a new type that has the same variants as the existing Status except for Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants