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

Support for bigint #861

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/neon-runtime/src/nan/primitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,7 @@ pub use neon_sys::Neon_Primitive_Number as number;

/// Gets the underlying value of a `v8::Number` object.
pub use neon_sys::Neon_Primitive_NumberValue as number_value;

pub use neon_sys::Neon_Primitive_BigInt as bigint;

pub use neon_sys::Neon_Primitive_BigIntValue as bigint_value;
3 changes: 3 additions & 0 deletions crates/neon-runtime/src/nan/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ pub use neon_sys::Neon_Tag_IsNull as is_null;
/// Indicates if the value type is `Number`.
pub use neon_sys::Neon_Tag_IsNumber as is_number;

/// Indicates if the value type is `BigInt`.
pub use neon_sys::Neon_Tag_IsBigInt as is_bigint;

/// Indicates if the value type is `Boolean`.
pub use neon_sys::Neon_Tag_IsBoolean as is_boolean;

Expand Down
57 changes: 57 additions & 0 deletions crates/neon-runtime/src/napi/bigint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
use crate::napi::bindings as napi;
use crate::raw::{Env, Local};
use std::mem::MaybeUninit;

/// Create a new bigint object
///
/// # Safety
///
/// `env` is a raw pointer. Please ensure it points to a napi_env that is valid for the current context.
pub unsafe fn new_bigint(env: Env, value: i64) -> Local {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, name this new since it's namespaced by the bigint module.

use neon_runtime::bigint;

bigint::new(...)

let mut local = MaybeUninit::zeroed();
let status = napi::create_bigint_int64(env, value, local.as_mut_ptr());
assert_eq!(status, napi::Status::Ok);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked to see if this can ever throw an exception--which is necessary for this assert to be safe. Can you double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjvalencik do you have some guidance on how to determine this? The napi docs for the function do not mention exceptions -- do I need to check the nodejs or v8 source code to determine this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not super clear. It's a manual task that's a combination of reading the Node/Node-API source and sometimes the JS spec.

I expect that this can't ever fail except with bad data (which Rust type system protects against).

local.assume_init()
}

/// Create a new bigint object
///
/// # Safety
///
/// `env` is a raw pointer. Please ensure it points to a napi_env that is valid for the current context.
pub unsafe fn new_bigint_from_u64(env: Env, value: u64) -> Local {
let mut local = MaybeUninit::zeroed();
let status = napi::create_bigint_uint64(env, value, local.as_mut_ptr());
assert_eq!(status, napi::Status::Ok);
local.assume_init()
}

/// Get the value of a bigint object
///
/// # Safety
///
/// `env` is a raw pointer. Please ensure it points to a napi_env that is valid for the current context.
/// `Local` must be an NAPI value associated with the given `Env`
pub unsafe fn value_i64(env: Env, p: Local) -> (i64, bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you return a Result instead? It feels like more idiomatic Rust to me.

Suggested change
pub unsafe fn value_i64(env: Env, p: Local) -> (i64, bool) {
pub unsafe fn value_i64(env: Env, p: Local) -> Result<i64, i64> {

Result::Err indicates that loss occurred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea 👍
I struggled with what to return here.

let mut value: i64 = 0;
let mut lossless = false;
let status =
napi::get_value_bigint_int64(env, p, &mut value as *mut _, &mut lossless as *mut _);
assert_eq!(status, napi::Status::Ok);
(value, lossless)
}

/// Get the value of a bigint object
///
/// # Safety
///
/// `env` is a raw pointer. Please ensure it points to a napi_env that is valid for the current context.
/// `Local` must be an NAPI value associated with the given `Env`
pub unsafe fn value_u64(env: Env, p: Local) -> (u64, bool) {
let mut value: u64 = 0;
let mut lossless = false;
let status =
napi::get_value_bigint_uint64(env, p, &mut value as *mut _, &mut lossless as *mut _);
assert_eq!(status, napi::Status::Ok);
(value, lossless)
}
32 changes: 32 additions & 0 deletions crates/neon-runtime/src/napi/bindings/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,38 @@ mod napi6 {
) -> Status;

fn get_instance_data(env: Env, data: *mut *mut c_void) -> Status;

fn create_bigint_int64(env: Env, value: i64, result: *mut Value) -> Status;

fn create_bigint_uint64(env: Env, value: u64, result: *mut Value) -> Status;

fn create_bigint_words(
env: Env,
sign_bit: isize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a std::os::raw::c_int or i32.

word_count: usize,
words: *mut u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
words: *mut u64,
words: *const u64,

Also, result is missing. I recommend using the bindings.rs output from nodejs-sys as a reference.

cat ./target/debug/build/nodejs-sys-*/out/bindings.rs

) -> Status;

fn get_value_bigint_int64(
env: Env,
value: Value,
result: *mut i64,
lossless: *mut bool,
) -> Status;

fn get_value_bigint_uint64(
env: Env,
value: Value,
result: *mut u64,
lossless: *mut bool,
) -> Status;

fn get_value_bigint_words(
env: Env,
value: Value,
sign_bit: *mut isize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment, c_int or i32.

words: *mut u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing word_count.

) -> Status;
}
);
}
Expand Down
2 changes: 2 additions & 0 deletions crates/neon-runtime/src/napi/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
pub mod array;
pub mod arraybuffer;
pub mod async_work;
#[cfg(feature = "napi-6")]
pub mod bigint;
pub mod buffer;
pub mod call;
pub mod convert;
Expand Down
4 changes: 4 additions & 0 deletions crates/neon-runtime/src/napi/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ pub unsafe fn is_object(env: Env, val: Local) -> bool {
is_type(env, val, napi::ValueType::Object)
}

