ISibboI/evalexpr

[FR]Support Short-circuit for eval bool exp

Opened this issue · 4 comments

    #[test]
    fn it_works() {
        let mut context = HashMapContext::new();
        // context.set_value("a".into(), false.into()).unwrap();
        let x = eval_boolean_with_context("true || a", &context).unwrap();
        println!("{}", x);
    }

It got an error: VariableIdentifierNotFound("a")。But if short-circuit is supported, it should be computeable.

This may be a useful feature. Feel free to write a pull request. I must unfortunately say though that I do not have enough time at the moment for code reviews for merging it fast.

Hello, I have a rough understanding of the project and here is the plan I thought of.
It looks like the evaluation strategy is calculating every argument and then combining them with operator:

evalexpr/src/tree/mod.rs

Lines 323 to 329 in b39bbd7

pub fn eval_with_context<C: Context>(&self, context: &C) -> EvalexprResult<Value> {
let mut arguments = Vec::new();
for child in self.children() {
arguments.push(child.eval_with_context(context)?);
}
self.operator().eval(&arguments, context)
}

Maybe we can make the first step lazily:

    ///Lazy variant of `Node#eval_with_context`
    pub fn eval_with_context_lazily<C: Context>(&self, context: &C) -> EvalexprResult<Value> {
        self.operator.eval_lazily(&self.children(), context)
    }

So the actual variable reading from context happens when using it, then the feature might be supported.

        ...
        And => {
                expect_operator_argument_amount(children.len(), 2)?;
                let left = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(left && children[1].eval_with_context_lazily(context)?.as_boolean()?))
            },
            Or => {
                expect_operator_argument_amount(children.len(), 2)?;
                let left = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(left || children[1].eval_with_context_lazily(context)?.as_boolean()?))
            },
            Not => {
                expect_operator_argument_amount(children.len(), 1)?;
                let a = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(!a))
            },
        ...

What is the best way? Add a lazy variant for eval_bool only or change the old?

I would not add new methods for this. Rather have the context type decide if boolean expressions should be evaluated lazily, or not. So add a function to the context trait fn evaluate_boolean_expression_with_short_circuit(&self) -> bool (or some other name). If the function returns true, the operators should use short circuit evaluation, if not, then not.

I think it makes sense to have short circuit evaluation be the default, so the existing context types should just unconditionally return true for this method.

bwsw commented

@ISibboI you can consider using ControlFlow in std::ops for that.