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.

Sign up for GitHub

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

Jump to bottom

Fix argument evaluation issue in conditional expressions #218

Closed
wants to merge 3 commits into from
Closed

Fix argument evaluation issue in conditional expressions #218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
456a8a7
Fix argument evaluation issue in conditional expressions
lorenzosinisi Aug 27, 2024
cf5d0bc
Merge branch 'turnhub:develop' into fix-do-not-evaluate-all-arguments…
lorenzosinisi Aug 28, 2024
561e81f
Merge branch 'develop' into fix-do-not-evaluate-all-arguments-in-advance
smn Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/expression/callbacks/eval_helpers.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ defmodule Expression.Callbacks.EvalHelpers do
@spec eval!(term, map) :: term
def eval!(ast, ctx) do
ast
|> Expression.Eval.eval!(ctx)
|> Expression.Eval.eval!(
ctx,
Map.get(ctx, "__CUSTOM_CALLBACK__", Expression.Callbacks.Standard)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

)
|> Expression.Eval.not_founds_as_nil()
end

Expand Down
15 changes: 1 addition & 14 deletions lib/expression/eval.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ defmodule Expression.Eval do

@numeric_kernel_operators [:+, :-, :*, :/, :>, :>=, :<, :<=]
@kernel_operators @numeric_kernel_operators ++ [:==, :!=]
@allowed_nested_function_arguments [:function, :lambda] ++ @kernel_operators

def eval!(ast, context, mod \\ Expression.Callbacks)

Expand Down Expand Up @@ -70,9 +69,8 @@ defmodule Expression.Eval do
def eval!({:function, opts}, context, mod) do
function_name = opts[:name] || raise "Functions need a name"
args = opts[:args] || []
arguments = Enum.reduce_while(args, [], &args_reducer(&1, function_name, context, mod, &2))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


case mod.handle(function_name, arguments, context) do
case mod.handle(function_name, args, Map.put(context, "__CUSTOM_CALLBACK__", mod)) do
{:ok, value} -> value
{:error, reason} -> "ERROR: #{inspect(reason)}"
end
Expand Down Expand Up @@ -312,15 +310,4 @@ defmodule Expression.Eval do

def handle_not_found({:not_found, _}), do: nil
def handle_not_found(value), do: value

defp args_reducer({type, _args} = function, function_name, context, mod, acc)
when type in @allowed_nested_function_arguments do
value = eval!(function, context, mod)
flag = if value == true && function_name == "or", do: :halt, else: :cont

{flag, acc ++ [[literal: value]]}
end

defp args_reducer(function, _function_name, _context, _mod, acc),
do: {:cont, acc ++ [function]}
end
10 changes: 10 additions & 0 deletions test/expression/eval_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,16 @@ defmodule Expression.EvalTest do
})
end

test "if with nil values" do
{:ok, ast, "", _, _, _} =
Parser.parse("@if(status,\nLEFT(status, 2),\nfalse)")

assert false ==
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Djack1010 Djack1010 Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

describe "lambdas" do
test "with map" do
{:ok, ast, "", _, _, _} = Parser.parse("@map(foo, &([&1, 'Button']))")
Expand Down
Loading