Rule: prefer-package-imports (#349)

This builds upon the design laid out in #282,
and on @sesponda's excellent work on that. My
approach for testing this was to implement an
actual "real world" linter rule from the backlog,
and `prefer-package-imports` seemed like a good
choice. The rule aggregates package names (or paths)
from each input file, along with any import paths.

The report rule then checks each import collected
against the list of package paths to determine if
the import matches a package. If not, the rule will
consider that a violation.

As is often the case when real world meets the original
design, "some" modifications proved to be necessary.

The collection of aggregate data is now run alongside
the usual linter rule evaluation, as we conveniently
may use the same input files for aggregating data.
Once that's done, we now send another eval call using
only the data aggregated as input. Each aggregate rule
is now provided only the data aggregated by _that rule_,
so there's no need for policy authors to traverse through
all of the aggregates, but one may simply refer to the
`input.aggregate` array for the same set of entries as
one collected previously.

I've also had to add an alternative schema for the input
when these rules are evaluated (currently only enforced
by `regal test`). Perhaps `anyOf` would be a better
option than two separate schemas, but I'll explore that
at a later point.

Fixes #326
Fixes #343

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

README.md

@@ -158,6 +158,7 @@ The following rules are currently available:
 | imports   | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input)         | Avoid importing input                                     |
 | imports   | [implicit-future-keywords](https://docs.styra.com/regal/rules/imports/implicit-future-keywords)   | Use explicit future keyword imports                       |
 | imports   | [import-shadows-import](https://docs.styra.com/regal/rules/imports/import-shadows-import)         | Import shadows another import                             |
+| imports   | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports)       | Prefer importing packages over rules                      |
 | imports   | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias)                     | Redundant alias                                           |
 | imports   | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import)         | Redundant import of data                                  |
 | 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       |

build/do.rq

