open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.67k stars 1.34k forks source link

unary `-` sign breaks when used for `input` document #5014

Open philipaconrad opened 2 years ago

philipaconrad commented 2 years ago

Short description

Quoting from the PR discussion on #4955, @shaded-enmity dug up this test case that fails mysteriously:

One thing I just discovered when refining the grammar for floating point numbers is the handling of unary -:

numeric := -1 # works fine

other := -input.number # error

The second example fails with:

1 error occurred: test_unary.rego:3: rego_parse_error: unexpected assign token: expected rule value term (e.g., other := <VALUE> { ... })
  other := -input.number

This looks like a possible parser bug, where a ref (input.number) is being treated differently somehow than a raw numeric constant.

Steps To Reproduce

opa eval --format=pretty 'output := -input.number'

Expected behavior

Ideally, the error should be different or non-existent for the opa eval example.

Additional Context

This behavior can also be observed on the Rego Playground.

stale[bot] commented 2 years ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

philipaconrad commented 2 years ago

This issue may be stale, but it's still an issue! :sweat_smile:

Hopefully, I or someone else will have time to triage this issue in the parser soon.

srenatus commented 2 years ago

@philipaconrad I've put it into the backlog; that way, it won't get picked up (and picked on) by the stale bot.

philipaconrad commented 1 year ago

The underlying issue here is that the parser has been treating unary minus parsing as part of how numeric literals are parsed (ex: -.05 is fine, but -input.x is not).

I suspect the best way to fix this might be to do rewriting into something like numbers.negate(input.x), since that would easily cover most of the obvious syntactic variations around numbers/variables/refs that are possible, at the expense of slightly more complicated parsing/AST structure. :slightly_frowning_face:

anakrish commented 1 year ago

I also ran into this issue recently.

Rego lists the following production

unary-expr      = "-" expr

which indicates that all expressions must be allowed after the - sign, at least syntactically.