Rule: comprehension-term-assignment (#1098)

Flag redundant assignment of simple values that could be used
directly as the comprehension term (or key/value).

Fixes #1073

Signed-off-by: Anders Eknert <[email protected]>

bundle/regal/ast/ast.rego

@@ -296,3 +296,9 @@ is_chained_rule_body(rule, lines) if {
 
 	startswith(col_text, "{")
 }
+
+assignment_terms(expr) := [expr.terms[1], expr.terms[2]] if {
+	expr.terms[0].type == "ref"
+	expr.terms[0].value[0].type == "var"
+	expr.terms[0].value[0].value == "assign"
+}

bundle/regal/ast/ast_test.rego

@@ -47,10 +47,9 @@ test_find_vars if {
 
 	vars := ast.find_vars(module.rules)
 
-	names := {name |
+	names := {var.value |
 		some var in vars
 		var.type == "var"
-		name := var.value
 	}
 
 	names == {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u"}
@@ -73,10 +72,9 @@ test_find_vars_comprehension_lhs if {
 
 	vars := ast.find_vars(module.rules)
 
-	names := {name |
+	names := {var.value |
 		some var in vars
 		var.type == "var"
-		name := var.value
 	}
 
 	names == {"a", "b", "c", "d", "e", "f", "g"}

bundle/regal/ast/search.rego

@@ -22,9 +22,9 @@ _find_assign_vars(value) := var if {
 # [a, b, c] := [1, 2, 3]
 # or
 # {a: b} := {"foo": "bar"}
-_find_assign_vars(value) := var if {
+_find_assign_vars(value) := vars if {
 	value[1].type in {"array", "object"}
-	var := _find_nested_vars(value[1])
+	vars := _find_nested_vars(value[1])
 }
 
 # var declared via `some`, i.e. `some x` or `some x, y`
@@ -34,19 +34,19 @@ _find_some_decl_vars(value) := [v |
 ]
 
 # single var declared via `some in`, i.e. `some x in y`
-_find_some_in_decl_vars(value) := var if {
+_find_some_in_decl_vars(value) := vars if {
 	arr := value[0].value
 	count(arr) == 3
 
-	var := _find_nested_vars(arr[1])
+	vars := _find_nested_vars(arr[1])
 }
 
 # two vars declared via `some in`, i.e. `some x, y in z`
-_find_some_in_decl_vars(value) := var if {
+_find_some_in_decl_vars(value) := vars if {
 	arr := value[0].value
 	count(arr) == 4
 
-	var := [v |
+	vars := [v |
 		some i in [1, 2]
 		some v in _find_nested_vars(arr[i])
 	]
@@ -64,11 +64,17 @@ find_ref_vars(value) := [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 {
-	key_var := [v | v := value.key; v.type == "var"; indexof(v.value, "$") == -1]
-	val_var := [v | v := value.value; v.type == "var"; indexof(v.value, "$") == -1]
+_find_every_vars(value) := vars if {
+	key_var := [value.key |
+		value.key.type == "var"
+		indexof(value.key.value, "$") == -1
+	]
+	val_var := [value.value |
+		value.value.type == "var"
+		indexof(value.value.value, "$") == -1
+	]
 
-	var := array.concat(key_var, val_var)
+	vars := array.concat(key_var, val_var)
 }
 
 # METADATA
@@ -215,6 +221,17 @@ found.symbols[rule_index] contains value.symbols if {
 	walk(rule, [_, value])
 }
 
+found.comprehensions[rule_index] contains value if {
+	some i, rule in _rules
+
+	# converting to string until https://github.com/open-policy-agent/opa/issues/6736 is fixed
+	rule_index := sprintf("%d", [i])
+
+	walk(rule, [_, value])
+
+	value.type in {"arraycomprehension", "objectcomprehension", "setcomprehension"}
+}
+
 # METADATA
 # description: |
 #   finds all vars declared in `rule` *before* the `location` provided

bundle/regal/config/provided/data.yaml

@@ -6,7 +6,7 @@ rules:
     annotation-without-metadata:
       level: error
     argument-always-wildcard:
-      except-function-name-pattern: "^mock_"
+      except-function-name-pattern: ^mock_
       level: error
     constant-condition:
       level: error
@@ -123,6 +123,8 @@ rules:
       level: error
     chained-rule-body:
       level: error
+    comprehension-term-assignment:
+      level: error
     default-over-else:
       level: error
       prefer-default-functions: false

bundle/regal/rules/bugs/impossible-not/impossible_not.rego

@@ -108,10 +108,7 @@ var_to_ref(terms) := [terms] if terms.type == "var"
 
 var_to_ref(terms) := terms.value if terms.type == "ref"
 
-to_string(ref) := concat(".", [path |
-	some part in ref
-	path := part.value
-])
+to_string(ref) := concat(".", [part.value | some part in ref])
 
 resolve(ref, _, _) := to_string(ref) if ref[0].value == "data"
 

bundle/regal/rules/bugs/invalid-metadata-attribute/invalid_metadata_attribute.rego

@@ -24,10 +24,9 @@ report contains violation if {
 	)
 }
 
-_block_to_string(block) := concat("\n", [line |
+_block_to_string(block) := concat("\n", [entry.text |
 	some i, entry in block
 	i > 0
-	line := entry.text
 ])
 
 _find_line(block, attribute) := [line |

bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego

@@ -56,19 +56,17 @@ is_array_conversion(value) if {
 
 	count(body) == 1
 
-	body[0].terms[0].type == "ref"
-	body[0].terms[0].value[0].type == "var"
-	body[0].terms[0].value[0].value == "assign"
+	[lhs, rhs] := ast.assignment_terms(body[0])
 
 	# Assignment to comprehension variable
-	body[0].terms[1].type == "var"
-	body[0].terms[1].value == var
+	lhs.type == "var"
+	lhs.value == var
 
 	# On the right hand side a ref with at least one wildcard
-	body[0].terms[2].type == "ref"
-	body[0].terms[2].value[0].type == "var"
+	rhs.type == "ref"
+	rhs.value[0].type == "var"
 
-	some ref_val in body[0].terms[2].value
+	some ref_val in rhs.value
 
 	ref_val.type == "var"
 	startswith(ref_val.value, "$")

bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego

@@ -42,10 +42,7 @@ custom_regal_package_and_import(pkg_path, path) if {
 # schemas:
 #   - input: schema.regal.aggregate
 aggregate_report contains violation if {
-	all_package_paths := {pkg |
-		some entry in input.aggregate
-		pkg := entry.aggregate_data.package_path
-	}
+	all_package_paths := {entry.aggregate_data.package_path | some entry in input.aggregate}
 
 	all_imports := {imp |
 		some entry in input.aggregate

bundle/regal/rules/style/comprehension-term-assignment/comprehension_term_assignment.rego

@@ -0,0 +1,86 @@
+# METADATA
+# description: Assignment can be moved to comprehension term
+package regal.rules.style["comprehension-term-assignment"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+import data.regal.util
+
+# METADATA
+# description: |
+#   find comprehensions where assignment in the body may be replaced by
+#   using the value in the comprehension term (or key/value) directly
+# scope: document
+
+# METADATA
+# description: find in set and array comprehensions
+report contains violation if {
+	comp := ast.found.comprehensions[_][_].value
+
+	# a single expression can't be moved to term position
+	count(comp.body) > 1
+
+	# limit to simple vars, not term vars in nested
+	# composite structures, function calls or whatever
+	comp.term.type == "var"
+
+	some expr in comp.body
+
+	[lhs, rhs] := ast.assignment_terms(expr)
+
+	lhs.type == comp.term.type
+	lhs.value == comp.term.value
+
+	# using any of these at the term position may be OK when the value
+	# is simple, like [{"first_name": name.first} | some name in names]
+	# but almost certainly hard to understand when more complex composite values
+	# or call chains are involved..
+	# trying to determine "complexity" is... hard / undesirable
+	# so let's just allow these assignnments and focus on what we do know
+	rhs.type in {"var", "ref"}
+	not _dynamic_ref(rhs)
+
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr))
+}
+
+# METADATA
+# description: find in object comprehensions (both keys and values)
+report contains violation if {
+	comp := ast.found.comprehensions[_][_].value
+
+	# a single expression can't be moved to term position
+	count(comp.body) > 1
+
+	# only true for object comprehension
+	comp.key
+
+	some expr in comp.body
+
+	[lhs, rhs] := ast.assignment_terms(expr)
+
+	some kind in ["key", "value"]
+
+	lhs.type == comp[kind].type
+	lhs.value == comp[kind].value
+
+	rhs.type in {"var", "ref"}
+	not _dynamic_ref(rhs)
+
+	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(expr))
+}
+
+_dynamic_ref(value) if {
+	value.type == "ref"
+
+	_call_or_non_static(value)
+}
+
+_call_or_non_static(ref) if ref.value[0].type == "call"
+
+_call_or_non_static(ref) if not _static_ref(ref)
+
+_static_ref(ref) if every part in util.rest(ref.value) {
+	part.type == "string"
+}