Support for `bigint` #861

zone117x · 2022-03-01T13:00:38Z

Initial support for JsBigInt (neon) / bigint (js). So far implemented wrappers for creating and getting bigints using i64 and u64.

TODO:

Unit tests
Doc updates
Support arbitrary-length bigints via napi_create_bigint_words
- Support rust u128 and i128 types by automatically using the above arbitrary-length API

Please let me know if this PR is headed in the right direction. I created it against the next/0.10 branch as I had difficulties getting the main branch working correctly with the existing project I have to test this feature.

kjvalencik

@zone117x Thanks so much for this amazing contribution! I appreciate the thought that went into it! I have left some comments.

Additionally, you can remove neon-sys / legacy code. This backend is no longer supported and we can set some feature flags to avoid it being compiled.

I think my main question is what the API should look like. Is it better to have separate constructors for each supported from_ type or is it better to have a trait and a single generic constructor? I think I favor being generic. What do you think?

kjvalencik · 2022-03-01T13:58:58Z

crates/neon-runtime/src/napi/bindings/functions.rs

+
+            fn create_bigint_words(
+                env: Env,
+                sign_bit: isize,


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

kjvalencik · 2022-03-01T14:00:44Z

crates/neon-runtime/src/napi/bindings/functions.rs

+                env: Env,
+                sign_bit: isize,
+                word_count: usize,
+                words: *mut u64,


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

kjvalencik · 2022-03-01T14:01:32Z

crates/neon-runtime/src/napi/bindings/functions.rs

+            fn get_value_bigint_words(
+                env: Env,
+                value: Value,
+                sign_bit: *mut isize,


Same as previous comment, c_int or i32.

kjvalencik · 2022-03-01T14:01:57Z

crates/neon-runtime/src/napi/bindings/functions.rs

+                env: Env,
+                value: Value,
+                sign_bit: *mut isize,
+                words: *mut u64,


Missing word_count.

kjvalencik · 2022-03-01T14:03:26Z

crates/neon-runtime/src/napi/bigint.rs

+/// # 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 {


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

use neon_runtime::bigint; bigint::new(...)

kjvalencik · 2022-03-01T14:14:20Z

src/types/bigint.rs

+}
+
+#[derive(Debug)]
+pub struct GetBigIntLossyValueResult<T> {


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 {}

kjvalencik · 2022-03-01T14:15:34Z

src/types/bigint.rs

+    }
+}
+
+impl Into<i64> for GetBigIntLossyValueResult<i64> {


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

}

}

kjvalencik · 2022-03-01T14:18:06Z

src/types/bigint.rs

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


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.

kjvalencik · 2022-03-01T14:19:14Z

src/types/bigint.rs

+        let value = x.into();
+        let local = unsafe { neon_runtime::bigint::new_bigint(env, value) };
+        let bigint = Handle::new_internal(JsBigInt(local));
+        bigint


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

Suggested change

bigint

Handle::new_internal(JsBigInt(local))

kjvalencik · 2022-03-01T14:20:16Z

src/types/bigint.rs

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


Match the JavaScript capitalization.

Suggested change

"bigint".to_string()

"BigInt".to_string()

zone117x · 2022-03-01T16:10:55Z

I think my main question is what the API should look like. Is it better to have separate constructors for each supported from_ type or is it better to have a trait and a single generic constructor? I think I favor being generic. What do you think?

Good question, I've spent some time thinking about that myself. In my opinion, the Neon code for bigint constructors should generally match the napi code -- there are three ways to create a bigint: napi_create_bigint_int64, napi_create_bigint_uint64, and napi_create_bigint_words.

The first two (construct bigint directly from a 64-bit integer) seem like a very common use-case. E.g. a lot of codebases use 64-bit ints where js number doesn't work, but arbitrary-length ints are overkill and can hurt performance. I think we could use single trait + generic to combine the u64 and i64 for these two cases, but I don't think we should lump in arbitrary-sized ints into it.

What are your thoughts?

kjvalencik · 2022-03-01T16:52:17Z

I agree with u64 and i64 being the common use cases and making that nice. We might be able to leave off arbitrary precision until a someone brings a compelling use case. Technically all of this could be accomplished with an ArrayBuffer and the BigInt constructor, so I think it's okay if the first revision is incomplete.

With that said, I don't think putting it behind a single trait and method is harmful. The implementation would be generic and specialized and shouldn't hurt performance. Did you have some example in mind where it would be harmful?

The disadvantage to a single generic method is that the API is less discoverable. It requires reading the trait definition and implementations to know that you can send a u64 to the API.

dherman · 2022-03-03T23:03:01Z

