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 BigInt for i64 and u64 #24

Merged
merged 19 commits into from
Dec 24, 2021
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Serialization is compatible with the deserialization, but it's limited to a sing
- `Array` for any Rust sequences.
- `Uint8Array` for byte buffers.
- Plain JavaScript object for typed Rust structures.
- `Number` for `u64` & `i64` (Can be configured to use BigInt via `serialize_64_bit_numbers_as_big_int(true)`)
mfish33 marked this conversation as resolved.
Show resolved Hide resolved

## License

Expand Down
17 changes: 17 additions & 0 deletions src/bindings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
use wasm_bindgen::prelude::*;
use js_sys::BigInt;

#[wasm_bindgen]
extern {
#[wasm_bindgen(js_name = BigInt)]
pub fn from_u64(x: u64) -> BigInt;

#[wasm_bindgen(js_name = BigInt)]
pub fn from_i64(x: i64) -> BigInt;

#[wasm_bindgen(js_name = BigInt)]
pub fn to_u64(x: BigInt) -> u64;

#[wasm_bindgen(js_name = BigInt)]
pub fn to_i64(x: BigInt) -> i64;
}
59 changes: 43 additions & 16 deletions src/de.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
use js_sys::{Array, ArrayBuffer, JsString, Number, Object, Reflect, Symbol, Uint8Array};
use serde::{de, serde_if_integer128};
use js_sys::{Array, ArrayBuffer, BigInt, JsString, Number, Object, Reflect, Symbol, Uint8Array};
use serde::{
de,
serde_if_integer128,
};
use crate::bindings;
use wasm_bindgen::{JsCast, JsValue};

use super::{static_str_to_js, Error, ObjectExt, Result};
Expand Down Expand Up @@ -212,6 +216,20 @@ impl Deserializer {
}
None
}

fn deserialize_from_js_number_signed<'de, V: de::Visitor<'de>>(&self, visitor: V) -> Result<V::Value> {
match self.as_safe_integer() {
Some(v) => visitor.visit_i64(v),
_ => self.invalid_type(visitor),
}
}

fn deserialize_from_js_number_unsigned<'de, V: de::Visitor<'de>>(&self, visitor: V) -> Result<V::Value> {
match self.as_safe_integer() {
Some(v) if v >= 0 => visitor.visit_u64(v as _),
_ => self.invalid_type(visitor),
}
}
}

impl<'de> de::Deserializer<'de> for Deserializer {
Expand Down Expand Up @@ -311,15 +329,15 @@ impl<'de> de::Deserializer<'de> for Deserializer {
// these to 64-bit methods to save some space in the generated WASM.

fn deserialize_i8<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_i64(visitor)
self.deserialize_from_js_number_signed(visitor)
}

fn deserialize_i16<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_i64(visitor)
self.deserialize_from_js_number_signed(visitor)
}

fn deserialize_i32<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_i64(visitor)
self.deserialize_from_js_number_signed(visitor)
}

serde_if_integer128! {
Expand All @@ -331,15 +349,15 @@ impl<'de> de::Deserializer<'de> for Deserializer {
// Same as above, but for `i64`.

fn deserialize_u8<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_u64(visitor)
self.deserialize_from_js_number_unsigned(visitor)
}

fn deserialize_u16<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_u64(visitor)
self.deserialize_from_js_number_unsigned(visitor)
}

fn deserialize_u32<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
self.deserialize_u64(visitor)
self.deserialize_from_js_number_unsigned(visitor)
}

serde_if_integer128! {
Expand All @@ -348,19 +366,28 @@ impl<'de> de::Deserializer<'de> for Deserializer {
}
}

// Define real `i64` / `u64` deserializers that try to cast from `f64`.

fn deserialize_i64<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
match self.as_safe_integer() {
Some(v) => visitor.visit_i64(v),
None => self.invalid_type(visitor),
if self.value.is_bigint() {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest to use .dyn_into() here instead to combine the is_* check and .into() cast (which does its own check internally) into one.

let big_int: BigInt = self.value.clone().into();
// Safe to do unwraps since we know the type is a bigint
let rust_string = big_int.to_string(10).unwrap().as_string().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

This one is still using strings, but shouldn't anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops


match rust_string.parse() {
Ok(v) => visitor.visit_i64(v),
Err(_) => self.invalid_type(visitor),
}
} else {
self.deserialize_from_js_number_signed(visitor)
}

}

fn deserialize_u64<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
match self.as_safe_integer() {
Some(v) if v >= 0 => visitor.visit_u64(v as _),
_ => self.invalid_type(visitor),
if self.value.is_bigint() {
let value = bindings::to_u64(self.value.into());
visitor.visit_u64(value)
} else {
self.deserialize_from_js_number_unsigned(visitor)
}
}

Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use wasm_bindgen::prelude::*;
mod de;
mod error;
mod ser;
mod bindings;

pub use de::Deserializer;
pub use error::Error;
Expand Down
19 changes: 16 additions & 3 deletions src/ser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use js_sys::{Array, JsString, Map, Object, Uint8Array};
use serde::ser::{self, Error as _, Serialize};
use wasm_bindgen::prelude::*;
use wasm_bindgen::JsCast;
use crate::bindings;

use super::{static_str_to_js, Error, ObjectExt};

Expand Down Expand Up @@ -214,6 +215,7 @@ impl ser::SerializeStruct for ObjectSerializer<'_> {
#[derive(Default)]
pub struct Serializer {
serialize_maps_as_objects: bool,
serialize_64_bit_numbers_as_big_int:bool
Copy link
Owner

Choose a reason for hiding this comment

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

(here and around) please make sure to run cargo fmt in the end, or, better yet, you might want to configure format-on-save in your editor.

}

