Increase test coverage (#1058)

I started looking into increasing the test coverage the other day,
and wow, there really turned out to be some bugs under those stones!

While there are a few silly additions here just for coverage, many of
the changes made here uncovered issues that when fixed had Regal report
many issues in our own code! So this was definitely a worthwhile endevour.

Will continue this work for sure, but to avoid it becoming a monster, and
to leave it in a good state, I'll follow up with more PRs later.

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

bundle/regal/ast/ast.rego

@@ -104,11 +104,30 @@ rule_names contains ref_to_string(rule.head.ref) if some rule in rules
 
 # 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, "$")
+#   determine if var in var (e.g. `x` in `input[x]`) is used as input or output
+# scope: document
+is_output_var(rule, var) if {
+	# test the cheap and common case first, and 'else' only when it's not
+	startswith(var.value, "$")
 } else if {
-	not ref.value in (find_names_in_scope(rule, location) - find_some_decl_names_in_scope(rule, location))
+	not var.value in (rule_names | imported_identifiers) # regal ignore:external-reference
+
+	num_above := sum([1 |
+		some above in find_vars_in_local_scope(rule, var.location)
+		above.value == var.value
+	])
+	num_some := sum([1 |
+		some name in find_some_decl_names_in_scope(rule, var.location)
+		name == var.value
+	])
+
+	# only the first ref variable in scope can be an output! meaning that:
+	# allow if {
+	#     some x
+	#     input[x]    # <--- output
+	#     data.bar[x] # <--- input
+	# }
+	num_above - num_some == 0
 }
 
 # METADATA
@@ -195,10 +214,6 @@ static_ref(ref) if every t in array.slice(ref.value, 1, count(ref.value)) {
 	t.type != "var"
 }
 
-static_rule_ref(ref) if every t in array.slice(ref, 1, count(ref)) {
-	t.type != "var"
-}
-
 # METADATA
 # description: provides a set of names of all built-in functions called in the input policy
 builtin_functions_called contains name if {
@@ -257,25 +272,6 @@ implicit_boolean_assignment(rule) if {
 # or sometimes, like this...
 implicit_boolean_assignment(rule) if rule.head.value.location == rule.head.location
 
-# or like this...
-implicit_boolean_assignment(rule) if {
-	# This handles the *quite* special case of
-	# `a.b if true`, which is "rewritten" to `a.b = true` *and*  where a location is still added to the value
-	# see https://github.com/open-policy-agent/opa/issues/6184 for details
-	#
-	# Do note that technically, it is possible to write a rule where the `true` value actually is on column 1, i.e.
-	#
-	# a.b =
-	# true
-	# if true
-	#
-	# If you write Rego like that — you're not going to use Regal anyway, are you? ¯\_(ツ)_/¯
-	rule.head.value.type == "boolean"
-	rule.head.value.value == true
-
-	rule.head.value.location.col == 1
-}
-
 # METADATA
 # description: |
 #   object containing all available built-in and custom functions in the

bundle/regal/ast/ast_test.rego

@@ -283,3 +283,21 @@ test_all_refs if {
 
 	text_refs == {":=", "data.foo.bar", "data.foo.bax", "data.foo.baz"}
 }
+
+test_provided_capabilities_never_undefined if {
+	capabilities.provided == {} with data.internal as {}
+}
+
+test_function_calls if {
+	calls := ast.function_calls["0"] with input as ast.with_rego_v1(`
+	rule if {
+		x := 1
+		f(2)
+	}`)
+
+	{"assign", "f"} == {call.name | some call in calls}
+}
+
+test_implicit_boolean_assignment if {
+	ast.implicit_boolean_assignment(ast.with_rego_v1(`a.b if true`).rules[0])
+}

bundle/regal/main/main.rego

@@ -24,10 +24,7 @@ rules_to_run[category][title] if {
 	not config.excluded_file(category, title, input.regal.file.name)
 }
 
-notices contains notice if {
-	some category, title
-	some notice in grouped_notices[category][title]
-}
+notices contains grouped_notices[_][_][_]
 
 grouped_notices[category][title] contains notice if {
 	some category, title

bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego

@@ -29,7 +29,7 @@ report contains violation if {
 	# "Partition" the args by their position
 	by_position := [s |
 		some i, _ in args_list[0]
-		s := [x | x := args_list[_][i]]
+		s := [item[i] | some item in args_list]
 	]
 
 	some position in by_position

bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego

@@ -22,20 +22,20 @@ import data.regal.result
 #   some x in data.foo.bar
 #   ```
 report contains violation if {
-	some rule_index, name
-
-	var_refs := _ref_vars[rule_index][name]
+	some rule_index
+	var_refs := _ref_vars[rule_index][_]
 
 	count(var_refs) == 1
 
 	some var in var_refs
 
 	not _var_in_head(input.rules[to_number(rule_index)].head, var.value)
 	not _var_in_call(ast.function_calls, rule_index, var.value)
+	not _ref_base_vars[rule_index][var.value]
 
 	# this is by far the most expensive condition to check, so only do
 	# so when all other conditions apply
-	ast.is_output_var(input.rules[to_number(rule_index)], var, var.location)
+	ast.is_output_var(input.rules[to_number(rule_index)], var)
 
 	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(var))
 }
@@ -47,6 +47,15 @@ _ref_vars[rule_index][var.value] contains var if {
 	not startswith(var.value, "$")
 }
 
+# "a" in "a[foo]", and not foo
+_ref_base_vars[rule_index][term.value] contains term if {
+	some rule_index
+	term := ast.found.refs[rule_index][_].value[0]
+
+	term.type == "var"
+	not startswith(term.value, "$")
+}
+
 _var_in_head(head, name) if head.value.value == name
 
 _var_in_head(head, name) if {
@@ -63,6 +72,12 @@ _var_in_head(head, name) if {
 	var.value == name
 }
 
+_var_in_head(head, name) if {
+	some i, var in head.ref
+	i > 0
+	var.value == name
+}
+
 _var_in_call(calls, rule_index, name) if _var_in_arg(calls[rule_index][_].args[_], name)
 
 _var_in_arg(arg, name) if {
@@ -71,16 +86,7 @@ _var_in_arg(arg, name) if {
 }
 
 _var_in_arg(arg, name) if {
-	arg.type == "ref"
-
-	some val in arg.value
-
-	val.type == "var"
-	val.value == name
-}
-
-_var_in_arg(arg, name) if {
-	arg.type in {"array", "object"}
+	arg.type in {"array", "object", "set"}
 
 	some var in ast.find_term_vars(arg)
 

bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego

@@ -72,6 +72,28 @@ test_success_not_unused_variable_in_head_value if {
 	r == set()
 }
 
+test_success_not_unused_variable_in_head_term_value if {
+	module := ast.with_rego_v1(`
+	success := {x} if {
+		input[x]
+	}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_not_unused_variable_in_head_term_key if {
+	module := ast.with_rego_v1(`
+	success contains {x} if {
+		input[x]
+	}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
 test_success_not_unused_variable_in_head_key if {
 	module := ast.with_rego_v1(`
 	success contains x if {
@@ -96,6 +118,19 @@ test_success_not_unused_output_variable_function_call if {
 	r == set()
 }
 
+test_success_not_unused_output_variable_function_call_arg_term if {
+	module := ast.with_rego_v1(`
+	success if {
+		some x
+		input[x]
+		f({x})
+	}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
 test_success_not_unused_output_variable_other_ref if {
 	module := ast.with_rego_v1(`
 	success if {

bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego

@@ -12,7 +12,7 @@ import data.regal.result
 # description: Missing capability for built-in function `strings.count`
 # custom:
 #   severity: warning
-notices contains result.notice(rego.metadata.chain()) if not capabilities.has_object_keys
+notices contains result.notice(rego.metadata.chain()) if not capabilities.has_strings_count
 
 # METADATA
 # description: flag calls to `count` where the first argument is a call to `indexof_n`

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

@@ -54,30 +54,27 @@ aggregate_report contains violation if {
 	}
 
 	some imp in all_imports
-	resolves(imp.path, all_package_paths)
+	_resolves(imp.path, all_package_paths)
 	not imp.path in all_package_paths
-	not imp.path in ignored_import_paths
+	not imp.path in _ignored_import_paths
 
 	violation := result.fail(rego.metadata.chain(), {"location": imp.location})
 }
 
-# METADATA
-# description: |
-#   returns true if the path "resolves" to *any*
-#   package part of the same length as the path
-resolves(path, pkg_paths) if count([path |
+# returns true if the path "resolves" to *any* package part of the same length as the path
+_resolves(path, pkg_paths) if count([path |
 	some pkg_path in pkg_paths
 	pkg_path == array.slice(path, 0, count(pkg_path))
 ]) > 0
 
-ignored_import_paths contains path if {
+_ignored_import_paths contains path if {
 	some item in cfg["ignore-import-paths"]
 	path := [part |
 		some i, p in split(item, ".")
-		part := normalize_part(i, p)
+		part := _normalize_part(i, p)
 	]
 }
 
-normalize_part(0, part) := part if part != "data"
+_normalize_part(0, part) := part if part != "data"
 
-normalize_part(i, part) := part if i > 0
+_normalize_part(i, part) := part if i > 0

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

@@ -98,14 +98,23 @@ test_success_aggregate_report_on_import_with_unresolved_path if {
 }
 
 test_success_aggregate_report_ignored_import_path if {
-	aggregate := {{"aggregate_data": {
-		"package_path": ["a"],
-		"imports": [{"path": ["b"], "location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b"}}],
-	}}}
+	aggregate := {
+		{"aggregate_data": {
+			"package_path": ["a"],
+			"imports": [{
+				"path": ["b", "c"],
+				"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b.c"},
+			}],
+		}},
+		{"aggregate_data": {
+			"package_path": ["b"],
+			"imports": [],
+		}},
+	}
 
 	r := rule.aggregate_report with input.aggregate as aggregate with config.for_rule as {
 		"level": "error",
-		"ignore-import-paths": ["data.b"],
+		"ignore-import-paths": ["data.b.c"],
 	}
 
 	r == set()

bundle/regal/rules/style/default-over-not/default_over_not.rego

@@ -46,6 +46,10 @@ _static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [r
 	some i, ref in rule.head.ref
 	i > 0
 ])) if {
-	count(rule.head.ref) > 1
-	ast.static_rule_ref(rule.head.ref)
+	c := count(rule.head.ref)
+	c > 1
+
+	every t in array.slice(rule.head.ref, 1, c) {
+		t.type != "var"
+	}
 }

bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego

@@ -29,7 +29,7 @@ report contains violation if {
 
 		# we don't need the location of each var, but using the first
 		# ref will do, and will hopefully help with caching the result
-		ast.is_output_var(rule, var, value.location)
+		ast.is_output_var(rule, var)
 	])
 
 	num_output_vars != 0

bundle/regal/rules/style/rule-length/rule_length_test.rego

@@ -73,6 +73,25 @@ test_success_rule_longer_than_configured_max_length_but_comments if {
 	r == set()
 }
 
+test_success_rule_longer_than_configured_max_length_but_no_body_and_exception_configured if {
+	module := regal.parse_module("policy.rego", `package p
+
+	my_short_rule := {
+		"a": input.a,
+		"b": input.b,
+		"c": input.c,
+		"d": input.d,
+	}
+	`)
+
+	r := rule.report with input as module with config.for_rule as {
+		"level": "error",
+		"max-rule-length": 2,
+		"except-empty-body": true,
+	}
+	r == set()
+}
+
 test_success_rule_length_equals_max_length if {
 	module := regal.parse_module("policy.rego", `package p
 

bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego

@@ -11,8 +11,7 @@ report contains violation if {
 	# No need to traverse rules here if we're not importing `in`
 	ast.imports_keyword(input.imports, "in")
 
-	some rule_index
-	symbols := ast.found.symbols[rule_index][_]
+	symbols := ast.found.symbols[_][_]
 
 	symbols[0].type == "call"
 	symbols[0].value[0].type == "ref"