Rule: use-if and use-contains (#470)

Recommend the use of `if` and `contains` when available
as capabilities.

Some fixes to tests where we didn't live up to this
standard ourselves. Great!

Fixes #468

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

README.md

@@ -204,6 +204,8 @@ The following rules are currently available:
 | idiomatic | [no-defined-entrypoint](https://docs.styra.com/regal/rules/idiomatic/no-defined-entrypoint)         | Missing entrypoint annotation                             |
 | idiomatic | [non-raw-regex-pattern](https://docs.styra.com/regal/rules/idiomatic/non-raw-regex-pattern)         | Use raw strings for regex patterns                        |
 | idiomatic | [prefer-set-or-object-rule](https://docs.styra.com/regal/rules/idiomatic/prefer-set-or-object-rule) | Prefer set or object rule over comprehension              |
+| idiomatic | [use-contains](https://docs.styra.com/regal/rules/idiomatic/use-contains)                           | Use the `contains` keyword                                |
+| idiomatic | [use-if](https://docs.styra.com/regal/rules/idiomatic/use-if)                                       | Use the `if` keyword                                      |
 | idiomatic | [use-in-operator](https://docs.styra.com/regal/rules/idiomatic/use-in-operator)                     | Use in to check for membership                            |
 | idiomatic | [use-some-for-output-vars](https://docs.styra.com/regal/rules/idiomatic/use-some-for-output-vars)   | Use `some` to declare output variables                    |
 | imports   | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input)           | Avoid importing input                                     |

bundle/regal/capabilities.rego

@@ -19,4 +19,10 @@ has_object_keys if "object.keys" in object.keys(config.capabilities.builtins)
 # if if if!
 has_if if "if" in config.capabilities.future_keywords
 
-has_if if "rego_v1_import" in config.capabilities.features
+has_if if has_rego_v1_feature
+
+has_contains if "contains" in config.capabilities.future_keywords
+
+has_contains if has_rego_v1_feature
+
+has_rego_v1_feature if "rego_v1_import" in config.capabilities.features

bundle/regal/config/config_test.rego

@@ -1,5 +1,7 @@
 package regal.config_test
 
+import future.keywords.if
+
 import data.regal.config
 
 rules_config := {"rules": {"test": {"test-case": {
@@ -18,28 +20,28 @@ params := {
 
 # disable all
 
-test_disable_all_no_config {
+test_disable_all_no_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable_all": true})
 
 	c == {"level": "ignore"}
 }
 
-test_disable_all_with_config {
+test_disable_all_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable_all": true})
 		with config.merged_config as rules_config
 
 	c == {"level": "ignore", "important_setting": 42}
 }
 
-test_disable_all_with_category_override {
+test_disable_all_with_category_override if {
 	p := object.union(params, {"disable_all": true, "enable_category": ["test"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
 
 	c == {"level": "error", "important_setting": 42}
 }
 
-test_disable_all_with_rule_override {
+test_disable_all_with_rule_override if {
 	p := object.union(params, {"disable_all": true, "enable": ["test-case"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
@@ -49,20 +51,20 @@ test_disable_all_with_rule_override {
 
 # disable category
 
-test_disable_category_no_config {
+test_disable_category_no_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable_category": ["test"]})
 
 	c == {"level": "ignore"}
 }
 
-test_disable_category_with_config {
+test_disable_category_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable_category": ["test"]})
 		with config.merged_config as rules_config
 
 	c == {"level": "ignore", "important_setting": 42}
 }
 
-test_disable_category_with_rule_override {
+test_disable_category_with_rule_override if {
 	p := object.union(params, {"disable_category": ["test"], "enable": ["test-case"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
@@ -72,13 +74,13 @@ test_disable_category_with_rule_override {
 
 # disable rule
 
-test_disable_single_rule {
+test_disable_single_rule if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable": ["test-case"]})
 
 	c == {"level": "ignore"}
 }
 
-test_disable_single_rule_with_config {
+test_disable_single_rule_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"disable": ["test-case"]})
 		with config.merged_config as rules_config
 
@@ -87,28 +89,28 @@ test_disable_single_rule_with_config {
 
 # enable all
 
-test_enable_all_no_config {
+test_enable_all_no_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable_all": true})
 
 	c == {"level": "error"}
 }
 
-test_enable_all_with_config {
+test_enable_all_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable_all": true})
 		with config.merged_config as rules_config
 
 	c == {"level": "error", "important_setting": 42}
 }
 
-test_enable_all_with_category_override {
+test_enable_all_with_category_override if {
 	p := object.union(params, {"enable_all": true, "disable_category": ["test"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
 
 	c == {"level": "ignore", "important_setting": 42}
 }
 
-test_enable_all_with_rule_override {
+test_enable_all_with_rule_override if {
 	p := object.union(params, {"enable_all": true, "disable": ["test-case"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
@@ -118,20 +120,20 @@ test_enable_all_with_rule_override {
 
 # disable category
 
-test_enable_category_no_config {
+test_enable_category_no_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable_category": ["test"]})
 
 	c == {"level": "error"}
 }
 
-test_enable_category_with_config {
+test_enable_category_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable_category": ["test"]})
 		with config.merged_config as rules_config
 
 	c == {"level": "error", "important_setting": 42}
 }
 
-test_enable_category_with_rule_override {
+test_enable_category_with_rule_override if {
 	p := object.union(params, {"enable_category": ["test"], "disable": ["test-case"]})
 	c := config.for_rule("test", "test-case") with data.eval.params as p
 		with config.merged_config as rules_config
@@ -141,20 +143,20 @@ test_enable_category_with_rule_override {
 
 # enable rule
 
-test_enable_single_rule {
+test_enable_single_rule if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable": ["test-case"]})
 
 	c == {"level": "error"}
 }
 
-test_enable_single_rule_with_config {
+test_enable_single_rule_with_config if {
 	c := config.for_rule("test", "test-case") with data.eval.params as object.union(params, {"enable": ["test-case"]})
 		with config.merged_config as rules_config
 
 	c == {"level": "error", "important_setting": 42}
 }
 
-test_all_rules_are_in_provided_configuration {
+test_all_rules_are_in_provided_configuration if {
 	missing_config := {title |
 		some category, title
 		data.regal.rules[category][title]
@@ -165,7 +167,7 @@ test_all_rules_are_in_provided_configuration {
 	count(missing_config) == 0
 }
 
-test_all_configured_rules_exist {
+test_all_configured_rules_exist if {
 	go_rules := {"opa-fmt"}
 
 	missing_rules := {title |

bundle/regal/config/exclusion_test.rego

@@ -1,5 +1,7 @@
 package regal.config_test
 
+import future.keywords.if
+
 import data.regal.config
 
 # map[pattern: string]map[filename: string]excluded: bool
@@ -42,7 +44,7 @@ cases := {
 	},
 }
 
-test_all_cases_are_as_expected {
+test_all_cases_are_as_expected if {
 	not_exp := {pattern: res |
 		subcases := cases[pattern]
 		res := {file: res1 |
@@ -63,34 +65,34 @@ rules_config_ignore_delta := {"rules": {"test": {"test-case": {"ignore": {"files
 
 config_ignore := {"ignore": {"files": ["p.rego"]}}
 
-test_excluded_file_default {
+test_excluded_file_default if {
 	e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params
 		with config.merged_config as rules_config_error
 
 	e == false
 }
 
-test_excluded_file_with_ignore {
+test_excluded_file_with_ignore if {
 	c := object.union(rules_config_error, rules_config_ignore_delta)
 	e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params
 		with config.merged_config as c
 	e == true
 }
 
-test_excluded_file_config {
+test_excluded_file_config if {
 	e := config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore
 	e == true
 }
 
-test_excluded_file_cli_flag {
+test_excluded_file_cli_flag if {
 	e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as object.union(
 		params,
 		{"ignore_files": ["p.rego"]},
 	)
 	e == true
 }
 
-test_excluded_file_cli_overrides_config {
+test_excluded_file_cli_overrides_config if {
 	e := config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore
 		with data.eval.params as object.union(params, {"ignore_files": [""]})
 	e == false

bundle/regal/config/provided/data.yaml

@@ -44,6 +44,10 @@ rules:
       level: error
     prefer-set-or-object-rule:
       level: error
+    use-contains:
+      level: error
+    use-if:
+      level: error
     use-in-operator:
       level: error
     use-some-for-output-vars:

bundle/regal/rules/idiomatic/use_contains.rego

@@ -0,0 +1,30 @@
+# METADATA
+# description: Use the `contains` keyword
+package regal.rules.idiomatic["use-contains"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.capabilities
+import data.regal.result
+
+# METADATA
+# description: Missing capability for keyword `contains`
+# custom:
+#   severity: warning
+notices contains result.notice(rego.metadata.chain()) if not capabilities.has_contains
+
+report contains violation if {
+	some rule in ast.rules
+
+	rule.head.key
+	not rule.head.value
+
+	text := base64.decode(rule.head.location.text)
+
+	not contains(text, " contains ")
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule))
+}

bundle/regal/rules/idiomatic/use_contains_test.rego

@@ -0,0 +1,47 @@
+package regal.rules.idiomatic["use-contains_test"]
+
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.idiomatic["use-contains"] as rule
+
+test_fail_should_use_contains if {
+	module := ast.with_future_keywords(`rule[item] {
+		some item in input.items
+	}`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "idiomatic",
+		"description": "Use the `contains` keyword",
+		"level": "error",
+		"location": {"col": 1, "file": "policy.rego", "row": 8, "text": "rule[item] {"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/use-contains", "idiomatic"),
+		}],
+		"title": "use-contains",
+	}}
+}
+
+test_success_uses_contains if {
+	module := ast.with_future_keywords(`rule contains item if {
+		some item in input.items
+	}`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_object_rule if {
+	module := ast.with_future_keywords(`rule[foo] := bar if {
+		foo := "bar"
+		bar := "baz"
+	}`)
+
+	r := rule.report with input as module
+	r == set()
+}

bundle/regal/rules/idiomatic/use_if.rego

@@ -0,0 +1,34 @@
+# METADATA
+# description: Use the `if` keyword
+package regal.rules.idiomatic["use-if"]
+
+import future.keywords.contains
+import future.keywords.if
+import future.keywords.in
+
+import data.regal.ast
+import data.regal.capabilities
+import data.regal.result
+
+# Note: think more about what UX we want when import_rego_v1
+# capbility is available. Should we simply just recommend that
+# and silence this rule in that case? I'm inclined to say yes.
+
+# 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
+
+	not ast.generated_body(rule)
+
+	head_len := count(base64.decode(rule.head.location.text))
+	text := trim_space(substring(base64.decode(rule.location.text), head_len, -1))
+
+	not startswith(text, "if")
+
+	violation := result.fail(rego.metadata.chain(), result.location(rule))
+}