impl Serializer {
Expand All @@ -228,6 +230,13 @@ impl Serializer {
self.serialize_maps_as_objects = value;
self
}

/// Set to `true` to serialize 64 bit numbers to Javascript `BigInt` instead of
mfish33 marked this conversation as resolved.
Show resolved Hide resolved
/// `Number`. `false` by default
pub fn serialize_64_bit_numbers_as_big_int(mut self, value: bool) -> Self {
self.serialize_64_bit_numbers_as_big_int = value;
self
}
}

macro_rules! forward_to_into {
Expand Down Expand Up @@ -267,8 +276,10 @@ impl<'s> ser::Serializer for &'s Serializer {
serialize_str(&str);
}

// TODO: we might want to support `BigInt` here in the future.
fn serialize_i64(self, v: i64) -> Result {
if self.serialize_64_bit_numbers_as_big_int {
return Ok(bindings::from_i64(v).into())
}
const MAX_SAFE_INTEGER: i64 = 9_007_199_254_740_991;
const MIN_SAFE_INTEGER: i64 = -MAX_SAFE_INTEGER;

Expand All @@ -281,9 +292,11 @@ impl<'s> ser::Serializer for &'s Serializer {
)))
}
}

// TODO: we might want to support `BigInt` here in the future.
fn serialize_u64(self, v: u64) -> Result {
if self.serialize_64_bit_numbers_as_big_int {
return Ok(bindings::from_u64(v).into())
}

const MAX_SAFE_INTEGER: u64 = 9_007_199_254_740_991;

if v <= MAX_SAFE_INTEGER {
Expand Down
49 changes: 46 additions & 3 deletions tests/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@ where
L: Serialize + DeserializeOwned + PartialEq + Debug,
R: Into<JsValue>,
{
let lhs_value = to_value(&lhs).unwrap();
test_via_into_with_config(lhs, rhs, &Serializer::new())
}

fn test_via_into_with_config<L, R>(lhs: L, rhs: R, serializer:&Serializer)
where
L: Serialize + DeserializeOwned + PartialEq + Debug,
R: Into<JsValue>,
{
let lhs_value = lhs.serialize(serializer).unwrap();
assert_eq!(lhs_value, rhs.into(), "to_value from {:?}", lhs);
let restored_lhs = from_value(lhs_value.clone()).unwrap();
assert_eq!(lhs, restored_lhs, "from_value from {:?}", lhs_value);
Expand Down Expand Up @@ -145,6 +153,9 @@ fn numbers() {
test_signed!(i32);
test_unsigned!(u32);

test_float!(f32);
test_float!(f64);

{
const MAX_SAFE_INTEGER: i64 = 9_007_199_254_740_991;

Expand All @@ -169,8 +180,40 @@ fn numbers() {
to_value(&std::u64::MAX).unwrap_err();
}

test_float!(f32);
test_float!(f64);
let big_int_serializer = Serializer::new().serialize_64_bit_numbers_as_big_int(true);

{
const MAX_SAFE_INTEGER: i64 = 9_007_199_254_740_991;

// u64 and i64 should serialize the same
test_via_into_with_config(0_i64, 0_u64, &big_int_serializer);
test_via_into_with_config(42_i64, 42_u64, &big_int_serializer);

// Js-numbers should also deserialize into 64 bit types
from_value::<i64>(JsValue::from_f64(1.0)).unwrap();
RReverser marked this conversation as resolved.
Show resolved Hide resolved
from_value::<i64>(JsValue::from_f64(-1.0)).unwrap();

// Test near max safe float
(MAX_SAFE_INTEGER + 1).serialize(&big_int_serializer).unwrap();
(-(MAX_SAFE_INTEGER + 1)).serialize(&big_int_serializer).unwrap();

// Handle extreme values
std::i64::MIN.serialize(&big_int_serializer).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Same here about missing assert_eq! or something.

std::i64::MAX.serialize(&big_int_serializer).unwrap();
}

{
const MAX_SAFE_INTEGER: u64 = 9_007_199_254_740_991;

test_via_into_with_config(0_u64, 0_i64, &big_int_serializer);
test_via_into_with_config(42_u64, 42_i64, &big_int_serializer);

from_value::<u64>(JsValue::from_f64(1.0)).unwrap();

test_via_into_with_config(MAX_SAFE_INTEGER, MAX_SAFE_INTEGER as i64, &big_int_serializer);
(MAX_SAFE_INTEGER + 1).serialize(&big_int_serializer).unwrap();
std::u64::MAX.serialize(&big_int_serializer).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Can you also please add a few tests for conversion from a BigInt that doesn't actually fit into 64 bits? (you can create one from a string, it's fine to do so in tests)

It should throw an error, but I suspect right now it's not handled correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a js file to do validation on the BigInt's. I think it is the easiest and lowest overhead way of handling the bounds check. It replaces the to_i64 and to_u64 bindings used previously

Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, the JS integration doesn't work in all wasm-bindgen modes, see Caveats. While I agree it would be lower overhead, I'd rather keep this crate working for all output modes, as otherwise it's a pretty significant breaking change.

Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned in the initial comment where I proposed the bindings idea, you can just compare the resulting u64 with the original BigInt instance. It will require one more roundtrip to JS but shouldn't be that bad and will be working in all modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the JS integration doesn't work in all wasm-bindgen modes, see Caveats. While I agree it would be lower overhead, I'd rather keep this crate working for all output modes, as otherwise it's a pretty significant breaking change.

I missed the lack of nodejs support while reading that page of the docs. I appreciate all of the help, I am pretty new to wasm-bindgen.

}
}

#[wasm_bindgen_test]
Expand Down