Fix false positive in use-some-for-output-vars (#1731)

Which was reported for nested ref head vars

Fixes #1489

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

Author: anderseknert

bundle/regal/ast/search.rego

@@ -272,14 +272,27 @@ found.expressions[rule_index] contains value if {
 	value.terms
 }
 
+# METADATA
+# description: |
+#   answers whether a variable of the given name (value) is declared in the
+#   local scope of the provided rule at the provided location
+is_in_local_scope(rule, location, value) if {
+	some var
+	found.vars[_rule_index(rule)][_][var].value == value
+	var.location != location
+
+	_before_location(rule.head, var, util.to_location_object(location))
+}
+
 # METADATA
 # description: |
 #   finds all vars declared in `rule` *before* the `location` provided
 #   note: this isn't 100% accurate, as it doesn't take into account `=`
 #   assignments / unification, but it's likely good enough since other rules
 #   recommend against those
 find_vars_in_local_scope(rule, location) := [var |
-	var := found.vars[_rule_index(rule)][_][_]
+	some var
+	found.vars[_rule_index(rule)][_][var]
 
 	not startswith(var.value, "$")
 	_before_location(rule.head, var, util.to_location_object(location))
@@ -309,32 +322,25 @@ _before_location(_, var, loc) if {
 
 # METADATA
 # description: find *only* names in the local scope, and not e.g. rule names
-find_names_in_local_scope(rule, location) := names if {
-	fn_arg_names := {arg.value |
-		some arg in rule.head.args
-		arg.type == "var"
-	}
-	var_names := {var.value | some var in find_vars_in_local_scope(rule, util.to_location_object(location))}
-
-	names := fn_arg_names | var_names
+find_names_in_local_scope(rule, location) := {var.value |
+	some var in find_vars_in_local_scope(rule, util.to_location_object(location))
 }
 
 # METADATA
 # description: |
 #   similar to `find_vars_in_local_scope`, but returns all variable names in scope
 #   of the given location *and* the rule names present in the scope (i.e. module)
-find_names_in_scope(rule, location) := names if {
-	locals := find_names_in_local_scope(rule, util.to_location_object(location))
-
-	# parens below added by opa-fmt :)
-	names := (rule_names | imported_identifiers) | locals
-}
+find_names_in_scope(rule, location) := (rule_names | imported_identifiers) | find_names_in_local_scope(
+	rule,
+	util.to_location_object(location),
+)
 
 # METADATA
 # description: |
 #   find all variables declared via `some` declarations (and *not* `some .. in`)
 #   in the scope of the given location
 find_some_decl_names_in_scope(rule, location) := {some_var.value |
+	loc := util.to_location_object(location)
 	some some_var in found.vars[_rule_index(rule)].some
-	_before_location(rule.head, some_var, util.to_location_object(location))
+	_before_location(rule.head, some_var, loc)
 }

bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars.rego

@@ -7,39 +7,39 @@ import data.regal.result
 import data.regal.util
 
 report contains violation if {
-	some rule_index, i
-	elem := ast.found.refs[rule_index][_].value[i]
+	some rule_index, term
+	ast.found.vars[rule_index].ref[term]
 
-	# first item can't be a loop or key ref
-	i != 0
-	elem.type == "var"
-	not ast.is_wildcard(elem)
+	not startswith(term.value, "$")
+	not term.value in ast.imported_identifiers
+	not term.value in ast.rule_names
 
 	rule := input.rules[to_number(rule_index)]
 
-	not elem.value in ast.find_names_in_scope(rule, elem.location)
+	not ast.is_in_local_scope(rule, term.location, term.value)
 
-	path := _location_path(rule, elem.location)
+	walk(rule, [path, term.location])
 
-	not _var_in_comprehension_body(elem, rule, path)
+	not _var_in_ref_head_declared(path, rule_index, term.value)
+	not _var_in_comprehension_body(path, term.value, rule)
 
-	violation := result.fail(rego.metadata.chain(), result.location(elem))
+	violation := result.fail(rego.metadata.chain(), result.location(term))
 }
 
-_location_path(rule, location) := path if walk(rule, [path, location])
-
-_var_in_comprehension_body(var, rule, path) if {
-	some v in _comprehension_body_vars(rule, path)
-	v.type == var.type
-	v.value == var.value
-}
-
-_comprehension_body_vars(rule, path) := [vars |
+_var_in_comprehension_body(path, value, rule) if {
 	some parent_path in array.reverse(util.all_paths(path))
 
 	node := object.get(rule, parent_path, {})
 
 	node.type in {"arraycomprehension", "objectcomprehension", "setcomprehension"}
 
-	vars := ast.find_vars(node.value.body)
-][0]
+	some v in ast.find_vars(node.value.body)
+	v.type == "var"
+	v.value == value
+}
+
+_var_in_ref_head_declared(path, rule_index, value) if {
+	path[0] == "head"
+	path[1] == "ref"
+	ast.found.vars[rule_index]["some"][_].value == value
+}

bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars_test.rego

@@ -160,3 +160,10 @@ test_success_not_an_output_var if {
 
 	r == set()
 }
+
+# verify fix for https://github.com/open-policy-agent/regal/issues/1489
+test_success_nested_ref_head_var_declared if {
+	r := rule.report with input as ast.policy(`rule[input[i]] contains i if some i`)
+
+	r == set()
+}

pkg/linter/linter_bench_test.go

@@ -11,12 +11,6 @@ import (
 	"github.com/open-policy-agent/regal/pkg/report"
 )
 
-// 752941230 ns/op	2513359004 B/op	52228468 allocs/op
-// 731859438 ns/op	2481404780 B/op	51310839 allocs/op main
-// 720459812 ns/op	2474991988 B/op	51155691 allocs/op
-// 704425916 ns/op	2476541140 B/op	51151877 allocs/op
-
-// BenchmarkRegalLintingItself-16    	       2	 717249188 ns/op	2477907556 B/op	51198136 allocs/op
 func BenchmarkRegalLintingItself(b *testing.B) {
 	conf := testutil.Must(config.FromPath(filepath.Join("..", "..", ".regal", "config.yaml")))(b)
 
@@ -34,8 +28,6 @@ func BenchmarkRegalLintingItself(b *testing.B) {
 	testutil.AssertNumViolations(b, 0, rep)
 }
 
-// 955670792 ns/op	3004937416 B/op	57408554 allocs/op
-// ...
 func BenchmarkRegalLintingItselfPrepareOnce(b *testing.B) {
 	conf := testutil.Must(config.FromPath(filepath.Join("..", "..", ".regal", "config.yaml")))(b)
 
@@ -54,8 +46,6 @@ func BenchmarkRegalLintingItselfPrepareOnce(b *testing.B) {
 	testutil.AssertNumViolations(b, 0, rep)
 }
 
-// 62198250 ns/op	42255370 B/op	  985472 allocs/op
-// ...
 func BenchmarkOnlyPrepare(b *testing.B) {
 	conf := testutil.Must(config.FromPath(filepath.Join("..", "..", ".regal", "config.yaml")))(b)
 	linter := NewLinter().WithInputPaths([]string{"../../bundle"}).WithUserConfig(conf)
@@ -65,9 +55,6 @@ func BenchmarkOnlyPrepare(b *testing.B) {
 	}
 }
 
-// 6	 168875708 ns/op	455586606 B/op	 8537889 allocs/op
-// 8	 139592615 ns/op	326694376 B/op	 5996314 allocs/op
-// ...
 func BenchmarkRegalNoEnabledRules(b *testing.B) {
 	linter := NewLinter().
 		WithInputPaths([]string{"../../bundle"}).
@@ -83,8 +70,6 @@ func BenchmarkRegalNoEnabledRules(b *testing.B) {
 	testutil.AssertNumViolations(b, 0, rep)
 }
 
-// 97546746 ns/op	401323023 B/op	 7427903 allocs/op
-// ...
 func BenchmarkRegalNoEnabledRulesPrepareOnce(b *testing.B) {
 	linter := NewLinter().
 		WithInputPaths([]string{"../../bundle"}).