Fix argument evaluation issue in conditional expressions

[lorenzosinisi]

The goal is to implement "lazy" evaluation of functions, specifically for conditional statements like @if. This is particularly useful when dealing with potentially nil values.

Current behavior:

@if(status, LEFT(status, 2), false) # this will fail because LEFT will be applied before IF

@if(IS_STRING(status), LEFT(status, 2), false) # even introducing a guard like "IS_STRING" this will fail because LEFT will be applied before IF

@if(LEN(status) > 0, LEFT(status, 2), false) # this will also fail because LEFT will be applied before IF

And the same will go for RIGHT, any String.split function etc.

If status is nil, this function currently throws an error because all arguments are evaluated eagerly, including LEFT(status, 2), which fails on a nil input.

Desired behavior:
The @if function should only evaluate the second argument (LEFT(status, 2)) if the first argument (status) is truthy. If status is falsy (including nil), it should immediately return the third argument (false) without evaluating the second argument.

Implementation approach:

1. Store a custom callback module in the reduction context.
2. Instead of modifying all existing functions to accept an additional argument, add a new key "CUSTOM_CALLBACK" to the context map.
3. This callback will be responsible for lazy evaluation of arguments.

Benefits:

1. Prevents errors when working with potentially nil values.
2. Improves performance by avoiding unnecessary computations.
3. Allows for more complex conditional logic without risking errors.

This approach maintains compatibility with existing functions while adding the capability for lazy evaluation where needed.

[lorenzosinisi]

What do you think @smn ? Is this acceptable?

[lorenzosinisi]

It should also make the library a lot faster because now it can short-circuit

[smn]

it's interesting as the intention of using reduce while here was to only evaluate until one of the things resolved to true. I'm happy that it's now solved but I'm unsure why it's solved. I'm going to need to spend some more time on this to make sure I understand.

[lorenzosinisi]

It seems like the only reason to do this here is:

• avoid issues with the "or" but I also don't understand why fixing it here
• avoid having to pass the custom callback as argument of every function both in the standard lib and custom callback module functions

[lorenzosinisi]

Given that the custom callback module does not change between function calls, I didn't see it as problematic to add it here and so pass it along every call to eval! in the Expression.Callbacks.EvalHelpers. Just like we store temporary values in the context with __value__, __captures, __type__ etc.

It is basically the implementation of the state monad

[lorenzosinisi]

Without this change, this AST will break because it will try to "LEFT" "nil" but this is not needed because if will evaluate to false and so return it

[Djack1010]

Thanks for this contribution! I was looking into a similar problem.
Could we extend this solution to and/or boolean evaluations? I've been trying a lazy evaluation of and with a nil value, but it seems to be failing. Here’s an example test:

test "and with nil values" do
  {:ok, ast, "", _, _, _} = Parser.parse("@and(isstring(status), LEFT(status, 2))")

  assert false == Eval.eval!(ast, %{"status" => nil})
end

[lorenzosinisi]

Hi @Djack1010, I don't think there is a good way to fix the "and" because unfortunately every argument of the function needs to be evaluated and not return error in order to check if the function can return true or false. This fix would only work for OR and IFs

[smn]

This feels like we need to give this some more thought, it's an entirely different means of specifying the callback module. In other places we're passing it in explicitly as an optional argument.

Maybe this helper just isn't useful enough to be defined in a separate module, would that prevent us having to supply the callback module as an argument? Especially because the eval! function is used in the callback module primarily.

[lorenzosinisi]

I assumed that this was done before the implantation of “or” in the same way. I think it is fixed because essentially this code was duplicating what “or” is doing in the body of its function 🤔And yes I agree that definining the callback like that changes things quite a bit but I haven’t found a better way to pass it as argument to all functions without changing all of their arity (and so make it also incompatible with all existing callback modules), any idea of how to still solve the issue with the test and keep it as argument?

[github-actions]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

1 out of 2 committers have signed the CLA.
✅ @smn
❌ @lorenzosinisi
_{You can retrigger this bot by commenting recheck in this Pull Request}

[smn]

I believe #223 shows that this is fixed, but please confirm.

[smn]

@lorenzosinisi I'm closing this PR again but please feel free to re-open if you feel the issue has not been properly addressed. Thanks again for your contribution!