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

Providing wrong arity to built-ins should explain actual problem #4054

Closed
anderseknert opened this issue Nov 24, 2021 · 3 comments · Fixed by #4059
Closed

Providing wrong arity to built-ins should explain actual problem #4054

anderseknert opened this issue Nov 24, 2021 · 3 comments · Fixed by #4059

Comments

@anderseknert
Copy link
Member

Short description

When providing too few arguments to a built-in function, a message about expression or variable being "unsafe" is returned. This doesn't really convey the real problem, which is of course that too few arguments were provided.

Steps To Reproduce

x := startswith("string")
1 error occurred: policy.rego:3: rego_unsafe_var_error: expression is unsafe

username = u {
    name := input.requestContext.principal.name
    startswith(name, "CN=")
    u = substring(3, count(name))
} else = input.requestContext.principal.name
1 error occurred: policy.rego:6: rego_unsafe_var_error: var u is unsafe

Expected behavior

Output to be something like "built-in X: expected <string, string, number>, got <string>"

@srenatus
Copy link
Contributor

Hmm this is interesting:

$ opa eval -fpretty 'startswith(1)'
1 error occurred: 1:1: rego_type_error: startswith: too few arguments
        have: (number)
        want: (string, string, boolean)
$ opa eval -fpretty 'x := startswith(1)'
1 error occurred: 1:1: rego_unsafe_var_error: var x is unsafe

seems like it's mostly a matter of returning the proper error. I'll look into this, seems like a good UX improvement to me.

@srenatus
Copy link
Contributor

We've got multiple stages in the compiler, and the type checker runs after the safety check. Since the safety check fails, that's the error we get.

However, there's also an "undefined function" check, and I think it's OK to amend that one: after all, wrong arity means wrong function.

@anderseknert
Copy link
Member Author

Thanks for looking into this @srenatus! Yeah, while it might seem pedantic, having clear error messages might be one of the most important investments in the debugging experience IMHO.. so I think it's time well spent :)

srenatus added a commit to srenatus/opa that referenced this issue Nov 30, 2021
Before, the "undefined function" check stage in the compiler (and query
compiler) only asserted that the function was known.

Now, we'll also check that the number of arguments _could be_ valid. If
it really is valid will be determined by the type checker at a later
stage.

However, asserting the arity early allows us to give more on-the-spot
error messages.

Fixes open-policy-agent#4054.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus added a commit that referenced this issue Dec 1, 2021
* ast/compile: check arity in undefined function stage

Before, the "undefined function" check stage in the compiler (and query
compiler) only asserted that the function was known.

Now, we'll also check that the number of arguments _could be_ valid. If
it really is valid will be determined by the type checker at a later
stage.

However, asserting the arity early allows us to give more on-the-spot
error messages.

Fixes #4054.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
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

Successfully merging a pull request may close this issue.

2 participants