Rule: prefer-some-in-iteration

Building on top of previous work to find vars and to determine the
"scope" of a variable, this commit adds a new rule in the style
category to prefer `some x in y` over `x := y[_]`.

There are also a few fixups here related to the new rule, and some
that were forgotten about in the previous rule added.

Note that there are some situations where a variable may thought of
as being in scope while not actually being in scope — such as when
iteration happens after a comprehension, as we have no way of keeping
the nesting level "state" currently. This should not lead to false
positives but could lead to a few cases being missed, which I think
is OK for the time being. Will need to investigate better methods
for determining scope, and likely outside of Rego.

Fixes #243

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

README.md

@@ -160,14 +160,15 @@ The following rules are currently available:
 | imports   | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input)         | Avoid importing input                                     |
 | imports   | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import)         | Redundant import of data                                  |
 | style     | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case)                   | Prefer snake_case for names                               |
-| style     | [todo-comment](https://docs.styra.com/regal/rules/style/todo-comment)                             | Avoid TODO comments                                       |
+| style     | [unconditional-assignment](https://docs.styra.com/regal/rules/style/unconditional-assignment)     | Unconditional assignment in rule body                     |
 | style     | [external-reference](https://docs.styra.com/regal/rules/style/external-reference)                 | Reference to input, data or rule ref in function body     |
 | style     | [function-arg-return](https://docs.styra.com/regal/rules/style/function-arg-return)               | Function argument used for return value                   |
 | style     | [line-length](https://docs.styra.com/regal/rules/style/line-length)                               | Line too long                                             |
 | style     | [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment)           | Comment should start with whitespace                      |
 | style     | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix)   | Avoid get_ and list_ prefix for rules and functions       |
+| style     | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration)     | Prefer `some .. in` for iteration                         |
+| style     | [todo-comment](https://docs.styra.com/regal/rules/style/todo-comment)                             | Avoid TODO comments                                       |
 | style     | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata)                   | Detached metadata annotation                              |
-| style     | [unconditional-assignment](https://docs.styra.com/regal/rules/style/unconditional-assignment)     | Unconditional assignment in rule body                     |
 | style     | [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator)       | Prefer := over = for assignment                           |
 | style     | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt)                                       | File should be formatted with `opa fmt`                   |
 | testing   | [identically-named-tests](https://docs.styra.com/regal/rules/testing/identically-named-tests)     | Multiple tests with same name                             |

bundle/regal/ast.rego

@@ -99,6 +99,16 @@ _find_some_in_decl_vars(_, value) := var if {
 	]
 }
 
+# find vars like input[x].foo[y] where x and y are vars
+# note: value.type == "ref" check must have been done before calling this function
+find_ref_vars(value) := [var |
+	some i, var in value.value
+
+	# ignore first element, as it is the base ref (like input or data)
+	i > 0
+	var.type == "var"
+]
+
 # one or two vars declared via `every`, i.e. `every x in y {}`
 # or `every`, i.e. `every x, y in y {}`
 _find_every_vars(_, value) := var if {
@@ -108,7 +118,16 @@ _find_every_vars(_, value) := var if {
 	var := array.concat(key_var, val_var)
 }
 
-_find_set_or_array_comprehension_vars(value) := [value.value.term | value.value.term.type == "var"]
+_find_term_vars(term) := [value |
+	# regal ignore:function-arg-return
+	walk(term, [_, value])
+
+	value.type == "var"
+]
+
+_find_set_or_array_comprehension_vars(value) := [value.value.term] if {
+	value.value.term.type == "var"
+} else := _find_term_vars(value.value.term)
 
 _find_object_comprehension_vars(value) := array.concat(key, val) if {
 	key := [value.value.key | value.value.key.type == "var"]
@@ -122,6 +141,10 @@ _find_vars(path, value, last) := _find_assign_vars(path, value) if {
 	value[0].value[0].value == "assign"
 }
 
+_find_vars(_, value, _) := find_ref_vars(value) if {
+	value.type == "ref"
+}
+
 _find_vars(path, value, last) := _find_some_in_decl_vars(path, value) if {
 	last == "symbols"
 	value[0].type == "call"
@@ -141,10 +164,20 @@ _find_vars(_, value, _) := _find_set_or_array_comprehension_vars(value) if {
 	value.type in {"setcomprehension", "arraycomprehension"}
 }
 
-_find_vars(path, value, _) := _find_object_comprehension_vars(value) if {
+_find_vars(_, value, _) := _find_object_comprehension_vars(value) if {
 	value.type == "objectcomprehension"
 }
 
+find_some_decl_vars(rule) := [var |
+	# regal ignore:function-arg-return
+	walk(rule, [path, value])
+
+	regal.last(path) == "symbols"
+	value[0].type != "call"
+
+	some var in _find_some_decl_vars(path, value)
+]
+
 # METADATA
 # description: |
 #   traverses all nodes under provided node (using `walk`), and returns an array with
@@ -163,12 +196,13 @@ _function_arg_names(rule) := {arg.value |
 
 # 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
+#   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 |
 	some var in find_vars(rule)
+	not startswith(var.value, "$")
 	_before_location(var, location)
 ]
 
@@ -183,15 +217,34 @@ _before_location(var, location) if {
 
 # 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)
+#   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 {
 	fn_arg_names := _function_arg_names(rule)
 	var_names := {var.value | some var in find_vars_in_local_scope(rule, location)}
 
+	# parens below added by opa-fmt :)
 	names := (rule_names | fn_arg_names) | var_names
 }
 
+# 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 |
+	some some_var in find_some_decl_vars(rule)
+	_before_location(some_var, location)
+}
+
+# METADATA
+# description: |
+#   determine if var in ref (e.g. `x` in `input[x]`) is used as input or output
+is_output_var(rule, ref, location) if {
+	startswith(ref.value, "$")
+} else if {
+	not ref.value in (find_names_in_scope(rule, location) - find_some_decl_names_in_scope(rule, location))
+}
+
 # METADATA
 # description: |
 #   traverses all nodes under provided node (using `walk`), and returns an array with
@@ -230,7 +283,7 @@ function_ret_in_args(fn_name, terms) if {
 	rest := array.slice(terms, 1, count(terms))
 
 	# for now, bail out of nested calls
-	not "call" in {t | t := rest[_].type}
+	not "call" in {term.type | some term in rest}
 
 	count(rest) > count(all_functions[fn_name].args)
 }

bundle/regal/ast_test.rego

@@ -15,30 +15,30 @@ test_find_vars if {
 
 	allow if {
 		a := global
-		b := [c | c := input[x]] # can't capture x
+		b := [c | c := input[d]]
 
-		every d in input {
-			d == "foo"
+		every e in input {
+			e == "foo"
 		}
 
-		every e, f in input.bar {
-			e == f
+		every f, g in input.bar {
+			f == g
 		}
 
-		some g, h
-		input.bar[g][h]
-		some i in input
-		some j, k in input
+		some h, i
+		input.bar[h][i]
+		some j in input
+		some k, l in input
 
-		[l, m, n] := [1, 2, 3]
+		[m, n, o] := [1, 2, 3]
 
-		[o, [p, _]] := [1, [2, 1]]
+		[p, [q, _]] := [1, [2, 1]]
 
-		some _, [q, r] in [["foo", "bar"], [1, 2]]
+		some _, [r, s] in [["foo", "bar"], [1, 2]]
 
-		{"x": s} := {"x": 1}
+		{"x": t} := {"x": 1}
 
-		some [t] in [[1]]
+		some [u] in [[1]]
 	}
 	`
 
@@ -52,7 +52,7 @@ test_find_vars if {
 		name := var.value
 	}
 
-	names == {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t"}
+	names == {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u"}
 }
 
 test_find_vars_comprehension_lhs if {
@@ -138,12 +138,12 @@ test_find_vars_in_local_scope if {
 
 	allow {
 		a := global
-		b := [c | c := input[x]] # can't capture x
+		b := [c | c := input[d]]
 
-		every d in input {
-			d == "foo"
-			e := "bar"
-			e == "foo"
+		every e in input {
+			f == "foo"
+			g := "bar"
+			h == "foo"
 		}
 	}`
 
@@ -162,11 +162,31 @@ test_find_vars_in_local_scope if {
 	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.a)) == set()
 	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.b)) == {"a"}
 	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.c)) == {"a", "b", "c"}
-	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.d)) == {"a", "b", "c"}
-	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.e)) == {"a", "b", "c", "d"}
+	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.d)) == {"a", "b", "c", "d"}
+	var_names(ast.find_vars_in_local_scope(allow_rule, var_locations.e)) == {"a", "b", "c", "d", "e"}
 }
 
