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

bug: $x.$number() incorrectly returns no value #155

Open
tommy opened this issue Jan 9, 2025 · 1 comment
Open

bug: $x.$number() incorrectly returns no value #155

tommy opened this issue Jan 9, 2025 · 1 comment

Comments

@tommy
Copy link
Contributor

tommy commented Jan 9, 2025

The . syntax introduces a "context" variable named $. Some built-in functions ($string, $number, $abs, etc) are meant to use the value of $ as the first parameter if no explicit arguments are provided. This is probably a discrepancy with our other functions as well.

/* in the Javascript reference implementation: */
( $x := "123"; $number($x) ) /* evals to 123 */ ,
( $x := "123"; $x.$number($) ) /* evals to 123 */ ,
( $x := "123"; $x.$number() ) /* evals to 123 */ ,

/* in jsonata-rs: */
( $x := "123"; $number($x) ) /* evals to 123 */ ,
( $x := "123"; $x.$number($) ) /* evals to 123 */ ,
( $x := "123"; $x.$number() ) /* returns no match */ ,

Note that this behavior is (as far as I can tell) specific to each function. That is, it's not the case that every function invoked in this way gets the context parameter injected. Eg:

/* Javascript */
( $f := function($arg) { $arg } ; $x := "123" ; $x.$f() ) /* returns no match */ ,
( $f := function($arg) { $arg } ; $x := "123" ; $f($x) ) /* returns "123" */ ,
@lmurri
Copy link

lmurri commented Jan 20, 2025

Hi! Was checking out the public repos and stumbled upon this so gave it a deep dive to start getting up to speed. Disclaimer: new to jsonata / rust so might be off :P

Per the Jsonata docs , if arg is not specified in $number(arg) then the context value is used as the value of arg. More generally (looking at the original JS implementation), each built in function has a defined signature , - in the signature implies that if the param is not supplied it should default to the context value . For example, the signature of $number is '<(nsb)-:n>' which means that the function optionally takes in a number or a string or a bool, or defaults to the context value if no value is provided. Thus, any function whose signature contains a - means that one of the parameter (the one before the -) is optional. So, we should confirm the behavior / update all the functions that have optional parameters (for example, $string() seems to be implemented correctly already, while $substringBefore() is not). Similarly, when registering a custom function you may specify a signature to have optional arguments that should be replaced with the context value at runtime.

In the JS implementation, the implicit context logic is built around the signature itself. There is a signature utility function called validate that actually (sneakily) also does the substitution of the optional missing arguments with the context value. This is then used by validateArguments which is used by applyInner which is the place where functions are actually applied.

Currently jsonata-rs does not have the concept of function signatures - this has been described at length in “The problem with function signatures” doc. The conclusion was to not support (in fact remove) function signatures. However, they are part of the test suite so not sure if the goal is to bring them back for feature parity. The “within reason” clause may very well apply here since lack of signature is in no way blocking (as the user can just pass in $ ).

However, I think it’s fair to argue that at least the built-in functions should match the API as defined by the JSONata public docs, i.e. $number() explicitly mentions that

If arg is not specified (i.e. this function is invoked with no arguments), then the context value is used as the value of arg.

This doesn’t really have to do with the implementation details of signatures and seems straightforward to implement (in fact, already working for $string() and it can be confusing that it works for string, but not number). Implementation can look like this, (would just need to follow same pattern for the rest of the built-in functions that have context parms per JS impl)

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

2 participants