diff --git a/README.md b/README.md index a8713ee1..3c494a30 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,7 @@ The following rules are currently available: | style | [messy-rule](https://docs.styra.com/regal/rules/style/messy-rule) | Messy incremental rule | | style | [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment) | Comment should start with whitespace | | style | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt) | File should be formatted with `opa fmt` | +| style | [pointless-reassignment](https://docs.styra.com/regal/rules/style/pointless-reassignment) | Pointless reassignment of variable | | style | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case) | Prefer snake_case for names | | style | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration) | Prefer `some .. in` for iteration | | style | [rule-length](https://docs.styra.com/regal/rules/style/rule-length) | Max rule length exceeded | diff --git a/bundle/regal/ast/ast.rego b/bundle/regal/ast/ast.rego index 53eb04d2..b8ea6490 100644 --- a/bundle/regal/ast/ast.rego +++ b/bundle/regal/ast/ast.rego @@ -47,13 +47,22 @@ _is_name(ref, pos) if { ref.type == "string" } -# allow := true, which expands to allow = true { true } +# METADATA +# description: | +# answers if the body was generated or not, i.e. not seen +# in the original Rego file — for example `x := 1` +# scope: document + +# METADATA +# description: covers case of allow := true, which expands to allow = true { true } generated_body(rule) if rule.body[0].location == rule.head.value.location +# METADATA +# description: covers case of default rules generated_body(rule) if rule["default"] == true -# rule["message"] or -# rule contains "message" +# METADATA +# description: covers case of rule["message"] or rule contains "message" generated_body(rule) if { rule.body[0].location.row == rule.head.key.location.row @@ -62,14 +71,19 @@ generated_body(rule) if { rule.body[0].location.col < rule.head.key.location.col } -# f("x") +# METADATA +# description: covers case of f("x") generated_body(rule) if rule.body[0].location == rule.head.location +# METADATA +# description: all the rules (excluding functions) in the input AST rules := [rule | some rule in input.rules not rule.head.args ] +# METADATA +# description: all the test rules in the input AST tests := [rule | some rule in input.rules not rule.head.args @@ -77,17 +91,27 @@ tests := [rule | startswith(ref_to_string(rule.head.ref), "test_") ] +# METADATA +# description: all the functions declared in the input AST functions := [rule | some rule in input.rules rule.head.args ] +# METADATA +# description: a list of the names for the giiven rule (if function) function_arg_names(rule) := [arg.value | some arg in rule.head.args] +# METADATA +# description: all the rule and function names in the input AST rule_and_function_names contains ref_to_string(rule.head.ref) if some rule in input.rules +# METADATA +# description: all identifers in the input AST (rule and functiin names, plus imported names) identifiers := rule_and_function_names | imported_identifiers +# METADATA +# description: all rule names in the input AST (excluding functions) rule_names contains ref_to_string(rule.head.ref) if some rule in rules _function_arg_names(rule) := {arg.value | @@ -104,6 +128,9 @@ is_output_var(rule, ref, location) if { not ref.value in (find_names_in_scope(rule, location) - find_some_decl_names_in_scope(rule, location)) } +# METADATA +# description: as the name implies, answers whether provided value is a ref +# scope: document default is_ref(_) := false is_ref(value) if value.type == "ref" @@ -162,7 +189,7 @@ static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [re } # METADATA -# description: provides a set of all built-in function calls made in input policy +# description: provides a set of names of all built-in functions called in the input policy builtin_functions_called contains name if { some value in all_refs @@ -236,8 +263,16 @@ implicit_boolean_assignment(rule) if { rule.head.value.location.col == 1 } +# METADATA +# description: | +# object containing all available built-in and custom functions in the +# scope of the input AST, keyed by function name all_functions := object.union(config.capabilities.builtins, function_decls(input.rules)) +# METADATA +# description: | +# set containing all available built-in and custom function names in the +# scope of the input AST all_function_names := object.keys(all_functions) negated_expressions[rule] contains value if { diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index ecdcb98d..bde164a4 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/bundle/regal/config/provided/data.yaml @@ -134,6 +134,8 @@ rules: level: error opa-fmt: level: error + pointless-reassignment: + level: error prefer-snake-case: level: error prefer-some-in-iteration: diff --git a/bundle/regal/rules/imports/circular_import.rego b/bundle/regal/rules/imports/circular_import.rego index d371c562..a49ce425 100644 --- a/bundle/regal/rules/imports/circular_import.rego +++ b/bundle/regal/rules/imports/circular_import.rego @@ -43,11 +43,10 @@ aggregate_report contains violation if { sorted_group := sort(g) - location := [e | + location := [loc | some m1 in sorted_group some m2 in sorted_group some loc in package_locations[m1][m2] - e := loc ][0] violation := result.fail( diff --git a/bundle/regal/rules/style/pointless_reassignment.rego b/bundle/regal/rules/style/pointless_reassignment.rego new file mode 100644 index 00000000..12006257 --- /dev/null +++ b/bundle/regal/rules/style/pointless_reassignment.rego @@ -0,0 +1,39 @@ +# METADATA +# description: Pointless reassignment of variable +package regal.rules.style["pointless-reassignment"] + +import rego.v1 + +import data.regal.ast +import data.regal.result + +# pointless reassignment in rule head +report contains violation if { + some rule in ast.rules + + ast.generated_body(rule) + + rule.head.value.type == "var" + count(rule.head.ref) == 1 + + violation := result.fail(rego.metadata.chain(), result.location(rule)) +} + +# pointless reassignment in rule body +report contains violation if { + some call in ast.all_refs + + call[0].value[0].type == "var" + call[0].value[0].value == "assign" + + call[2].type == "var" + + violation := result.fail(rego.metadata.chain(), result.location(call)) +} + +assign_calls contains call if { + some call in ast.all_refs + + call[0].value[0].type == "var" + call[0].value[0].value == "assign" +} diff --git a/bundle/regal/rules/style/pointless_reassignment_test.rego b/bundle/regal/rules/style/pointless_reassignment_test.rego new file mode 100644 index 00000000..16f5dfa3 --- /dev/null +++ b/bundle/regal/rules/style/pointless_reassignment_test.rego @@ -0,0 +1,52 @@ +package regal.rules.style["pointless-reassignment_test"] + +import rego.v1 + +import data.regal.ast +import data.regal.config + +import data.regal.rules.style["pointless-reassignment"] as rule + +test_pointless_reassignment_in_rule_head if { + module := ast.with_rego_v1(` + foo := "foo" + + bar := foo + `) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Pointless reassignment of variable", + "level": "error", + "location": {"col": 2, "file": "policy.rego", "row": 8, "text": "\tbar := foo"}, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/pointless-reassignment", "style"), + }], + "title": "pointless-reassignment", + }} +} + +test_pointless_reassignment_in_rule_body if { + module := ast.with_rego_v1(` + rule if { + foo := "foo" + + bar := foo + } + `) + + r := rule.report with input as module + r == {{ + "category": "style", + "description": "Pointless reassignment of variable", + "level": "error", + "location": {"col": 7, "file": "policy.rego", "row": 9, "text": "\t\tbar := foo"}, + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/pointless-reassignment", "style"), + }], + "title": "pointless-reassignment", + }} +} diff --git a/docs/rules/style/pointless-reassignment.md b/docs/rules/style/pointless-reassignment.md new file mode 100644 index 00000000..fbe769e7 --- /dev/null +++ b/docs/rules/style/pointless-reassignment.md @@ -0,0 +1,62 @@ +# pointless-reassignment + +**Summary**: Pointless reassignment of variable + +**Category**: Style + +**Avoid** +```rego +package policy + +allow if { + users := all_users + any_admin(users) +} +``` + +**Prefer** +```rego +package policy + +allow if { + any_admin(all_users) +} +``` + +## Rationale + +Values and variables are immutable in Rego, so reassigning the value of one variable to another only adds noise. + +## Exceptions + +Reassigning the value of a long reference often helps readability, and especially so when it needs to be referenced +multiple times: + +```rego +package policy + +allow if { + users := input.context.permissions.users + any_admin(users) +} +``` + +This rule does not consider such assignments violations. + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + style: + pointless-reassignment: + # one of "error", "warning", "ignore" + level: error +``` + +## Community + +If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules, +or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community +[Slack](https://communityinviter.com/apps/styracommunity/signup)! diff --git a/e2e/testdata/violations/most_violations.rego b/e2e/testdata/violations/most_violations.rego index 56ffcbc7..44c8554f 100644 --- a/e2e/testdata/violations/most_violations.rego +++ b/e2e/testdata/violations/most_violations.rego @@ -214,6 +214,8 @@ yoda_condition if { "foo" == input.bar } +pointless_reassignment := yoda_condition + ### Testing ### # this will also trigger the test-outside-test-package rule diff --git a/internal/embeds/templates/builtin/builtin_test.rego.tpl b/internal/embeds/templates/builtin/builtin_test.rego.tpl index 4e4b9969..710cac82 100644 --- a/internal/embeds/templates/builtin/builtin_test.rego.tpl +++ b/internal/embeds/templates/builtin/builtin_test.rego.tpl @@ -24,6 +24,6 @@ test_rule_named_foo_not_allowed if { "description": "documentation", "ref": config.docs.resolve_url("$baseUrl/$category/{{.NameOriginal}}", "{{.Category}}"), }], - "title": "{{.NameOriginal}}" + "title": "{{.NameOriginal}}", }} }