Rule: if-empty-body

Flag empty "body" like `{}` as that's no longer
considered a body.

Also some light refactoring here, breaking out
capabilities checks to a separate package. More to
happen here soon!

Fixes #451

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

README.md

@@ -185,6 +185,7 @@ The following rules are currently available:
 | Category  |                                                Title                                                |                        Description                        |
 |-----------|-----------------------------------------------------------------------------------------------------|-----------------------------------------------------------|
 | bugs      | [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition)                    | Constant condition                                        |
+| bugs      | [if-empty-object](https://docs.styra.com/regal/rules/bugs/if-empty-object)                          | Empty object following `if`                               |
 | bugs      | [inconsistent-args](https://docs.styra.com/regal/rules/bugs/inconsistent-args)                      | Inconsistently named function arguments                   |
 | bugs      | [invalid-metadata-attribute](https://docs.styra.com/regal/rules/bugs/invalid-metadata-attribute)    | Invalid attribute in metadata annotation                  |
 | bugs      | [not-equals-in-loop](https://docs.styra.com/regal/rules/bugs/not-equals-in-loop)                    | Use of != in loop                                         |

bundle/regal/capabilities.rego

@@ -0,0 +1,20 @@
+package regal.capabilities
+
+import data.regal.config
+
+import future.keywords.if
+import future.keywords.in
+
+default provided := {}
+
+# METADATA
+# description: |
+#   The capabilities object for Regal itself. Use `config.capabilities`
+#   to get the capabilities for the target environment (i.e. the policies
+#   getting linted).
+provided := data.internal.capabilities
+
+# if if if!
+has_if if "if" in config.capabilities.future_keywords
+
+has_if if "rego_v1_import" in config.capabilities.features

bundle/regal/config/provided/data.yaml

@@ -2,6 +2,8 @@ rules:
   bugs:
     constant-condition:
       level: error
+    if-empty-object:
+      level: error
     inconsistent-args:
       level: error
     invalid-metadata-attribute:

bundle/regal/regal.rego

@@ -7,12 +7,3 @@
 # schemas:
 # - input: schema.regal.ast
 package regal
-
-default capabilities := {}
-
-# METADATA
-# description: |
-#   The capabilities object for Regal itself. Use `config.capabilities`
-#   to get the capabilities for the target environment (i.e. the policies
-#   getting linted).
-capabilities := data.internal.capabilities

bundle/regal/rules/bugs/if_empty_object.rego

@@ -0,0 +1,27 @@
+# METADATA
+# description: Empty object following `if`
+package regal.rules.bugs["if-empty-object"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.capabilities
+import data.regal.result
+
+# METADATA
+# description: Missing capability for keyword `if`
+# custom:
+#   severity: warning
+notices contains result.notice(rego.metadata.chain()) if not capabilities.has_if
+
+report contains violation if {
+	some rule in input.rules
+
+	count(rule.body) == 1
+
+	rule.body[0].terms.type == "object"
+	rule.body[0].terms.value == []
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule))
+}

bundle/regal/rules/bugs/if_empty_object_test.rego

@@ -0,0 +1,33 @@
+package regal.rules.bugs["if-empty-object_test"]
+
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.bugs["if-empty-object"] as rule
+
+test_fail_if_empty_object if {
+	module := ast.with_future_keywords("rule if {}")
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Empty object following `if`",
+		"level": "error",
+		"location": {"col": 1, "file": "policy.rego", "row": 8, "text": "rule if {}"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/if-empty-object", "bugs"),
+		}],
+		"title": "if-empty-object",
+	}}
+}
+
+test_success_if_non_empty_object if {
+	# this is arguably just as useless, but we'll defer
+	# to the constant-condition rule for these cases
+	module := ast.with_future_keywords(`rule if {"foo": "bar"}`)
+	r := rule.report with input as module
+	r == set()
+}

bundle/regal/rules/bugs/unused_return_value_test.rego

