Decimal (NOT YET FOR MERGE) #225

bheylin · 2020-08-25T19:37:29Z

This is a draft implementation of integrating a Decimal type that depends on ['no_float'] being set.

The ['no_float'] dependency is required because the Decimal type hijacks the float parser as the Rhai language has no support for typed literals. Without a typed literal, the parser has no way to decide whether to store a decimal literal as a float or a Decimal.

I use the rust_decimal library as the Decimal implementation as it is pure Rust and still being maintained.

…l create was the choosen implementation, but this is liable to change. As it stands, the decimal type works, but only when 'no_float' is set, as the Decimal type needs to hijack the float parser since there isn't typed literals in the Rhai language.

Replaced the arithmetic operators defined in arithmetics.rs and logic.rs with efficient hard coded versions in fn_call.rs Added tests for Decimal array usage. Added support for use of decimals in arrays in array_basic.rs

Added 'no_float' as a dependency of 'decimal' as the decimal feature hijacks the float parser, since the Rhai language has no way to distinguish between float literals and decimal literals.

schungx

This is great! Some comments in this review.

Cargo.toml

schungx · 2020-08-26T00:29:13Z

doc/src/language/json.md

-JSON numbers are all floating-point while Rhai supports integers (`INT`) and floating-point (`FLOAT`) if
-the [`no_float`] feature is not used.  Most common generators of JSON data distinguish between
+JSON numbers are all floating-point while Rhai supports integers (`INT`) floating-point (`FLOAT`) if
+the [`no_float`] feature is not used and decimal is the [`decimal`] feature is used.  


and decimal if the [decimal] feature is used

schungx · 2020-08-26T00:30:21Z

doc/src/language/numbers.md

@@ -10,6 +10,9 @@ The default system integer type (also aliased to `INT`) is `i64`. It can be turn
 Floating-point numbers are also supported if not disabled with [`no_float`]. The default system floating-point type is `i64`
 (also aliased to `FLOAT`).

+Decimal numbers are also supported if explicitly enabled with ['decimal'].
+Be aware that Decimal support replaces float support, the two features cannot be enabled at the same time.


add: "... meaning that [decimal] implies [no_float]." to be explicitly clear?

schungx · 2020-08-26T01:24:53Z

doc/src/language/values-and-types.md

@@ -32,6 +33,8 @@ If only 32-bit integers are needed, enabling the [`only_i32`] feature will remov
 This is useful on some 32-bit targets where using 64-bit integers incur a performance penalty.

 If no floating-point is needed or supported, use the [`no_float`] feature to remove it.
+If decimal support is necessary, use the [`decimal`] feature, but be aware that decimal and float support cannot cco-exist in the same build.


cco-exist

schungx · 2020-08-26T01:26:02Z

doc/src/start/builds/minimal.md

@@ -31,7 +31,7 @@ Opt-Out of Features
 ------------------

 Opt out of as many features as possible, if they are not needed, to reduce code size because, remember, by default
-all code is compiled into the final binary since what a script requires cannot be predicted.
+all code except [`decimal`] is compiled into the final binary since what a script requires cannot be predicted.


Well, actually there are a few opt-in features, such as serde. So this statement is not technically correct.

schungx · 2020-08-26T01:26:21Z

doc/src/start/features.md

@@ -17,6 +17,7 @@ more control over what a script can (or cannot) do.
 | `sync`              | Restrict all values types to those that are `Send + Sync`. Under this feature, all Rhai types, including [`Engine`], [`Scope`] and [`AST`], are all `Send + Sync`.                                         |
 | `no_optimize`       | Disable [script optimization].                                                                                                                                                                             |
 | `no_float`          | Disable floating-point numbers and math.                                                                                                                                                                   |
+| `decimal`           | Enable decimal number support (through the 'rust_decimal' create). This will disable floating point support.


create -> crate

schungx · 2020-08-26T01:28:15Z

src/any.rs

