Fix false positive in messy-rule when ref head rules are used (#927)

Also moved out the `static_rule_name` function from the ast package
ast it was only used in one location and not generally useful.

Fixes #905

Signed-off-by: Anders Eknert <anders@styra.com>

bundle/regal/ast/ast.rego

@@ -194,16 +194,14 @@ _exclude_arg("assign", 0, _)
 all_rules_refs contains refs[_][_]
 
 # METADATA
-# title: all_refs
 # description: set containing all references found in the input AST
 # scope: document
 all_refs contains value if some value in all_rules_refs
 
 all_refs contains imported.path if some imported in input.imports
 
 # METADATA
-# title: ref_to_string
-# description:  returns the "path" string of any given ref value
+# description: returns the "path" string of any given ref value
 ref_to_string(ref) := concat(".", [_ref_part_to_string(i, part) | some i, part in ref])
 
 _ref_part_to_string(0, ref) := ref.value
@@ -215,6 +213,17 @@ _ref_part_to_string(i, ref) := concat("", ["$", ref.value]) if {
 	i > 0
 }
 
+# METADATA
+# description: |
+#   returns the string representation of a ref up until its first
+#   non-static (i.e. variable) value, if any:
+#   foo.bar -> foo.bar
+#   foo.bar[baz] -> foo.bar
+ref_static_to_string(ref) := ss if {
+	rs := ref_to_string(ref)
+	ss := substring(rs, 0, indexof(rs, ".$"))
+}
+
 static_ref(ref) if every t in array.slice(ref.value, 1, count(ref.value)) {
 	t.type != "var"
 }
@@ -223,21 +232,6 @@ static_rule_ref(ref) if every t in array.slice(ref, 1, count(ref)) {
 	t.type != "var"
 }
 
-# METADATA
-# description: |
-#   return the name of a rule if, and only if it only has static parts with
-#   no vars. This could be "username", or "user.name", but not "user[name]"
-# scope: document
-static_rule_name(rule) := rule.head.ref[0].value if count(rule.head.ref) == 1
-
-static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [ref.value |
-	some i, ref in rule.head.ref
-	i > 0
-])) if {
-	count(rule.head.ref) > 1
-	static_rule_ref(rule.head.ref)
-}
-
 # METADATA
 # description: provides a set of names of all built-in functions called in the input policy
 builtin_functions_called contains name if {

bundle/regal/ast/ast_test.rego

@@ -320,18 +320,3 @@ test_all_refs if {
 
 	text_refs == {":=", "data.foo.bar", "data.foo.bax", "data.foo.baz"}
 }
-
-test_static_rule_name_one_part if {
-	rule := {"head": {"ref": [{"type": "var", "value": "username"}]}}
-	ast.static_rule_name(rule) == "username"
-}
-
-test_static_rule_name_multi_part if {
-	rule := {"head": {"ref": [{"type": "var", "value": "user"}, {"type": "string", "value": "name"}]}}
-	ast.static_rule_name(rule) == "user.name"
-}
-
-test_static_rule_name_var_part if {
-	rule := {"head": {"ref": [{"type": "var", "value": "user"}, {"type": "var", "value": "name"}]}}
-	not ast.static_rule_name(rule)
-}

bundle/regal/rules/style/default_over_not.rego

@@ -16,7 +16,7 @@ report contains violation if {
 	not rule["default"]
 	ast.generated_body(rule)
 
-	name := ast.static_rule_name(rule)
+	name := _static_rule_name(rule)
 	value := rule.head.value
 
 	ast.static_ref(value)
@@ -27,7 +27,7 @@ report contains violation if {
 	sibling_rules := [sibling |
 		some j, sibling in ast.rules
 		i != j
-		ast.static_rule_name(sibling) == name
+		_static_rule_name(sibling) == name
 	]
 
 	some sibling in sibling_rules
@@ -39,3 +39,13 @@ report contains violation if {
 
 	violation := result.fail(rego.metadata.chain(), result.location(sibling.body[0]))
 }
+
+_static_rule_name(rule) := rule.head.ref[0].value if count(rule.head.ref) == 1
+
+_static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [ref.value |
+	some i, ref in rule.head.ref
+	i > 0
+])) if {
+	count(rule.head.ref) > 1
+	ast.static_rule_ref(rule.head.ref)
+}

bundle/regal/rules/style/messy_rule.rego

@@ -10,16 +10,16 @@ import data.regal.result
 report contains violation if {
 	some i, rule1 in input.rules
 
-	cur_name := ast.ref_to_string(rule1.head.ref)
+	cur_name := ast.ref_static_to_string(rule1.head.ref)
 
 	some j, rule2 in input.rules
 
 	j > i
 
-	nxt_name := ast.ref_to_string(rule2.head.ref)
+	nxt_name := ast.ref_static_to_string(rule2.head.ref)
 	cur_name == nxt_name
 
-	previous_name := ast.ref_to_string(input.rules[j - 1].head.ref)
+	previous_name := ast.ref_static_to_string(input.rules[j - 1].head.ref)
 	previous_name != nxt_name
 
 	violation := result.fail(rego.metadata.chain(), result.location(rule2))

bundle/regal/rules/style/messy_rule_test.rego

@@ -72,6 +72,19 @@ test_success_non_incremental_nested_rule_definiton if {
 	r == set()
 }
 
+test_success_non_messy_ref_head_rules if {
+	module := ast.with_rego_v1(`
+	keywords[foo.bar] contains "foo"
+
+	keywords[foo] contains "foo"
+
+	keywords[foo.baz] contains "foo"
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
 test_fail_messy_incremental_nested_variable_rule_definiton if {
 	module := ast.with_rego_v1(`
 	base[x].foo := 5 if { x := 1 }