From c3c590089d4be3883c677a7187f4cbd05485c494 Mon Sep 17 00:00:00 2001
From: Anders Eknert <anders@styra.com>
Date: Wed, 4 Sep 2024 10:49:48 +0200
Subject: [PATCH] Increase test coverage

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 <anders@styra.com>
---
 bundle/regal/ast/ast.rego                     | 50 +++++++++----------
 bundle/regal/ast/ast_test.rego                | 18 +++++++
 bundle/regal/main/main.rego                   |  5 +-
 .../inconsistent-args/inconsistent_args.rego  |  2 +-
 .../unused_output_variable.rego               | 34 +++++++------
 .../unused_output_variable_test.rego          | 35 +++++++++++++
 .../use-strings-count/use_strings_count.rego  |  2 +-
 .../prefer_package_imports.rego               | 19 +++----
 .../prefer_package_imports_test.rego          | 19 +++++--
 .../default-over-not/default_over_not.rego    |  8 ++-
 .../prefer_some_in_iteration.rego             |  2 +-
 .../style/rule-length/rule_length_test.rego   | 19 +++++++
 .../unnecessary-some/unnecessary_some.rego    |  3 +-
 .../use_assignment_operator.rego              | 13 ++---
 .../metasyntactic_variable.rego               |  2 +-
 .../test_outside_test_package_test.rego       | 12 +++++
 bundle/regal/util/util_test.rego              | 20 ++++++++
 17 files changed, 185 insertions(+), 78 deletions(-)
 create mode 100644 bundle/regal/util/util_test.rego

diff --git a/bundle/regal/ast/ast.rego b/bundle/regal/ast/ast.rego
index b0bcebc0..ce7613dc 100644
--- a/bundle/regal/ast/ast.rego
+++ b/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
diff --git a/bundle/regal/ast/ast_test.rego b/bundle/regal/ast/ast_test.rego
index 59d7e523..9709cc27 100644
--- a/bundle/regal/ast/ast_test.rego
+++ b/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])
+}
diff --git a/bundle/regal/main/main.rego b/bundle/regal/main/main.rego
index 0b7c8b4f..9d601f97 100644
--- a/bundle/regal/main/main.rego
+++ b/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
diff --git a/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego b/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego
index 13b355c7..9aa5b81f 100644
--- a/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego
+++ b/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
diff --git a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego
index 1c21cc3d..90f41390 100644
--- a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego
+++ b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego
@@ -22,9 +22,8 @@ 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
 
@@ -32,10 +31,11 @@ report contains violation if {
 
 	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)
 
diff --git a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego
index 01b44678..2833f988 100644
--- a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego
+++ b/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 {
diff --git a/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego b/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego
index 496c5e2e..a63a8d58 100644
--- a/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego
+++ b/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`
diff --git a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego
index 5edbfa9c..61125a03 100644
--- a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego
+++ b/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
diff --git a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego
index 867fc913..8c596c18 100644
--- a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego
+++ b/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()
diff --git a/bundle/regal/rules/style/default-over-not/default_over_not.rego b/bundle/regal/rules/style/default-over-not/default_over_not.rego
index 1995290d..fd6e2dcf 100644
--- a/bundle/regal/rules/style/default-over-not/default_over_not.rego
+++ b/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"
+	}
 }
diff --git a/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego b/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego
index 5d286018..5a1ab019 100644
--- a/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego
+++ b/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
diff --git a/bundle/regal/rules/style/rule-length/rule_length_test.rego b/bundle/regal/rules/style/rule-length/rule_length_test.rego
index bbd5dc3f..28e5fe10 100644
--- a/bundle/regal/rules/style/rule-length/rule_length_test.rego
+++ b/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
 
diff --git a/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego b/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego
index 55bf5c15..fbe8a40e 100644
--- a/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego
+++ b/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"
diff --git a/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego b/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego
index af46ff4a..0af171d3 100644
--- a/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego
+++ b/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego
@@ -20,7 +20,7 @@ report contains violation if {
 
 	loc := result.location(rule)
 
-	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}}))
+	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}}))
 }
 
 report contains violation if {
@@ -32,7 +32,7 @@ report contains violation if {
 
 	loc := result.location(result.location(rule.head.ref[0]))
 
-	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}}))
+	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}}))
 }
 
 report contains violation if {
@@ -54,12 +54,7 @@ report contains violation if {
 	# assignment
 	regex.match(`else\s*=`, loc.location.text)
 
-	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}}))
+	violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}}))
 }
 
-default eq_col(_) := 1
-
-eq_col(loc) := pos + 1 if {
-	pos := indexof(loc.location.text, "=")
-	pos != -1
-}
+_eq_col(loc) := max([0, indexof(loc.location.text, "=")]) + 1
diff --git a/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego b/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego
index 4e504ab5..c821e596 100644
--- a/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego
+++ b/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego
@@ -47,7 +47,7 @@ report contains violation if {
 
 	lower(var.value) in metasyntactic
 
-	ast.is_output_var(input.rules[to_number(i)], var, var.location)
+	ast.is_output_var(input.rules[to_number(i)], var)
 
 	violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(var))
 }
diff --git a/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego b/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego
index 825e18a2..c5e6e7e4 100644
--- a/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego
+++ b/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego
@@ -41,6 +41,18 @@ test_success_test_inside_test_package if {
 	result == set()
 }
 
+test_success_test_inside_test_package_named_just_test if {
+	ast := regal.parse_module("test.rego", `
+	package test
+
+	import rego.v1
+
+	test_foo if { false }
+	`)
+	result := rule.report with input as ast
+	result == set()
+}
+
 # https://github.com/StyraInc/regal/issues/176
 test_success_test_prefixed_function if {
 	ast := regal.parse_module("foo_test.rego", `
diff --git a/bundle/regal/util/util_test.rego b/bundle/regal/util/util_test.rego
new file mode 100644
index 00000000..ae38f849
--- /dev/null
+++ b/bundle/regal/util/util_test.rego
@@ -0,0 +1,20 @@
+package regal.util_test
+
+import rego.v1
+
+import data.regal.util
+
+test_find_duplicates if {
+	util.find_duplicates([1, 1, 2, 3, 3, 3]) == {{0, 1}, {3, 4, 5}}
+}
+
+test_json_pretty if {
+	# oh, the things you do for test coverage
+	util.json_pretty({"x": [1, 2, 3]}) == `{
+  "x": [
+    1,
+    2,
+    3
+  ]
+}`
+}