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: walk-no-path #1219

Merged
merged 1 commit into from
Oct 23, 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 @@ -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 |
Expand Down
29 changes: 28 additions & 1 deletion bundle/regal/ast/ast.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions bundle/regal/ast/ast_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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"}
}

Expand Down
3 changes: 1 addition & 2 deletions bundle/regal/ast/search.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
Expand Down Expand Up @@ -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))
]

Expand Down
3 changes: 2 additions & 1 deletion bundle/regal/config/provided/data.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -120,6 +119,8 @@ rules:
performance:
defer-assignment:
level: error
walk-no-path:
level: error
with-outside-test-context:
level: error
style:
Expand Down
6 changes: 6 additions & 0 deletions bundle/regal/lsp/completion/location/location_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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"}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]))
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)]

Expand Down
61 changes: 61 additions & 0 deletions bundle/regal/rules/performance/walk-no-path/walk_no_path.rego
Original file line number Diff line number Diff line change
@@ -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
}
Loading