ISibboI / evalexpr

A powerful expression evaluation crate 🦀.
GNU Affero General Public License v3.0
320 stars 52 forks source link

[FR]Support Short-circuit for eval bool exp #155

Open Leojangh opened 11 months ago

Leojangh commented 11 months ago
    #[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.

ISibboI commented 11 months ago

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.

Leojangh commented 11 months ago

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: https://github.com/ISibboI/evalexpr/blob/b39bbd76856e22eb3663386460ed4fabc7781749/src/tree/mod.rs#L323-L329 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?

ISibboI commented 11 months ago

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 9 months ago

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