Refactor rule metadata logic

Some additional refactoring made possible now that each rule
resides in its own file. Extend this further to have each rule
in its own package, and move the metadata from the `report`
rule in each package and to the package declaration. We may
then use `rego.metadata.chain()` instead, which includes the
path of the package containing both the category and the title,
thus allowing us to skip having to redeclare that in annotations.

This has a number of benefits:

- Tests are now proper _unit_ tests, and no longer test the whole
  category.
- Checking if a rule is ignored can now be the responsibility of the
  main rule, and linter rules need no longer be concerned with that
  unless they have unique configuration options.
- Reduce the amount of clutter in each rule related to metadata, and
  with that the possibility of mistakes.

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

.golangci.yaml

@@ -28,6 +28,7 @@ linters:
     - ireturn
     - funlen
     - nolintlint
+    - depguard
 linters-settings:
   tagliatelle:
     case:

README.md

@@ -122,6 +122,7 @@ Documentation:	https://github.com/StyraInc/regal/blob/main/docs/rules/style/use-
 Regal comes with a set of built-in rules, grouped by category.
 
 - **bugs**: Common mistakes, potential bugs and inefficiencies in Rego policies.
+- **idiomatic**: Suggestions for more idiomatic constructs.
 - **imports**: Best practices for imports.
 - **style**: [Rego Style Guide](https://github.com/StyraInc/rego-style-guide) rules.
 - **testing**: Rules for testing and development.

bundle/regal/config/confg_test.rego

@@ -158,3 +158,26 @@ test_enable_single_rule_with_config {
 
 	c == {"level": "error", "important_setting": 42}
 }
+
+test_all_rules_are_in_provided_configuration {
+	missing_config := {title |
+		some category, title
+		data.regal.rules[category][title]
+		not endswith(title, "_test")
+		not data.regal.config.provided.rules[category][title]
+	}
+
+	count(missing_config) == 0
+}
+
+test_all_configured_rules_exist {
+	go_rules := {"opa-fmt"}
+
+	missing_rules := {title |
+		some category, title
+		data.regal.config.provided.rules[category][title]
+		not data.regal.rules[category][title]
+	}
+
+	count(missing_rules - go_rules) == 0
+}

bundle/regal/config/provided/data.yaml

@@ -12,6 +12,11 @@ rules:
       level: error
     unused-return-value:
       level: error
+  idiomatic:
+    custom-has-key-construct:
+      level: error
+    custom-in-construct:
+      level: error
   imports:
     avoid-importing-input:
       level: error

bundle/regal/main.rego

@@ -4,6 +4,8 @@ import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
+import data.regal.config
+
 report contains violation if {
 	not is_object(input)
 
@@ -24,14 +26,30 @@ report contains violation if {
 	}
 }
 
+to_meta(category, title) := {
+	"custom": {"category": category},
+	"title": title,
+}
+
+# Check bundled rules
 report contains violation if {
-	violation := data.regal.rules[_].report[_]
+	some category, title
+	config.merged_config.rules[category][title]
+
+	config.for_rule(to_meta(category, title)).level != "ignore"
+
+	violation := data.regal.rules[category][title].report[_]
 
 	not ignored(violation, ignore_directives)
 }
 
+# Check custom rules
 report contains violation if {
-	violation := data.custom.regal.rules[_].report[_]
+	some category, title
+
+	violation := data.custom.regal.rules[category][title].report[_]
+
+	config.for_rule(to_meta(category, title)).level != "ignore"
 
 	not ignored(violation, ignore_directives)
 }

bundle/regal/main_test.rego

@@ -5,7 +5,7 @@ test_main_basic_input_success {
 	report == set()
 }
 
-test_main_multiple_failues {
+test_main_multiple_failures {
 	policy := `package p
 
 	# both camel case and unification operator
@@ -17,6 +17,28 @@ test_main_multiple_failues {
 	count(report) == 2
 }
 
+test_main_expect_failure {
+	policy := `package p
+
+	camelCase := "yes"
+	`
+	report := data.regal.main.report with input as regal.parse_module("p.rego", policy)
+		with data.regal.config.for_rule as {"level": "error"}
+
+	count(report) == 1
+}
+
+test_main_ignore_rule_config {
+	policy := `package p
+
+	camelCase := "yes"
+	`
+	report := data.regal.main.report with input as regal.parse_module("p.rego", policy)
+		with data.regal.config.for_rule as {"level": "ignore"}
+
+	count(report) == 0
+}
+
 test_main_ignore_directive_failure {
 	policy := `package p
 

bundle/regal/result.rego

@@ -5,7 +5,58 @@ import future.keywords.in
 
 import data.regal.config
 
-fail(metadata, details) := violation if {
+# Provided rules, i.e. regal.rules.category.title
+fail(chain, details) := violation if {
+	is_array(chain) # from rego.metadata.chain()
+
+	some link in chain
+	link.annotations.scope == "package"
+
+	some category, title
+	["regal", "rules", category, title] = link.path
+
+	# NOTE: consider allowing annotation to override any derived values?
+	annotation := object.union(link.annotations, {
+		"custom": {"category": category},
+		"title": title,
+		"related_resources": _related_resources(link.annotations, category, title),
+	})
+
+	violation := _fail_annotated(annotation, details)
+}
+
+# Custom rules, i.e. custom.regal.rules.category.title
+fail(chain, details) := violation if {
+	is_array(chain) # from rego.metadata.chain()
+
+	some link in chain
+	link.annotations.scope == "package"
+
+	some category, title
+	["custom", "regal", "rules", category, title] = link.path
+
+	# NOTE: consider allowing annotation to override any derived values?
+	annotation := object.union(link.annotations, {
+		"custom": {"category": category},
+		"title": title,
+		"related_resources": _related_resources(link.annotations, category, title),
+	})
+
+	violation := _fail_annotated(annotation, details)
+}
+
+_related_resources(annotations, _, _) := annotations.related_resources
+
+_related_resources(annotations, category, title) := rr if {
+	not annotations.related_resources
+	rr := [{
+		"description": "documentation",
+		"ref": sprintf("%s/%s/%s", [config.docs.base_url, category, title]),
+	}]
+}
+
+_fail_annotated(metadata, details) := violation if {
+	is_object(metadata) # from rego.metadata.rule()
 	with_location := object.union(metadata, details)
 	category := with_location.custom.category
 	with_category := object.union(with_location, {
@@ -22,6 +73,8 @@ fail(metadata, details) := violation if {
 	)
 }
 
+fail(metadata, details) := _fail_annotated(metadata, details)
+
 resource_urls(related_resources, category) := [r |
 	some item in related_resources
 	r := object.union(object.remove(item, ["ref"]), {"ref": config.docs.resolve_url(item.ref, category)})

bundle/regal/rules/bugs/constant_condition.rego

@@ -1,10 +1,11 @@
-package regal.rules.bugs
+# METADATA
+# description: Constant condition
+package regal.rules.bugs["constant-condition"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.config
 import data.regal.result
 
 # NOTE: The constant condition checks currently don't do nesting!
@@ -31,36 +32,16 @@ probably_no_body(rule) if {
 	rule.body[0].terms.value == true
 }
 
-# METADATA
-# title: constant-condition
-# description: Constant condition
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/constant-condition
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in _rules_with_bodies
 	some expr in rule.body
 
 	expr.terms.type in _scalars
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }
 
-# METADATA
-# title: constant-condition
-# description: Constant condition
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/constant-condition
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in _rules_with_bodies
 	some expr in rule.body
 
@@ -70,5 +51,5 @@ report contains violation if {
 	expr.terms[1].type in _scalars
 	expr.terms[2].type in _scalars
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }

bundle/regal/rules/bugs/constant_condition_test.rego

@@ -1,12 +1,14 @@
-package regal.rules.bugs_test
+package regal.rules.bugs["constant-condition_test"]
 
 import future.keywords.if
 
+import data.regal.ast
 import data.regal.config
-import data.regal.rules.bugs.common_test.report
+
+import data.regal.rules.bugs["constant-condition"] as rule
 
 test_fail_simple_constant_condition if {
-	r := report(`allow {
+	r := rule.report with input as ast.policy(`allow {
 	1
 	}`)
 	r == {{
@@ -23,11 +25,12 @@ test_fail_simple_constant_condition if {
 }
 
 test_success_static_condition_probably_generated if {
-	report(`allow { true }`) == set()
+	r := rule.report with input as ast.policy(`allow { true }`)
+	r == set()
 }
 
 test_fail_operator_constant_condition if {
-	r := report(`allow {
+	r := rule.report with input as ast.policy(`allow {
 	1 == 1
 	}`)
 	r == {{
@@ -44,5 +47,6 @@ test_fail_operator_constant_condition if {
 }
 
 test_success_non_constant_condition if {
-	report(`allow { 1 == input.one }`) == set()
+	r := rule.report with input as ast.policy(`allow { 1 == input.one }`)
+	r == set()
 }

bundle/regal/rules/bugs/not_equals_in_loop.rego

@@ -1,23 +1,14 @@
-package regal.rules.bugs
+# METADATA
+# description: Use of != in loop
+package regal.rules.bugs["not-equals-in-loop"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.config
 import data.regal.result
 
-# METADATA
-# title: not-equals-in-loop
-# description: Use of != in loop
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/not-equals-in-loop
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in input.rules
 	some expr in rule.body
 
@@ -32,5 +23,5 @@ report contains violation if {
 	neq_term.value[i].type == "var"
 	startswith(neq_term.value[i].value, "$")
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr.terms[0]))
+	violation := result.fail(rego.metadata.chain(), result.location(expr.terms[0]))
 }