Rule: argument-always-wildcard

I did not expect that I'd have to fix any issues in Regal as a result of
this, but that was a rather nice surprise!

Fixes #872

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

README.md

@@ -183,6 +183,7 @@ The following rules are currently available:
 
 |  Category   |                                                 Title                                                 |                        Description                        |
 |-------------|-------------------------------------------------------------------------------------------------------|-----------------------------------------------------------|
+| bugs        | [argument-always-wildcard](https://docs.styra.com/regal/rules/bugs/argument-always-wildcard)          | Argument is always a wildcard                             |
 | 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                                            |

bundle/regal/ast/search.rego

@@ -11,7 +11,7 @@ _find_nested_vars(obj) := [value |
 # simple assignment, i.e. `x := 100` returns `x`
 # always returns a single var, but wrapped in an
 # array for consistency
-_find_assign_vars(_, value) := var if {
+_find_assign_vars(value) := var if {
 	value[1].type == "var"
 	var := [value[1]]
 }
@@ -20,27 +20,27 @@ _find_assign_vars(_, value) := var if {
 # [a, b, c] := [1, 2, 3]
 # or
 # {a: b} := {"foo": "bar"}
-_find_assign_vars(_, value) := var if {
+_find_assign_vars(value) := var if {
 	value[1].type in {"array", "object"}
 	var := _find_nested_vars(value[1])
 }
 
 # var declared via `some`, i.e. `some x` or `some x, y`
-_find_some_decl_vars(_, value) := [v |
+_find_some_decl_vars(value) := [v |
 	some v in value
 	v.type == "var"
 ]
 
 # single var declared via `some in`, i.e. `some x in y`
-_find_some_in_decl_vars(_, value) := var if {
+_find_some_in_decl_vars(value) := var if {
 	arr := value[0].value
 	count(arr) == 3
 
 	var := _find_nested_vars(arr[1])
 }
 
 # two vars declared via `some in`, i.e. `some x, y in z`
-_find_some_in_decl_vars(_, value) := var if {
+_find_some_in_decl_vars(value) := var if {
 	arr := value[0].value
 	count(arr) == 4
 
@@ -62,7 +62,7 @@ find_ref_vars(value) := [var |
 
 # one or two vars declared via `every`, i.e. `every x in y {}`
 # or `every`, i.e. `every x, y in y {}`
-_find_every_vars(_, value) := var if {
+_find_every_vars(value) := var if {
 	key_var := [v | v := value.key; v.type == "var"; indexof(v.value, "$") == -1]
 	val_var := [v | v := value.value; v.type == "var"; indexof(v.value, "$") == -1]
 
@@ -88,7 +88,7 @@ _find_object_comprehension_vars(value) := array.concat(key, val) if {
 	val := [value.value.value | value.value.value.type == "var"]
 }
 
-_find_vars(_, value, last) := {"term": find_term_vars(function_ret_args(fn_name, value))} if {
+_find_vars(value, last) := {"term": find_term_vars(function_ret_args(fn_name, value))} if {
 	last == "terms"
 	value[0].type == "ref"
 	value[0].value[0].type == "var"
@@ -101,7 +101,7 @@ _find_vars(_, value, last) := {"term": find_term_vars(function_ret_args(fn_name,
 	function_ret_in_args(fn_name, value)
 }
 
-_find_vars(path, value, last) := {"assign": _find_assign_vars(path, value)} if {
+_find_vars(value, last) := {"assign": _find_assign_vars(value)} if {
 	last == "terms"
 	value[0].type == "ref"
 	value[0].value[0].type == "var"
@@ -112,35 +112,35 @@ _find_vars(path, value, last) := {"assign": _find_assign_vars(path, value)} if {
 # left-hand side is equally dubious, but we'll treat `x = 1` as `x := 1` for
 # the purpose of this function until we have a more robust way of dealing with
 # unification
-_find_vars(path, value, last) := {"assign": _find_assign_vars(path, value)} if {
+_find_vars(value, last) := {"assign": _find_assign_vars(value)} if {
 	last == "terms"
 	value[0].type == "ref"
 	value[0].value[0].type == "var"
 	value[0].value[0].value == "eq"
 }
 
-_find_vars(_, value, _) := {"ref": find_ref_vars(value)} if value.type == "ref"
+_find_vars(value, _) := {"ref": find_ref_vars(value)} if value.type == "ref"
 
-_find_vars(path, value, last) := {"somein": _find_some_in_decl_vars(path, value)} if {
+_find_vars(value, last) := {"somein": _find_some_in_decl_vars(value)} if {
 	last == "symbols"
 	value[0].type == "call"
 }
 
-_find_vars(path, value, last) := {"some": _find_some_decl_vars(path, value)} if {
+_find_vars(value, last) := {"some": _find_some_decl_vars(value)} if {
 	last == "symbols"
 	value[0].type != "call"
 }
 
-_find_vars(path, value, last) := {"every": _find_every_vars(path, value)} if {
+_find_vars(value, last) := {"every": _find_every_vars(value)} if {
 	last == "terms"
 	value.domain
 }
 
-_find_vars(_, value, _) := {"setorarraycomprehension": _find_set_or_array_comprehension_vars(value)} if {
+_find_vars(value, _) := {"setorarraycomprehension": _find_set_or_array_comprehension_vars(value)} if {
 	value.type in {"setcomprehension", "arraycomprehension"}
 }
 
-_find_vars(_, value, _) := {"objectcomprehension": _find_object_comprehension_vars(value)} if {
+_find_vars(value, _) := {"objectcomprehension": _find_object_comprehension_vars(value)} if {
 	value.type == "objectcomprehension"
 }
 
@@ -159,7 +159,7 @@ find_some_decl_vars(rule) := [var | some var in vars[_rule_index(rule)]["some"]]
 find_vars(node) := [var |
 	walk(node, [path, value])
 
-	var := _find_vars(path, value, regal.last(path))[_][_]
+	var := _find_vars(value, regal.last(path))[_][_]
 ]
 
 # hack to work around the different input models of linting vs. the lsp package.. we
@@ -189,7 +189,7 @@ vars[rule_index][context] contains var if {
 
 	walk(rule, [path, value])
 
-	some context, vars in _find_vars(path, value, regal.last(path))
+	some context, vars in _find_vars(value, regal.last(path))
 	some var in vars
 }
 

bundle/regal/config/provided/data.yaml

@@ -3,6 +3,8 @@ features:
     check-version: true
 rules:
   bugs:
+    argument-always-wildcard:
+      level: error
     constant-condition:
       level: error
     deprecated-builtin:

bundle/regal/rules/bugs/argument_always_wildcard.rego

@@ -0,0 +1,31 @@
+# METADATA
+# description: Argument is always a wildcard
+package regal.rules.bugs["argument-always-wildcard"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+function_groups[name] contains fn if {
+	some fn in ast.functions
+
+	name := ast.ref_to_string(fn.head.ref)
+}
+
+report contains violation if {
+	some functions in function_groups
+
+	fn := any_member(functions)
+
+	some pos in numbers.range(0, count(fn.head.args) - 1)
+
+	every function in functions {
+		function.head.args[pos].type == "var"
+		startswith(function.head.args[pos].value, "$")
+	}
+
+	violation := result.fail(rego.metadata.chain(), result.location(fn.head.args[pos]))
+}
+
+any_member(s) := [x | some x in s][0]

bundle/regal/rules/bugs/argument_always_wildcard_test.rego

@@ -0,0 +1,97 @@
+package regal.rules.bugs["argument-always-wildcard_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.bugs["argument-always-wildcard"] as rule
+
+test_fail_single_function_single_argument_always_a_wildcard if {
+	module := ast.with_rego_v1(`
+	f(_) := 1
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Argument is always a wildcard",
+		"level": "error",
+		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
+		}],
+		"title": "argument-always-wildcard",
+	}}
+}
+
+test_fail_single_argument_always_a_wildcard if {
+	module := ast.with_rego_v1(`
+	f(_) := 1
+	f(_) := 2
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Argument is always a wildcard",
+		"level": "error",
+		"location": {"col": 4, "file": "policy.rego", "row": 6, "text": "\tf(_) := 1"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
+		}],
+		"title": "argument-always-wildcard",
+	}}
+}
+
+test_fail_single_argument_always_a_wildcard_default_function if {
+	module := ast.with_rego_v1(`
+	default f(_) := 1
+	f(_) := 2
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Argument is always a wildcard",
+		"level": "error",
+		"location": {"col": 12, "file": "policy.rego", "row": 6, "text": "\tdefault f(_) := 1"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
+		}],
+		"title": "argument-always-wildcard",
+	}}
+}
+
+test_fail_multiple_argument_always_a_wildcard if {
+	module := ast.with_rego_v1(`
+	f(x, _) := x + 1
+	f(x, _) := x + 2
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Argument is always a wildcard",
+		"level": "error",
+		"location": {"col": 7, "file": "policy.rego", "row": 6, "text": "\tf(x, _) := x + 1"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/argument-always-wildcard", "bugs"),
+		}],
+		"title": "argument-always-wildcard",
+	}}
+}
+
+test_success_multiple_argument_not_always_a_wildcard if {
+	module := ast.with_rego_v1(`
+	f(x, _) := x + 1
+	f(_, y) := y + 2
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}

docs/rules/bugs/argument-always-wildcard.md

@@ -0,0 +1,94 @@
+# argument-always-wildcard
+
+**Summary**: Argument is always a wildcard
+
+**Category**: Bugs
+
+**Avoid**
+```rego
+package policy
+
+import rego.v1
+
+# there's only one definition of the last_name function in
+# this package, and the second argument is never used
+last_name(name, _) := lname if {
+    parts := split(name, " ")
+    lname := parts[count(parts) - 1]
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import rego.v1
+
+last_name(name) := lname if {
+    parts := split(name, " ")
+    lname := parts[count(parts) - 1]
+}
+```
+
+## Rationale
+
+Function definitions may use wildcard variables as arguments to indicate that the value is not used in the body of
+the function. This helps make the function definition more readable, as it's immediately clear which of the arguments
+are used in that definition of the function. This is particularly useful for incrementally defined functions:
+
+```rego
+package policy
+
+import rego.v1
+
+default authorized(_, _) := false
+
+authorized(user, _) if {
+    # some logic to determine if authorized
+}
+
+# or
+
+authorized(user, _) if {
+    # some further logic to determine if authorized
+}
+```
+
+In the example above, the second argument is a wildcard in all definitions, and could just as well be removed for a
+cleaner definition. More likely, the argument was meant to be _used_, if only in one of the definitions:
+
+```rego
+package policy
+
+import rego.v1
+
+default authorized(_, _) := false
+
+authorized(user, _) if {
+    # some logic to determine if authorized
+}
+
+# or
+
+authorized(_, request) if {
+    # some further logic to determine if authorized
+}
+```
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  bugs:
+    argument-always-wildcard:
+      # 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

@@ -91,6 +91,10 @@ impossible_not if {
 	not partial
 }
 
+argument_always_wildcard(_) if true
+
+argument_always_wildcard(_) if true
+
 ### Idiomatic ###
 
 custom_has_key_construct(map, key) if {