opa icon indicating copy to clipboard operation
opa copied to clipboard

Update Rego EBNF grammar

Open shaded-enmity opened this issue 2 years ago • 3 comments

While working on Rego grammar for tree-sitter (https://github.com/shaded-enmity/tree-sitter-rego/blob/complete-ebnf-grammar/grammar.js => https://github.com/FallenAngel97/tree-sitter-rego/pull/2/files) I encountered a couple of problems that I'd like to bring up for discussion:

  1. The first symbol for rule-head-obj and rule-head-func in the current grammar is marked as optional, however that makes both rules boil down to rule-head-comp. This patch changes the first term of the two rules into non-optionals.
  2. The first symbol of the expr-infix rule is [ term "=" ], which causes tree-sitter to error because expr-infix now conflicts with itself. This patch removes that symbol and adds assignment := and unification = as an infix binary operation rules.
  3. Add support for in membership operator via the membership rule
  4. Add support for parenthesized expressions (necessary for 3. but also to support general stuff like number := 2 * (3 + 2) via the expr-parens rule

shaded-enmity avatar Aug 02 '22 14:08 shaded-enmity

Thanks for taking a look at the EBNF grammar, @shaded-enmity! :smile: The parser in OPA is not generated directly from tools, so it's possible for the grammar definition docs to drift out of line with the language, or have issues that aren't immediately noticed.

I won't be able to review this PR today, but I'll try to take a look at it this week if I can. A good tree-sitter grammar would be useful for tools authors.

philipaconrad avatar Aug 02 '22 17:08 philipaconrad

Thanks for taking a look at the EBNF grammar, @shaded-enmity! smile The parser in OPA is not generated directly from tools, so it's possible for the grammar definition docs to drift out of line with the language, or have issues that aren't immediately noticed.

I won't be able to review this PR today, but I'll try to take a look at it this week if I can. A good tree-sitter grammar would be useful for tools authors.

No problem, take your time - I've just added two new rules (membership and expr-parens) hopefully bringing the grammar definition close to completion (fingers crossed!), but I'll keep adding test cases and breaking things :).

Once I figure out the details how to do it I'll post a link to a tree-sitter playground (something like https://tree-sitter.github.io/tree-sitter/playground) so that you can just type in any Rego code and see how the current version of the parser handles it.

shaded-enmity avatar Aug 05 '22 18:08 shaded-enmity

Playground is up and running:

https://shaded-enmity.github.io/tree-sitter-rego/

shaded-enmity avatar Aug 05 '22 19:08 shaded-enmity

@shaded-enmity I got to try out the playground demo with a non-trivial example or two, and it looks awesome!

I'll do a more in-depth review of the grammar changes tomorrow morning to make sure nothing major got missed, and then I think this PR will be good-to-merge. :smile:

philipaconrad avatar Aug 11 '22 01:08 philipaconrad

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

Is this by design or a bug? :smiley:

shaded-enmity avatar Aug 11 '22 15:08 shaded-enmity

Is this by design or a bug? :smiley:

@shaded-enmity That is almost certainly a parser/evaluator bug. :bug:

You can file an issue around that test case if you'd like, or I can file the issue later today. :slightly_smiling_face: I'm hoping it will be straightforward to fix, since this looks like a case where a ref (like input.number) is just being treated differently than a raw numeric constant.

philipaconrad avatar Aug 11 '22 16:08 philipaconrad

@philipaconrad Thanks for creating the issue and apologies for trailing off a bit - first I had covid so I had to take a break and now I'm configuring a new laptop and I'm not in the zone yet.

I'll push an update later today containing:

  • Changes to membership as per @srenatus 's feedback
  • Unary -
  • Changes to rule-body to accommodate for direct literals

shaded-enmity avatar Aug 18 '22 12:08 shaded-enmity