diff --git a/bundle/regal/ast/ast.rego b/bundle/regal/ast/ast.rego index b0bcebc0..ce7613dc 100644 --- a/bundle/regal/ast/ast.rego +++ b/bundle/regal/ast/ast.rego @@ -104,11 +104,30 @@ rule_names contains ref_to_string(rule.head.ref) if some rule in rules # METADATA # description: | -# determine if var in ref (e.g. `x` in `input[x]`) is used as input or output -is_output_var(rule, ref, location) if { - startswith(ref.value, "$") +# determine if var in var (e.g. `x` in `input[x]`) is used as input or output +# 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, "$") } else if { - not ref.value in (find_names_in_scope(rule, location) - find_some_decl_names_in_scope(rule, location)) + not var.value in (rule_names | imported_identifiers) # regal ignore:external-reference + + num_above := sum([1 | + some above in find_vars_in_local_scope(rule, var.location) + above.value == var.value + ]) + num_some := sum([1 | + some name in find_some_decl_names_in_scope(rule, var.location) + name == var.value + ]) + + # only the first ref variable in scope can be an output! meaning that: + # allow if { + # some x + # input[x] # <--- output + # data.bar[x] # <--- input + # } + num_above - num_some == 0 } # METADATA @@ -195,10 +214,6 @@ static_ref(ref) if every t in array.slice(ref.value, 1, count(ref.value)) { t.type != "var" } -static_rule_ref(ref) if every t in array.slice(ref, 1, count(ref)) { - t.type != "var" -} - # METADATA # description: provides a set of names of all built-in functions called in the input policy builtin_functions_called contains name if { @@ -257,25 +272,6 @@ implicit_boolean_assignment(rule) if { # or sometimes, like this... implicit_boolean_assignment(rule) if rule.head.value.location == rule.head.location -# or like this... -implicit_boolean_assignment(rule) if { - # This handles the *quite* special case of - # `a.b if true`, which is "rewritten" to `a.b = true` *and* where a location is still added to the value - # see https://github.com/open-policy-agent/opa/issues/6184 for details - # - # Do note that technically, it is possible to write a rule where the `true` value actually is on column 1, i.e. - # - # a.b = - # true - # if true - # - # If you write Rego like that — you're not going to use Regal anyway, are you? ¯\_(ツ)_/¯ - rule.head.value.type == "boolean" - rule.head.value.value == true - - rule.head.value.location.col == 1 -} - # METADATA # description: | # object containing all available built-in and custom functions in the diff --git a/bundle/regal/ast/ast_test.rego b/bundle/regal/ast/ast_test.rego index 59d7e523..9709cc27 100644 --- a/bundle/regal/ast/ast_test.rego +++ b/bundle/regal/ast/ast_test.rego @@ -283,3 +283,21 @@ test_all_refs if { text_refs == {":=", "data.foo.bar", "data.foo.bax", "data.foo.baz"} } + +test_provided_capabilities_never_undefined if { + capabilities.provided == {} with data.internal as {} +} + +test_function_calls if { + calls := ast.function_calls["0"] with input as ast.with_rego_v1(` + rule if { + x := 1 + f(2) + }`) + + {"assign", "f"} == {call.name | some call in calls} +} + +test_implicit_boolean_assignment if { + ast.implicit_boolean_assignment(ast.with_rego_v1(`a.b if true`).rules[0]) +} diff --git a/bundle/regal/main/main.rego b/bundle/regal/main/main.rego index 0b7c8b4f..9d601f97 100644 --- a/bundle/regal/main/main.rego +++ b/bundle/regal/main/main.rego @@ -24,10 +24,7 @@ rules_to_run[category][title] if { not config.excluded_file(category, title, input.regal.file.name) } -notices contains notice if { - some category, title - some notice in grouped_notices[category][title] -} +notices contains grouped_notices[_][_][_] grouped_notices[category][title] contains notice if { some category, title diff --git a/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego b/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego index 13b355c7..9aa5b81f 100644 --- a/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego +++ b/bundle/regal/rules/bugs/inconsistent-args/inconsistent_args.rego @@ -29,7 +29,7 @@ report contains violation if { # "Partition" the args by their position by_position := [s | some i, _ in args_list[0] - s := [x | x := args_list[_][i]] + s := [item[i] | some item in args_list] ] some position in by_position diff --git a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego index 1c21cc3d..90f41390 100644 --- a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego +++ b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable.rego @@ -22,9 +22,8 @@ import data.regal.result # some x in data.foo.bar # ``` report contains violation if { - some rule_index, name - - var_refs := _ref_vars[rule_index][name] + some rule_index + var_refs := _ref_vars[rule_index][_] count(var_refs) == 1 @@ -32,10 +31,11 @@ report contains violation if { not _var_in_head(input.rules[to_number(rule_index)].head, var.value) not _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 # so when all other conditions apply - ast.is_output_var(input.rules[to_number(rule_index)], var, var.location) + ast.is_output_var(input.rules[to_number(rule_index)], var) violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(var)) } @@ -47,6 +47,15 @@ _ref_vars[rule_index][var.value] contains var if { not startswith(var.value, "$") } +# "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_head(head, name) if head.value.value == name _var_in_head(head, name) if { @@ -63,6 +72,12 @@ _var_in_head(head, name) if { var.value == name } +_var_in_head(head, name) if { + some i, var in head.ref + i > 0 + var.value == name +} + _var_in_call(calls, rule_index, name) if _var_in_arg(calls[rule_index][_].args[_], name) _var_in_arg(arg, name) if { @@ -71,16 +86,7 @@ _var_in_arg(arg, name) if { } _var_in_arg(arg, name) if { - arg.type == "ref" - - some val in arg.value - - val.type == "var" - val.value == name -} - -_var_in_arg(arg, name) if { - arg.type in {"array", "object"} + arg.type in {"array", "object", "set"} some var in ast.find_term_vars(arg) diff --git a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego index 01b44678..2833f988 100644 --- a/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego +++ b/bundle/regal/rules/bugs/unused-output-variable/unused_output_variable_test.rego @@ -72,6 +72,28 @@ test_success_not_unused_variable_in_head_value if { r == set() } +test_success_not_unused_variable_in_head_term_value if { + module := ast.with_rego_v1(` + success := {x} if { + input[x] + } + `) + + r := rule.report with input as module + r == set() +} + +test_success_not_unused_variable_in_head_term_key if { + module := ast.with_rego_v1(` + success contains {x} if { + input[x] + } + `) + + r := rule.report with input as module + r == set() +} + test_success_not_unused_variable_in_head_key if { module := ast.with_rego_v1(` success contains x if { @@ -96,6 +118,19 @@ test_success_not_unused_output_variable_function_call if { r == set() } +test_success_not_unused_output_variable_function_call_arg_term if { + module := ast.with_rego_v1(` + success if { + some x + input[x] + f({x}) + } + `) + + r := rule.report with input as module + r == set() +} + test_success_not_unused_output_variable_other_ref if { module := ast.with_rego_v1(` success if { diff --git a/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego b/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego index 496c5e2e..a63a8d58 100644 --- a/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego +++ b/bundle/regal/rules/idiomatic/use-strings-count/use_strings_count.rego @@ -12,7 +12,7 @@ import data.regal.result # description: Missing capability for built-in function `strings.count` # custom: # severity: warning -notices contains result.notice(rego.metadata.chain()) if not capabilities.has_object_keys +notices contains result.notice(rego.metadata.chain()) if not capabilities.has_strings_count # METADATA # description: flag calls to `count` where the first argument is a call to `indexof_n` diff --git a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego index 5edbfa9c..61125a03 100644 --- a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego +++ b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports.rego @@ -54,30 +54,27 @@ aggregate_report contains violation if { } some imp in all_imports - resolves(imp.path, all_package_paths) + _resolves(imp.path, all_package_paths) not imp.path in all_package_paths - not imp.path in ignored_import_paths + not imp.path in _ignored_import_paths violation := result.fail(rego.metadata.chain(), {"location": imp.location}) } -# METADATA -# description: | -# returns true if the path "resolves" to *any* -# package part of the same length as the path -resolves(path, pkg_paths) if count([path | +# returns true if the path "resolves" to *any* package part of the same length as the path +_resolves(path, pkg_paths) if count([path | some pkg_path in pkg_paths pkg_path == array.slice(path, 0, count(pkg_path)) ]) > 0 -ignored_import_paths contains path if { +_ignored_import_paths contains path if { some item in cfg["ignore-import-paths"] path := [part | some i, p in split(item, ".") - part := normalize_part(i, p) + part := _normalize_part(i, p) ] } -normalize_part(0, part) := part if part != "data" +_normalize_part(0, part) := part if part != "data" -normalize_part(i, part) := part if i > 0 +_normalize_part(i, part) := part if i > 0 diff --git a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego index 867fc913..8c596c18 100644 --- a/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego +++ b/bundle/regal/rules/imports/prefer-package-imports/prefer_package_imports_test.rego @@ -98,14 +98,23 @@ test_success_aggregate_report_on_import_with_unresolved_path if { } test_success_aggregate_report_ignored_import_path if { - aggregate := {{"aggregate_data": { - "package_path": ["a"], - "imports": [{"path": ["b"], "location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b"}}], - }}} + aggregate := { + {"aggregate_data": { + "package_path": ["a"], + "imports": [{ + "path": ["b", "c"], + "location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b.c"}, + }], + }}, + {"aggregate_data": { + "package_path": ["b"], + "imports": [], + }}, + } r := rule.aggregate_report with input.aggregate as aggregate with config.for_rule as { "level": "error", - "ignore-import-paths": ["data.b"], + "ignore-import-paths": ["data.b.c"], } r == set() diff --git a/bundle/regal/rules/style/default-over-not/default_over_not.rego b/bundle/regal/rules/style/default-over-not/default_over_not.rego index 1995290d..fd6e2dcf 100644 --- a/bundle/regal/rules/style/default-over-not/default_over_not.rego +++ b/bundle/regal/rules/style/default-over-not/default_over_not.rego @@ -46,6 +46,10 @@ _static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [r some i, ref in rule.head.ref i > 0 ])) if { - count(rule.head.ref) > 1 - ast.static_rule_ref(rule.head.ref) + c := count(rule.head.ref) + c > 1 + + every t in array.slice(rule.head.ref, 1, c) { + t.type != "var" + } } diff --git a/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego b/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego index 5d286018..5a1ab019 100644 --- a/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego +++ b/bundle/regal/rules/style/prefer-some-in-iteration/prefer_some_in_iteration.rego @@ -29,7 +29,7 @@ report contains violation if { # we don't need the location of each var, but using the first # ref will do, and will hopefully help with caching the result - ast.is_output_var(rule, var, value.location) + ast.is_output_var(rule, var) ]) num_output_vars != 0 diff --git a/bundle/regal/rules/style/rule-length/rule_length_test.rego b/bundle/regal/rules/style/rule-length/rule_length_test.rego index bbd5dc3f..28e5fe10 100644 --- a/bundle/regal/rules/style/rule-length/rule_length_test.rego +++ b/bundle/regal/rules/style/rule-length/rule_length_test.rego @@ -73,6 +73,25 @@ test_success_rule_longer_than_configured_max_length_but_comments if { r == set() } +test_success_rule_longer_than_configured_max_length_but_no_body_and_exception_configured if { + module := regal.parse_module("policy.rego", `package p + + my_short_rule := { + "a": input.a, + "b": input.b, + "c": input.c, + "d": input.d, + } + `) + + r := rule.report with input as module with config.for_rule as { + "level": "error", + "max-rule-length": 2, + "except-empty-body": true, + } + r == set() +} + test_success_rule_length_equals_max_length if { module := regal.parse_module("policy.rego", `package p diff --git a/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego b/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego index 55bf5c15..fbe8a40e 100644 --- a/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego +++ b/bundle/regal/rules/style/unnecessary-some/unnecessary_some.rego @@ -11,8 +11,7 @@ report contains violation if { # No need to traverse rules here if we're not importing `in` ast.imports_keyword(input.imports, "in") - some rule_index - symbols := ast.found.symbols[rule_index][_] + symbols := ast.found.symbols[_][_] symbols[0].type == "call" symbols[0].value[0].type == "ref" diff --git a/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego b/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego index af46ff4a..0af171d3 100644 --- a/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego +++ b/bundle/regal/rules/style/use-assignment-operator/use_assignment_operator.rego @@ -20,7 +20,7 @@ report contains violation if { loc := result.location(rule) - violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}})) + violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}})) } report contains violation if { @@ -32,7 +32,7 @@ report contains violation if { loc := result.location(result.location(rule.head.ref[0])) - violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}})) + violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}})) } report contains violation if { @@ -54,12 +54,7 @@ report contains violation if { # assignment regex.match(`else\s*=`, loc.location.text) - violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": eq_col(loc)}})) + violation := result.fail(rego.metadata.chain(), object.union(loc, {"location": {"col": _eq_col(loc)}})) } -default eq_col(_) := 1 - -eq_col(loc) := pos + 1 if { - pos := indexof(loc.location.text, "=") - pos != -1 -} +_eq_col(loc) := max([0, indexof(loc.location.text, "=")]) + 1 diff --git a/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego b/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego index 4e504ab5..c821e596 100644 --- a/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego +++ b/bundle/regal/rules/testing/metasyntactic-variable/metasyntactic_variable.rego @@ -47,7 +47,7 @@ report contains violation if { lower(var.value) in metasyntactic - ast.is_output_var(input.rules[to_number(i)], var, var.location) + ast.is_output_var(input.rules[to_number(i)], var) violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(var)) } diff --git a/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego b/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego index 825e18a2..c5e6e7e4 100644 --- a/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego +++ b/bundle/regal/rules/testing/test-outside-test-package/test_outside_test_package_test.rego @@ -41,6 +41,18 @@ test_success_test_inside_test_package if { result == set() } +test_success_test_inside_test_package_named_just_test if { + ast := regal.parse_module("test.rego", ` + package test + + import rego.v1 + + test_foo if { false } + `) + result := rule.report with input as ast + result == set() +} + # https://github.com/StyraInc/regal/issues/176 test_success_test_prefixed_function if { ast := regal.parse_module("foo_test.rego", ` diff --git a/bundle/regal/util/util_test.rego b/bundle/regal/util/util_test.rego new file mode 100644 index 00000000..ae38f849 --- /dev/null +++ b/bundle/regal/util/util_test.rego @@ -0,0 +1,20 @@ +package regal.util_test + +import rego.v1 + +import data.regal.util + +test_find_duplicates if { + util.find_duplicates([1, 1, 2, 3, 3, 3]) == {{0, 1}, {3, 4, 5}} +} + +test_json_pretty if { + # oh, the things you do for test coverage + util.json_pretty({"x": [1, 2, 3]}) == `{ + "x": [ + 1, + 2, + 3 + ] +}` +}