Singe file impossible-not (#713)

Improve this new rule to have the `report` rule handle single file violations, while the aggregate report ignores violations where the `not` ref and the partial rule is in the same file. Signed-off-by: Anders Eknert <[email protected]>
StyraInc · May 13, 2024 · e225bda · e225bda
1 parent 2023f32
commit e225bda
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 60 deletions.
diff --git a/bundle/regal/ast/imports.rego b/bundle/regal/ast/imports.rego
@@ -20,6 +20,19 @@ imported_identifiers contains imported_identifier(imp) if {
 	imp.path.value[0].value in {"input", "data"}
 }
 
+# METADATA
+# description: |
+#   map of all imported paths in the input module, keyed by their identifier or "namespace"
+resolved_imports[identifier] := path if {
+	some _import in imports
+
+	_import.path.value[0].value == "data"
+	count(_import.path.value) > 1
+
+	identifier := imported_identifier(_import)
+	path := [part.value | some part in _import.path.value]
+}
+
 imported_identifier(imp) := imp.alias
 
 imported_identifier(imp) := regal.last(imp.path.value).value if not imp.alias

diff --git a/bundle/regal/rules/bugs/impossible_not.rego b/bundle/regal/rules/bugs/impossible_not.rego
@@ -7,85 +7,93 @@ 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]
+package_path := [part.value | some part in input["package"].path]
 
-	imported_symbols := {symbol: path |
-		some _import in input.imports
+multivalue_rules contains path if {
+	some rule in ast.rules
 
-		_import.path.value[0].value == "data"
-		count(_import.path.value) > 1
+	rule.head.key
+	not rule.head.value
 
-		symbol := imported_symbol(_import)
-		path := [part.value | some part in _import.path.value]
+	# ignore general ref head rules for now
+	every path in array.slice(rule.head.ref, 1, count(rule.head.ref)) {
+		path.type == "string"
 	}
 
-	multivalue_rules := {path |
-		some rule in ast.rules
+	path := concat(".", array.concat(package_path, [p |
+		some ref in rule.head.ref
+		p := ref.value
+	]))
+}
 
-		rule.head.key
-		not rule.head.value
+negated_refs contains negated_ref if {
+	some i, rule in input.rules
 
-		# ignore general ref head rules for now
-		every path in array.slice(rule.head.ref, 1, count(rule.head.ref)) {
-			path.type == "string"
-		}
+	walk(rule, [_, value])
 
-		path := concat(".", array.concat(package_path, [p |
-			some ref in rule.head.ref
-			p := ref.value
-		]))
-	}
+	value.negated
 
-	negated_refs := [negated_ref |
-		some i, rule in input.rules
+	# 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"}
 
-		walk(rule, [_, value])
+	ref := var_to_ref(value.terms)
 
-		value.negated
+	# for now, ignore ref if it has variable components
+	every path in array.slice(ref, 1, count(ref)) {
+		path.type == "string"
+	}
 
-		# 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"}
+	# 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)}
 
-		ref := var_to_ref(value.terms)
+	negated_ref := {
+		"ref": ref,
+		"resolved_path": resolve(ref, package_path, ast.resolved_imports),
+	}
+}
 
-		# for now, ignore ref if it has variable components
-		every path in array.slice(ref, 1, count(ref)) {
-			path.type == "string"
-		}
+aggregate contains result.aggregate(rego.metadata.chain(), {
+	"imported_symbols": ast.resolved_imports,
+	"multivalue_rules": multivalue_rules,
+	"negated_refs": negated_refs,
+})
 
-		# 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)}
+report contains violation if {
+	some entry in aggregate
+	some negated in entry.aggregate_data.negated_refs
+
+	negated.resolved_path in entry.aggregate_data.multivalue_rules
 
-		negated_ref := {
-			"ref": ref,
-			"resolved_path": resolve(ref, package_path, imported_symbols),
-		}
-	]
+	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)]),
+	}})
 
-	entry := result.aggregate(rego.metadata.chain(), {
-		"imported_symbols": imported_symbols,
-		"multivalue_rules": multivalue_rules,
-		"negated_refs": negated_refs,
-	})
+	violation := result.fail(rego.metadata.chain(), loc)
 }
 
 # METADATA
 # schemas:
 #   - input: schema.regal.aggregate
 aggregate_report contains violation if {
-	all_multivalue_refs := {path |
+	all_multivalue_refs := {{"path": path, "file": entry.aggregate_source.file} |
 		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
+	some multivalue_ref in all_multivalue_refs
+
+	# violations in same file handled by non-aggregate "report"
+	multivalue_ref.file != entry.aggregate_source.file
 
-	negated.resolved_path in all_multivalue_refs
+	negated.resolved_path == multivalue_ref.path
 
 	loc := object.union(result.location(negated.ref), {"location": {
 		"file": entry.aggregate_source.file,
@@ -102,10 +110,6 @@ 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

diff --git a/bundle/regal/rules/bugs/impossible_not_test.rego b/bundle/regal/rules/bugs/impossible_not_test.rego
@@ -118,6 +118,38 @@ test_success_multivalue_not_reference_invalidated_by_function_argument if {
 	r == set()
 }
 
+test_success_multivalue_not_reference_in_same_file_not_reported_in_aggregate_report if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+
+	test_partial if {
+		not partial
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": agg1}
+	r == set()
+}
+
+test_fail_multivalue_not_reference_in_same_file_reported_in_normal_report if {
+	module := regal.parse_module("p1.rego", `package foo
+
+	import rego.v1
+
+	partial contains "foo"
+
+	test_partial if {
+		not partial
+	}
+	`)
+
+	r := rule.report with input as module
+	r == expected_with_location({"col": 7, "file": "p1.rego", "row": 8, "text": "not partial"})
+}
+
 expected := {
 	"category": "bugs",
 	"description": "Impossible `not` condition",
@@ -130,9 +162,3 @@ expected := {
 }
 
 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)
-}
diff --git a/docs/rules/bugs/impossible-not.md b/docs/rules/bugs/impossible-not.md
@@ -4,7 +4,7 @@
 
 **Category**: Bugs
 
-**Type**: Aggregate - only runs when more than one file is provided for linting
+**Type**: Aggregate - runs both on single files as well as when more than one file is provided for linting
 
 **Avoid**
 ```rego