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 <[email protected]>

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.
 

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

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:

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"]

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})}

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"),

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)!