+
+        #[cfg(feature = "decimal")]
+        {
+            boxed = match unsafe_cast_box::<_, rust_decimal::Decimal>(boxed) {


I suppose Decimal is Copy so it can use unsafe_try_cast to save on one boxing operation?

Maybe just copy the FLOAT block:

#[cfg(feature = "decimal")] if type_id == TypeId::of::<Decimal>() { return <dyn Any>::downcast_ref::<Decimal>(&value) .unwrap() .clone() .into(); }

Also, if you have serde enabled, it may attempt to serialize f32 or f64 into a Dynamic via Dynamic::from. Therefore, you'd want to also handle the case where 1) no_float, 2) decimal, 3) serde (which I suppose must have floats...), 4) the type is f32 or f64. In such a case, you should convert the floating-point number into Decimal.:

#[cfg(feature = "decimal")] #[cfg(feature = "serde")] if type_id == TypeId::of::<f64>() { return dec!(<dyn Any>::downcast_ref::<f64>(&value).unwrap().clone()).into(); } else if type_id == TypeId::of::<f32>() { return dec!(<dyn Any>::downcast_ref::<f32>(&value).unwrap().clone()).into(); }

Using unsafe_try_cast is possible, but this breaks the transfer of ownership pattern at play, where the Result::Err<Box<...> returned from unsafe_cast_box is used to move the boxed value back from the unsafe_try_cast.

The problem being that unsafe_try_cast returns an Option meaning it consumes the boxed value. I'm unsure how to resolve that. Maybe by borrowing the contents of the Box to unsafe_try_cast?

Sorry, my mistake. You can't use unsafe_try_cast here. Just downcast_ref and then clone as my example.

They key points are: 1) Decimal is Copy so cloning is cheaper than boxing (supposedly), 2) You should handle the rate conversion cases to/from f32/f64.

downcast_ref version is pushed.

I have to look at the serde case closer later on. I need to understand the flow there. rust_decimal has serde support.

Yes. I think it probably defaults to serialize to/from JSON as f64 or f32. Thus you need to consider some of the plumbing in the serde feature that serializes to/from Dynamic.

However, it is not a big deal, as I can clean them up later on.

schungx · 2020-08-26T01:28:36Z

src/any.rs

@@ -692,6 +716,14 @@ impl Dynamic {
            };
        }

+        #[cfg(feature = "decimal")]
+        if type_id == TypeId::of::<Decimal>() {


Again, consider unsafe_try_cast if Decimal is Copy

unsafe_try_cast here should be fine!

unsafe_try_cast version is pushed.

schungx · 2020-08-26T01:30:49Z

src/fn_call.rs

@@ -1203,6 +1206,27 @@ pub fn run_builtin_binary_op(
        }
    }

+    #[cfg(feature = "decimal")]


I don't know if a Decimal can overflow or underflow. You may want to actually copy the INT code block instead of the FLOAT code block if you need checked arithmetic... At least the division should trap divide-by-zero...

I'll have to take a look at how this all fits together a bit closer. Decimal can overflow, but it has checked versions for add, sub, etc that return an Option.

Rhai also has Checked* traits that return an Option, so all good there. I'll see later on today if I can use those traits for the Decimal type.

Yes, look into the add, sub etc. functions. Those are checked versions. You can copy that template for your own add_decimal etc. Essentially if it overflows, convert it into an error.

Well, actually I see that Decimal seems to implement num_traits::CheckedAdd etc. If so, maybe you can simply use the generic versions of add, sub etc. and no need to write your own versions.

src/packages/arithmetic.rs

…l' feature is set.

bheylin · 2020-08-26T06:51:22Z

All good 😉 this will be ready tomorrow as my fence just blew over in a storm, so that’s first priority 😆 Brian

…

On 26 Aug 2020, at 08:45, Stephen Chung ***@***.***> wrote: @schungx commented on this pull request. In src/any.rs: > @@ -578,6 +593,15 @@ impl Dynamic { Ok(s) => return (*s).into(), Err(val) => val, }; + + #[cfg(feature = "decimal")] + { + boxed = match unsafe_cast_box::<_, rust_decimal::Decimal>(boxed) { They key points are: 1) Decimal is Copy so cloning is cheaper than boxing (supposedly), 2) You should handle the rate conversion cases to/from f32/f64. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

src/packages/arithmetic.rs

bheylin · 2020-08-26T09:06:04Z

@schungx Ok I'm gonna be busy with adulting for the rest of the day. I'll come back to the open issues tomorrow morning. have a good one 😉

src/packages/arithmetic.rs

bheylin · 2020-09-01T06:49:55Z

Hey hey, after an unexpected break I'm back in business ;)

schungx · 2020-09-01T06:58:34Z

Great to have you back! But a bit of a "doh!"...

I just merged in a major revision that implements a plugins system with procedural macros.

The files under packages have been changed significantly. You may have to do some revision to merge into that.

Or you can leave it to me... I can apply your changes to the new code base when you're ready.

Sorry!

bheylin · 2020-09-01T07:09:09Z

