Skip to content

Commit

Permalink
Rule: var-shadows-builtin (#893)
Browse files Browse the repository at this point in the history
Also in this PR is a slight change of behavior, where variables found
in comprehension heads are no longer treated as assigned. This as they
_aren't_, and the assignment happens in the comprehension body (or outside
the comprehension). This change will uncover a few more
`use-some-for-output-vars` issues in policy, which was also fixed as part
of this PR.

Fixes #823

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored Jul 2, 2024
1 parent 7773c3c commit b7c7385
Show file tree
Hide file tree
Showing 10 changed files with 120 additions and 23 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ The following rules are currently available:
| bugs | [rule-shadows-builtin](https://docs.styra.com/regal/rules/bugs/rule-shadows-builtin) | Rule name shadows built-in |
| bugs | [top-level-iteration](https://docs.styra.com/regal/rules/bugs/top-level-iteration) | Iteration in top-level assignment |
| bugs | [unassigned-return-value](https://docs.styra.com/regal/rules/bugs/unassigned-return-value) | Non-boolean return value unassigned |
| bugs | [var-shadows-builtin](https://docs.styra.com/regal/rules/bugs/var-shadows-builtin) | Variable name shadows built-in |
| bugs | [zero-arity-function](https://docs.styra.com/regal/rules/bugs/zero-arity-function) | Avoid functions without args |
| custom | [forbidden-function-call](https://docs.styra.com/regal/rules/custom/forbidden-function-call) | Forbidden function call |
| custom | [naming-convention](https://docs.styra.com/regal/rules/custom/naming-convention) | Naming convention violation |
Expand Down
19 changes: 0 additions & 19 deletions bundle/regal/ast/search.rego
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,6 @@ find_term_vars(term) := [value |
value.type == "var"
]

_find_set_or_array_comprehension_vars(value) := [value.value.term] if {
value.value.term.type == "var"
} else := find_term_vars(value.value.term)

_find_object_comprehension_vars(value) := array.concat(key, val) if {
key := [value.value.key | value.value.key.type == "var"]
val := [value.value.value | value.value.value.type == "var"]
}

_find_vars(value, last) := {"term": find_term_vars(function_ret_args(fn_name, value))} if {
last == "terms"
value[0].type == "ref"
Expand Down Expand Up @@ -136,14 +127,6 @@ _find_vars(value, last) := {"every": _find_every_vars(value)} if {
value.domain
}

_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 {
value.type == "objectcomprehension"
}

_rule_index(rule) := sprintf("%d", [i]) if {
some i, r in _rules # regal ignore:external-reference
r == rule
Expand Down Expand Up @@ -173,8 +156,6 @@ _rules := data.workspace.parsed[input.regal.file.uri].rules if not input.rules
# object containing all variables found in the input AST, keyed first by the index of
# the rule where the variables were found (as a numeric string), and then the context
# of the variable, which will be one of:
# - objectcomprehension
# - setorarraycomprehension
# - term
# - assign
# - every
Expand Down
2 changes: 1 addition & 1 deletion bundle/regal/config/exclusion.rego
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pattern_compiler(pattern) := ps1 if {
ps1 := {pat |
some _p, _ in ps
nps := trailing_slash(_p)
nps[pat]
some pat, _ in nps
}
}

Expand Down
4 changes: 2 additions & 2 deletions bundle/regal/config/exclusion_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ cases := {

test_all_cases_are_as_expected if {
not_exp := {pattern: res |
subcases := cases[pattern]
some pattern, subcases in cases
res := {file: res1 |
exp := subcases[file]
some file, exp in subcases
act := config.exclude(pattern, file)
exp != act
res1 := {"exp": exp, "act": act}
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 @@ -40,6 +40,8 @@ rules:
level: error
unassigned-return-value:
level: error
var-shadows-builtin:
level: error
zero-arity-function:
level: error
custom:
Expand Down
16 changes: 16 additions & 0 deletions bundle/regal/rules/bugs/var_shadows_builtin.rego
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# METADATA
# description: Variable name shadows built-in
package regal.rules.bugs["var-shadows-builtin"]

import rego.v1

import data.regal.ast
import data.regal.result

report contains violation if {
var := ast.vars[_][_][_]

var.value in ast.builtin_namespaces

violation := result.fail(rego.metadata.chain(), result.location(var))
}
33 changes: 33 additions & 0 deletions bundle/regal/rules/bugs/var_shadows_builtin_test.rego
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package regal.rules.bugs["var-shadows-builtin_test"]

import rego.v1

import data.regal.ast
import data.regal.capabilities
import data.regal.config

import data.regal.rules.bugs["var-shadows-builtin"] as rule

test_fail_var_shadows_builtin if {
module := ast.with_rego_v1(`allow if http := "yes"`)

r := rule.report with input as module with data.internal.combined_config as {"capabilities": capabilities.provided}
r == {{
"category": "bugs",
"description": "Variable name shadows built-in",
"level": "error",
"location": {"col": 10, "file": "policy.rego", "row": 5, "text": "allow if http := \"yes\""},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/var-shadows-builtin", "bugs"),
}],
"title": "var-shadows-builtin",
}}
}

test_success_var_does_not_shadow_builtin if {
module := ast.with_rego_v1(`allow if answer := "yes"`)

r := rule.report with input as module with data.internal.combined_config as {"capabilities": capabilities.provided}
r == set()
}
3 changes: 2 additions & 1 deletion docs/rules/bugs/rule-shadows-builtin.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ behavior.
This linter rule provides the following configuration options:

```yaml
rules:
rules:
bugs:
rule-shadows-builtin:
# one of "error", "warning", "ignore"
Expand All @@ -36,6 +36,7 @@ rules:
- OPA Docs: [Built-in Functions](https://www.openpolicyagent.org/docs/latest/policy-reference/#built-in-functions)
- OPA Repo: [builtin_metadata.json](https://github.com/open-policy-agent/opa/blob/main/builtin_metadata.json)
- Regal Docs: [var-shadows-builting](https://docs.styra.com/regal/rules/bugs/var-shadows-builtin)
## Community
Expand Down
61 changes: 61 additions & 0 deletions docs/rules/bugs/var-shadows-builtin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# var-shadows-builtin

**Summary**: Variable shadows built-in

**Category**: Bugs

**Avoid**
```rego
package policy
import rego.v1
# variable `http` shadows `http.send` built-in function
allow if {
http := startswith(input.url, "http://")
# do something with http
}
```

**Prefer**
```rego
package policy
import rego.v1
# variable `is_http` doesn't shadow any built-in function
allow if {
is_http := startswith(input.url, "http://")
# do something with is_http
}
```

## Rationale

Using the name of built-in functions or operators as variable names can lead to confusion and unexpected behavior.
A variable that shadows a built-in function (or the namespace of a function, like `http` in `http.send`) prevents any
function in that namespace to be used later in the rule. Avoid this!

## Configuration Options

This linter rule provides the following configuration options:

```yaml
rules:
bugs:
var-shadows-builtin:
# one of "error", "warning", "ignore"
level: error
```
## Related Resources
- OPA Docs: [Built-in Functions](https://www.openpolicyagent.org/docs/latest/policy-reference/#built-in-functions)
- OPA Repo: [builtin_metadata.json](https://github.com/open-policy-agent/opa/blob/main/builtin_metadata.json)
- Regal Docs: [rule-shadows-builting](https://docs.styra.com/regal/rules/bugs/rule-shadows-builtin)
## 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)!
2 changes: 2 additions & 0 deletions e2e/testdata/violations/most_violations.rego
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ argument_always_wildcard(_) if true
# title: annotation without metadata
some_rule := true

var_shadows_builtin if http := true

### Idiomatic ###

custom_has_key_construct(map, key) if {
Expand Down

0 comments on commit b7c7385

Please sign in to comment.