Add From<i/u128> for JsBigInt #1971

dranikpg · 2022-03-23T13:09:39Z

This Pull Request fixes 1970.

It changes the following:

Added From, From for JsBigInt
Added From, From for Numeric

dranikpg · 2022-03-23T13:20:39Z

Should i128 really be clamped to f64 like i64 is?

impl From<i64> for JsValue {
    #[inline]
    fn from(value: i64) -> Self {
        if let Ok(value) = i32::try_from(value) {
            Self::Integer(value)
        } else {
            Self::Rational(value as f64)
        }
    }
}

I'd rather expect something like this, as accuracy would be very low for big values otherwise

impl From<i128> for JsValue {
    #[inline]
    fn from(value: i128) -> Self {
        if let Ok(value) = i32::try_from(value) {
            Self::Integer(value)
        } else {
            Self::BigInt(value.into()) // i.e. JsBigInt::from()
        }
    }    
}

Razican

Thank you for the contribution!! Looks great :)

codecov · 2022-03-23T14:34:14Z

Codecov Report

Merging #1971 (5d9154e) into main (e2630fa) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1971      +/-   ##
==========================================
- Coverage   45.90%   45.87%   -0.04%     
==========================================
  Files         206      206              
  Lines       17116    17124       +8     
==========================================
- Hits         7857     7855       -2     
- Misses       9259     9269      +10

