-
Notifications
You must be signed in to change notification settings - Fork 37
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
One of the rules I've wanted since the project was started, but not until now would this have been wildly expensive to add. With the recent fixes that allow us to reuse the result of `walk`ing the AST, that's no longer the case. Still, this required quite some time and effort to get right! NOTE: this rule currently only considers output variables as found in references, i.e. `x` in `input[x]` and NOT `x` in `some x, _ in input`. While that would be easy to add, I'm not sure if those are really "output variables" or should be considered as such... 🤔 or if we should just have another rules for "unused iteration variable" or something like that. Either way, this is a big step forward! Fixes #60 Signed-off-by: Anders Eknert <anders@styra.com>
- Loading branch information
1 parent
6a0a9b5
commit 160d9b4
Showing
9 changed files
with
371 additions
and
21 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # METADATA | ||
| # description: Unused output variable | ||
| package regal.rules.bugs["unused-output-variable"] | ||
|
|
||
| import rego.v1 | ||
|
|
||
| import data.regal.ast | ||
| import data.regal.result | ||
|
|
||
| # METADATA | ||
| # description: | | ||
| # The `report` set contains unused output vars from `some` declarations. Using a | ||
| # stricter ruleset than OPA, Regal considers an output var "unused" if it's used | ||
| # only once in a ref, as that usage may just as well be replaced by a wildcard. | ||
| # ``` | ||
| # some y | ||
| # x := data.foo.bar[y] | ||
| # # y not used later | ||
| # ``` | ||
| # Would better be written as: | ||
| # ``` | ||
| # some x in data.foo.bar | ||
| # ``` | ||
| report contains violation if { | ||
| some rule_index, name | ||
|
|
||
| var_refs := _ref_vars[rule_index][name] | ||
|
|
||
| count(var_refs) == 1 | ||
|
|
||
| some var in var_refs | ||
|
|
||
| not _var_in_head(input.rules[to_number(rule_index)].head, var.value) | ||
| not _var_in_call(ast.function_calls, rule_index, var.value) | ||
|
|
||
| violation := result.fail(rego.metadata.chain(), result.ranged_location_from_text(var)) | ||
| } | ||
|
|
||
| _ref_vars[rule_index][var.value] contains var if { | ||
| some rule_index | ||
| var := ast.vars[rule_index].ref[_] | ||
|
|
||
| not startswith(var.value, "$") | ||
| } | ||
|
|
||
| _var_in_head(head, name) if head.value.value == name | ||
|
|
||
| _var_in_head(head, name) if { | ||
| some var in ast.find_term_vars(head.value.value) | ||
|
|
||
| var.value == name | ||
| } | ||
|
|
||
| _var_in_head(head, name) if head.key.value == name | ||
|
|
||
| _var_in_head(head, name) if { | ||
| some var in ast.find_term_vars(head.key.value) | ||
|
|
||
| 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 { | ||
| arg.type == "var" | ||
| arg.value == name | ||
| } | ||
|
|
||
| _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"} | ||
|
|
||
| some var in ast.find_term_vars(arg) | ||
|
|
||
| var.value == name | ||
| } |
109 changes: 109 additions & 0 deletions
109
bundle/regal/rules/bugs/unused_output_variable_test.rego
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| package regal.rules.bugs["unused-output-variable_test"] | ||
|
|
||
| import rego.v1 | ||
|
|
||
| import data.regal.ast | ||
| import data.regal.config | ||
|
|
||
| import data.regal.rules.bugs["unused-output-variable"] as rule | ||
|
|
||
| test_fail_unused_output_variable if { | ||
| module := ast.with_rego_v1(` | ||
| fail if { | ||
| input[x] | ||
| } | ||
| `) | ||
|
|
||
| r := rule.report with input as module | ||
| r == {{ | ||
| "category": "bugs", | ||
| "description": "Unused output variable", | ||
| "level": "error", | ||
| "location": {"col": 9, "end": {"col": 10, "row": 7}, "file": "policy.rego", "row": 7, "text": "\t\tinput[x]"}, | ||
| "related_resources": [{ | ||
| "description": "documentation", | ||
| "ref": config.docs.resolve_url("$baseUrl/$category/unused-output-variable", "bugs"), | ||
| }], | ||
| "title": "unused-output-variable", | ||
| }} | ||
| } | ||
|
|
||
| test_fail_unused_output_variable_some if { | ||
| module := ast.with_rego_v1(` | ||
| fail if { | ||
| some xx | ||
| input[xx] | ||
| } | ||
| `) | ||
|
|
||
| r := rule.report with input as module | ||
| r == {{ | ||
| "category": "bugs", | ||
| "description": "Unused output variable", | ||
| "level": "error", | ||
| "location": {"col": 9, "end": {"col": 11, "row": 8}, "file": "policy.rego", "row": 8, "text": "\t\tinput[xx]"}, | ||
| "related_resources": [{ | ||
| "description": "documentation", | ||
| "ref": config.docs.resolve_url("$baseUrl/$category/unused-output-variable", "bugs"), | ||
| }], | ||
| "title": "unused-output-variable", | ||
| }} | ||
| } | ||
|
|
||
| test_success_unused_wildcard if { | ||
| module := ast.with_rego_v1(` | ||
| success if { | ||
| input[_] | ||
| } | ||
| `) | ||
|
|
||
| r := rule.report with input as module | ||
| r == set() | ||
| } | ||
|
|
||
| test_success_not_unused_variable_in_head_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_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_output_variable_function_call if { | ||
| module := ast.with_rego_v1(` | ||
| success if { | ||
| some x | ||
| input[x] | ||
| regex.match("[x]", 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 { | ||
| some x | ||
| input[x] == data.foo[x] | ||
| } | ||
| `) | ||
|
|
||
| r := rule.report with input as module | ||
| r == set() | ||
| } |
Oops, something went wrong.