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

Problem with HashMap/Vec values conversion when adding as variable #122

Open
evgenyz opened this issue Feb 19, 2025 · 2 comments
Open

Problem with HashMap/Vec values conversion when adding as variable #122

evgenyz opened this issue Feb 19, 2025 · 2 comments

Comments

@evgenyz
Copy link

evgenyz commented Feb 19, 2025

Consider this snippet

let program = Program::compile("1 + numbers[0]").unwrap();
let mut context = Context::default();

// This could also come directly from Serde in a form of e.g. HashMap<String, Value> 
let numbers: Vec<serde_json::Value> = vec![2.into()];
context.add_variable("numbers", numbers).unwrap();
let value = program.execute(&context).unwrap();

Expected return value: 3.

Actual result: Result::unwrap() on an Err value: UnsupportedBinaryOperator("add", Int(1), UInt(2))

Somehow serde_json::Value::Number is converted into cel_interpreter::Value::UInt by default, which
is quite unhelpful. Not sure how to work around it.

v0.9.0

@clarkmcc
Copy link
Owner

clarkmcc commented Feb 21, 2025

I'm looking at this in #123. The main problem with implicit casting between types (uint/int) for binary operations (add/sub/mul/div) is what happens when there's an overflow? Another question is, what is returned if I do uint + int, is it a uint? What if the int is negative and larger than the uint? You have a situation where the type of the result of the binary operation is dependent on the values of the inputs, not the types of the inputs.

According to the cel-spec:

Note that currently there are no automatic arithmetic conversions for the numeric types (int, uint, and double). The arithmetic operators typically contain overloads for arguments of the same numeric type, but not for mixed-type arguments. Therefore an expression like 1 + 1u is going to fail to dispatch. To perform mixed-type arithmetic, use explicit conversion functions such as uint(1) + 1u. Such explicit conversions will maintain their meaning even if arithmetic conversions are added in the future.

So the only other thing we could look at is why a JSON value by default serializes to a uint rather than an int which would probably solve the surface level issue you're facing, without addressing the underlying cause of your problem (binary ops across types).

@evgenyz
Copy link
Author

evgenyz commented Feb 21, 2025

No questions to inability to add int to uint, all according to spec. But it'd really help to cast serde's Number into Int (just as CEL does with number literals by default), unless that would violate the int type.

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 a pull request may close this issue.

2 participants