-var_names(vars) := {var.value | some var in vars}
+test_find_vars_in_local_scope_complex_comprehension_term if {
+	policy := `
+	package p
+
+	import future.keywords
+
+	allow {
+		a := [{"b": b} | c := input[b]]
+	}`
+
+	module := regal.parse_module("p.rego", policy)
+
+	allow_rule := module.rules[0]
+
+	ast.find_vars_in_local_scope(allow_rule, {"col": 10, "row": 10}) == [
+		{"location": {"col": 3, "file": "p.rego", "row": 7}, "type": "var", "value": "a"},
+		{"location": {"col": 15, "file": "p.rego", "row": 7}, "type": "var", "value": "b"},
+		{"location": {"col": 20, "file": "p.rego", "row": 7}, "type": "var", "value": "c"},
+		{"location": {"col": 31, "file": "p.rego", "row": 7}, "type": "var", "value": "b"},
+	]
+}
 
 test_find_names_in_scope if {
 	policy := `
@@ -182,7 +202,7 @@ test_find_names_in_scope if {
 
 	allow {
 		a := global
-		b := [c | c := input[x]] # can't capture x
+		b := [c | c := input[_]]
 
 		every d in input {
 			d == "foo"
@@ -199,3 +219,41 @@ test_find_names_in_scope if {
 
 	in_scope == {"bar", "global", "comp", "allow", "a", "b", "c", "d", "e"}
 }
+
+test_find_some_decl_vars if {
+	policy := `
+	package p
+
+	allow {
+		foo := 1
+		some x
+		input[x]
+		some y, z
+		input[y][z] == x
+	}`
+
+	module := regal.parse_module("p.rego", policy)
+
+	some_vars := ast.find_some_decl_vars(module.rules[0])
+
+	var_names(some_vars) == {"x", "y", "z"}
+}
+
+test_find_some_decl_names_in_scope if {
+	policy := `package p
+
+	allow {
+		foo := 1
+		some x
+		input[x]
+		some y, z
+		input[y][z] == x
+	}`
+
+	module := regal.parse_module("p.rego", policy)
+
+	ast.find_some_decl_names_in_scope(module.rules[0], {"col": 1, "row": 6}) == {"x"}
+	ast.find_some_decl_names_in_scope(module.rules[0], {"col": 1, "row": 8}) == {"x", "y", "z"}
+}
+
+var_names(vars) := {var.value | some var in vars}

bundle/regal/config/exclusion.rego

@@ -38,8 +38,7 @@ pattern_compiler(pattern) := ps1 if {
 	p1 := leading_slash(p)
 	ps := leading_doublestar_pattern(p1)
 	ps1 := {pat |
-		some _p
-		ps[_p]
+		some _p, _ in ps
 		nps := trailing_slash(_p)
 		nps[pat]
 	}

bundle/regal/config/provided/data.yaml

@@ -57,6 +57,9 @@ rules:
       level: error
     prefer-snake-case:
       level: error
+    prefer-some-in-iteration:
+      level: error
+      ignore-nesting-level: 2
     todo-comment:
       level: error
     unconditional-assignment:

bundle/regal/rules/bugs/not_equals_in_loop.rego

@@ -19,9 +19,9 @@ report contains violation if {
 	some neq_term in array.slice(expr.terms, 1, count(expr.terms))
 	neq_term.type == "ref"
 
-	some i
-	neq_term.value[i].type == "var"
-	startswith(neq_term.value[i].value, "$")
+	some value in neq_term.value
+	value.type == "var"
+	startswith(value.value, "$")
 
 	violation := result.fail(rego.metadata.chain(), result.location(expr.terms[0]))
 }

bundle/regal/rules/bugs/top_level_iteration.rego

@@ -21,7 +21,7 @@ report contains violation if {
 	violation := result.fail(rego.metadata.chain(), result.location(rule.head))
 }
 
-_rule_names := {name | name := input.rules[_].head.name}
+_rule_names := {rule.head.name | some rule in input.rules}
 
 # regal ignore:external-reference
 illegal_value_ref(value) if not value in _rule_names

bundle/regal/rules/custom/naming_convention.rego

@@ -32,8 +32,6 @@ report contains violation if {
 
 # target: rule
 report contains violation if {
-	cfg := config.for_rule({"custom": {"category": "custom"}, "title": "naming-convention"})
-
 	some convention in cfg.conventions
 	some target in convention.targets
 
@@ -55,8 +53,6 @@ report contains violation if {
 
 # target: function
 report contains violation if {
-	cfg := config.for_rule({"custom": {"category": "custom"}, "title": "naming-convention"})
-
 	some convention in cfg.conventions
 	some target in convention.targets
 
@@ -78,8 +74,6 @@ report contains violation if {
 
 # target: var
 report contains violation if {
-	cfg := config.for_rule({"custom": {"category": "custom"}, "title": "naming-convention"})
-
 	some convention in cfg.conventions
 	some target in convention.targets