Rule: var-shadows-builtin (#893)

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 <[email protected]>

README.md

@@ -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                               |

bundle/regal/ast/search.rego

@@ -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"
@@ -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
@@ -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

bundle/regal/config/exclusion.rego

@@ -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
 	}
 }
 

bundle/regal/config/exclusion_test.rego

@@ -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}

bundle/regal/config/provided/data.yaml

@@ -40,6 +40,8 @@ rules:
       level: error
     unassigned-return-value:
       level: error
+    var-shadows-builtin:
+      level: error
     zero-arity-function:
       level: error
   custom:

bundle/regal/rules/bugs/var_shadows_builtin.rego

@@ -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))
+}

bundle/regal/rules/bugs/var_shadows_builtin_test.rego

@@ -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()
+}

docs/rules/bugs/rule-shadows-builtin.md

@@ -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"
@@ -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
 

docs/rules/bugs/var-shadows-builtin.md

@@ -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, "https://")
+    # 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, "https://")
+    # 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)!

e2e/testdata/violations/most_violations.rego

@@ -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 {