From 97c05a485411a9c01c61698ed122184e71b5968e Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Tue, 24 Oct 2023 15:31:59 +0200 Subject: [PATCH] Rule: `one-liner-rule` Custom rule to suggest one-liner syntax where possible. Taking into account: * Line length of shortened rule must not exceed max line length (configurable, default 120) * Comments in rule body cancels suggestion, as we can't turn those to one-liners without changing "semantics" of the comment Fixes #420 Signed-off-by: Anders Eknert --- README.md | 6 +- bundle/regal/ast.rego | 12 +- bundle/regal/config/provided/data.yaml | 3 + bundle/regal/rules/custom/one_liner_rule.rego | 95 +++++++++++ .../rules/custom/one_liner_rule_test.rego | 153 ++++++++++++++++++ .../custom/prefer_value_in_head_test.rego | 1 - docs/rules/custom/one-liner-rule.md | 60 +++++++ 7 files changed, 318 insertions(+), 12 deletions(-) create mode 100644 bundle/regal/rules/custom/one_liner_rule.rego create mode 100644 bundle/regal/rules/custom/one_liner_rule_test.rego create mode 100644 docs/rules/custom/one-liner-rule.md diff --git a/README.md b/README.md index b80febfc..3f11da7d 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,7 @@ The following rules are currently available: | bugs | [zero-arity-function](https://docs.styra.com/regal/rules/bugs/zero-arity-function) | Avoid functions without args | | custom | [forbidden-function-call](https://docs.styra.com/regal/rules/custom/forbidden-function-call) | Forbidden function call | | custom | [naming-convention](https://docs.styra.com/regal/rules/custom/naming-convention) | Naming convention violation | +| custom | [one-liner-rule](https://docs.styra.com/regal/rules/custom/one-liner-rule) | Rule body could be made a one-liner | | custom | [prefer-value-in-head](https://docs.styra.com/regal/rules/custom/prefer-value-in-head) | Prefer value in rule head | | idiomatic | [custom-has-key-construct](https://docs.styra.com/regal/rules/idiomatic/custom-has-key-construct) | Custom function may be replaced by `in` and `object.keys` | | idiomatic | [custom-in-construct](https://docs.styra.com/regal/rules/idiomatic/custom-in-construct) | Custom function may be replaced by `in` keyword | @@ -256,8 +257,9 @@ your project, team or organization. This typically includes things like naming c ensure that, for example, all package names adhere to an organizational standard, like having a prefix matching the organization name. -Since these rules require configuration provided by the user, they are disabled by default. In order to enable them, -see the configuration options available for each rule for how to configure them according to your requirements. +Since these rules require configuration provided by the user, or are more opinionated than other rules, they are +disabled by default. In order to enable them, see the configuration options available for each rule for how to configure +them according to your requirements. For more advanced requirements, see the guide on writing [custom rules](/docs/custom-rules.md) in Rego. diff --git a/bundle/regal/ast.rego b/bundle/regal/ast.rego index 5121b0d8..6bf6844b 100644 --- a/bundle/regal/ast.rego +++ b/bundle/regal/ast.rego @@ -201,9 +201,7 @@ _find_vars(path, value, last) := _find_assign_vars(path, value) if { value[0].value[0].value == "assign" } -_find_vars(_, value, _) := find_ref_vars(value) if { - value.type == "ref" -} +_find_vars(_, value, _) := find_ref_vars(value) if value.type == "ref" _find_vars(path, value, last) := _find_some_in_decl_vars(path, value) if { last == "symbols" @@ -224,9 +222,7 @@ _find_vars(_, value, _) := _find_set_or_array_comprehension_vars(value) if { value.type in {"setcomprehension", "arraycomprehension"} } -_find_vars(_, value, _) := _find_object_comprehension_vars(value) if { - value.type == "objectcomprehension" -} +_find_vars(_, value, _) := _find_object_comprehension_vars(value) if value.type == "objectcomprehension" find_some_decl_vars(rule) := [var | walk(rule, [path, value]) @@ -264,9 +260,7 @@ find_vars_in_local_scope(rule, location) := [var | _before_location(var, location) ] -_before_location(var, location) if { - var.location.row < location.row -} +_before_location(var, location) if var.location.row < location.row _before_location(var, location) if { var.location.row == location.row diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index 2336dbc9..49992c25 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/bundle/regal/config/provided/data.yaml @@ -22,6 +22,9 @@ rules: level: ignore naming-convention: level: ignore + one-liner-rule: + level: ignore + max-line-length: 120 prefer-value-in-head: level: ignore idiomatic: diff --git a/bundle/regal/rules/custom/one_liner_rule.rego b/bundle/regal/rules/custom/one_liner_rule.rego new file mode 100644 index 00000000..700b98a3 --- /dev/null +++ b/bundle/regal/rules/custom/one_liner_rule.rego @@ -0,0 +1,95 @@ +# METADATA +# description: Rule body could be made a one-liner +package regal.rules.custom["one-liner-rule"] + +import future.keywords.contains +import future.keywords.if +import future.keywords.in + +import data.regal.config +import data.regal.result + +# No need to traverse rules here if we're not importing `if` +imports_if if { + some imp in input.imports + + contains_if(imp) +} + +contains_if(imp) if { + count(imp.path.value) == 2 + imp.path.value[0].value == "future" + imp.path.value[1].value == "keywords" +} + +contains_if(imp) if { + count(imp.path.value) == 3 + imp.path.value[0].value == "future" + imp.path.value[1].value == "keywords" + imp.path.value[2].value == "if" +} + +contains_if(imp) if { + count(imp.path.value) == 2 + imp.path.value[0].value == "rego" + imp.path.value[1].value == "v1" +} + +cfg := config.for_rule("custom", "one-liner-rule") + +# regal ignore:rule-length +report contains violation if { + imports_if + + # Note: this covers both rules and functions, which is what we want here + some rule in input.rules + + # Bail out of rules with else for now. It is possible that they can be made + # one-liners, but they'll often be longer than the preferred line length + # We can come back to this later, but for now let's just make this an + # exception documented for this rule + not rule["else"] + + # Single expression in body required for one-liner + count(rule.body) == 1 + + # Note that this will give us the text representation of the whole rule, + # which we'll need as the "if" is only visible here ¯\_(ツ)_/¯ + text := base64.decode(rule.location.text) + lines := [line | + some s in split(text, "\n") + line := trim_space(s) + ] + + # Technically, the `if` could be on another line, but who would do that? + regex.match(`\s+if`, lines[0]) + rule_body_brackets(lines) + + # ideally we'd take style preference into account but for now assume tab == 4 spaces + # then just add the sum of the line counts minus the removed '{' character + # redundant parens added by `opa fmt` :/ + ((4 + count(lines[0])) + count(lines[1])) - 1 < max_line_length + + not comment_in_body(rule, object.get(input, "comments", []), lines) + + violation := result.fail(rego.metadata.chain(), result.location(rule.head)) +} + +# K&R style +rule_body_brackets(lines) if endswith(lines[0], "{") + +# Allman style +rule_body_brackets(lines) if { + not endswith(lines[0], "{") + startswith(lines[1], "{") +} + +comment_in_body(rule, comments, lines) if { + some comment in comments + comment.Location.row > rule.location.row + comment.Location.row < rule.location.row + count(lines) +} + +default max_line_length := 120 + +max_line_length := cfg["max-line-length"] diff --git a/bundle/regal/rules/custom/one_liner_rule_test.rego b/bundle/regal/rules/custom/one_liner_rule_test.rego new file mode 100644 index 00000000..210c38ce --- /dev/null +++ b/bundle/regal/rules/custom/one_liner_rule_test.rego @@ -0,0 +1,153 @@ +package regal.rules.custom["one-liner-rule_test"] + +import future.keywords.if +import future.keywords.in + +import data.regal.ast +import data.regal.config + +import data.regal.rules.custom["one-liner-rule"] as rule + +test_fail_could_be_one_liner if { + module := ast.policy(` + + import future.keywords.if + + allow if { + input.yes + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == expected_with_location({"col": 2, "file": "policy.rego", "row": 7, "text": "\tallow if {"}) +} + +test_fail_could_be_one_liner_all_keywords if { + module := ast.policy(` + + import future.keywords + + allow if { + input.yes + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == expected_with_location({"col": 2, "file": "policy.rego", "row": 7, "text": "\tallow if {"}) +} + +test_fail_could_be_one_liner_allman_style if { + module := ast.policy(` + + import future.keywords.if + + allow if + { + input.yes + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == expected_with_location({"col": 2, "file": "policy.rego", "row": 7, "text": "\tallow if"}) +} + +test_success_if_not_imported if { + module := ast.policy(` + allow := true if { + 1 == 1 + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +test_success_too_long_for_a_one_liner if { + module := ast.with_future_keywords(` + rule := "quite a long text up here" if { + some_really_long_rule_name_in_fact_53_characters_long == another_long_rule_but_only_45_characters_long + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +test_success_too_long_for_a_one_liner_configured_line_length if { + module := ast.with_future_keywords(` + rule if { + some_really_long_rule_name_in_fact_53_characters_long + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error", "max-line-length": 50} + r == set() +} + +test_success_no_one_liner_comment_in_rule_body if { + module := ast.with_future_keywords(` + no_one_liner if { + # Surely one equals one + 1 == 1 + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +test_success_no_one_liner_comment_in_rule_body_same_line if { + module := ast.with_future_keywords(` + no_one_liner if { + 1 == 1 # Surely one equals one + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +test_success_no_one_liner_comment_in_rule_body_line_below if { + module := ast.with_future_keywords(` + no_one_liner if { + 1 == 1 + # Surely one equals one + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +# This will have to be gated with capability version +# later as this will be forced from 1.0 +test_success_does_not_use_if if { + module := ast.policy(` + allow { + 1 == 1 + } + `) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +test_success_already_a_one_liner if { + module := ast.with_future_keywords(`allow if 1 == 1`) + + r := rule.report with input as module with config.for_rule as {"level": "error"} + r == set() +} + +expected := { + "category": "custom", + "description": "Rule body could be made a one-liner", + "level": "error", + "related_resources": [{ + "description": "documentation", + "ref": config.docs.resolve_url("$baseUrl/$category/one-liner-rule", "custom"), + }], + "title": "one-liner-rule", +} + +expected_with_location(location) := {object.union(expected, {"location": location})} diff --git a/bundle/regal/rules/custom/prefer_value_in_head_test.rego b/bundle/regal/rules/custom/prefer_value_in_head_test.rego index f0d0cb8b..23e7a287 100644 --- a/bundle/regal/rules/custom/prefer_value_in_head_test.rego +++ b/bundle/regal/rules/custom/prefer_value_in_head_test.rego @@ -96,7 +96,6 @@ expected := { "category": "custom", "description": "Prefer value in rule head", "level": "error", - "location": {"col": 1, "file": "policy.rego", "row": 3, "text": "foo := true"}, "related_resources": [{ "description": "documentation", "ref": config.docs.resolve_url("$baseUrl/$category/prefer-value-in-head", "custom"), diff --git a/docs/rules/custom/one-liner-rule.md b/docs/rules/custom/one-liner-rule.md new file mode 100644 index 00000000..b18662fc --- /dev/null +++ b/docs/rules/custom/one-liner-rule.md @@ -0,0 +1,60 @@ +# one-liner-rule + +**Summary**: Rule body could be made a one-liner + +**Category**: Custom + +**Avoid** +```rego +package policy + +import future.keywords.if + +allow if { + is_admin +} + +is_admin if { + "admin" in input.user.roles +} +``` + +**Prefer** +```rego +package policy + +import future.keywords.if + +allow if is_admin + +is_admin if "admin" in input.user.roles +``` + +## Rationale + +Rules with only a single expression in the body may omit the curly braces around the body, and be written as a +one-liner. This makes simple rules read more like English, and will have more rules fit on the screen. + +As with other rules in the `custom` category, this is not necessarily a general recommendation, but a style preference +teams or organizations might want to standardize on. As such, it must be enabled via configuration. + +## Configuration Options + +This linter rule provides the following configuration options: + +```yaml +rules: + custom: + one-liner-rule: + # note that all rules in the "custom" category are disabled by default + # (i.e. level "ignore") + # + # 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)!