pub unsafe fn is_bigint(env: Env, val: Local) -> bool {
is_type(env, val, napi::ValueType::BigInt)
}

pub unsafe fn is_array(env: Env, val: Local) -> bool {
let mut result = false;
assert_eq!(
Expand Down
4 changes: 4 additions & 0 deletions crates/neon-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ extern "C" {
pub fn Neon_Primitive_Number(out: &mut Local, isolate: Isolate, v: f64);
pub fn Neon_Primitive_NumberValue(p: Local) -> f64;

pub fn Neon_Primitive_BigInt(out: &mut Local, isolate: Isolate, v: i64);
pub fn Neon_Primitive_BigIntValue(out: &mut Local, isolate: Isolate, v: i64);

pub fn Neon_Scope_Escape(
isolate: Isolate,
out: &mut Local,
Expand Down Expand Up @@ -295,6 +298,7 @@ extern "C" {
pub fn Neon_Tag_IsUndefined(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsNull(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsNumber(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsBigInt(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsBoolean(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsString(isolate: Isolate, val: Local) -> bool;
pub fn Neon_Tag_IsObject(isolate: Isolate, val: Local) -> bool;
Expand Down
8 changes: 8 additions & 0 deletions src/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ use crate::lifecycle::InstanceData;
use crate::object::class::Class;
use crate::object::{Object, This};
use crate::result::{JsResult, NeonResult, Throw};
#[cfg(feature = "napi-6")]
use crate::types::bigint::JsBigInt;
#[cfg(feature = "napi-1")]
use crate::types::boxed::{Finalize, JsBox};
#[cfg(feature = "napi-1")]
Expand Down Expand Up @@ -530,6 +532,12 @@ pub trait Context<'a>: ContextInternal<'a> {
JsDate::new(self, value)
}

#[cfg(feature = "napi-6")]
#[cfg_attr(docsrs, doc(cfg(feature = "napi-6")))]
fn bigint(&mut self, value: impl Into<i64>) -> Handle<'a, JsBigInt> {
JsBigInt::new(self, value)
}

/// Produces a handle to the JavaScript global object.
fn global(&mut self) -> Handle<'a, JsObject> {
JsObject::build(|out| unsafe {
Expand Down
2 changes: 2 additions & 0 deletions src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ pub use crate::task::Task;
#[cfg(feature = "legacy-runtime")]
#[doc(no_inline)]
pub use crate::types::BinaryData;
#[cfg(feature = "napi-6")]
pub use crate::types::JsBigInt;
#[cfg(all(feature = "napi-1", feature = "promise-api"))]
#[doc(no_inline)]
pub use crate::types::JsPromise;
Expand Down
107 changes: 107 additions & 0 deletions src/types/bigint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
use super::{private::ValueInternal, Value};
use crate::context::internal::Env;
use crate::context::Context;
use crate::handle::{internal::TransparentNoCopyWrapper, Handle, Managed};
use neon_runtime;
use neon_runtime::raw;
use std::fmt;
use std::fmt::Debug;

#[repr(C)]
#[derive(Debug, Copy, Clone)]
#[cfg_attr(docsrs, doc(cfg(feature = "napi-6")))]
pub struct JsBigInt(raw::Local);

impl Value for JsBigInt {}

unsafe impl TransparentNoCopyWrapper for JsBigInt {
type Inner = raw::Local;

fn into_inner(self) -> Self::Inner {
self.0
}
}

impl Managed for JsBigInt {
fn to_raw(&self) -> raw::Local {
self.0
}

fn from_raw(_: Env, h: raw::Local) -> Self {
JsBigInt(h)
}
}

#[derive(Debug)]
pub struct GetBigIntLossyValueResult<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fairly common in Rust to put the data in the Err variant when it is still usable after a partial failure. For example, see LockResult on Mutex.

I think it would be more idiomatic to create an Error wrapper (mostly for clear printing and ? error propagation) that indicates data loss. If the user doesn't mind the loss, they can add .unwrap_or_else(Error::into_inner).

struct Error<T>(T);

impl<T> Error<T> {
    fn into_inner(self) -> T {
        self.0
    }
}

impl<T> std::error::Error for Error {}

pub value: T,
/// Indicates whether the BigInt value was converted losslessly
pub lossless: bool,
}

impl<T> fmt::Display for GetBigIntLossyValueResult<T>
where
T: fmt::Display,
{
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.value)
}
}

impl Into<i64> for GetBigIntLossyValueResult<i64> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically From is implemented instead of Into since there is a reflexive implementation of Into.

Suggested change
impl Into<i64> for GetBigIntLossyValueResult<i64> {
impl From<GetBigIntLossyValueResult<i64>> for i64 {
fn from(other: GetBigIntLossyValueResult<i64>) -> Self {
self.value
}
}

fn into(self) -> i64 {
self.value
}
}

impl Into<u64> for GetBigIntLossyValueResult<u64> {
fn into(self) -> u64 {
self.value
}
}

impl JsBigInt {
pub fn new<'a, C: Context<'a>, T: Into<i64>>(cx: &mut C, x: T) -> Handle<'a, JsBigInt> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making this generic over a trait? It could work well to allow providing an implementation for num-bigint.

Nit, n is preferable to x for consistency with other code in Neon.

let env = cx.env().to_raw();
let value = x.into();
let local = unsafe { neon_runtime::bigint::new_bigint(env, value) };
let bigint = Handle::new_internal(JsBigInt(local));
bigint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit let the type be returned directly instead of the unused variable binding.

Suggested change
bigint
Handle::new_internal(JsBigInt(local))

}

pub fn from_u64<'a, C: Context<'a>, T: Into<u64>>(cx: &mut C, x: T) -> Handle<'a, JsBigInt> {
let env = cx.env().to_raw();
let value = x.into();
let local = unsafe { neon_runtime::bigint::new_bigint_from_u64(env, value) };
let bigint = Handle::new_internal(JsBigInt(local));
bigint
}

pub fn value_i64<'a, C: Context<'a>>(self, cx: &mut C) -> GetBigIntLossyValueResult<i64> {
let env = cx.env().to_raw();
let result = unsafe { neon_runtime::bigint::value_i64(env, self.to_raw()) };
GetBigIntLossyValueResult {
value: result.0,
lossless: result.1,
}
}

pub fn value_u64<'a, C: Context<'a>>(self, cx: &mut C) -> GetBigIntLossyValueResult<u64> {
let env = cx.env().to_raw();
let result = unsafe { neon_runtime::bigint::value_u64(env, self.to_raw()) };
GetBigIntLossyValueResult {
value: result.0,
lossless: result.1,
}
}
}

impl ValueInternal for JsBigInt {
fn name() -> String {
"bigint".to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match the JavaScript capitalization.

Suggested change
"bigint".to_string()
"BigInt".to_string()

}

fn is_typeof<Other: Value>(env: Env, other: &Other) -> bool {
unsafe { neon_runtime::tag::is_bigint(env.to_raw(), other.to_raw()) }
}
}
4 changes: 4 additions & 0 deletions src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ pub mod function;
#[cfg(all(feature = "napi-1", feature = "promise-api"))]
pub(crate) mod promise;

#[cfg(feature = "napi-6")]
pub(crate) mod bigint;
pub(crate) mod private;
pub(crate) mod utf8;

Expand All @@ -106,6 +108,8 @@ use std::fmt::Debug;
use std::marker::PhantomData;
use std::os::raw::c_void;

#[cfg(feature = "napi-6")]
pub use self::bigint::JsBigInt;
#[cfg(feature = "legacy-runtime")]
pub use self::binary::{BinaryData, BinaryViewType, JsArrayBuffer, JsBuffer};
#[cfg(feature = "napi-1")]
Expand Down
6 changes: 6 additions & 0 deletions test/napi/src/js/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ pub fn is_number(mut cx: FunctionContext) -> JsResult<JsBoolean> {
Ok(cx.boolean(result))
}

pub fn is_bigint(mut cx: FunctionContext) -> JsResult<JsBoolean> {
let val: Handle<JsValue> = cx.argument(0)?;
let result = val.is_a::<JsBigInt, _>(&mut cx);
Ok(cx.boolean(result))
}

pub fn is_object(mut cx: FunctionContext) -> JsResult<JsBoolean> {
let val: Handle<JsValue> = cx.argument(0)?;
let result = val.is_a::<JsObject, _>(&mut cx);
Expand Down
1 change: 1 addition & 0 deletions test/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> {
cx.export_function("is_error", is_error)?;
cx.export_function("is_null", is_null)?;
cx.export_function("is_number", is_number)?;
cx.export_function("is_bigint", is_bigint)?;
cx.export_function("is_object", is_object)?;
cx.export_function("is_string", is_string)?;
cx.export_function("is_undefined", is_undefined)?;
Expand Down