fix: Have use-some-for-output-vars find comprehension body vars (#1069

) Previously reported as a false positive in some cases. Fixes #528 Signed-off-by: Anders Eknert <[email protected]>
StyraInc · Sep 6, 2024 · 8778ec2 · 8778ec2
1 parent 1ef7511
commit 8778ec2
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 6 deletions.
diff --git a/bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars.rego b/bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars.rego
@@ -6,6 +6,7 @@ import rego.v1
 
 import data.regal.ast
 import data.regal.result
+import data.regal.util
 
 report contains violation if {
 	some rule_index, i
@@ -15,7 +16,32 @@ report contains violation if {
 	i != 0
 	elem.type == "var"
 	not startswith(elem.value, "$")
-	not elem.value in ast.find_names_in_scope(input.rules[to_number(rule_index)], elem.location)
 
-	violation := result.fail(rego.metadata.chain(), result.location(elem))
+	rule := input.rules[to_number(rule_index)]
+
+	not elem.value in ast.find_names_in_scope(rule, elem.location)
+
+	path := _location_path(rule, elem.location)
+
+	not var_in_comprehension_body(elem, rule, path)
+
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(elem))
 }
+
+_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 |
+	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]
diff --git a/bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars_test.rego b/bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars_test.rego
@@ -14,7 +14,16 @@ test_fail_output_var_not_declared if {
 		"category": "idiomatic",
 		"description": "Use `some` to declare output variables",
 		"level": "error",
-		"location": {"col": 31, "file": "policy.rego", "row": 4, "text": "\t\t\"admin\" == input.user.roles[i]"},
+		"location": {
+			"col": 31,
+			"file": "policy.rego",
+			"row": 4,
+			"end": {
+				"col": 32,
+				"row": 4,
+			},
+			"text": "\t\t\"admin\" == input.user.roles[i]",
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/use-some-for-output-vars", "idiomatic"),
@@ -23,6 +32,7 @@ test_fail_output_var_not_declared if {
 	}}
 }
 
+# regal ignore:rule-length
 test_fail_multiple_output_vars_not_declared if {
 	r := rule.report with input as ast.policy(`allow {
 		foo := input.foo[i].bar[j]
@@ -32,7 +42,16 @@ test_fail_multiple_output_vars_not_declared if {
 			"category": "idiomatic",
 			"description": "Use `some` to declare output variables",
 			"level": "error",
-			"location": {"col": 20, "file": "policy.rego", "row": 4, "text": "\t\tfoo := input.foo[i].bar[j]"},
+			"location": {
+				"col": 20,
+				"file": "policy.rego",
+				"row": 4,
+				"end": {
+					"col": 21,
+					"row": 4,
+				},
+				"text": "\t\tfoo := input.foo[i].bar[j]",
+			},
 			"related_resources": [{
 				"description": "documentation",
 				"ref": config.docs.resolve_url("$baseUrl/$category/use-some-for-output-vars", "idiomatic"),
@@ -43,7 +62,16 @@ test_fail_multiple_output_vars_not_declared if {
 			"category": "idiomatic",
 			"description": "Use `some` to declare output variables",
 			"level": "error",
-			"location": {"col": 27, "file": "policy.rego", "row": 4, "text": "\t\tfoo := input.foo[i].bar[j]"},
+			"location": {
+				"col": 27,
+				"file": "policy.rego",
+				"row": 4,
+				"end": {
+					"col": 28,
+					"row": 4,
+				},
+				"text": "\t\tfoo := input.foo[i].bar[j]",
+			},
 			"related_resources": [{
 				"description": "documentation",
 				"ref": config.docs.resolve_url("$baseUrl/$category/use-some-for-output-vars", "idiomatic"),
@@ -63,7 +91,16 @@ test_fail_only_one_declared if {
 		"category": "idiomatic",
 		"description": "Use `some` to declare output variables",
 		"level": "error",
-		"location": {"col": 27, "file": "policy.rego", "row": 5, "text": "\t\tfoo := input.foo[i].bar[j]"},
+		"location": {
+			"col": 27,
+			"file": "policy.rego",
+			"row": 5,
+			"end": {
+				"col": 28,
+				"row": 5,
+			},
+			"text": "\t\tfoo := input.foo[i].bar[j]",
+		},
 		"related_resources": [{
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/use-some-for-output-vars", "idiomatic"),
@@ -80,6 +117,14 @@ test_success_uses_some if {
 	r == set()
 }
 
+test_success_var_in_comprehension_body if {
+	r := rule.report with input as ast.with_rego_v1(`build_obj(params) if {
+		paths := {"foo": ["bar"]}
+		param_objects := [f(paths[key], val) | some key, val in paths]
+	}`)
+	r == set()
+}
+
 test_success_some_iteration if {
 	rule.report with input as ast.with_rego_v1(`allow if {
 		some i in input