ast/parser: Detect function rule head + contains keyword. #5526

philipaconrad · 2023-01-03T23:30:54Z

Previously, we did not detect misuses of the contains keyword, which is only valid as a syntactic sugar for partial set definition rules.

This PR adds some logic to detect when a function rule head has been paired with the contains keyword, which should generally be an error condition.

~~Note: We can't catch the following case, because the number of args is zero, and we can't distinguish nil from [] in Go.~~
EDIT: I was a smidge tired when I made the initial PR, and mixed up the nil rules in Golang around slices. It's len() that doesn't distinguish between nil and []. Silly mistakes happen, even to the best of us. 😅 (Thanks for the correction, @srenatus!)

package test

# Both of these cases result in an error condition:
p() contains x {
    x := 1
}

q(a) contains x {
    x := a
}

Fixes: #5525

philipaconrad · 2023-01-03T23:43:43Z

ast/compile_test.go

@@ -1734,6 +1734,24 @@ p {
 	}
 }

+func TestCompilerCheckSafetyFunctionAndContainsKeyword(t *testing.T) {


Originally, the check lived under check.go, and so the compiler was doing the error checking. I left the extra test in, since it's not hurting anything, and it's nice to have a downstream safety check.

philipaconrad · 2023-01-03T23:44:41Z

ast/parser.go

@@ -831,6 +831,10 @@ func (p *Parser) parseHead(defaultRule bool) (*Head, bool) {

 	switch p.s.tok {
 	case tokens.Contains: // NOTE: no Value for `contains` heads, we return here
+		// Catch error case of using 'contains' with a function definition rule head.


This is located before the scan to have the error location land on the keyword. Using p.save() + p.restore() later would have a similar effect.

philipaconrad · 2023-01-03T23:45:56Z

ast/check.go

@@ -213,7 +213,6 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) {
 	var tpe types.Type

 	if len(rule.Head.Args) > 0 {


This is a minor diff that got left in after a local rebase (when the check moved from check.go -> parser.go).

srenatus · 2023-01-04T08:56:56Z

we can't distinguish nil from [] in Go

We can't? I believe only len() on a slice can't, but we could do it differently. See this playground.

But there might be more going on here, I just got surprised by the statement in isolation 🙃

srenatus

Thanks! Nitpicks only. 👍

ast/compile_test.go

srenatus · 2023-01-04T08:54:50Z

ast/parser.go

@@ -831,6 +831,10 @@ func (p *Parser) parseHead(defaultRule bool) (*Head, bool) {

 	switch p.s.tok {
 	case tokens.Contains: // NOTE: no Value for `contains` heads, we return here
+		// Catch error case of using 'contains' with a function definition rule head.
+		if len(head.Args) > 0 {
+			p.illegal("the contains keyword can only be used with partial set definitions (e.g., %s contains <VALUE> { ... })", name)


So we've tacitly tried to shift terminology from "partial object", "partial set" and "complete rule" to "multi-value rule" and "single-value rule", with the introduction of ref heads.

Rule head before after

p contains x partial set multi-value rule

p[x] = y partial object single-value rule

p = y complete rule single-value rule

This looks like another opportunity for using "multi-value rule".

Thanks for the correction! I noticed the unfamiliar "multi-value rule" language in the error messages, but wasn't exactly sure how to interpret what I was seeing.

I've rolled the language shift into my latest changes.

philipaconrad · 2023-01-04T23:09:36Z

I just got surprised by the statement in isolation

@srenatus You were right to be surprised! Because I was wrong, and the PR has wound up better for the correction. 😅 For some reason, I had gotten confused about the nil rules in Go, and that resulted in a less-accurate check.

I've fixed the goofy logic, and added an extra test case to check for the "function with 0 arguments" case.

Previously, we did not detect misuses of the `contains` keyword, which is only valid as a syntactic sugar for partial set definition rules. This commit adds some logic to detect when a function rule head has been paired with the `contains` keyword, which should generally be an error condition. Fixes: open-policy-agent#5525 Signed-off-by: Philip Conrad <[email protected]>

srenatus

👏 🎉 Thanks!

philipaconrad added the bug label Jan 3, 2023

philipaconrad requested a review from srenatus January 3, 2023 23:30

philipaconrad self-assigned this Jan 3, 2023

philipaconrad force-pushed the rt-error-function-and-contains branch from 748314e to edef6d0 Compare January 3, 2023 23:31

philipaconrad commented Jan 3, 2023

View reviewed changes

srenatus previously approved these changes Jan 4, 2023

View reviewed changes

philipaconrad dismissed srenatus’s stale review via 5b6a6f6 January 4, 2023 22:57

philipaconrad force-pushed the rt-error-function-and-contains branch 2 times, most recently from 5b6a6f6 to 376ddff Compare January 4, 2023 23:00

philipaconrad force-pushed the rt-error-function-and-contains branch from 376ddff to 841c0ee Compare January 4, 2023 23:11

srenatus approved these changes Jan 5, 2023

View reviewed changes

Merge branch 'main' into rt-error-function-and-contains

55b2e3a

srenatus merged commit 5eccbb4 into open-policy-agent:main Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ast/parser: Detect function rule head + contains keyword. #5526

ast/parser: Detect function rule head + contains keyword. #5526

philipaconrad commented Jan 3, 2023 •

edited

Loading

philipaconrad Jan 3, 2023 •

edited

Loading

philipaconrad Jan 3, 2023

philipaconrad Jan 3, 2023

srenatus commented Jan 4, 2023 •

edited

Loading

srenatus left a comment

srenatus Jan 4, 2023

philipaconrad Jan 4, 2023

philipaconrad commented Jan 4, 2023

srenatus left a comment

		@@ -213,7 +213,6 @@ func (tc typeChecker) checkRule(env TypeEnv, as AnnotationSet, rule Rule) {
		var tpe types.Type

		if len(rule.Head.Args) > 0 {

Rule head	before	after
p contains x	partial set	multi-value rule
p[x] = y	partial object	single-value rule
p = y	complete rule	single-value rule

ast/parser: Detect function rule head + contains keyword. #5526

ast/parser: Detect function rule head + contains keyword. #5526

Conversation

philipaconrad commented Jan 3, 2023 • edited Loading

philipaconrad Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

philipaconrad Jan 3, 2023

Choose a reason for hiding this comment

philipaconrad Jan 3, 2023

Choose a reason for hiding this comment

srenatus commented Jan 4, 2023 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

srenatus Jan 4, 2023

Choose a reason for hiding this comment

philipaconrad Jan 4, 2023

Choose a reason for hiding this comment

philipaconrad commented Jan 4, 2023

srenatus left a comment

Choose a reason for hiding this comment

philipaconrad commented Jan 3, 2023 •

edited

Loading

philipaconrad Jan 3, 2023 •

edited

Loading

srenatus commented Jan 4, 2023 •

edited

Loading