Nah don't worry about it, I'll merge in the changes ;)

schungx · 2020-09-04T02:27:31Z

@ObsceneGiraffe let me know when this can be merged.

bheylin · 2020-09-04T08:59:20Z

@schungx well all the tests are passing. I merged the changes in upstream yesterday. But the Serde work remains to be done. I'm starting that now.

bheylin · 2020-09-04T09:03:47Z

I took a look at splitting the gen_arithmetic_functions marco up into gen_bitwise_functions so that I could use that for defining the Decimal functions too. I had some trouble making the if y == 0 test generic in the fn divide().

Decimal implemented trait Zero but all the integer types do not for some reason?

But anyways, I'm going stash that work and get on with the serde / Decimal integration.

schungx · 2020-09-04T09:57:07Z

But anyways, I'm going stash that work and get on with the serde / Decimal integration.

No prob.

I took a look at splitting the gen_arithmetic_functions marco up into gen_bitwise_functions so that I could use that for defining the Decimal functions too.

You can, but gen_arithmetic_functions currently is registered in three groups. If you split it, then it doubles to six groups.

May not be worth it, as having a dedicated module like what you have is perhaps simpler.

I had some trouble making the if y == 0 test generic in the fn divide().

This is why Zero is gone: rust-lang/rfcs#369

I'd hesitated to pull in num_traits just to have it though...

bheylin · 2020-09-04T10:26:08Z

You can, but gen_arithmetic_functions currently is registered in three groups. If you split it, then it doubles to six groups.

May not be worth it, as having a dedicated module like what you have is perhaps simpler.

Yea the increase in groups is not worth the trivial duplication in the decimal_functions module.

This is why Zero is gone: rust-lang/rfcs#369

And good to know

bheylin · 2020-09-04T10:58:08Z

On the serde support. I don't think the Dynamic type should be modified to support converting floats to Decimal, as that's an inherently dangerous operation in terms of loss of precision.

rust_decimal itself has implementations of serde::Serialize and serde::Deserialize traits. The rust_decimal implementations convert directly to and from string by default unless serde_float is enabled.

From what I see, I would have to integrate the Decimal impls for serde::Serialize and serde::Deserialize into the Rhai DynamicDeserializer and DynamicSerializer in de.rs and ser.rs respectively.

schungx · 2020-09-04T11:07:28Z

However, I think the serde serialization trait requires you to implement a serializer for f32 and f64... I'm not particularly fluent with serde, so maybe you can just unimplemented!() them.

bheylin · 2020-09-04T11:11:07Z

Yep serde is new for me too. But that's where I'm heading with the serde support.

schungx · 2020-09-04T11:18:12Z

A reminder: Rhai serde support is essentially a new serialization format. Dynamic is a format (like JSON), which things can serialize into.

That's why it is ser::to_dynamic and de::from_dynamic and not the other way round.

bheylin · 2020-09-14T09:08:41Z

@schungx I'm having some trouble figuring out how to integrate the serde definitions in rust_decimal into the Deserializer impl in de.rs:160.

rust_decimal has various serde::Deserialize impls in serde_types.rs and it looks like I need to integrate that Deserialize impl into the match statement in de.rs:160. It's unclear to me what I need to do here. Should I somehow create a new instance of rust_decimal::serde_types::DecimalVisitor? Or should I directly call Deserialize::deserialize directly on the Decimal type.

My serde knowledge is quite limited, I could so with help from someone with more knowledge of serde. Can you put me in contact with someone to discuss this integration?

schungx · 2020-09-14T10:11:54Z

I'm not an expert in serde also... Actually I did the ground work and somebody else refined it...

Deserialization means going from Dynamic to Decimal. Maybe it doesn't need any special treatment... because serde only understands f32 and f64... So it can be Dynamic -> f64 -> Decimal, but then you may have rounding errors...

Let me think about this some more...

bheylin · 2020-09-14T10:25:12Z

Yea that's what I'm trying to avoid. I want to warn if someone is trying to deserialize from floats to Decimal. I explicitly want to only handle string -> Decimal

schungx · 2020-09-14T10:31:09Z

If there is built-in support for String -> Decimal, then it is simple...

Just do:

Union::Decimal(_) => self.deserialize_str(visitor),

fn deserialize_str<V: Visitor<'de>>(self, visitor: V) -> Result<V::Value, Box<EvalAltResult>> {
    if self.value.type_id() == TypeId::of::<Decimal>() {
        // ... get the Decimal, then convert into a string
    } else {
        self.value.downcast_ref::<ImmutableString>().map_or_else(|| self.type_error(), |x| visit_borrowed_str(x.as_str()))
    }
}

