From 05a10f0a90ed29ae0eca43b9d2be78adafad4047 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Wed, 8 Nov 2023 12:11:58 +0100 Subject: [PATCH] Rule: `inconsistent-args` Warn on inconsistent argument names in repeated function declarations. Fixes #444 Signed-off-by: Anders Eknert --- README.md | 1 + bundle/regal/config/provided/data.yaml | 2 + bundle/regal/result.rego | 12 +-- .../regal/rules/bugs/inconsistent_args.rego | 63 ++++++++++++++ .../rules/bugs/inconsistent_args_test.rego | 83 ++++++++++++++++++ .../equals_pattern_matching_test.rego | 6 +- docs/rules/bugs/inconsistent-args.md | 86 +++++++++++++++++++ e2e/testdata/violations/most_violations.rego | 8 ++ 8 files changed, 252 insertions(+), 9 deletions(-) create mode 100644 bundle/regal/rules/bugs/inconsistent_args.rego create mode 100644 bundle/regal/rules/bugs/inconsistent_args_test.rego create mode 100644 docs/rules/bugs/inconsistent-args.md diff --git a/README.md b/README.md index 5033d37c..31f358b8 100644 --- a/README.md +++ b/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" | diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index ec4c17a7..13c6d5b7 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/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: diff --git a/bundle/regal/result.rego b/bundle/regal/result.rego index 0d3b923d..c4c6dba1 100644 --- a/bundle/regal/result.rego +++ b/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 diff --git a/bundle/regal/rules/bugs/inconsistent_args.rego b/bundle/regal/rules/bugs/inconsistent_args.rego new file mode 100644 index 00000000..9cb84dce --- /dev/null +++ b/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] diff --git a/bundle/regal/rules/bugs/inconsistent_args_test.rego b/bundle/regal/rules/bugs/inconsistent_args_test.rego new file mode 100644 index 00000000..0329cad0 --- /dev/null +++ b/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) +} diff --git a/bundle/regal/rules/idiomatic/equals_pattern_matching_test.rego b/bundle/regal/rules/idiomatic/equals_pattern_matching_test.rego index 2137535d..3e729bc5 100644 --- a/bundle/regal/rules/idiomatic/equals_pattern_matching_test.rego +++ b/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) } diff --git a/docs/rules/bugs/inconsistent-args.md b/docs/rules/bugs/inconsistent-args.md new file mode 100644 index 00000000..07bea7c8 --- /dev/null +++ b/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)! diff --git a/e2e/testdata/violations/most_violations.rego b/e2e/testdata/violations/most_violations.rego index 9b7ec000..c0a36448 100644 --- a/e2e/testdata/violations/most_violations.rego +++ b/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) {