Impacted Files	Coverage Δ
boa_engine/src/bigint.rs	`35.03% <0.00%> (-4.07%)`	⬇️
boa_engine/src/value/mod.rs	`51.09% <0.00%> (-0.76%)`	⬇️
boa_engine/src/value/operations.rs	`38.85% <0.00%> (-0.32%)`	⬇️
boa_engine/src/builtins/string/mod.rs	`62.74% <0.00%> (+0.21%)`	⬆️
...ine/src/syntax/parser/expression/assignment/mod.rs	`30.84% <0.00%> (+1.86%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2630fa...5d9154e. Read the comment docs.

jasonwilliams

Looks good to me

Razican · 2022-03-24T06:57:12Z

Should i128 really be clamped to f64 like i64 is?

impl From<i64> for JsValue {

    #[inline]

    fn from(value: i64) -> Self {

        if let Ok(value) = i32::try_from(value) {

            Self::Integer(value)

        } else {

            Self::Rational(value as f64)

        }

    }

}

I'd rather expect something like this, as accuracy would be very low for big values otherwise

impl From<i128> for JsValue {

    #[inline]

    fn from(value: i128) -> Self {

        if let Ok(value) = i32::try_from(value) {

            Self::Integer(value)

        } else {

            Self::BigInt(value.into()) // i.e. JsBigInt::from()

        }

    }    

}

I would say this can be treated in a separate issue, it's a subjective topic :)

HalidOdat · 2022-03-24T07:25:48Z

boa_engine/src/value/mod.rs

+impl From<i128> for Numeric {
+    #[inline]
+    fn from(value: i128) -> Self {
+        Self::BigInt(value.into())
+    }
+}
+


This should be a number (f64) instead of BigInt

Well, we are already doing this with i64, and BigInt would make sure that it doesn't lose precision:

boa/boa_engine/src/value/mod.rs

Lines 1003 to 1008 in e2630fa

impl From<i64> for Numeric {

#[inline]

fn from(value: i64) -> Self {

Self::BigInt(value.into())

}

}

Well, we are already doing this with i64

Ummm.... This is wrong, It should be a number, not bigint.

BigInt would make sure that it doesn't lose precision

It's fine if we lose precision (we lose precision if we type a number value which is too big for f64 in JavaScript, same with From<i64> for JsValue), If we don't want loss of precision we create a bigint and we can convert it into Numeric.
The behaviour of From<u/iN> should be uniform, to do some number and others bigint is confuzing.

There was some discussion here too: #1961

From my point of view, we should be able to convert an f64 to JsValue or Numeric, and a BigInt too. But nothing else (at most an i32). If the user wants to convert a i128 to a BigInt, they can first convert it to a BigInt and then convert the BigInt to a Numeric or a JsValue.

We shouldn't probably force anything, but give some options. Maybe a TryFrom implementation could be nice.

From my point of view, we should be able to convert an f64 to JsValue or Numeric, and a BigInt too. But nothing else (at most an i32). If the user wants to convert a i128 to a BigInt, they can first convert it to a BigInt and then convert the BigInt to a Numeric or a JsValue.

We shouldn't probably force anything, but give some options. Maybe a TryFrom implementation could be nice.

Yup. agreed

boa_engine/src/value/mod.rs

HalidOdat · 2022-03-29T04:24:01Z

boa_engine/src/value/mod.rs

+impl From<i128> for Numeric {
+    #[inline]
+    fn from(value: i128) -> Self {
+        Self::BigInt(value.into())
+    }
+}


We should remove this, as well as From<u128> for Numeric. If the user wants to a bigint or number they should explicitly say so.

Besides it's out of the scope of this PR of adding From<i/u128> for JsBigInt

lastmjs · 2022-03-30T14:43:06Z

Don't we want to implement the From<i128> or From<u128> trait for JsValue? Why aren't we adding an implementation in here: https://github.com/boa-dev/boa/blob/main/boa_engine/src/value/conversions.rs?

I would like to be able to do the following:

pub fn into_js_value_i128(number: i128) -> boa_engine::JsValue {
    number.into()
}

pub fn into_js_value_u128(number: u128) -> boa_engine::JsValue {
    number.into()
}

I've noticed that f32, i128, i16, i8, u128, u16, and u8 are all missing implementations for From for JsValue, and if I'm not mistaken that seems like the most ergonomic way to use .into() or JsValue::from().

lastmjs · 2022-05-20T13:33:30Z

What can we do to move this forward? I am currently in need of this now and it would be nice to get back on the main branch of Boa.

Razican · 2022-10-10T12:30:01Z

This will be handled as part of #2276.

Add From<i/u128> for JsBitInt

5d9154e

dranikpg changed the title ~~Add From<i/u128> for JsBitInt~~ Add From<i/u128> for JsBigInt Mar 23, 2022

Razican approved these changes Mar 23, 2022

View reviewed changes

Razican requested review from HalidOdat, jasonwilliams, raskad, jedel1043 and RageKnify March 23, 2022 14:26

Razican added this to the v0.15.0 milestone Mar 23, 2022

Razican added API enhancement New feature or request labels Mar 23, 2022

jasonwilliams approved these changes Mar 23, 2022

View reviewed changes

HalidOdat reviewed Mar 24, 2022

View reviewed changes

HalidOdat reviewed Mar 29, 2022

View reviewed changes

This was referenced Mar 30, 2022

Perfect conversions from JsValue -> arbitrary Rust types -> JsValue demergent-labs/azle#142

Closed

Integer conversion conventions #1991

Open

JsValue from primitives #1993

Closed

Razican removed this from the v0.15.0 milestone Jun 1, 2022

lastmjs mentioned this pull request Jun 25, 2022

Implement i128 to JsValue conversions #1970

Closed

Razican closed this Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add From<i/u128> for JsBigInt #1971

Add From<i/u128> for JsBigInt #1971

dranikpg commented Mar 23, 2022

dranikpg commented Mar 23, 2022

Razican left a comment

codecov bot commented Mar 23, 2022

jasonwilliams left a comment

Razican commented Mar 24, 2022

HalidOdat Mar 24, 2022

Razican Mar 24, 2022

HalidOdat Mar 25, 2022

Razican Mar 27, 2022

HalidOdat Mar 29, 2022

HalidOdat Mar 29, 2022

lastmjs commented Mar 30, 2022 •

edited

Loading

lastmjs commented May 20, 2022

Razican commented Oct 10, 2022

	impl From<i64> for Numeric {
	#[inline]
	fn from(value: i64) -> Self {
	Self::BigInt(value.into())
	}
	}

Add From<i/u128> for JsBigInt #1971

Add From<i/u128> for JsBigInt #1971

Conversation

dranikpg commented Mar 23, 2022

dranikpg commented Mar 23, 2022

Razican left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2022

Codecov Report

jasonwilliams left a comment

Choose a reason for hiding this comment

Razican commented Mar 24, 2022

HalidOdat Mar 24, 2022

Choose a reason for hiding this comment

Razican Mar 24, 2022

Choose a reason for hiding this comment

HalidOdat Mar 25, 2022

Choose a reason for hiding this comment

Razican Mar 27, 2022

Choose a reason for hiding this comment

HalidOdat Mar 29, 2022

Choose a reason for hiding this comment

HalidOdat Mar 29, 2022

Choose a reason for hiding this comment

lastmjs commented Mar 30, 2022 • edited Loading

lastmjs commented May 20, 2022

Razican commented Oct 10, 2022

lastmjs commented Mar 30, 2022 •

edited

Loading