That would convert a Dynamic value with Decimal type into a string for serde. Then, serde will convert it into your Decimal from that string, if that's what you plan to convert to...

Or you may want to use u64 or u128 as the conversion payload to avoid allocations, or a byte array...

schungx · 2021-02-27T04:30:22Z

Decimal is now built into Rhai. Retiring this PR.

bheylin · 2021-03-25T09:28:35Z

Thanks @schungx, apologies, I disappeared into the wilderness for a while.

ObsceneGiraffe added 8 commits August 24, 2020 10:02

Added more decimal tests and ran fmt over the code.

7070478

Added %, unary - and abs tests to decimal.

d86eb2c

Removed the unnecessary DecimalWrapper and arithmetic functions.

b2f38ff

Replaced the arithmetic operators defined in arithmetics.rs and logic.rs with efficient hard coded versions in fn_call.rs Added tests for Decimal array usage. Added support for use of decimals in arrays in array_basic.rs

Rephrased the 'no_decimal' feature as 'decimal'.

1bb489f

Added 'no_float' as a dependency of 'decimal' as the decimal feature hijacks the float parser, since the Rhai language has no way to distinguish between float literals and decimal literals.

Added a json test to tests/decimal.ts

cc2276f

Added the missing Dynamic::as_decimal(...)

3e24766

Added decimal references to the documentation.

4fe5813

bheylin changed the title ~~Decimal (NOT YET READY FOR MERGE)~~ Decimal (NOT YET FOR MERGE) Aug 25, 2020

schungx reviewed Aug 26, 2020

View reviewed changes

ObsceneGiraffe added 2 commits August 26, 2020 07:42

Documentation corrections.

c10c1ab

The rust_decimal crate is now optional and only used what the 'decima…

6af3358

…l' feature is set.

ObsceneGiraffe added 3 commits August 26, 2020 09:49

Removed unnecessary Decimal unboxing from the Dynamic::from::<>().

baae77d

Removed unnecessary Decimal unboxing from the Dynamic::try_cast().

6bd35ea

Removed usage of num_traits::One from the Decimal::sign impl.

5e080af

schungx reviewed Aug 26, 2020

View reviewed changes

src/packages/arithmetic.rs Outdated Show resolved Hide resolved

ObsceneGiraffe added 2 commits August 26, 2020 11:03

Fixed sign logic for Decimal.

e27b787

cargo fmt.

6c2d501

schungx reviewed Aug 26, 2020

View reviewed changes

src/packages/arithmetic.rs Outdated Show resolved Hide resolved

Forced return types to INT for Decimal::sign()

8220241

ObsceneGiraffe added 2 commits September 3, 2020 10:05

Merge remote-tracking branch 'upstream/master' into decimal

f3e8361

Added checked basic arithmetic ops for Decimal.

086c4cf

schungx closed this Feb 27, 2021

Decimal (NOT YET FOR MERGE) #225

Decimal (NOT YET FOR MERGE) #225

Conversation

bheylin commented Aug 25, 2020

schungx left a 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

schungx Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

schungx Aug 26, 2020 • 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

bheylin Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bheylin commented Aug 26, 2020 via email

bheylin commented Aug 26, 2020

bheylin commented Sep 1, 2020

schungx commented Sep 1, 2020

bheylin commented Sep 1, 2020

schungx commented Sep 4, 2020

bheylin commented Sep 4, 2020

bheylin commented Sep 4, 2020

schungx commented Sep 4, 2020

bheylin commented Sep 4, 2020

bheylin commented Sep 4, 2020

schungx commented Sep 4, 2020 • edited Loading

bheylin commented Sep 4, 2020

schungx commented Sep 4, 2020 • edited Loading

bheylin commented Sep 14, 2020

schungx commented Sep 14, 2020 • edited Loading

bheylin commented Sep 14, 2020

schungx commented Sep 14, 2020 • edited Loading

schungx commented Feb 27, 2021

bheylin commented Mar 25, 2021

schungx Aug 26, 2020 •

edited

Loading

schungx Aug 26, 2020 •

edited

Loading

bheylin Aug 26, 2020 •

edited

Loading

schungx commented Sep 4, 2020 •

edited

Loading

schungx commented Sep 4, 2020 •

edited

Loading

schungx commented Sep 14, 2020 •

edited

Loading

schungx commented Sep 14, 2020 •

edited

Loading