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

Variable references in function definitions don't use lexical scoping in the same scope #706

Open
smking opened this issue Jul 31, 2024 · 4 comments

Comments

@smking
Copy link

smking commented Jul 31, 2024

Function definitions are supposed to be closures: "When a lambda function is defined, the evaluation engine takes a snapshot of the environment and stores it with the function body definition." (https://bit.ly/3YmoKHC). However, the evaluator stores a reference instead of a snapshot of the environment, so references inside a function definition to a variable defined later in the same scope will refer to updated variable values.

For example, This returns an undefined value as expected (https://try.jsonata.org/umn7J2aFi):

(
    $f := function() { $x };
    $f()
)

And this returns 42 as expected because the value of $x is defined before the function definition (https://try.jsonata.org/xvUAkj0sJ):

(
    $x := 42;
    $f := function() { $x };
    $f()
)

However, if $x is assigned after the function definition but before the function call, the function call will use the new value of $x, which is unexpected behaviour (https://try.jsonata.org/ev2AQwZIW):

(
    $x := 42;
    $f := function() { $x };
    $x := 10;
    $f()
)

A possible solution is to store a copy instead of a reference to the environment here: https://github.com/jsonata-js/jsonata/blob/master/src/jsonata.js#L1568

@mikhail-barg
Copy link

For me current behaviour is an expected one.

@smking
Copy link
Author

smking commented Aug 2, 2024

This may just be a documentation issue. For example, the big example at the top of https://docs.jsonata.org/programming requires the current behavior to define $sin before $cos.

@andrew-coleman
Copy link
Member

Thanks for raising this, it's an interesting point.

My initial reaction was that this is a bug for the reasons you've given, and that your proposed solution would fix it (notwithstanding that this would be a breaking change, as highlighted by the last comment).

Unfortunately, implementing this fix would preclude the ability to write recursive functions since the variable $f that's bound to the function won't be in scope at the time that the lambda is evaluated. Even if I was to 'special case' this somehow, it would still preclude writing mutually recursive functions.

So I think we are going to have to stick with the status quo and clarify the documentation.

@smking
Copy link
Author

smking commented Aug 20, 2024

Thanks Andrew. This makes sense.

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

No branches or pull requests

3 participants