Rule: messy-rule (#714)

Help identify incremental rule definitions that aren't grouped together.

Also fix two violations against this rule that we had in our own codebase.

Fixes #672

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

README.md

@@ -246,6 +246,7 @@ The following rules are currently available:
 | style       | [file-length](https://docs.styra.com/regal/rules/style/file-length)                                   | Max file length exceeded                                  |
 | style       | [function-arg-return](https://docs.styra.com/regal/rules/style/function-arg-return)                   | Function argument used for return value                   |
 | style       | [line-length](https://docs.styra.com/regal/rules/style/line-length)                                   | Line too long                                             |
+| style       | [messy-rule](https://docs.styra.com/regal/rules/style/messy-rule)                                     | Messy incremental rule                                    |
 | style       | [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment)               | Comment should start with whitespace                      |
 | style       | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt)                                           | File should be formatted with `opa fmt`                   |
 | style       | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case)                       | Prefer snake_case for names                               |

bundle/regal/config/provided/data.yaml

@@ -116,6 +116,8 @@ rules:
     line-length:
       level: error
       max-line-length: 120
+    messy-rule:
+      level: error
     no-whitespace-comment:
       level: error
     opa-fmt:

bundle/regal/main.rego

@@ -13,6 +13,26 @@ lint_aggregate.violations := aggregate_report
 
 lint.violations := report
 
+rules_to_run[category][title] 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)
+}
+
+notices contains notice if {
+	some category, title
+	some notice in grouped_notices[category][title]
+}
+
+grouped_notices[category][title] contains notice if {
+	some category, title
+	rules_to_run[category][title]
+
+	some notice in data.regal.rules[category][title].notices
+}
+
 # METADATA
 # description: Runs all rules against an input AST and produces a report
 # entrypoint: true
@@ -36,26 +56,6 @@ report contains violation if {
 	}
 }
 
-rules_to_run[category][title] 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)
-}
-
-notices contains notice if {
-	some category, title
-	some notice in grouped_notices[category][title]
-}
-
-grouped_notices[category][title] contains notice if {
-	some category, title
-	rules_to_run[category][title]
-
-	some notice in data.regal.rules[category][title].notices
-}
-
 # Check bundled rules
 report contains violation if {
 	some category, title

bundle/regal/result.rego

@@ -94,6 +94,8 @@ fail(metadata, details) := violation if {
 	violation := _fail_annotated_custom(annotation, details)
 }
 
+fail(metadata, details) := _fail_annotated(metadata, details)
+
 notice(metadata) := result if {
 	is_array(metadata)
 	rule_meta := metadata[0]
@@ -152,8 +154,6 @@ _fail_annotated_custom(metadata, details) := violation if {
 	violation := object.remove(with_category, ["custom", "scope", "schemas"])
 }
 
-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/style/messy_rule.rego

@@ -0,0 +1,26 @@
+# METADATA
+# description: Messy incremental rule
+package regal.rules.style["messy-rule"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	some i, rule1 in input.rules
+
+	cur_name := ast.ref_to_string(rule1.head.ref)
+
+	some j, rule2 in input.rules
+
+	j > i
+
+	nxt_name := ast.ref_to_string(rule2.head.ref)
+	cur_name == nxt_name
+
+	previous_name := ast.ref_to_string(input.rules[j - 1].head.ref)
+	previous_name != nxt_name
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule2))
+}

bundle/regal/rules/style/messy_rule_test.rego

@@ -0,0 +1,99 @@
+package regal.rules.style["messy-rule_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.style["messy-rule"] as rule
+
+test_success_non_messy_definition if {
+	module := ast.with_rego_v1(`
+	foo if true
+
+	foo if 5 == 1
+
+	bar if false
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_fail_messy_definition if {
+	module := ast.with_rego_v1(`
+	foo if true
+
+	bar if false
+
+	foo if 5 == 1
+	`)
+
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\tfoo if 5 == 1"})
+}
+
+test_fail_messy_default_definition if {
+	module := ast.with_rego_v1(`
+	default foo := true
+
+	bar if false
+
+	foo if 5 == 1
+	`)
+
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\tfoo if 5 == 1"})
+}
+
+test_fail_messy_nested_rule_definiton if {
+	module := ast.with_rego_v1(`
+	base.foo if true
+
+	bar if false
+
+	base.foo if 5 == 1
+	`)
+
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\tbase.foo if 5 == 1"})
+}
+
+test_success_non_incremental_nested_rule_definiton if {
+	module := ast.with_rego_v1(`
+	base.foo if true
+
+	bar if false
+
+	base.bar if 5 == 1
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_fail_messy_incremental_nested_variable_rule_definiton if {
+	module := ast.with_rego_v1(`
+	base[x].foo := 5 if { x := 1 }
+
+	bar if false
+
+	base[x].foo := 1 if { x := 1 }
+	`)
+
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\tbase[x].foo := 1 if { x := 1 }"})
+}
+
+expected := {
+	"category": "style",
+	"description": "Messy incremental rule",
+	"level": "error",
+	"related_resources": [{
+		"description": "documentation",
+		"ref": config.docs.resolve_url("$baseUrl/$category/messy-rule", "style"),
+	}],
+	"title": "messy-rule",
+}
+
+expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)

docs/rules/style/messy-rule.md

@@ -0,0 +1,57 @@
+# messy-rule
+
+**Summary**: Messy incremental rule
+
+**Category**: Style
+
+**Avoid**
+
+```rego
+package policy
+
+allow if something
+
+unrelated_rule if {
+    # ...
+}
+
+allow if something_else
+```
+
+**Prefer**
+
+```rego
+package policy
+
+allow if something
+
+allow if something_else
+
+unrelated_rule if {
+    # ...
+}
+```
+
+## Rationale
+
+Rules that are definecd incrementally should have their definitions grouped together, as this makes the code easier to
+follow. While this is mostly a style preference, having incremental rules grouped also allows editors like VS Code to
+"know" that the rules belong together, allowing them to be smarter when displaying the symbols of a workspace.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  style:
+    messy-rule:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Community
+
+If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules,
+or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community
+[Slack](https://communityinviter.com/apps/styracommunity/signup)!

e2e/testdata/violations/most_violations.rego

@@ -220,9 +220,14 @@ print_or_trace_call if {
 	print("forbidden!")
 }
 
+abc if true
+
 # metasyntactic variable
 foo := "bar"
 
+# messy rule
+abc if false
+
 # dubious print sprintf
 y if {
 	print(sprintf("name is: %s domain is: %s", [input.name, input.domain]))