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

Improve error when rule declared with assignment operator #1541

Closed
tsandall opened this issue Jul 3, 2019 · 5 comments
Closed

Improve error when rule declared with assignment operator #1541

tsandall opened this issue Jul 3, 2019 · 5 comments

Comments

@tsandall
Copy link
Member

tsandall commented Jul 3, 2019

If you try to define a rule with the assignment operator you get a confusing error message:

package x

p := 1
rego_parse_error: rule name conflicts with built-in function

This is likely because the parsed AST is something like...

package x

assign(p, 1)

We should special-case the error handling to report something more actionable, for example:

rego_parse_error: unexpected := operator (did you mean <rule-name> = ...?)
@srenatus
Copy link
Contributor

Couldn't we allow top-level assignments like that, alternatively?

@tsandall
Copy link
Member Author

We could support the := operator at the package level, we just need to be careful about semantics.

Inside of a query:

p := 1
p := 2  # not allowed, redeclaration error at compile-time

Inside of a package:

p := 1
p := 2 # same behaviour?

cc @timothyhinrichs curious what you think. I know this will make @mikol happy.

If we made this change we should probably update the REPL to behave the same way. Today in the REPL using := multiple times will redeclare the rule:

> a := 1
Rule 'a' re-defined in package repl. Type 'show' to see rules.
> show
package repl

a = 1
> a := 2
Rule 'a' re-defined in package repl. Type 'show' to see rules.
> show
package repl

a = 2

Using the REPL behavior in packages would be problematic because of multiple files.

@mikol
Copy link
Contributor

mikol commented Jul 15, 2019

If we made this change we should probably update the REPL to behave the same way.

Seems acceptable. unset is already supported and necessary (albeit less convenient than allowing a rule to be rewritten using :=; but I’d argue that REPL semantics should match package semantics whenever possible).

tsandall added a commit to tsandall/opa that referenced this issue Aug 7, 2019
These changes update the AST to allow users to declare rules with the
:= operator. The compiler checks that the rule has not been declared
elsewhere in the package. These changes help bridge the gap between
the REPL and modules.

Fixes open-policy-agent#1541

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@sneko
Copy link

sneko commented Feb 5, 2020

Sorry but I'm not sure what's the right way of doing now...

  • Doing aaa := "hello" will throw rego_parse_error: rule name conflicts with built-in function
  • Doing aaa = hello seems correct

Can you confirm the second one is the right way to define variable outside queries?

Thank you,

@tsandall
Copy link
Member Author

tsandall commented Feb 5, 2020

@sneko if you're seeing that parse error it's because you are running an old version of OPA. This feature was added in v0.14.0. If you're running an older version of OPA then use =.

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

No branches or pull requests

4 participants