@@ -71,6 +71,8 @@ job.tasks {
 #   - https://github.com/golangci/golangci-lint
 #   - https://github.com/open-policy-agent/opa
 job.pr {
+	build(true)
+
 	# format
 	fmt_all
 	write_readme

bundle/regal/ast.rego

@@ -5,9 +5,7 @@ import future.keywords.every
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.opa
-
-builtin_names := object.keys(opa.builtins)
+builtin_names := object.keys(data.regal.opa.builtins)
 
 # METADATA
 # description: |
@@ -350,7 +348,7 @@ implicit_boolean_assignment(rule) if {
 	rule.head.value.location.col == 1
 }
 
-all_functions := object.union(opa.builtins, function_decls(input.rules))
+all_functions := object.union(data.regal.opa.builtins, function_decls(input.rules))
 
 all_function_names := object.keys(all_functions)
 

bundle/regal/config/provided/data.yaml

@@ -38,6 +38,8 @@ rules:
       level: error
     import-shadows-import:
       level: error
+    prefer-package-imports:
+      level: error
     redundant-alias:
       level: error
     redundant-data-import:

bundle/regal/main.rego

@@ -6,6 +6,12 @@ import future.keywords.in
 
 import data.regal.config
 
+lint.violations := report
+
+lint.aggregates := aggregate
+
+lint_aggregate.violations := aggregate_report
+
 report contains violation if {
 	not is_object(input)
 
@@ -51,11 +57,73 @@ report contains violation if {
 	not ignored(violation, ignore_directives)
 }
 
-aggregate contains aggregate if {
+# Collect aggregates in bundled rules
+aggregate[category_title] contains entry if {
+	some category, title
+	config.merged_config.rules[category][title]
+
+	config.for_rule(category, title).level != "ignore"
+	not config.excluded_file(category, title, input.regal.file.name)
+
+	entry := data.regal.rules[category][title].aggregate[_]
+
+	category_title := concat("/", [category, title])
+}
+
+# Collect aggregates in custom rules
+aggregate[category_title] contains entry if {
 	some category, title
+
 	config.for_rule(category, title).level != "ignore"
 	not config.excluded_file(category, title, input.regal.file.name)
-	aggregate := data.custom.regal.rules[category][title].aggregate[_]
+
+	entry := data.custom.regal.rules[category][title].aggregate[_]
+
+	category_title := concat("/", [category, title])
+}
+
+# METADATA
+# description: Check bundled rules using aggregated data
+# schemas:
+#   - input: schema.regal.aggregate
+aggregate_report contains violation if {
+	some category, title
+	config.merged_config.rules[category][title]
+
+	config.for_rule(category, title).level != "ignore"
+	not config.excluded_file(category, title, input.regal.file.name)
+
+	key := concat("/", [category, title])
+	input_for_rule := object.remove(
+		object.union(input, {"aggregate": input.aggregates_internal[key]}),
+		["aggregates_internal"],
+	)
+
+	violation := data.regal.rules[category][title].aggregate_report[_] with input as input_for_rule
+
+	not ignored(violation, ignore_directives)
+}
+
+# METADATA
+# description: Check custom rules using aggregated data
+# schemas:
+#   - input: schema.regal.aggregate
+aggregate_report contains violation if {
+	some key in object.keys(input.aggregates_internal)
+	[category, title] := split(key, "/")
+
+	config.for_rule(category, title).level != "ignore"
+	not config.excluded_file(category, title, input.regal.file.name)
+
+	input_for_rule := object.remove(
+		object.union(input, {"aggregate": input.aggregates_internal[key]}),
+		["aggregates_internal"],
+	)
+
+	# regal ignore:prefer-some-in-iteration
+	violation := data.custom.regal.rules[category][title].aggregate_report[_] with input as input_for_rule
+
+	not ignored(violation, ignore_directives)
 }
 
 ignored(violation, directives) if {

bundle/regal/result.rego

@@ -5,6 +5,56 @@ import future.keywords.in
 
 import data.regal.config
 
+# METADATA
+# description: |
+#  The result.aggregate function works similarly to `result.fail`, but instead of producing
+#  a violation returns an entry to be aggregated for later evaluation. This is useful in
+#  aggregate rules (and only in aggregate rules) as it provides a uniform format for
+#  aggregate data entries. Example return value:
+#
+#  {
+#      "rule": {
+#          "category": "testing",
+#          "title": "aggregation",
+#      },
+#      "aggregate_source": {
+#          "file": "policy.rego",
+#          "package_path": ["a", "b", "c"],
+#      },
+#      "aggregate_data": {
+#          "foo": "bar",
+#          "baz": [1, 2, 3],
+#      },
+#  }
+#
+aggregate(chain, aggregate_data) := entry if {
+	is_array(chain)
+
+	some link in chain
+	link.annotations.scope == "package"
+
+	[category, title] := _category_title_from_path(link.path)
+
+	entry := {
+		"rule": {
+			"category": category,
+			"title": title,
+		},
+		"aggregate_source": {
+			"file": input.regal.file.name,
+			"package_path": [part.value |
+				some i, part in input["package"].path
+				i > 0
+			],
+		},
+		"aggregate_data": aggregate_data,
+	}
+}
+
+_category_title_from_path(path) := [category, title] if ["regal", "rules", category, title] = path
+
+_category_title_from_path(path) := [category, title] if ["custom", "regal", "rules", category, title] = path
+
 # Provided rules, i.e. regal.rules.category.title
 fail(chain, details) := violation if {
 	is_array(chain) # from rego.metadata.chain()

bundle/regal/result_test.rego

@@ -1,9 +1,11 @@
 package regal.result_test
 
+import future.keywords.if
+
 import data.regal.config
 import data.regal.result
 
-test_no_related_resources_in_result_fail_on_custom_rule_unless_provided {
+test_no_related_resources_in_result_fail_on_custom_rule_unless_provided if {
 	chain := [
 		{"path": ["custom", "regal", "rules", "categoy", "name", "report"]},
 		{
@@ -25,7 +27,7 @@ test_no_related_resources_in_result_fail_on_custom_rule_unless_provided {
 	}
 }
 
-test_related_resources_in_result_fail_on_custom_rule_when_provided {
+test_related_resources_in_result_fail_on_custom_rule_when_provided if {
 	chain := [
 		{"path": ["custom", "regal", "rules", "categoy", "name", "report"]},
 		{
@@ -55,7 +57,7 @@ test_related_resources_in_result_fail_on_custom_rule_when_provided {
 	}
 }
 
-test_related_resources_generated_by_result_fail_for_builtin_rule {
+test_related_resources_generated_by_result_fail_for_builtin_rule if {
 	chain := [
 		{"path": ["regal", "rules", "category", "name", "report"]},
 		{
@@ -80,3 +82,69 @@ test_related_resources_generated_by_result_fail_for_builtin_rule {
 		"title": "name",
 	}
 }
+
+test_aggregate_function_builtin_rule if {
+	chain := [
+		{"path": ["regal", "rules", "testing", "aggregation", "report"]},
+		{
+			"annotations": {
+				"scope": "package",
+				"description": "Testing result.aggregate function",
+			},
+			"path": ["regal", "rules", "testing", "aggregation"],
+		},
+	]
+
+	r := result.aggregate(chain, {"foo": "bar", "baz": [1, 2, 3]}) with input as {
+		"regal": {"file": {"name": "policy.rego"}},
+		"package": {"path": [{"value": "data"}, {"value": "a"}, {"value": "b"}, {"value": "c"}]},
+	}
+
+	r == {
+		"rule": {
+			"category": "testing",
+			"title": "aggregation",
+		},
+		"aggregate_source": {
+			"file": "policy.rego",
+			"package_path": ["a", "b", "c"],
+		},
+		"aggregate_data": {
+			"foo": "bar",
+			"baz": [1, 2, 3],
+		},
+	}
+}
+
+test_aggregate_function_custom_rule if {
+	chain := [
+		{"path": ["custom", "regal", "rules", "testing", "aggregation", "report"]},
+		{
+			"annotations": {
+				"scope": "package",
+				"description": "Testing result.aggregate function",
+			},
+			"path": ["custom", "regal", "rules", "testing", "aggregation"],
+		},
+	]
+
+	r := result.aggregate(chain, {"foo": "bar", "baz": [1, 2, 3]}) with input as {
+		"regal": {"file": {"name": "policy.rego"}},
+		"package": {"path": [{"value": "data"}, {"value": "a"}, {"value": "b"}, {"value": "c"}]},
+	}
+
+	r == {
+		"rule": {
+			"category": "testing",
+			"title": "aggregation",
+		},
+		"aggregate_source": {
+			"file": "policy.rego",
+			"package_path": ["a", "b", "c"],
+		},
+		"aggregate_data": {
+			"foo": "bar",
+			"baz": [1, 2, 3],
+		},
+	}
+}

bundle/regal/rules/bugs/unused_return_value.rego

@@ -7,7 +7,6 @@ import future.keywords.if
 import future.keywords.in
 
 import data.regal.ast
-import data.regal.opa
 import data.regal.result
 
 report contains violation if {
@@ -20,7 +19,7 @@ report contains violation if {
 	ref_name := expr.terms[0].value[0].value
 	ref_name in ast.builtin_names
 
-	opa.builtins[ref_name].result.type != "boolean"
+	data.regal.opa.builtins[ref_name].result.type != "boolean"
 
 	# no violation if the return value is declared as the last function argument
 	# see the function-arg-return rule for *that* violation

bundle/regal/rules/imports/prefer_package_imports.rego

@@ -0,0 +1,63 @@
+# METADATA
+# description: Prefer importing packages over rules
+package regal.rules.imports["prefer-package-imports"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.config
+import data.regal.result
+
+cfg := config.for_rule("imports", "prefer-package-imports")
+
+aggregate contains entry if {
+	imports_with_location := [imp |
+		some _import in input.imports
+
+		_import.path.value[0].value == "data"
+		count(_import.path.value) > 1
+		path := [part.value | some part in array.slice(_import.path.value, 1, count(_import.path.value))]
+
+		imp := object.union(result.location(_import), {"path": path})
+	]
+
+	entry := result.aggregate(rego.metadata.chain(), {
+		"imports": imports_with_location,
+		"package_path": [part.value | some i, part in input["package"].path; i > 0],
+	})
+}
+
+# METADATA
+# 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_imports := {imp |
+		some entry in input.aggregate
+		some imp in entry.aggregate_data.imports
+	}
+
+	some imp in all_imports
+	not imp.path in all_package_paths
+	not imp.path in ignored_import_paths
+
+	violation := result.fail(rego.metadata.chain(), {"location": imp.location})
+}
+
+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)
+	]
+}
+
+normalize_part(0, part) := part if part != "data"
+
+normalize_part(i, part) := part if i > 0