Rule: walk-no-path (#1219)

Suggest replacing the path variable with a wildcard where possible, as
the walk built-in is able to optimize such calls. More info in the docs
included in the PR.

Also:
- Add `ast.is_wildcard` convenience function and use it consistently

Fixes #1154

Signed-off-by: Anders Eknert <anders@styra.com>

README.md

@@ -254,6 +254,7 @@ The following rules are currently available:
 | imports     | [unresolved-import](https://docs.styra.com/regal/rules/imports/unresolved-import)                       | Unresolved import                                         |
 | imports     | [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1)                                   | Use `import rego.v1`                                      |
 | performance | [defer-assignment](https://docs.styra.com/regal/rules/performance/defer-assignment)                     | Assignment can be deferred                                |
+| performance | [walk-no-path](https://docs.styra.com/regal/rules/performance/walk-no-path)                             | Call to `walk` can be optimized                           |
 | performance | [with-outside-test-context](https://docs.styra.com/regal/rules/performance/with-outside-test-context)   | `with` used outside test context                          |
 | style       | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix)         | Avoid `get_` and `list_` prefix for rules and functions   |
 | style       | [chained-rule-body](https://docs.styra.com/regal/rules/style/chained-rule-body)                         | Avoid chaining rule bodies                                |

bundle/regal/ast/ast.rego

@@ -46,6 +46,13 @@ is_constant(term) if {
 	not has_term_var(term.value)
 }
 
+# METADATA
+# description: true if provided term represents a wildcard (`_`) variable
+is_wildcard(term) if {
+	term.type == "var"
+	startswith(term.value, "$")
+}
+
 default builtin_names := set()
 
 # METADATA
@@ -160,7 +167,7 @@ rule_names contains ref_to_string(rule.head.ref) if some rule in rules
 # scope: document
 is_output_var(rule, var) if {
 	# test the cheap and common case first, and 'else' only when it's not
-	startswith(var.value, "$")
+	is_wildcard(var)
 } else if {
 	not var.value in (rule_names | imported_identifiers) # regal ignore:external-reference
 
@@ -381,6 +388,26 @@ var_in_head(head, name) if {
 	var.value == name
 }
 
+# METADATA
+# description: |
+#   true if var of `name` is referenced in any `calls` (likely,
+#   `ast.function_calls`) in the rule of given `rule_index`
+# scope: document
+var_in_call(calls, rule_index, name) if _var_in_arg(calls[rule_index][_].args[_], name)
+
+_var_in_arg(arg, name) if {
+	arg.type == "var"
+	arg.value == name
+}
+
+_var_in_arg(arg, name) if {
+	arg.type in {"array", "object", "set"}
+
+	some var in find_term_vars(arg)
+
+	var.value == name
+}
+
 # METADATA
 # description: answers wether provided expression is an assignment (using `:=`)
 is_assignment(expr) if {

bundle/regal/ast/ast_test.rego

@@ -45,7 +45,7 @@ test_find_vars if {
 
 	module := regal.parse_module("p.rego", policy)
 
-	vars := ast.find_vars(module.rules)
+	vars := ast.find_vars(module.rules) with data.internal.combined_config as {"capabilities": capabilities.provided}
 
 	names := {var.value |
 		some var in vars
@@ -70,7 +70,7 @@ test_find_vars_comprehension_lhs if {
 
 	module := regal.parse_module("p.rego", policy)
 
-	vars := ast.find_vars(module.rules)
+	vars := ast.find_vars(module.rules) with data.internal.combined_config as {"capabilities": capabilities.provided}
 
 	names := {var.value |
 		some var in vars
@@ -236,6 +236,8 @@ test_find_names_in_scope if {
 	allow_rule := module.rules[3]
 
 	in_scope := ast.find_names_in_scope(allow_rule, {"col": 1, "row": 30}) with input as module
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
+
 	in_scope == {"bar", "global", "comp", "allow", "a", "b", "c", "d", "e"}
 }
 

bundle/regal/ast/search.rego

@@ -59,7 +59,6 @@ _find_some_in_decl_vars(value) := vars if {
 find_ref_vars(value) := [var |
 	some i, var in value.value
 
-	# ignore first element, as it is the base ref (like input or data)
 	i > 0
 	var.type == "var"
 ]
@@ -249,7 +248,7 @@ found.comprehensions[rule_index] contains value if {
 find_vars_in_local_scope(rule, location) := [var |
 	var := found.vars[_rule_index(rule)][_][_] # regal ignore:external-reference
 
-	not startswith(var.value, "$")
+	not is_wildcard(var)
 	_before_location(rule, var, util.to_location_object(location))
 ]
 

bundle/regal/config/provided/data.yaml

@@ -15,7 +15,6 @@ rules:
     duplicate-rule:
       level: error
     if-empty-object:
-      # deprecated with the introduction of if-object-literal
       level: ignore
     if-object-literal:
       level: error
@@ -120,6 +119,8 @@ rules:
   performance:
     defer-assignment:
       level: error
+    walk-no-path:
+      level: error
     with-outside-test-context:
       level: error
   style:

bundle/regal/lsp/completion/location/location_test.rego

@@ -3,6 +3,7 @@ package regal.lsp.completion.location_test
 import rego.v1
 
 import data.regal.ast
+import data.regal.capabilities
 
 import data.regal.lsp.completion.location
 
@@ -69,21 +70,26 @@ another if {
 
 	r2 := location.find_locals(module.rules, {"row": 6, "col": 10}) with input as module
 		with input.regal.file.lines as lines
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r2 == {"x"}
 
 	r3 := location.find_locals(module.rules, {"row": 10, "col": 1}) with input as module
 		with input.regal.file.lines as lines
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r3 == {"a", "b"}
 
 	r4 := location.find_locals(module.rules, {"row": 10, "col": 6}) with input as module
 		with input.regal.file.lines as lines
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r4 == {"a", "b", "c"}
 
 	r5 := location.find_locals(module.rules, {"row": 15, "col": 1}) with input as module
 		with input.regal.file.lines as lines
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r5 == {"x", "y"}
 
 	r6 := location.find_locals(module.rules, {"row": 16, "col": 1}) with input as module
 		with input.regal.file.lines as lines
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r6 == {"x", "y", "z"}
 }

bundle/regal/rules/bugs/argument-always-wildcard/argument_always_wildcard.rego

@@ -17,7 +17,7 @@ report contains violation if {
 
 	every function in functions {
 		function.head.args[pos].type == "var"
-		startswith(function.head.args[pos].value, "$")
+		ast.is_wildcard(function.head.args[pos])
 	}
 
 	not _function_name_excepted(config.for_rule("bugs", "argument-always-wildcard"), name)

bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego

@@ -43,7 +43,7 @@ _inconsistent_args(position) if {
 	named_vars := {arg.value |
 		some arg in position
 		arg.type == "var"
-		not startswith(arg.value, "$")
+		not ast.is_wildcard(arg)
 	}
 	count(named_vars) > 1
 }

bundle/regal/rules/bugs/not-equals-in-loop/not_equals_in_loop.rego

@@ -18,8 +18,7 @@ report contains violation if {
 	neq_term.type == "ref"
 
 	some value in neq_term.value
-	value.type == "var"
-	startswith(value.value, "$")
+	ast.is_wildcard(value)
 
 	violation := result.fail(rego.metadata.chain(), result.location(expr.terms[0]))
 }

bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego

@@ -30,7 +30,7 @@ report contains violation if {
 	some var in var_refs
 
 	not ast.var_in_head(input.rules[to_number(rule_index)].head, var.value)
-	not _var_in_call(ast.function_calls, rule_index, var.value)
+	not ast.var_in_call(ast.function_calls, rule_index, var.value)
 	not _ref_base_vars[rule_index][var.value]
 
 	# this is by far the most expensive condition to check, so only do
@@ -44,29 +44,13 @@ _ref_vars[rule_index][var.value] contains var if {
 	some rule_index
 	var := ast.found.vars[rule_index].ref[_]
 
-	not startswith(var.value, "$")
+	not ast.is_wildcard(var)
 }
 
 # "a" in "a[foo]", and not foo
 _ref_base_vars[rule_index][term.value] contains term if {
 	some rule_index
 	term := ast.found.refs[rule_index][_].value[0]
 
-	term.type == "var"
-	not startswith(term.value, "$")
-}
-
-_var_in_call(calls, rule_index, name) if _var_in_arg(calls[rule_index][_].args[_], name)
-
-_var_in_arg(arg, name) if {
-	arg.type == "var"
-	arg.value == name
-}
-
-_var_in_arg(arg, name) if {
-	arg.type in {"array", "object", "set"}
-
-	some var in ast.find_term_vars(arg)
-
-	var.value == name
+	not ast.is_wildcard(term)
 }

bundle/regal/rules/custom/naming-convention/naming_convention_test.rego

@@ -3,6 +3,7 @@ package regal.rules.custom["naming-convention_test"]
 import rego.v1
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 import data.regal.rules.custom["naming-convention"] as rule
 
@@ -162,7 +163,9 @@ test_fail_multiple_conventions if {
 		{"targets": ["package"], "pattern": `^acmecorp\.[a-z_\.]+$`},
 		{"targets": ["rule", "variable"], "pattern": "^bar$|^foo_bar$"},
 	]}
-	r := rule.report with input as policy with config.for_rule as cfg
+	r := rule.report with input as policy
+		with config.for_rule as cfg
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 
 	r == {
 		expected(

bundle/regal/rules/idiomatic/custom-has-key-construct/custom_has_key_construct.rego

@@ -39,13 +39,11 @@ report contains violation if {
 
 # normalize var to always always be on the left hand side
 _normalize_eq_terms(terms) := [terms[1], terms[2]] if {
-	terms[1].type == "var"
-	startswith(terms[1].value, "$")
+	ast.is_wildcard(terms[1])
 	terms[2].type == "ref"
 }
 
 _normalize_eq_terms(terms) := [terms[2], terms[1]] if {
 	terms[1].type == "ref"
-	terms[2].type == "var"
-	startswith(terms[2].value, "$")
+	ast.is_wildcard(terms[2])
 }

bundle/regal/rules/idiomatic/custom-in-construct/custom_in_construct.rego

@@ -25,8 +25,7 @@ report contains violation if {
 
 	var.value in arg_names
 	ref.value[0].value in arg_names
-	ref.value[1].type == "var"
-	startswith(ref.value[1].value, "$")
+	ast.is_wildcard(ref.value[1])
 
 	violation := result.fail(rego.metadata.chain(), result.location(rule.head))
 }

bundle/regal/rules/idiomatic/equals-pattern-matching/equals_pattern_matching.rego

@@ -71,13 +71,11 @@ report contains violation if {
 
 # normalize var to always always be on the left hand side
 _normalize_eq_terms(terms, scalar_types) := [terms[1], terms[2]] if {
-	terms[1].type == "var"
-	not startswith(terms[1].value, "$")
+	not ast.is_wildcard(terms[1])
 	terms[2].type in scalar_types
 }
 
 _normalize_eq_terms(terms, scalar_types) := [terms[2], terms[1]] if {
 	terms[1].type in scalar_types
-	terms[2].type == "var"
-	not startswith(terms[2].value, "$")
+	not ast.is_wildcard(terms[2])
 }

bundle/regal/rules/idiomatic/prefer-set-or-object-rule/prefer_set_or_object_rule.rego

@@ -68,6 +68,5 @@ _is_array_conversion(value) if {
 
 	some ref_val in rhs.value
 
-	ref_val.type == "var"
-	startswith(ref_val.value, "$")
+	ast.is_wildcard(ref_val)
 }

bundle/regal/rules/idiomatic/use-in-operator/use_in_operator.rego

@@ -37,9 +37,8 @@ _non_loop_term(terms) := [{"pos": i + 1, "term": term} |
 _loop_term(term) if {
 	term.type == "ref"
 	term.value[0].type == "var"
-	last := regal.last(term.value)
-	last.type == "var"
-	startswith(last.value, "$")
+
+	ast.is_wildcard(regal.last(term.value))
 }
 
 _static_term(term) if term.type in {"array", "boolean", "object", "null", "number", "set", "string", "var"}

bundle/regal/rules/idiomatic/use-some-for-output-vars/use_some_for_output_vars.rego

@@ -15,7 +15,7 @@ report contains violation if {
 	# first item can't be a loop or key ref
 	i != 0
 	elem.type == "var"
-	not startswith(elem.value, "$")
+	not ast.is_wildcard(elem)
 
 	rule := input.rules[to_number(rule_index)]
 

bundle/regal/rules/performance/walk-no-path/walk_no_path.rego

@@ -0,0 +1,61 @@
+# METADATA
+# description: Call to `walk` can be optimized
+package regal.rules.performance["walk-no-path"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	some rule_index
+	call := ast.function_calls[rule_index][_]
+
+	call.name == "walk"
+	call.args[1].type == "array"
+	call.args[1].value[0].type == "var"
+
+	path_var := call.args[1].value[0]
+
+	not ast.is_wildcard(path_var)
+	not ast.var_in_head(input.rules[to_number(rule_index)].head, path_var.value)
+
+	not _var_in_other_call(ast.function_calls, rule_index, path_var)
+	not _var_in_ref(rule_index, path_var)
+
+	violation := result.fail(rego.metadata.chain(), result.location(call))
+}
+
+# similar to ast.var_in_call, but here we need to discern that
+# the var is not the same var declared in the walk itself
+_var_in_other_call(calls, rule_index, var) if _var_in_arg(calls[rule_index][_].args[_], var)
+
+_var_in_arg(arg, var) if {
+	arg.type == "var"
+	arg.value == var.value
+
+	arg.location != var.location
+}
+
+_var_in_arg(arg, var) if {
+	arg.type in {"array", "object", "set"}
+
+	some term_var in ast.find_term_vars(arg)
+
+	term_var.value == var.value
+	term_var.location != var.location
+}
+
+_var_in_ref(rule_index, var) if {
+	some ref_var in ast.found.vars[rule_index].ref
+
+	var.value == ref_var.value
+}
+
+_var_in_ref(rule_index, var) if {
+	term := ast.found.refs[rule_index][_].value[0] # regal ignore:external-reference
+
+	not ast.is_wildcard(term)
+
+	var.value == term.value
+}