From c6ed47db1c10847d4c95efc5e22cc06d5def77db Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Tue, 2 Jul 2024 08:47:36 +0200 Subject: [PATCH] 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 --- README.md | 1 + bundle/regal/ast/search.rego | 34 +++---- bundle/regal/config/provided/data.yaml | 2 + .../rules/bugs/argument_always_wildcard.rego | 31 ++++++ .../bugs/argument_always_wildcard_test.rego | 97 +++++++++++++++++++ docs/rules/bugs/argument-always-wildcard.md | 94 ++++++++++++++++++ e2e/testdata/violations/most_violations.rego | 4 + 7 files changed, 246 insertions(+), 17 deletions(-) create mode 100644 bundle/regal/rules/bugs/argument_always_wildcard.rego create mode 100644 bundle/regal/rules/bugs/argument_always_wildcard_test.rego create mode 100644 docs/rules/bugs/argument-always-wildcard.md diff --git a/README.md b/README.md index 751c87f1..3d5def52 100644 --- a/README.md +++ b/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 | diff --git a/bundle/regal/ast/search.rego b/bundle/regal/ast/search.rego index 91221101..08302d3d 100644 --- a/bundle/regal/ast/search.rego +++ b/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,19 +20,19 @@ _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 @@ -40,7 +40,7 @@ _find_some_in_decl_vars(_, value) := var if { } # 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 } diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index bde164a4..44939d50 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/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: diff --git a/bundle/regal/rules/bugs/argument_always_wildcard.rego b/bundle/regal/rules/bugs/argument_always_wildcard.rego new file mode 100644 index 00000000..cd50072d --- /dev/null +++ b/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] diff --git a/bundle/regal/rules/bugs/argument_always_wildcard_test.rego b/bundle/regal/rules/bugs/argument_always_wildcard_test.rego new file mode 100644 index 00000000..f7572772 --- /dev/null +++ b/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() +} diff --git a/docs/rules/bugs/argument-always-wildcard.md b/docs/rules/bugs/argument-always-wildcard.md new file mode 100644 index 00000000..f40e7dd1 --- /dev/null +++ b/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)! diff --git a/e2e/testdata/violations/most_violations.rego b/e2e/testdata/violations/most_violations.rego index 44c8554f..cc17e00a 100644 --- a/e2e/testdata/violations/most_violations.rego +++ b/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 {