@@ -3,14 +3,15 @@ package regal.rules.bugs["unused-return-value_test"]
 import future.keywords.if
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 import data.regal.rules.bugs["unused-return-value"] as rule
 
 test_fail_unused_return_value if {
 	r := rule.report with input as ast.with_future_keywords(`allow {
 		indexof("s", "s")
 	}`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "bugs",
 		"description": "Non-boolean return value unused",
@@ -26,18 +27,18 @@ test_fail_unused_return_value if {
 
 test_success_unused_boolean_return_value if {
 	r := rule.report with input as ast.with_future_keywords(`allow { startswith("s", "s") }`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }
 
 test_success_return_value_assigned if {
 	r := rule.report with input as ast.with_future_keywords(`allow { x := indexof("s", "s") }`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }
 
 test_success_function_arg_return_ignored if {
-	r := rule.report with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+	r := rule.report with data.internal.combined_config as {"capabilities": capabilities.provided}
 		with input as ast.with_future_keywords(`allow {
 		indexof("s", "s", i)
 	}`)

bundle/regal/rules/custom/forbidden_function_call_test.rego

@@ -15,7 +15,7 @@ test_fail_forbidden_function if {
 		"level": "error",
 		"forbidden-functions": ["http.send"],
 	}
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": data.regal.capabilities.provided}
 
 	r == {{
 		"category": "custom",

bundle/regal/rules/custom/one_liner_rule.rego

@@ -7,6 +7,7 @@ import future.keywords.if
 import future.keywords.in
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 import data.regal.result
 
@@ -16,12 +17,7 @@ cfg := config.for_rule("custom", "one-liner-rule")
 # description: Missing capability for keyword `if`
 # custom:
 #   severity: warning
-notices contains result.notice(rego.metadata.chain()) if not has_if
-
-# if if if!
-has_if if "if" in config.capabilities.future_keywords
-
-has_if if "rego_v1_import" in config.capabilities.features
+notices contains result.notice(rego.metadata.chain()) if not capabilities.has_if
 
 report contains violation if {
 	# No need to traverse rules here if we're not importing `if`

bundle/regal/rules/idiomatic/non_raw_regex_pattern_test.rego

@@ -3,13 +3,14 @@ package regal.rules.idiomatic["non-raw-regex-pattern_test"]
 import future.keywords.if
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 
 import data.regal.rules.idiomatic["non-raw-regex-pattern"] as rule
 
 test_fail_non_raw_rule_head if {
 	r := rule.report with input as ast.policy(`x := regex.match("[0-9]+", "1")`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "idiomatic",
 		"description": "Use raw strings for regex patterns",
@@ -27,7 +28,7 @@ test_fail_non_raw_rule_body if {
 	r := rule.report with input as ast.policy(`allow {
 		regex.is_valid("[0-9]+")
 	}`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "idiomatic",
 		"description": "Use raw strings for regex patterns",
@@ -43,7 +44,7 @@ test_fail_non_raw_rule_body if {
 
 test_fail_pattern_in_second_arg if {
 	r := rule.report with input as ast.policy(`r := regex.replace("a", "[a]", "b")`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "idiomatic",
 		"description": "Use raw strings for regex patterns",
@@ -59,6 +60,6 @@ test_fail_pattern_in_second_arg if {
 
 test_success_when_using_raw_string if {
 	r := rule.report with input as ast.policy("v := regex.is_valid(`[0-9]+`)")
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }

bundle/regal/rules/imports/import_shadows_builtin_test.rego

@@ -4,6 +4,7 @@ import future.keywords.if
 import future.keywords.in
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 
 import data.regal.rules.imports["import-shadows-builtin"] as rule
@@ -12,7 +13,7 @@ test_fail_import_shadows_builtin_name if {
 	module := ast.policy(`import data.print`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "imports",
 		"description": "Import shadows built-in namespace",
@@ -30,7 +31,7 @@ test_fail_import_shadows_builtin_namespace if {
 	module := ast.policy(`import input.foo.http`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "imports",
 		"description": "Import shadows built-in namespace",
@@ -48,14 +49,14 @@ test_success_import_does_not_shadows_builtin_name if {
 	module := ast.policy(`import data.users`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }
 
 test_success_import_shadows_but_alias_does_not if {
 	module := ast.policy(`import data.http as http_attributes`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }

bundle/regal/rules/style/function_arg_return_test.rego

@@ -3,13 +3,14 @@ package regal.rules.style["function-arg-return_test"]
 import future.keywords.if
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 import data.regal.rules.style["function-arg-return"] as rule
 
 test_fail_function_arg_return_value if {
 	r := rule.report with input as ast.policy(`foo := i { indexof("foo", "o", i) }`)
 		with config.for_rule as {"level": "error"}
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "style",
 		"description": "Function argument used for return value",
@@ -26,7 +27,7 @@ test_fail_function_arg_return_value if {
 test_fail_function_arg_return_value_multi_part_ref if {
 	r := rule.report with input as ast.policy(`foo := r { regex.match("foo", "foo", r) }`)
 		with config.for_rule as {"level": "error"}
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "style",
 		"description": "Function argument used for return value",
@@ -46,6 +47,6 @@ test_success_function_arg_return_value_except_function if {
 			"level": "error",
 			"except-functions": ["indexof"],
 		}
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == set()
 }

bundle/regal/rules/testing/dubious_print_sprintf_test.rego

@@ -4,6 +4,7 @@ import future.keywords.if
 import future.keywords.in
 
 import data.regal.ast
+import data.regal.capabilities
 import data.regal.config
 
 import data.regal.rules.testing["dubious-print-sprintf"] as rule
@@ -14,7 +15,7 @@ test_fail_print_sprintf if {
 	}`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "testing",
 		"description": "Dubious use of print and sprintf",
@@ -40,7 +41,7 @@ test_fail_bodies_print_sprintf if {
 	}`)
 
 	r := rule.report with input as module
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": capabilities.provided}
 	r == {{
 		"category": "testing",
 		"description": "Dubious use of print and sprintf",

bundle/regal/rules/testing/print_or_trace_call_test.rego

@@ -14,7 +14,7 @@ test_fail_call_to_print_and_trace if {
 
 		x := [i | i = 0; trace("bar")]
 	}`)
-		with data.internal.combined_config as {"capabilities": data.regal.capabilities}
+		with data.internal.combined_config as {"capabilities": data.regal.capabilities.provided}
 	r == {
 		expected_with_location({
 			"col": 3,

docs/rules/bugs/if-empty-object.md

@@ -0,0 +1,42 @@
+# if-empty-object
+
+**Summary**: Empty object following `if`
+
+**Category**: Bugs
+
+**Avoid**
+```rego
+package policy
+
+import future.keywords.if
+
+allow if {}
+```
+
+## Rationale
+
+An empty rule body would previously be flagged as an error. With the introduction and use of the `if` keyword, that is
+no longer the case. In fact, empty `{}` is not considered a rule body _at all_, but rather an empty object, meaning
+that `if {}` will always evaluate. This is likely a mistake, and while hopefully caught by tests, should be avoided.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  bugs:
+    if-empty-object:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Related Resources
+
+- Regal Docs: [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition)
+
+## 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

@@ -66,6 +66,8 @@ inconsistent_args(b, a) {
 	b == a
 }
 
+if_empty_object if {}
+
 ### Idiomatic ###
 
 custom_has_key_construct(map, key) {