Rule: default-over-else (#360)

Prefer default assignment over catch-all `else`. This rule
was originally meant to endorse the new default function construct,
but having tested it out thoroughly for this rule, I'm now more
inclined to have it flagged. I find that counter-intuitive, and
as such more likely to cause harm than good. Those who disagree
may configure the `prefer-default-functions` option. Either way,
default assignment over the equivalent `else` feels more idiomatic
to me, so the rule feels relevant despite not doing what originally
intended.

Fixes #211

Signed-off-by: Anders Eknert <anders@styra.com>

README.md

@@ -163,6 +163,7 @@ The following rules are currently available:
 | imports   | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import)         | Redundant import of data                                  |
 | style     | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix)   | Avoid get_ and list_ prefix for rules and functions       |
 | style     | [chained-rule-body](https://docs.styra.com/regal/rules/style/chained-rule-body)                   | Avoid chaining rule bodies                                |
+| style     | [default-over-else](https://docs.styra.com/regal/rules/style/default-over-else)                   | Prefer default assignment over fallback else              |
 | style     | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata)                   | Detached metadata annotation                              |
 | style     | [external-reference](https://docs.styra.com/regal/rules/style/external-reference)                 | Reference to input, data or rule ref in function body     |
 | style     | [file-length](https://docs.styra.com/regal/rules/style/file-length)                               | Max file length exceeded                                  |

bundle/regal/ast.rego

@@ -5,6 +5,21 @@ import future.keywords.every
 import future.keywords.if
 import future.keywords.in
 
+scalar_types := {"boolean", "null", "number", "string"}
+
+# regal ignore:external-reference
+is_constant(value) if value.type in scalar_types
+
+is_constant(value) if {
+	value.type in {"array", "object"}
+	not has_var(value)
+}
+
+has_var(value) if {
+	walk(value.value, [_, term])
+	term.type == "var"
+}
+
 builtin_names := object.keys(data.regal.opa.builtins)
 
 # METADATA

bundle/regal/config/config.rego

@@ -38,6 +38,8 @@ empty_level_to_provided(category, title, conf) := object.union(
 	conf.level == ""
 } else := object.union(conf, {"level": "error"}) if conf.level == ""
 
+default for_rule(_, _) := {"level": "error"}
+
 # METADATA
 # description: |
 #   Returns the configuration applied (i.e. the provided configuration
@@ -50,19 +52,14 @@ for_rule(category, title) := _with_level(category, title, "ignore") if {
 } else := c if {
 	m := merged_config.rules[category][title]
 	c := object.union(m, {"level": rule_level(m)})
-} else := {"level": "error"} if {
-	# regal ignore:external-reference
-	not merged_config.rules[category][title]
 }
 
 _with_level(category, title, level) := c if {
 	m := merged_config.rules[category][title]
 	c := object.union(m, {"level": level})
 } else := {"level": level}
 
-rule_level(cfg) := "error" if {
-	not cfg.level
-}
+default rule_level(_) := "error"
 
 rule_level(cfg) := cfg.level
 

bundle/regal/config/provided/data.yaml

@@ -49,6 +49,9 @@ rules:
       level: error
     chained-rule-body:
       level: error
+    default-over-else:
+      level: error
+      prefer-default-functions: false
     detached-metadata:
       level: error
     external-reference:

bundle/regal/rules/bugs/constant_condition.rego

@@ -6,6 +6,7 @@ import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
+import data.regal.ast
 import data.regal.result
 
 # NOTE: The constant condition checks currently don't do nesting!
@@ -14,12 +15,6 @@ import data.regal.result
 
 _operators := {"equal", "gt", "gte", "lt", "lte", "neq"}
 
-# We could probably include arrays and objects too, as a single compound value
-# is not very useful, but it's not as clear cut as scalars, as you could have
-# something like {"a": foo(input.x) == "bar"} which is not a constant condition,
-# however meaningless it may be. Maybe consider for another rule?
-_scalars := {"boolean", "null", "number", "string"}
-
 _rules_with_bodies := [rule |
 	some rule in input.rules
 	not probably_no_body(rule)
@@ -36,7 +31,11 @@ report contains violation if {
 	some rule in _rules_with_bodies
 	some expr in rule.body
 
-	expr.terms.type in _scalars
+	# We could probably include arrays and objects too, as a single compound value
+	# is not very useful, but it's not as clear cut as scalars, as you could have
+	# something like {"a": foo(input.x) == "bar"} which is not a constant condition,
+	# however meaningless it may be. Maybe consider for another rule?
+	expr.terms.type in ast.scalar_types
 
 	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }
@@ -48,8 +47,8 @@ report contains violation if {
 	expr.terms[0].value[0].type == "var"
 	expr.terms[0].value[0].value in _operators
 
-	expr.terms[1].type in _scalars
-	expr.terms[2].type in _scalars
+	expr.terms[1].type in ast.scalar_types
+	expr.terms[2].type in ast.scalar_types
 
 	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }

bundle/regal/rules/style/default_over_else.rego

@@ -0,0 +1,42 @@
+# METADATA
+# description: Prefer default assignment over fallback else
+package regal.rules.style["default-over-else"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.config
+import data.regal.result
+
+cfg := config.for_rule("style", "default-over-else")
+
+report contains violation if {
+	some rule in considered_rules
+
+	# walking is expensive but necessary here, since there could be
+	# any number of `else` clauses nested below. no need to traverse
+	# the rule if there isn't a single `else` present though!
+	rule["else"]
+
+	walk(rule, [_, value])
+
+	# quoting is needed as `else` is a keyword
+	else_body := value["else"].body
+	else_head := value["else"].head
+
+	# we don't know for sure, but if all that's in the body is an empty
+	# `true`, it's likely an implicit body (i.e. one not printed)
+	count(else_body) == 1
+	else_body[0].terms.type == "boolean"
+	else_body[0].terms.value == true
+
+	ast.is_constant(else_head.value)
+
+	violation := result.fail(rego.metadata.chain(), result.location(else_head))
+}
+
+considered_rules := input.rules if cfg["prefer-default-functions"] == true
+
+considered_rules := [rule | some rule in input.rules; not rule.head.args] if not cfg["prefer-default-functions"]

bundle/regal/rules/style/default_over_else_test.rego

@@ -0,0 +1,108 @@
+package regal.rules.style["default-over-else_test"]
+
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.style["default-over-else"] as rule
+
+test_fail_conditionless_else_simple_rule if {
+	module := ast.policy(`
+	x := 1 {
+		input.x
+	} else := 2 {
+		input.y
+	} else := 3
+	`)
+
+	r := rule.report with input as module
+	r == with_location({"col": 4, "file": "policy.rego", "row": 8, "text": "\t} else := 3"})
+}
+
+test_fail_conditionless_else_object_assignment if {
+	module := ast.policy(`
+	x := {"foo": "bar"} {
+		input.x
+	} else := {"bar": "foo"}
+	`)
+
+	r := rule.report with input as module
+	r == with_location({"col": 4, "file": "policy.rego", "row": 6, "text": "\t} else := {\"bar\": \"foo\"}"})
+}
+
+test_success_conditionless_else_not_constant if {
+	module := ast.policy(`
+	y := input.y
+
+	x := {"foo": "bar"} {
+		input.x
+	} else := {"bar": y}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_conditionless_else_input_ref if {
+	module := ast.policy(`
+	x := {"foo": "bar"} {
+		input.x
+	} else := input.foo
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_conditionless_else_custom_function if {
+	module := ast.policy(`
+	x(y) := y {
+		input.foo
+	} else := 1
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_fail_conditionless_else_custom_function_prefer_default_functions if {
+	module := ast.policy(`
+	x(y) := y {
+		input.foo
+	} else := 1
+	`)
+
+	r := rule.report with input as module with config.for_rule as {
+		"level": "error",
+		"prefer-default-functions": true,
+	}
+	r == with_location({"col": 4, "file": "policy.rego", "row": 6, "text": "\t} else := 1"})
+}
+
+test_success_conditionless_else_custom_function_not_constant if {
+	module := ast.policy(`
+	x(y) := y + 1 {
+		input.foo
+	} else := y
+	`)
+
+	r := rule.report with input as module with config.for_rule as {
+		"level": "error",
+		"prefer-default-functions": true,
+	}
+	r == set()
+}
+
+with_location(location) := {{
+	"category": "style",
+	"description": "Prefer default assignment over fallback else",
+	"level": "error",
+	"location": location,
+	"related_resources": [{
+		"description": "documentation",
+		"ref": config.docs.resolve_url("$baseUrl/$category/default-over-else", "style"),
+	}],
+	"title": "default-over-else",
+}}

bundle/regal/rules/style/rule_length.rego

@@ -6,7 +6,6 @@ import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.ast
 import data.regal.config
 import data.regal.result
 

bundle/regal/rules/style/rule_length_test.rego

@@ -3,7 +3,6 @@ package regal.rules.style["rule-length_test"]
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.ast
 import data.regal.config
 
 import data.regal.rules.style["rule-length"] as rule

docs/rules/style/default-over-else.md

@@ -0,0 +1,106 @@
+# default-over-else
+
+**Summary**: Prefer default assignment over fallback `else`
+
+**Category**: Style
+
+**Avoid**
+```rego
+package policy
+
+import future.keywords.if
+
+permisisions := ["read", "write"] if {
+    input.user == "admin"
+} else := ["read"]
+```
+
+**Prefer**
+```rego
+package policy
+
+import future.keywords.if
+
+default permisisions := ["read"]
+
+permisisions := ["read", "write"] if {
+    input.user == "admin"
+}
+```
+
+## Rationale
+
+The `else` keyword has a single purpose in Rego — to allow a policy author to control the order of evaluation. Whether
+several `else`-clauses are chained or not, it's common to use a last "fallback" `else` to cover all cases not covered by
+the conditions in the preceding `else`-bodies. A kind of "catch all", or "default" condition. This is useful, but Rego
+arguably provides a more idiomatic construct for default assignment: the
+[default keyword](https://www.openpolicyagent.org/docs/latest/policy-language/#default-keyword).
+
+While the end result is the same, default assignment has the benefit of more clearly — and **before** the conditional
+assignments — communicating what the *safe* option is. This is particularly important for
+[entrypoint](https://docs.styra.com/regal/rules/idiomatic/no-defined-entrypoint) rules, where the
+default value of a rule is a part of the rule's contract.
+
+## Exceptions
+
+OPA [v0.55.0](https://github.com/open-policy-agent/opa/releases/tag/v0.55.0) introduced support for the default keyword
+for custom functions. This means that `else` fallbacks in functions may now be rewritten to use default assignment too:
+
+```rego
+package policy
+
+first_name(full_name) := split(full_name, " ")[0] if {
+    full_name != ""
+} else := "Unknown"
+```
+
+Could now be written as:
+
+```rego
+package policy
+
+default first_name(_) := "Unknown"
+
+first_name(full_name) := split(full_name, " ")[0] if {
+    full_name != ""
+}
+```
+
+Default value assignment for functions however come with a big caveat — the default case will only be triggered if all
+arguments passed to the function evaluate to a *defined value*. Thus, calling the `first_name` function from our above
+example is **not** guaranteed to return a value of `"Unknown"`:
+
+```rego
+# undefined if `input.name` is undefined
+fname := first_name(input.name)
+```
+
+Whether deemed acceptable or not, this differs enough from default assignment of rules to make this preference opt-in 
+rather than opt-out. Use the `prefer-default-functions` configuration option to control whether `default` assignment
+should be preferred over `else` fallbacks also for custom functions. The default value (no pun intended!) of this config
+option is `false`.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  style:
+    default-over-else:
+      # one of "error", "warning", "ignore"
+      level: error
+      # whether to prefer default assignment over
+      # `else` fallbacks for custom functions
+      prefer-default-functions: false
+```
+
+## Related Resources
+
+- OPA Docs: [Default Keyword](https://www.openpolicyagent.org/docs/latest/policy-language/#default-keyword)
+
+## 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)!