Rule: inconsistent-args (#448)

Warn on inconsistent argument names in repeated function declarations.

Fixes #444

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

README.md

@@ -185,6 +185,7 @@ The following rules are currently available:
 | Category  |                                                Title                                                |                        Description                        |
 |-----------|-----------------------------------------------------------------------------------------------------|-----------------------------------------------------------|
 | bugs      | [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition)                    | Constant condition                                        |
+| 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                  |
 | bugs      | [not-equals-in-loop](https://docs.styra.com/regal/rules/bugs/not-equals-in-loop)                    | Use of != in loop                                         |
 | bugs      | [rule-named-if](https://docs.styra.com/regal/rules/bugs/rule-named-if)                              | Rule named "if"                                           |

bundle/regal/config/provided/data.yaml

@@ -2,6 +2,8 @@ rules:
   bugs:
     constant-condition:
       level: error
+    inconsistent-args:
+      level: error
     invalid-metadata-attribute:
       level: error
     not-equals-in-loop:

bundle/regal/result.rego

@@ -56,10 +56,10 @@ _category_title_from_path(path) := [category, title] if ["regal", "rules", categ
 _category_title_from_path(path) := [category, title] if ["custom", "regal", "rules", category, title] = path
 
 # Provided rules, i.e. regal.rules.category.title
-fail(chain, details) := violation if {
-	is_array(chain) # from rego.metadata.chain()
+fail(metadata, details) := violation if {
+	is_array(metadata) # from rego.metadata.chain()
 
-	some link in chain
+	some link in metadata
 	link.annotations.scope == "package"
 
 	some category, title
@@ -76,10 +76,10 @@ fail(chain, details) := violation if {
 }
 
 # Custom rules, i.e. custom.regal.rules.category.title
-fail(chain, details) := violation if {
-	is_array(chain) # from rego.metadata.chain()
+fail(metadata, details) := violation if {
+	is_array(metadata) # from rego.metadata.chain()
 
-	some link in chain
+	some link in metadata
 	link.annotations.scope == "package"
 
 	some category, title

bundle/regal/rules/bugs/inconsistent_args.rego

@@ -0,0 +1,63 @@
+# METADATA
+# description: Inconsistently named function arguments
+package regal.rules.bugs["inconsistent-args"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	count(ast.functions) > 0
+
+	# Comprehension indexing — as made obvious here it would be great
+	# to have block level scoped ignore directives...
+	function_args_by_name := {name: args_list |
+		some i
+
+		# regal ignore:prefer-some-in-iteration
+		name := ast.ref_to_string(ast.functions[i].head.ref)
+		args_list := [args |
+			some j
+
+			# regal ignore:prefer-some-in-iteration
+			ast.ref_to_string(ast.functions[j].head.ref) == name
+
+			# regal ignore:prefer-some-in-iteration
+			args := ast.functions[j].head.args
+		]
+		count(args_list) > 1
+	}
+
+	some name, args_list in function_args_by_name
+
+	# "Partition" the args by their position
+	by_position := [s |
+		some i, _ in args_list[0]
+		s := [x | x := args_list[_][i]]
+	]
+
+	some position in by_position
+
+	inconsistent_args(position)
+
+	violation := result.fail(rego.metadata.chain(), result.location(find_function_by_name(name)))
+}
+
+inconsistent_args(position) if {
+	named_vars := {arg.value |
+		some arg in position
+		arg.type == "var"
+		not startswith(arg.value, "$")
+	}
+	count(named_vars) > 1
+}
+
+# Return the _second_ function found by name, as that
+# is reasonably the location the inconsistency is found
+find_function_by_name(name) := [fn |
+	some fn in ast.functions
+	ast.ref_to_string(fn.head.ref) == name
+][1]

bundle/regal/rules/bugs/inconsistent_args_test.rego

@@ -0,0 +1,83 @@
+package regal.rules.bugs["inconsistent-args_test"]
+
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+
+import data.regal.rules.bugs["inconsistent-args"] as rule
+
+test_fail_inconsistent_args if {
+	module := ast.with_future_keywords(`
+	foo(a, b) if a == b
+	foo(b, a) if b > a
+
+	bar(b, a) if b > a
+	`)
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\tfoo(b, a) if b > a"})
+}
+
+test_fail_nested_inconsistent_args if {
+	module := ast.with_future_keywords(`
+	a.b.foo(a, b) if a == b
+	a.b.foo(b, a) if b > a
+	`)
+	r := rule.report with input as module
+	r == expected_with_location({"col": 2, "file": "policy.rego", "row": 10, "text": "\ta.b.foo(b, a) if b > a"})
+}
+
+test_success_not_inconsistent_args if {
+	module := ast.with_future_keywords(`
+	foo(a, b) if a == b
+	foo(a, b) if a > b
+
+	bar(b, a) if b > a
+	bar(b, a) if b == a
+
+	qux(c, a) if c == a
+	`)
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_using_wildcard if {
+	module := ast.with_future_keywords(`
+	foo(a, b) if a == b
+	foo(_, b) if b.foo
+
+	qux(c, a) if c == a
+	`)
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_using_pattern_matching if {
+	module := ast.with_future_keywords(`
+	foo(a, b) if a == b
+	foo(a, "foo") if a.foo
+
+	qux(c, a) if c == a
+	`)
+	r := rule.report with input as module
+	r == set()
+}
+
+expected := {
+	"category": "bugs",
+	"description": "Inconsistently named function arguments",
+	"level": "error",
+	"related_resources": [{
+		"description": "documentation",
+		"ref": "https://docs.styra.com/regal/rules/bugs/inconsistent-args",
+	}],
+	"title": "inconsistent-args",
+}
+
+expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
+
+expected_with_location(location) := {object.union(expected, {"location": loc}) |
+	some loc in location
+} if {
+	is_set(location)
+}

bundle/regal/rules/idiomatic/equals_pattern_matching_test.rego

@@ -83,8 +83,8 @@ expected := {
 
 expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
 
-expected_with_location(locations) := {object.union(expected, {"location": location}) |
-	some location in locations
+expected_with_location(location) := {object.union(expected, {"location": loc}) |
+	some loc in location
 } if {
-	is_set(locations)
+	is_set(location)
 }

docs/rules/bugs/inconsistent-args.md

@@ -0,0 +1,86 @@
+# inconsistent-args
+
+**Summary**: Inconsistently named function arguments
+
+**Category**: Bugs
+
+**Avoid**
+```rego
+package policy
+
+import future.keywords.if
+import future.keywords.in
+
+find_vars(rule, node) if node in rule
+
+# Order of arguments changed, or at least it looks like it
+find_vars(node, rule) if {
+    walk(rule, [path, value])
+    # ...
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import future.keywords.if
+import future.keywords.in
+
+find_vars(rule, node) if node in rule
+
+find_vars(rule, node) if {
+    walk(rule, [path, value])
+    # ...
+}
+```
+
+## Rationale
+
+Whenever a custom function declaration is repeated, the argument names should remain consistent in each declaration.
+
+Inconsistently named function arguments is a likely source of bugs, and should be avoided.
+
+## Exceptions
+
+Using wildcards (`_`) in place of unused arguments is always allowed, and in fact enforced by the compiler:
+
+```rego
+package policy
+
+import future.keywords.if
+import future.keywords.in
+
+find_vars(rule, node) if node in rule
+
+# We don't use `node` here
+find_vars(rule, _) if {
+    walk(rule, [path, value])
+    # ...
+}
+```
+
+Using [pattern matching for equality](https://docs.styra.com/regal/rules/idiomatic/equals-pattern-matching) checks is
+also allowed.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules: 
+  bugs:
+    inconsistent-args:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Related Resources
+
+- Regal Docs: [equals-pattern-matching](https://docs.styra.com/regal/rules/idiomatic/equals-pattern-matching)
+
+## 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

@@ -58,6 +58,14 @@ unused_return_value {
 
 zero_arity_function() := true
 
+inconsistent_args(a, b) {
+	a == b
+}
+
+inconsistent_args(b, a) {
+	b == a
+}
+
 ### Idiomatic ###
 
 custom_has_key_construct(map, key) {