Rule: default-over-not (#521)

Also, start to break up ast.rego into more files, as we started
to violate the `file-length` rule. We should continue on this work
in follow-up PRs, but for now just make sure we aren't breaking
our own rules.

Fixes #517

Signed-off-by: Anders Eknert <[email protected]>

README.md

@@ -225,6 +225,7 @@ The following rules are currently available:
 | 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     | [default-over-not](https://docs.styra.com/regal/rules/style/default-over-not)                       | Prefer default assignment over negated condition          |
 | 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 → bundle/regal/ast/ast.rego

@@ -326,6 +326,24 @@ static_ref(ref) if every t in array.slice(ref.value, 1, count(ref.value)) {
 	t.type != "var"
 }
 
+static_rule_ref(ref) if every t in array.slice(ref, 1, count(ref)) {
+	t.type != "var"
+}
+
+# METADATA
+# description: |
+#   return the name of a rule if, and only if it only has static parts with
+#   no vars. This could be "username", or "user.name", but not "user[name]"
+static_rule_name(rule) := rule.head.ref[0].value if count(rule.head.ref) == 1
+
+static_rule_name(rule) := concat(".", array.concat([rule.head.ref[0].value], [ref.value |
+	some i, ref in rule.head.ref
+	i > 0
+])) if {
+	count(rule.head.ref) > 1
+	static_rule_ref(rule.head.ref)
+}
+
 # METADATA
 # description: provides a set of all built-in function calls made in input policy
 builtin_functions_called contains name if {
@@ -415,82 +433,3 @@ is_chained_rule_body(rule, lines) if {
 
 	startswith(col_text, "{")
 }
-
-default imports := []
-
-# METADATA
-# description: |
-#   same as input.imports but with a default value (`[]`), making
-#   it safe to refer to without risk of halting evaluation
-imports := input.imports
-
-imports_has_path(imports, path) if {
-	some imp in imports
-
-	_arr(imp) == path
-}
-
-# METADATA
-# description: |
-#   returns whether a keyword is imported in the policy, either explicitly
-#   like "future.keywords.if" or implicitly like "future.keywords" or "rego.v1"
-imports_keyword(imports, keyword) if {
-	some imp in imports
-
-	_has_keyword(_arr(imp), keyword)
-}
-
-_arr(xs) := [y.value | some y in xs.path.value]
-
-_has_keyword(["future", "keywords"], _)
-
-_has_keyword(["future", "keywords", keyword], keyword)
-
-_has_keyword(["rego", "v1"], _)
-
-comments_decoded := [decoded |
-	some comment in input.comments
-	decoded := object.union(comment, {"Text": base64.decode(comment.Text)})
-]
-
-comments["blocks"] := comment_blocks(comments_decoded)
-
-comments["metadata_attributes"] := {
-	"scope",
-	"title",
-	"description",
-	"related_resources",
-	"authors",
-	"organizations",
-	"schemas",
-	"entrypoint",
-	"custom",
-}
-
-comment_blocks(comments) := [partition |
-	rows := [row |
-		some comment in comments
-		row := comment.Location.row
-	]
-	breaks := _splits(rows)
-
-	some j, k in breaks
-	partition := array.slice(
-		comments,
-		breaks[j - 1] + 1,
-		k + 1,
-	)
-]
-
-_splits(xs) := array.concat(
-	array.concat(
-		# [-1] ++ [ all indices where there's a step larger than one ] ++ [length of xs]
-		# the -1 is because we're adding +1 in array.slice
-		[-1],
-		[i |
-			some i in numbers.range(0, count(xs) - 1)
-			xs[i + 1] != xs[i] + 1
-		],
-	),
-	[count(xs)],
-)

bundle/regal/ast_test.rego → bundle/regal/ast/ast_test.rego

@@ -291,3 +291,18 @@ test_all_refs if {
 
 	text_refs == {":=", "data.foo.bar", "data.foo.bax", "data.foo.baz"}
 }
+
+test_static_rule_name_one_part if {
+	rule := {"head": {"ref": [{"type": "var", "value": "username"}]}}
+	ast.static_rule_name(rule) == "username"
+}
+
+test_static_rule_name_multi_part if {
+	rule := {"head": {"ref": [{"type": "var", "value": "user"}, {"type": "string", "value": "name"}]}}
+	ast.static_rule_name(rule) == "user.name"
+}
+
+test_static_rule_name_var_part if {
+	rule := {"head": {"ref": [{"type": "var", "value": "user"}, {"type": "var", "value": "name"}]}}
+	not ast.static_rule_name(rule)
+}

bundle/regal/ast/comments.rego

@@ -0,0 +1,50 @@
+package regal.ast
+
+import rego.v1
+
+comments_decoded := [decoded |
+	some comment in input.comments
+	decoded := object.union(comment, {"Text": base64.decode(comment.Text)})
+]
+
+comments["blocks"] := comment_blocks(comments_decoded)
+
+comments["metadata_attributes"] := {
+	"scope",
+	"title",
+	"description",
+	"related_resources",
+	"authors",
+	"organizations",
+	"schemas",
+	"entrypoint",
+	"custom",
+}
+
+comment_blocks(comments) := [partition |
+	rows := [row |
+		some comment in comments
+		row := comment.Location.row
+	]
+	breaks := _splits(rows)
+
+	some j, k in breaks
+	partition := array.slice(
+		comments,
+		breaks[j - 1] + 1,
+		k + 1,
+	)
+]
+
+_splits(xs) := array.concat(
+	array.concat(
+		# [-1] ++ [ all indices where there's a step larger than one ] ++ [length of xs]
+		# the -1 is because we're adding +1 in array.slice
+		[-1],
+		[i |
+			some i in numbers.range(0, count(xs) - 1)
+			xs[i + 1] != xs[i] + 1
+		],
+	),
+	[count(xs)],
+)

bundle/regal/ast/imports.rego

@@ -0,0 +1,35 @@
+package regal.ast
+
+import rego.v1
+
+default imports := []
+
+# METADATA
+# description: |
+#   same as input.imports but with a default value (`[]`), making
+#   it safe to refer to without risk of halting evaluation
+imports := input.imports
+
+imports_has_path(imports, path) if {
+	some imp in imports
+
+	_arr(imp) == path
+}
+
+# METADATA
+# description: |
+#   returns whether a keyword is imported in the policy, either explicitly
+#   like "future.keywords.if" or implicitly like "future.keywords" or "rego.v1"
+imports_keyword(imports, keyword) if {
+	some imp in imports
+
+	_has_keyword(_arr(imp), keyword)
+}
+
+_arr(xs) := [y.value | some y in xs.path.value]
+
+_has_keyword(["future", "keywords"], _)
+
+_has_keyword(["future", "keywords", keyword], keyword)
+
+_has_keyword(["rego", "v1"], _)

bundle/regal/config/provided/data.yaml

@@ -81,6 +81,8 @@ rules:
     default-over-else:
       level: error
       prefer-default-functions: false
+    default-over-not:
+      level: error
     detached-metadata:
       level: error
     external-reference:

bundle/regal/rules/style/default_over_not.rego

@@ -0,0 +1,41 @@
+# METADATA
+# description: Prefer default assignment over negated condition
+package regal.rules.style["default-over-not"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	some i, rule in ast.rules
+
+	# part 1 — find unconditional static ref assignment
+	# example: `rule := input.foo`
+
+	not rule["default"]
+	ast.generated_body(rule)
+
+	name := ast.static_rule_name(rule)
+	value := rule.head.value
+
+	ast.static_ref(value)
+
+	# part 2 - find corresponding assignment of constant on negated condition
+	# example: `rule := 1 if not input.foo`
+
+	sibling_rules := [sibling |
+		some j, sibling in ast.rules
+		i != j
+		ast.static_rule_name(sibling) == name
+	]
+
+	some sibling in sibling_rules
+
+	ast.is_constant(sibling.head.value)
+	count(sibling.body) == 1
+	sibling.body[0].negated
+	ast.ref_to_string(sibling.body[0].terms.value) == ast.ref_to_string(value.value)
+
+	violation := result.fail(rego.metadata.chain(), result.location(sibling.body[0]))
+}

bundle/regal/rules/style/default_over_not_test.rego

@@ -0,0 +1,45 @@
+package regal.rules.style["default-over-not_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.style["default-over-not"] as rule
+
+test_fail_default_over_not if {
+	module := ast.with_rego_v1(`
+	user := input.user
+	user := "foo" if not input.user
+	`)
+	r := rule.report with input as module
+	r == {{
+		"category": "style",
+		"description": "Prefer default assignment over negated condition",
+		"level": "error",
+		"location": {"col": 19, "file": "policy.rego", "row": 7, "text": "\tuser := \"foo\" if not input.user"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/default-over-not", "style"),
+		}],
+		"title": "default-over-not",
+	}}
+}
+
+test_success_non_constant_value if {
+	module := ast.with_rego_v1(`
+	user := input.user
+	user := var if not input.user
+	`)
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_var_in_ref if {
+	module := ast.with_rego_v1(`
+	user := input[x].user
+	user := "foo" if not input[x].user
+	`)
+	r := rule.report with input as module
+	r == set()
+}

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

@@ -0,0 +1,54 @@
+# default-over-not
+
+**Summary**: Prefer default assignment over negated condition
+
+**Category**: Style
+
+**Avoid**
+```rego
+package policy
+
+import future.keywords.if
+
+username := input.user.name
+
+username := "anonymous" if not input.user.name
+```
+
+**Prefer**
+```rego
+package policy
+
+default username := "anonymous"
+
+username := input.user.name
+```
+
+## Rationale
+
+While both forms are valid, using the `default` keyword to assign a constant value in the fallback case better
+communicates intent, avoids negation where it isn't needed, and requires less instructions to evaluate. Note that this
+rule only covers simple cases where one rule assigns the "happy" path, and another rule assigns on the same condition
+negated. This is by design, as using `not` and negation may very well be the right choice for more complex cases!
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  style:
+    default-over-not:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## 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)!

e2e/testdata/violations/most_violations.rego

@@ -177,6 +177,10 @@ default_over_else := 1 if {
 	input.x
 } else := 3
 
+default_over_not := input.foo
+
+default_over_not := "foo" if not input.foo
+
 unnecessary_some if {
 	some "x" in ["x"]
 }