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

Argument names for functions should be mangled when using value-argument syntax #2039

Open
JeffreyMercado opened this issue Jan 3, 2020 · 5 comments

Comments

@JeffreyMercado
Copy link

Describe the bug
When you define a function but want value semantics for arguments, you could use the shorthand:

def function($arg): ...;

which is equivalent to:

def function(arg): arg as $arg | ...;

https://stedolan.github.io/jq/manual/#DefiningFunctions

This poses a problem if the value-argument name happens to share the name of a function that's in scope, the local argument is used. Can be confusing (see repro).

Instead, some sort of name mangling should be performed so it's equivalent to something along the lines of:

def function(_function1_arg_): _function1_arg_ as $arg | ...;
# mangled: _[function name][function arity]_[arg name]_

To Reproduce

def fn($str; $length):
    ($str|length) as $len
    | $len;
fn("abc"; 99) # would expect to return 3, returns 99

workaround, change arg name:

def fn2($str; $length_):
    ($str|length) as $len
    | $len;
fn2("abc"; 99) # would expect to return 3, returns 3

Expected behavior
The argument name should not conflict with names in scope when value-argument shorthand is used. Slight semantic change but I'd argue it's less confusing.

Environment (please complete the following information):
jq <=1.6

Additional context
Was defining some helper functions and wanted to create string padding functions and started with this initially.

def rpad($str; $length; $pad): ($str|length) as $len | # <- problem!
    (select($len < $length) | "\($str)\($pad*($length-$len))") // $str
    ;
def rpad($str; $length):
    rpad($str; $length; " ")
    ;

Fix using the name $size instead of $length.

@nicowilliams
Copy link
Contributor

I think I must have sensed this would be a problem when I added that... This isn't quite a bug because the docs should make it obvious to the user, however, it is easy to be surprised by this.

This should be trivial to fix in gen_function() in src/compile.c, where we should mangle the name of the formal before binding it to the body.

However, by now there may exist code that depends on this behavior, so I think perhaps it's too late to change this. Let's sleep on it.

@nicowilliams
Copy link
Contributor

Perhaps we should add a warning to the documentation about this.

@JeffreyMercado
Copy link
Author

A warning pointing out this scenario would be helpful. Perhaps in a future future version, consider have a "fix" as a breaking change. When using this syntax, one shouldn't expect the value-param $arg to define both arg and $arg IMHO.

I took a shot at the fix in JeffreyMercado:mangle-params

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented Jan 16, 2020

I actually think that mangling is a really nice approach.
Also, I think that since the parameter name is scoped to the function and will only be visible in the disassembly it could be unnecessary to include the function name and arity.
Instead, maybe the mangled name should include a character that can't be entered in the correct jq program but can be used as part of the string symbol programmatically: the $ itself!
This way, the mangled parameter name could become ... $arg or arg$

@nicowilliams
Copy link
Contributor

I don't like mangling. Either bind the non-$ symbol as-is, or not at all. Also, I don't think we can make either change backwards-compatibly. I've been thinking that we could have a thing at the top of a jq program/module that selects a language version if we must make backwards-incompatible changes, but even that is somewhat limited in that it would be easy to have alternative parsers and alternative builtins, but it's less easy to have alternative compilers and linkers (though maybe we can just have flags in the blocks for that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants