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

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

Merged
merged 2 commits into from
Aug 27, 2023

Conversation

hardbyte
Copy link
Contributor

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
Copy link
Owner

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

@clarkmcc
Copy link
Owner

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
Copy link
Contributor Author

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?

@hardbyte hardbyte mentioned this pull request Aug 24, 2023
15 tasks
@clarkmcc
Copy link
Owner

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

@clarkmcc clarkmcc merged commit 9f7210f into clarkmcc:master Aug 27, 2023
2 checks passed
@hardbyte hardbyte deleted the feature/64bit-numbors branch August 28, 2023 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants