Rule: impossible-not (StyraInc#698)

See docs in PR (and issue) for further details.

Fixes StyraInc#603

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

README.md

@@ -198,6 +198,7 @@ The following rules are currently available:
 | bugs        | [deprecated-builtin](https://docs.styra.com/regal/rules/bugs/deprecated-builtin)                      | Avoid using deprecated built-in functions                 |
 | bugs        | [duplicate-rule](https://docs.styra.com/regal/rules/bugs/duplicate-rule)                              | Duplicate rule                                            |
 | bugs        | [if-empty-object](https://docs.styra.com/regal/rules/bugs/if-empty-object)                            | Empty object following `if`                               |
+| bugs        | [impossible-not](https://docs.styra.com/regal/rules/bugs/impossible-not)                              | Impossible `not` condition                                |
 | 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/config/provided/data.yaml

@@ -8,6 +8,8 @@ rules:
       level: error
     if-empty-object:
       level: error
+    impossible-not:
+      level: error
     inconsistent-args:
       level: error
     invalid-metadata-attribute:

bundle/regal/rules/bugs/impossible_not.rego

@@ -0,0 +1,136 @@
+# METADATA
+# description: Impossible `not` condition
+package regal.rules.bugs["impossible-not"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+# regal ignore:rule-length
+aggregate contains entry if {
+	package_path := [part.value | some part in input["package"].path]
+
+	imported_symbols := {symbol: path |
+		some _import in input.imports
+
+		_import.path.value[0].value == "data"
+		count(_import.path.value) > 1
+
+		symbol := imported_symbol(_import)
+		path := [part.value | some part in _import.path.value]
+	}
+
+	multivalue_rules := {path |
+		some rule in ast.rules
+
+		rule.head.key
+		not rule.head.value
+
+		# ignore general ref head rules for now
+		every path in array.slice(rule.head.ref, 1, count(rule.head.ref)) {
+			path.type == "string"
+		}
+
+		path := concat(".", array.concat(package_path, [p |
+			some ref in rule.head.ref
+			p := ref.value
+		]))
+	}
+
+	negated_refs := [negated_ref |
+		some i, rule in input.rules
+
+		walk(rule, [_, value])
+
+		value.negated
+
+		# if terms is an array, it's a function call, and most likely not "impossible"
+		is_object(value.terms)
+		value.terms.type in {"ref", "var"}
+
+		ref := var_to_ref(value.terms)
+
+		# for now, ignore ref if it has variable components
+		every path in array.slice(ref, 1, count(ref)) {
+			path.type == "string"
+		}
+
+		# ignore negated local vars
+		not ref[0].value in ast.function_arg_names(rule)
+		not ref[0].value in {var.value | some var in ast.find_vars_in_local_scope(rule, value.location)}
+
+		negated_ref := {
+			"ref": ref,
+			"resolved_path": resolve(ref, package_path, imported_symbols),
+		}
+	]
+
+	entry := result.aggregate(rego.metadata.chain(), {
+		"imported_symbols": imported_symbols,
+		"multivalue_rules": multivalue_rules,
+		"negated_refs": negated_refs,
+	})
+}
+
+# METADATA
+# schemas:
+#   - input: schema.regal.aggregate
+aggregate_report contains violation if {
+	all_multivalue_refs := {path |
+		some entry in input.aggregate
+		some path in entry.aggregate_data.multivalue_rules
+	}
+
+	some entry in input.aggregate
+	some negated in entry.aggregate_data.negated_refs
+
+	negated.resolved_path in all_multivalue_refs
+
+	loc := object.union(result.location(negated.ref), {"location": {
+		"file": entry.aggregate_source.file,
+		# note that the "not" isn't present in the AST, so we'll add it manually to the text
+		# in the location to try and make it clear where the issue is (as opposed to just
+		# printing the ref)
+		"text": sprintf("not %s", [to_string(negated.ref)]),
+	}})
+
+	violation := result.fail(rego.metadata.chain(), loc)
+}
+
+var_to_ref(terms) := [terms] if terms.type == "var"
+
+var_to_ref(terms) := terms.value if terms.type == "ref"
+
+imported_symbol(imp) := imp.alias
+
+imported_symbol(imp) := regal.last(imp.path.value).value if not imp.alias
+
+to_string(ref) := concat(".", [path |
+	some part in ref
+	path := part.value
+])
+
+resolve(ref, _, _) := to_string(ref) if ref[0].value == "data"
+
+# imported symbol
+resolve(ref, _, imported_symbols) := concat(".", resolved) if {
+	ref[0].value != "data"
+
+	resolved := array.concat(
+		imported_symbols[ref[0].value],
+		[part.value | some part in array.slice(ref, 1, count(ref))],
+	)
+}
+
+# not imported — must be local or package
+resolve(ref, pkg_path, imported_symbols) := concat(".", resolved) if {
+	ref[0].value != "data"
+
+	not imported_symbols[ref[0].value]
+
+	resolved := array.concat(
+		pkg_path,
+		[part.value | some part in ref],
+	)
+}

bundle/regal/rules/bugs/impossible_not_test.rego

@@ -0,0 +1,138 @@
+package regal.rules.bugs["impossible-not_test"]
+
+import rego.v1
+
+import data.regal.rules.bugs["impossible-not"] as rule
+
+test_fail_multivalue_not_reference_same_package if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package foo
+
+	import rego.v1
+
+	test_foo if {
+		not partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == expected_with_location({"col": 7, "file": "p2.rego", "row": 6, "text": "not partial"})
+}
+
+test_fail_multivalue_not_reference_different_package_using_direct_reference if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	import rego.v1
+
+	test_foo if {
+		not data.foo.partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == expected_with_location({"col": 7, "file": "p2.rego", "row": 6, "text": "not data.foo.partial"})
+}
+
+test_fail_multivalue_not_reference_different_package_using_import if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+
+	another contains "bar"
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	import rego.v1
+
+	import data.foo
+
+	test_foo if {
+		not foo.partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == expected_with_location({"col": 7, "file": "p2.rego", "row": 8, "text": "not foo.partial"})
+}
+
+test_success_multivalue_not_reference_invalidated_by_local_var if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	import rego.v1
+
+	import data.foo
+
+	test_foo if {
+		foo := input.bar
+		not foo.partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+test_success_multivalue_not_reference_invalidated_by_function_argument if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	import rego.v1
+
+	import data.foo
+
+	my_function(foo) if {
+		not foo.partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+expected := {
+	"category": "bugs",
+	"description": "Impossible `not` condition",
+	"level": "error",
+	"related_resources": [{
+		"description": "documentation",
+		"ref": "https://docs.styra.com/regal/rules/bugs/impossible-not",
+	}],
+	"title": "impossible-not",
+}
+
+expected_with_location(location) := {object.union(expected, {"location": location})} if is_object(location)
+
+expected_with_location(location) := {object.union(expected, {"location": loc}) |
+	some loc in location
+} if {
+	is_set(location)
+}

bundle/regal/rules/imports/unresolved_import.rego

@@ -46,7 +46,6 @@ aggregate contains entry if {
 aggregate_report contains violation if {
 	all_known_refs := {path |
 		some entry in input.aggregate
-
 		some path in entry.aggregate_data.exported_refs
 	}
 

docs/rules/bugs/impossible-not.md

@@ -0,0 +1,85 @@
+# impossible-not
+
+**Summary**: Impossible `not` condition
+
+**Category**: Bugs
+
+**Type**: Aggregate - only runs when more than one file is provided for linting
+
+**Avoid**
+```rego
+package policy
+
+import rego.v1
+
+report contains violation if {
+    # ... some conditions
+}
+```
+
+```rego
+package policy_test
+
+import rego.v1
+
+import data.policy
+
+test_report_is_empty {
+    # evaluation will stop here, as even an empty set is "true"
+    not policy.report
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import rego.v1
+
+report contains violation if {
+    # ... some conditions
+}
+```
+
+```rego
+package policy_test
+
+import rego.v1
+
+import data.policy
+
+test_report_is_empty {
+    count(policy.report) == 0
+}
+```
+
+## Rationale
+
+The `not` keyword negates the expression that follows it. A common mistake, especially in tests, is to use `not`
+to test the result of evaluating a partial (i.e. multi-value) rule. However, as even an empty set is considered
+"truthy", the `not` will in that case always evaluate to `false`. There are more cases where `not` is impossible,
+or a [constant condition](https://docs.styra.com/regal/rules/bugs/constant-condition), but references to partial
+rules are by far the most common. For tests where you want to assert the set is empty or has a specific number of
+items, use the built-in `count` function instead.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  bugs:
+    impossible-not:
+      # 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

@@ -83,6 +83,12 @@ redundant_existence_check if {
 
 deprecated_builtin := all([true])
 
+partial contains "foo"
+
+impossible_not if {
+	not partial
+}
+
 ### Idiomatic ###
 
 custom_has_key_construct(map, key) if {