This is awesome!

I think I favor the trait approach for the constructor, too. It's mildly more future-proof for other argument types (like strings, or maybe even third-party math crates) without needing to keep growing the API surface, and it makes for a nicely clean and general single cx.bigint() entry point too.

kjvalencik · 2022-03-03T23:13:04Z

Idea of what a trait approach might look like:

pub trait ToJsBigInt {
    fn to_big_int<'cx, C: Context<'cx>(&self, cx: &mut C) -> Handle<'cx, JsBigInt>;
}

impl JsBigInt {
    pub fn new<'cx, C: Context<'cx>, T: ToJsBigInt>(cx: &mut C, v: T) -> Handle<'cx, JsBigInt> {
        v.to_big_int(&mut cx)
    }
}

impl ToJsBigInt for u64 {
    fn to_big_int<'cx, C: Context<'cx>(&self, cx: &mut C) -> Handle<'cx, JsBigInt> {
        todo!()
    }
}

impl ToJsBigInt for i64 {
    fn to_big_int<'cx, C: Context<'cx>(&self, cx: &mut C) -> Handle<'cx, JsBigInt> {
        todo!()
    }
}

impl ToJsBigInt for &[u64] {
    fn to_big_int<'cx, C: Context<'cx>(&self, cx: &mut C) -> Handle<'cx, JsBigInt> {
        todo!()
    }
}

trait Context<'cx> {
    fn big_int<T: ToJsBigInt>(&mut self, v: T) -> Handle<'cx, JsBigInt> {
        v.to_big_int(&mut cx)
    }
}

Since we can't provide multiple blanket impl for Into<_> without conflicting, it does require creating from types like u32 to hint the type.

I.e., instead of JsBigInt::from_u64(&mut cx, 0u32) it becomes JsBigInt::new(&mut cx, u64::from(0u32)).

zone117x · 2022-03-09T12:49:32Z

Thanks for the feedback, I'll address the updates as soon as I have some time.

@kjvalencik was this PR closed intentionally?

kjvalencik · 2022-03-09T13:51:23Z

@zone117x Sorry! It was closed unintentionally when the next/0.10 branch was deleted after merging it into main.

I've re-opened this PR and changed the target branch to main.

kjvalencik · 2023-02-06T14:05:07Z

@zone117x is this still something you are interested in working on? If not, I have some bandwidth to take it over--giving you credit, of course!

Cheers.

zone117x · 2023-02-06T15:51:11Z

@zone117x is this still something you are interested in working on? If not, I have some bandwidth to take it over--giving you credit, of course!

Cheers.

Sorry, I do not have bandwidth to continue working on this. Please feel free to take over!

kjvalencik · 2023-02-24T17:59:00Z

@zone117x Thanks again for working on this! This feature was merged in #963 with you as co-author. Cheers!

zone117x added 3 commits March 1, 2022 12:36

feat: initial bigint support

6609523

feat: bigint from/to i64 and u64

15adb7c

chore: cargo fmt

bdb936a

kjvalencik requested changes Mar 1, 2022

View reviewed changes

kjvalencik deleted the branch neon-bindings:main March 4, 2022 17:47

kjvalencik closed this Mar 4, 2022

kjvalencik reopened this Mar 9, 2022

kjvalencik changed the base branch from next/0.10 to main March 9, 2022 13:50

kjvalencik closed this Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `bigint` #861

Support for `bigint` #861

zone117x commented Mar 1, 2022

kjvalencik left a comment •

edited

Loading

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

kjvalencik Mar 1, 2022

zone117x commented Mar 1, 2022

kjvalencik commented Mar 1, 2022

dherman commented Mar 3, 2022

kjvalencik commented Mar 3, 2022 •

edited

Loading

zone117x commented Mar 9, 2022

kjvalencik commented Mar 9, 2022

kjvalencik commented Feb 6, 2023

zone117x commented Feb 6, 2023

kjvalencik commented Feb 24, 2023

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

Support for bigint #861

Support for bigint #861

Conversation

zone117x commented Mar 1, 2022

kjvalencik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zone117x commented Mar 1, 2022

kjvalencik commented Mar 1, 2022

dherman commented Mar 3, 2022

kjvalencik commented Mar 3, 2022 • edited Loading

zone117x commented Mar 9, 2022

kjvalencik commented Mar 9, 2022

kjvalencik commented Feb 6, 2023

zone117x commented Feb 6, 2023

kjvalencik commented Feb 24, 2023

Support for `bigint` #861

Support for `bigint` #861

kjvalencik left a comment •

edited

Loading

kjvalencik commented Mar 3, 2022 •

edited

Loading