Unexpected behavior when rule name contains dash

[anderseknert]

Dashes are not allowed in (simple) rule names, and need to be quoted in refs. Users unaware of this will however not learn this from OPA, as this surprisingly parses and compiles:

package p

foo-bar := 1

It will not evaluate though (undefined). Compiling with strict mode on will result in an error, but not one you'd reasonably expect:

2 errors occurred:
policy.rego:3: rego_compile_error: unused argument foo. (hint: use _ (wildcard variable) instead)
policy.rego:3: rego_compile_error: unused argument bar. (hint: use _ (wildcard variable) instead)

Running opa fmt on this policy tells us what OPA seems to see here:

package p

minus(foo, bar) := 1

Which while it explains the observed behavior, doesn't explain why a dash (or a minus if you will) in between two words "expands" to minus with the symbols to the left and right as arguments.

Checking the AST confirms this representation as well, so this issue seems to be in the parser.

Desired behavior here would IMO be that this rendered an error explaining that the dash is not valid at that position.

[anderseknert]

I should probably add that the dash/minus isn't special here — any operator (built-in function with an infix attribute) will behave the same way, i.e. p/q := 1, a+b := 2, etc. The dash is just most likely to be actually used here.

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

[mmzzuu]

Hi opa maintainers,

I have looked into this issue to potentially contribute, and came across some questions, which I would like to discuss with you first:

Shadowing of built-in functions:

If any infix operator is used in a rulename, it will currently result in shadowing the behavior of this built-in function. So if we extend your example as follows:

package p

foo-bar := 1

a := 9-2

foo-bar := 1 will be interpreted as minus(foo, bar) := 1, shadowing the built-in function minus.

--> So, a will evaluate to 1.

I think this behavior is not expected by 99% of the users and could lead to problems, which are not so obvious to find. Also, according to the Rego grammar, this is not valid syntax. So raising the error would make sense in my opinion - but it would also mean we would remove this (not obvious and I think not documented?) capability to shadow built-in functions using the infix form.

• If we raise the error - do we still want to allow shadowing the built-in functions when they are written in the prefix form (e.g. minus(foo, bar) := 1)? (the linter already discovers shadowing of built-in functions, which might be sufficient - see documentation). Also, if the prefix form is used, it should be obvious to the user and thus intended, and it is also valid syntax according to the Rego grammar. So from this perspective, it would make sense to only focus on the infix operator form, but what do you think?

Scope and impact of error:

• Should we raise the error only for '-' or for all infix operators? To be in line with the Rego grammar definition, all infix operators on the lhs of rules should raise an error.

• When users currently have such rules in use (intentionally or not), the policy will no longer work after implementing the error. This may cause problems which we could help solve by providing information in the release notes - what do you think?

[anderseknert]

Hi @mmzzuu, and good to see you're back! 😃

I personally wouldn't mind that we forbid shadowing built-ins entirely, and I'd be happy to discuss that too. But as you noted, at least we have Regal flag that already.

I think for now, let's try to isolate this issue purely to how this form is parsed. If we make this a parser error, there isn't going to be a shadowing issue here, as foo-bar := 1 won't be parsed as minus(foo, bar) := 1 but an invalid rule head ref.

[tsandall]

+1 for scoping this to disallow builtin function infix operators and not dealing with built-in function shadowing for now. If we do try to solve the built-in shadowing problem we need be aware that such a change would cause any new built-in function to break existing policies using that name (which seems suboptimal.)

[anderseknert]

Good point, @tsandall. Not sure what our stance on strict mode expanding in scope is (or maybe a v2 strict mode), but perhaps that'd be a reasonable middle ground to consider for the future. So that the policy below would continue to "work" as before, but fail in future versions of strict mode:

package p

minus(x, y) := x + y

x := 10 - 5  # 15

I think it's reasonable to believe people who run with strict mode on would want things to break on shadowing conflicts. But OTOH, maybe Regal is "strict mode v2" already, and even more now as an integrated OPA project.

So yeah, long story short, let's make that infix issue a parser error @mmzzuu 😄