Skip to content

Commit

Permalink
Fix false positive in rules not counting imports in scope (#592)
Browse files Browse the repository at this point in the history
Fixes #590

That issue only mentions `top-level-iteration`, but I found
the same behavior in `use-some-for-output-vars`, so this fixes
that too.

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored Mar 11, 2024
1 parent 9e9a30e commit fff72d9
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 11 deletions.
4 changes: 3 additions & 1 deletion bundle/regal/ast/ast.rego
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ function_arg_names(rule) := [arg.value | some arg in rule.head.args]

rule_and_function_names contains name(rule) if some rule in input.rules

identifiers := rule_and_function_names | imported_identifiers

rule_names contains name(rule) if some rule in rules

# METADATA
Expand Down Expand Up @@ -294,7 +296,7 @@ find_names_in_scope(rule, location) := names if {
var_names := {var.value | some var in find_vars_in_local_scope(rule, location)}

# parens below added by opa-fmt :)
names := (rule_names | fn_arg_names) | var_names
names := ((rule_names | imported_identifiers) | fn_arg_names) | var_names
}

# METADATA
Expand Down
14 changes: 14 additions & 0 deletions bundle/regal/ast/imports.rego
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ default imports := []
# it safe to refer to without risk of halting evaluation
imports := input.imports

# METADATA
# description: |
# set of all names imported in the input module, meaning commonly the last part of any
# imported ref, like "bar" in "data.foo.bar", or an alias like "baz" in "data.foo.bar as baz".
imported_identifiers contains imported_identifier(imp) if {
some imp in imports

imp.path.value[0].value in {"input", "data"}
}

imported_identifier(imp) := imp.alias

imported_identifier(imp) := regal.last(imp.path.value).value if not imp.alias

imports_has_path(imports, path) if {
some imp in imports

Expand Down
14 changes: 4 additions & 10 deletions bundle/regal/rules/bugs/top_level_iteration.rego
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,20 @@ report contains violation if {
last := regal.last(rule.head.value.value)

last.type == "var"
illegal_value_ref(last.value, rule)
illegal_value_ref(last.value, rule, ast.identifiers)

violation := result.fail(rego.metadata.chain(), result.location(rule.head))
}

_path(loc) := concat(".", {l.value | some l in loc})

illegal_value_ref(value, rule) if {
# regal ignore:external-reference
not value in ast.rule_and_function_names
illegal_value_ref(value, rule, identifiers) if {
not value in identifiers
not is_arg_or_input(value, rule)
}

is_arg_or_input(value, rule) if value in ast.function_arg_names(rule)

is_arg_or_input(value, _) if startswith(_path(value), "input.")

# ideally would be able to just say
# is_arg_or_input("input", _)
# but the formatter rewrites that to
# is_arg_or_input("input", _) = true
# ...meh
is_arg_or_input("input", _) := true
is_arg_or_input("input", _)
8 changes: 8 additions & 0 deletions bundle/regal/rules/bugs/top_level_iteration_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,11 @@ test_success_top_level_param if {
r := rule.report with input as ast.with_rego_v1(`x(y) := input.foo.bar[y]`)
r == set()
}

test_success_top_level_import if {
r := rule.report with input as ast.with_rego_v1(`
import data.x
y := input[x]`)
r == set()
}

0 comments on commit fff72d9

Please sign in to comment.