clarkmcc / cel-rust

Common Expression Language interpreter written in Rust
https://crates.io/crates/cel-interpreter
MIT License
363 stars 18 forks source link

Updated integer types from 32-bit to 64-bit #16

Closed hardbyte closed 1 year ago

hardbyte commented 1 year ago

Changes CEL UInt and Int to use 64 bits instead of 32 as per the spec:

CEL supports only 64-bit integers and 64-bit IEEE double-precision floating-point.

I added/kept a conversion from i32 to Value, but couldn't use the impl_conversions macro so I haven't implemented for Option or any of the ResolveResult versions.

clarkmcc commented 1 year ago

I can dig into this and make sure it all works properly with the new custom functions. Thanks for the PR!

clarkmcc commented 1 year ago

Based on what I can see, it looks like you figured out the impl_conversions macro. Everything looks good there. I removed the i32 conversion (do you see any reason to support those behind the scenes) and resolved a few tests. The following patch should make everything good to go.

Index: interpreter/src/functions.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/functions.rs b/interpreter/src/functions.rs
--- a/interpreter/src/functions.rs  (revision 3ca5d2c418ce2752615e09edeb31180502eef424)
+++ b/interpreter/src/functions.rs  (date 1693178284518)
@@ -394,7 +394,7 @@

         for (name, script) in tests {
             let mut ctx = Context::default();
-            ctx.add_variable("foo", HashMap::from([("bar", 1)]));
+            ctx.add_variable("foo", HashMap::from([("bar", 1i64)]));
             assert_eq!(test_script(script, Some(ctx)), Ok(true.into()), "{}", name);
         }
     }
Index: interpreter/src/magic.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/magic.rs b/interpreter/src/magic.rs
--- a/interpreter/src/magic.rs  (revision 3ca5d2c418ce2752615e09edeb31180502eef424)
+++ b/interpreter/src/magic.rs  (date 1693178235215)
@@ -19,12 +19,6 @@
     Rc<Vec<Value>> => Value::List
 );

-impl From<i32> for Value {
-    fn from(value: i32) -> Self {
-        Value::Int(value as i64)
-    }
-}
-
 /// Describes any type that can be converted from a [`Value`] into itself.
 /// This is commonly used to convert from [`Value`] into primitive types,
 /// e.g. from `Value::Bool(true) -> true`. This trait is auto-implemented
Index: interpreter/src/lib.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/lib.rs b/interpreter/src/lib.rs
--- a/interpreter/src/lib.rs    (revision 3ca5d2c418ce2752615e09edeb31180502eef424)
+++ b/interpreter/src/lib.rs    (date 1693178274424)
@@ -176,7 +176,7 @@
         // Test that we can merge two maps
         assert_output(
             "{'a': 1} + {'a': 2, 'b': 3}",
-            Ok(HashMap::from([("a", 2), ("b", 3)]).into()),
+            Ok(HashMap::from([("a", 2i64), ("b", 3)]).into()),
         );
     }

@@ -216,7 +216,7 @@

         for (name, script, error) in tests {
             let mut ctx = Context::default();
-            ctx.add_variable("foo", HashMap::from([("bar", 1)]));
+            ctx.add_variable("foo", HashMap::from([("bar", 1i64)]));
             let res = test_script(script, Some(ctx));
             assert_eq!(res, error.into(), "{}", name);
         }
hardbyte commented 1 year ago

Cool - I've included your changes. I had added the i32 conversion as I wasn't stoked with end users having to specify i64 when adding an integer variable to the context.

use cel_interpreter::{Context, Program};

fn main() {
    let program = Program::compile("foo * 2").unwrap();
    let mut context = Context::default();
    context.add_variable("foo", 10);

    let value = program.execute(&context).unwrap();
    assert_eq!(value, 20.into());
}

Will fail with:

error[E0277]: the trait bound `Value: From<i32>` is not satisfied
  --> example/src/variables.rs:6:33
   |
6  |     context.add_variable("foo", 10);
   |             ------------        ^^ the trait `From<i32>` is not implemented for `Value`
   |             |
   |             required by a bound introduced by this call
   |

Perhaps we can make that nicer?

clarkmcc commented 1 year ago

Got it, fair point. I like the conversion, let's put that back in.