Rule: pointless-reassignment (#878)

Also:
- fix the one violation of this rule found in our code :P
- add metadata annotations to a bunch of rules

Fixes #867

Signed-off-by: Anders Eknert <[email protected]>

README.md

@@ -242,6 +242,7 @@ The following rules are currently available:
 | 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       | [pointless-reassignment](https://docs.styra.com/regal/rules/style/pointless-reassignment)             | Pointless reassignment of variable                        |
 | style       | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case)                       | Prefer snake_case for names                               |
 | style       | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration)         | Prefer `some .. in` for iteration                         |
 | style       | [rule-length](https://docs.styra.com/regal/rules/style/rule-length)                                   | Max rule length exceeded                                  |

bundle/regal/ast/ast.rego

@@ -47,13 +47,22 @@ _is_name(ref, pos) if {
 	ref.type == "string"
 }
 
-# allow := true, which expands to allow = true { true }
+# METADATA
+# description: |
+#   answers if the body was generated or not, i.e. not seen
+#   in the original Rego file — for example `x := 1`
+# scope: document
+
+# METADATA
+# description: covers case of allow := true, which expands to allow = true { true }
 generated_body(rule) if rule.body[0].location == rule.head.value.location
 
+# METADATA
+# description: covers case of default rules
 generated_body(rule) if rule["default"] == true
 
-# rule["message"] or
-# rule contains "message"
+# METADATA
+# description: covers case of rule["message"] or rule contains "message"
 generated_body(rule) if {
 	rule.body[0].location.row == rule.head.key.location.row
 
@@ -62,32 +71,47 @@ generated_body(rule) if {
 	rule.body[0].location.col < rule.head.key.location.col
 }
 
-# f("x")
+# METADATA
+# description: covers case of f("x")
 generated_body(rule) if rule.body[0].location == rule.head.location
 
+# METADATA
+# description: all the rules (excluding functions) in the input AST
 rules := [rule |
 	some rule in input.rules
 	not rule.head.args
 ]
 
+# METADATA
+# description: all the test rules in the input AST
 tests := [rule |
 	some rule in input.rules
 	not rule.head.args
 
 	startswith(ref_to_string(rule.head.ref), "test_")
 ]
 
+# METADATA
+# description: all the functions declared in the input AST
 functions := [rule |
 	some rule in input.rules
 	rule.head.args
 ]
 
+# METADATA
+# description: a list of the names for the giiven rule (if function)
 function_arg_names(rule) := [arg.value | some arg in rule.head.args]
 
+# METADATA
+# description: all the rule and function names in the input AST
 rule_and_function_names contains ref_to_string(rule.head.ref) if some rule in input.rules
 
+# METADATA
+# description: all identifers in the input AST (rule and functiin names, plus imported names)
 identifiers := rule_and_function_names | imported_identifiers
 
+# METADATA
+# description: all rule names in the input AST (excluding functions)
 rule_names contains ref_to_string(rule.head.ref) if some rule in rules
 
 _function_arg_names(rule) := {arg.value |
@@ -104,6 +128,9 @@ is_output_var(rule, ref, location) if {
 	not ref.value in (find_names_in_scope(rule, location) - find_some_decl_names_in_scope(rule, location))
 }
 
+# METADATA
+# description: as the name implies, answers whether provided value is a ref
+# scope: document
 default is_ref(_) := false
 
 is_ref(value) if value.type == "ref"
@@ -162,7 +189,7 @@ static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [re
 }
 
 # METADATA
-# description: provides a set of all built-in function calls made in input policy
+# description: provides a set of names of all built-in functions called in the input policy
 builtin_functions_called contains name if {
 	some value in all_refs
 
@@ -236,8 +263,16 @@ implicit_boolean_assignment(rule) if {
 	rule.head.value.location.col == 1
 }
 
+# METADATA
+# description: |
+#   object containing all available built-in and custom functions in the
+#   scope of the input AST, keyed by function name
 all_functions := object.union(config.capabilities.builtins, function_decls(input.rules))
 
+# METADATA
+# description: |
+#   set containing all available built-in and custom function names in the
+#   scope of the input AST
 all_function_names := object.keys(all_functions)
 
 negated_expressions[rule] contains value if {

bundle/regal/config/provided/data.yaml

@@ -134,6 +134,8 @@ rules:
       level: error
     opa-fmt:
       level: error
+    pointless-reassignment:
+      level: error
     prefer-snake-case:
       level: error
     prefer-some-in-iteration:

bundle/regal/rules/imports/circular_import.rego

@@ -43,11 +43,10 @@ aggregate_report contains violation if {
 
 	sorted_group := sort(g)
 
-	location := [e |
+	location := [loc |
 		some m1 in sorted_group
 		some m2 in sorted_group
 		some loc in package_locations[m1][m2]
-		e := loc
 	][0]
 
 	violation := result.fail(

bundle/regal/rules/style/pointless_reassignment.rego

@@ -0,0 +1,39 @@
+# METADATA
+# description: Pointless reassignment of variable
+package regal.rules.style["pointless-reassignment"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+# pointless reassignment in rule head
+report contains violation if {
+	some rule in ast.rules
+
+	ast.generated_body(rule)
+
+	rule.head.value.type == "var"
+	count(rule.head.ref) == 1
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule))
+}
+
+# pointless reassignment in rule body
+report contains violation if {
+	some call in ast.all_refs
+
+	call[0].value[0].type == "var"
+	call[0].value[0].value == "assign"
+
+	call[2].type == "var"
+
+	violation := result.fail(rego.metadata.chain(), result.location(call))
+}
+
+assign_calls contains call if {
+	some call in ast.all_refs
+
+	call[0].value[0].type == "var"
+	call[0].value[0].value == "assign"
+}

bundle/regal/rules/style/pointless_reassignment_test.rego

@@ -0,0 +1,52 @@
+package regal.rules.style["pointless-reassignment_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.style["pointless-reassignment"] as rule
+
+test_pointless_reassignment_in_rule_head if {
+	module := ast.with_rego_v1(`
+	foo := "foo"
+
+	bar := foo
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "style",
+		"description": "Pointless reassignment of variable",
+		"level": "error",
+		"location": {"col": 2, "file": "policy.rego", "row": 8, "text": "\tbar := foo"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/pointless-reassignment", "style"),
+		}],
+		"title": "pointless-reassignment",
+	}}
+}
+
+test_pointless_reassignment_in_rule_body if {
+	module := ast.with_rego_v1(`
+	rule if {
+		foo := "foo"
+
+		bar := foo
+	}
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "style",
+		"description": "Pointless reassignment of variable",
+		"level": "error",
+		"location": {"col": 7, "file": "policy.rego", "row": 9, "text": "\t\tbar := foo"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/pointless-reassignment", "style"),
+		}],
+		"title": "pointless-reassignment",
+	}}
+}

docs/rules/style/pointless-reassignment.md

@@ -0,0 +1,62 @@
+# pointless-reassignment
+
+**Summary**: Pointless reassignment of variable
+
+**Category**: Style
+
+**Avoid**
+```rego
+package policy
+
+allow if {
+    users := all_users
+    any_admin(users)
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+allow if {
+    any_admin(all_users)
+}
+```
+
+## Rationale
+
+Values and variables are immutable in Rego, so reassigning the value of one variable to another only adds noise.
+
+## Exceptions
+
+Reassigning the value of a long reference often helps readability, and especially so when it needs to be referenced
+multiple times:
+
+```rego
+package policy
+
+allow if {
+    users := input.context.permissions.users
+    any_admin(users)
+}
+```
+
+This rule does not consider such assignments violations.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  style:
+    pointless-reassignment:
+      # 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

@@ -214,6 +214,8 @@ yoda_condition if {
 	"foo" == input.bar
 }
 
+pointless_reassignment := yoda_condition
+
 ### Testing ###
 
 # this will also trigger the test-outside-test-package rule

internal/embeds/templates/builtin/builtin_test.rego.tpl

@@ -24,6 +24,6 @@ test_rule_named_foo_not_allowed if {
 			"description": "documentation",
 			"ref": config.docs.resolve_url("$baseUrl/$category/{{.NameOriginal}}", "{{.Category}}"),
 		}],
-		"title": "{{.NameOriginal}}"
+		"title": "{{.NameOriginal}}",
 	}}
 }