Rule: duplicate-rule (#530)

As the name implies, flag duplicate rules when found. See the docs included
in the PR for more details, including current limitations.

This rule should be an aggregate rule, but it was hairy enough to pull off
with a single file scope, so I decided to hold off on that for now. This
provides value already, so let's ship it and consider looking into making
this aggregate later.

Fixes #427

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

README.md

@@ -190,6 +190,7 @@ The following rules are currently available:
 |-----------|-----------------------------------------------------------------------------------------------------|-----------------------------------------------------------|
 | bugs      | [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition)                    | Constant condition                                        |
 | bugs      | [deprecated-builtin](https://docs.styra.com/regal/rules/bugs/deprecated-builtin)                    | Avoid using deprecated built-in functions                 |
+| bugs      | [duplicate-rule](https://docs.styra.com/regal/rules/bugs/duplicate-rule)                            | Duplicate rule                                            |
 | bugs      | [if-empty-object](https://docs.styra.com/regal/rules/bugs/if-empty-object)                          | Empty object following `if`                               |
 | bugs      | [inconsistent-args](https://docs.styra.com/regal/rules/bugs/inconsistent-args)                      | Inconsistently named function arguments                   |
 | bugs      | [invalid-metadata-attribute](https://docs.styra.com/regal/rules/bugs/invalid-metadata-attribute)    | Invalid attribute in metadata annotation                  |

bundle/regal/config/provided/data.yaml

@@ -4,6 +4,8 @@ rules:
       level: error
     deprecated-builtin:
       level: error
+    duplicate-rule:
+      level: error
     if-empty-object:
       level: error
     inconsistent-args:

bundle/regal/rules/bugs/duplicate_rule.rego

@@ -0,0 +1,69 @@
+# METADATA
+# description: Duplicate rule
+package regal.rules.bugs["duplicate-rule"]
+
+import rego.v1
+
+import data.regal.result
+import data.regal.util
+
+report contains violation if {
+	some indices in duplicates
+
+	first := indices[0]
+	rest := array.slice(indices, 1, count(indices))
+
+	dup_locations := [location |
+		some index in rest
+		location := input.rules[index].location
+	]
+
+	violation := result.fail(rego.metadata.chain(), object.union(
+		result.location(input.rules[first]),
+		{"description": message(dup_locations)},
+	))
+}
+
+message(locations) := sprintf("Duplicate rule found at line %d", [locations[0].row]) if count(locations) == 1
+
+message(locations) := sprintf(
+	"Duplicate rules found at lines %s",
+	[concat(", ", [line |
+		some location in locations
+		line := sprintf("%d", [location.row])
+	])],
+) if {
+	count(locations) > 1
+}
+
+rules_as_text := [base64.decode(rule.location.text) | some rule in input.rules]
+
+duplicates contains indices if {
+	# Remove whitespace from textual representation of rule and create a hash from the result.
+	# This provides a decent, and importantly *cheap*, approximation of duplicates. We can then
+	# parse the text of these suspected duplicate rules to get a more exact result.
+	rules_hashed := [crypto.md5(regex.replace(text, `\s+`, "")) | some text in rules_as_text]
+
+	some possible_duplicates in util.find_duplicates(rules_hashed)
+
+	# need to include the original index here to be able to backtrack that to the rule
+	asts := {index: ast |
+		some index in possible_duplicates
+
+		module := sprintf("package p\n\nimport rego.v1\n\n%s", [rules_as_text[index]])
+
+		# note that we _don't_ use regal.parse_module here, as we do not want location
+		# information — only the structure of the AST must match
+		ast := rego.parse_module("", module)
+	}
+
+	keys := [key | some key, _ in asts]
+	vals := [val | some val in asts]
+
+	indices := [keys[index] |
+		some dups in util.find_duplicates(vals)
+		some index in dups
+	]
+
+	count(indices) > 0
+}

bundle/regal/rules/bugs/duplicate_rule_test.rego

@@ -0,0 +1,78 @@
+package regal.rules.bugs["duplicate-rule_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.bugs["duplicate-rule"] as rule
+
+test_fail_simple_duplicate_rule if {
+	module := ast.with_rego_v1(`
+	allow if {
+		input.foo
+	}
+
+	allow if {
+		input.foo
+	}
+	`)
+
+	r := rule.report with input as module
+
+	r == {{
+		"category": "bugs",
+		"description": "Duplicate rule found at line 10",
+		"level": "error",
+		"location": {"col": 2, "file": "policy.rego", "row": 6, "text": "\tallow if {"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/duplicate-rule", "bugs"),
+		}],
+		"title": "duplicate-rule",
+	}}
+}
+
+test_success_similar_but_not_duplicate_rule if {
+	module := ast.with_rego_v1(`
+	allow if input.foo == "bar"
+
+	allow if input.foo == "bar "
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_fail_multiple_duplicate_rules if {
+	module := ast.with_rego_v1(`
+
+	# varying whitespace in each just for good measure
+	# these should still count as duplicates
+
+	allow if {
+      input.foo
+	}
+
+	allow if {
+		input.foo
+	}
+
+	allow if {
+		  input.foo
+	}
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Duplicate rules found at lines 14, 18",
+		"level": "error",
+		"location": {"col": 2, "file": "policy.rego", "row": 10, "text": "\tallow if {"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/duplicate-rule", "bugs"),
+		}],
+		"title": "duplicate-rule",
+	}}
+}

bundle/regal/util/util.rego

@@ -3,3 +3,18 @@ package regal.util
 import rego.v1
 
 is_snake_case(str) if str == lower(str)
+
+# METADATA
+# description: |
+#   returns a set of sets containing all indices of duplicates in the array,
+#   so e.g. [1, 1, 2, 3, 3, 3] would return {{0, 1}, {3, 4, 5}} and so on
+find_duplicates(arr) := {indices |
+	some i, x in arr
+
+	indices := {j |
+		some j, y in arr
+		x == y
+	}
+
+	count(indices) > 1
+}

docs/rules/bugs/duplicate-rule.md

@@ -0,0 +1,61 @@
+# duplicate-rule
+
+**Summary**: Duplicate rule
+
+**Category**: Bugs
+
+**Avoid**
+```rego
+package policy
+
+import future.keywords.if
+
+allow if user.is_admin
+
+allow if user.is_developer
+
+# we already covered this!
+allow if user.is_admin
+```
+
+**Prefer**
+```rego
+package policy
+
+import future.keywords.if
+
+allow if user.is_admin
+
+allow if user.is_developer
+```
+
+## Rationale
+
+Duplicated rules are likely a mistake, perhaps from pasting contents from another file.
+
+This rule identifies rules that are _identical_ in terms of their name, assigned value, and body — excluding
+whitespace. In technical terms, if two or more rules share the same abstract syntax tree, they are considered
+to be duplicates.
+
+## Exceptions
+
+Note that this rule currently works at the scope of a single file. If you're using the same package across multiple
+files, there could still be duplicates across those files. This will be addressed in a future version of this rule.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  bugs:
+    duplicated-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

@@ -38,6 +38,14 @@ constant_condition if {
 	1 == 1
 }
 
+duplicate_rule if {
+	input.foo
+}
+
+duplicate_rule if {
+	input.foo
+}
+
 # METADATA
 # invalid-metadata-attribute: true
 should := "fail"
@@ -193,9 +201,9 @@ yoda_condition if {
 
 ### Testing ###
 
-# this will also tringger the test-outside-test-package rule
-test_identically_named_tests := true
+# this will also trigger the test-outside-test-package rule
 test_identically_named_tests := true
+test_identically_named_tests := false
 
 todo_test_bad if {
 	input.bad