Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule: argument-always-wildcard #883

Merged
merged 1 commit into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
34 changes: 17 additions & 17 deletions bundle/regal/ast/search.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
}
Expand All @@ -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

Expand All @@ -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]

Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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"
}

Expand All @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 2 additions & 0 deletions bundle/regal/config/provided/data.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ features:
check-version: true
rules:
bugs:
argument-always-wildcard:
level: error
constant-condition:
level: error
deprecated-builtin:
Expand Down
31 changes: 31 additions & 0 deletions bundle/regal/rules/bugs/argument_always_wildcard.rego
Original file line number Diff line number Diff line change
@@ -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]
97 changes: 97 additions & 0 deletions bundle/regal/rules/bugs/argument_always_wildcard_test.rego
Original file line number Diff line number Diff line change
@@ -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()
}
94 changes: 94 additions & 0 deletions docs/rules/bugs/argument-always-wildcard.md
Original file line number Diff line number Diff line change
@@ -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 {
anderseknert marked this conversation as resolved.
Show resolved Hide resolved
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)!
4 changes: 4 additions & 0 deletions e2e/testdata/violations/most_violations.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down