Have redundant-existence-check include function args (#1279)

Fixes #1226

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

bundle/regal/result/result.rego

@@ -196,7 +196,7 @@ _with_text(loc_obj) := loc if {
 # METADATA
 # description: |
 #   returns a "normalized" location object from the location value found in the AST.
-#   new code should most often use one of the ranged_ location functions instea, as
+#   new code should most often use one of the ranged_ location functions instead, as
 #   that will also include an `"end"` location attribute
 # scope: document
 location(x) := _with_text(util.to_location_object(x.location))

bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check.rego

@@ -29,6 +29,27 @@ report contains violation if {
 	violation := result.fail(rego.metadata.chain(), result.ranged_from_ref(expr.terms.value))
 }
 
+# METADATA
+# description: |
+#  check for redundant existence checks of function args in function bodies
+#  note: this only scans "top level" expressions in the function body, and not
+#  e.g. those nested inside of comprehensions, every bodies, etc.. while this
+#  would certainly be possible, the cost does not justify the benefit, as it's
+#  quite unlikely that existence checks are found there
+report contains violation if {
+	some func in ast.functions
+	some expr in func.body
+
+	expr.terms.type == "var"
+
+	some arg in func.head.args
+
+	arg.type == "var"
+	arg.value == expr.terms.value
+
+	violation := result.fail(rego.metadata.chain(), result.location(expr.terms))
+}
+
 # METADATA
 # description: check for redundant existence checks in rule head assignment
 report contains violation if {

bundle/regal/rules/bugs/redundant-existence-check/redundant_existence_check_test.rego

@@ -66,3 +66,22 @@ test_fail_redundant_existence_check_head_assignment_of_ref if {
 		"title": "redundant-existence-check",
 	}}
 }
+
+test_fail_redundant_existence_check_function_arg if {
+	module := ast.with_rego_v1(`
+	fun(foo) if {
+		foo
+	}`)
+	r := rule.report with input as module
+	r == {{
+		"category": "bugs",
+		"description": "Redundant existence check",
+		"level": "error",
+		"location": {"col": 3, "end": {"col": 6, "row": 7}, "file": "policy.rego", "row": 7, "text": "\t\tfoo"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/redundant-existence-check", "bugs"),
+		}],
+		"title": "redundant-existence-check",
+	}}
+}

docs/rules/bugs/redundant-existence-check.md

@@ -14,6 +14,11 @@ employee if {
     input.user.email
     endswith(input.user.email, "@acmecorp.com")
 }
+
+is_admin(user) if {
+    user
+    "admin" in user.roles
+}
 ```
 
 **Prefer**
@@ -29,6 +34,10 @@ employee if {
 # alternatively
 
 employee if endswith(input.user.email, "@acmecorp.com")
+
+is_admin(user) if {
+    "admin" in user.roles
+}
 ```
 
 ## Rationale
@@ -42,6 +51,29 @@ variable.
 used later in the rule, it won't be flagged. While the existence check _could_ be redundant even in that case, it could
 also be used to avoid making some expensive computation, an `http.send` call, or whatnot.
 
+## Exceptions
+
+Function arguments where a boolean value is expected will be flagged as redundant existence checks, even though the
+intent was to check the boolean condition.
+
+```rego
+report(user, is_admin) if {
+    is_admin
+
+    # more conditions
+}
+```
+
+For these cases, prefer to be explicit about what the assertion is checking:
+
+```rego
+report(user, is_admin) if {
+    is_admin == false # or true, != false, etc.
+
+    # more conditions
+}
+```
+
 ## Configuration Options
 
 This linter rule provides the following configuration options:

pkg/linter/linter.go

@@ -699,23 +699,19 @@ func (l Linter) paramsToRulesConfig() map[string]any {
 }
 
 func (l Linter) prepareRegoArgs(query ast.Body) ([]func(*rego.Rego), error) {
-	var regoArgs []func(*rego.Rego)
-
-	roots := []string{"eval"}
-
 	dataBundle := bundle.Bundle{
 		Data:     l.paramsToRulesConfig(),
-		Manifest: bundle.Manifest{Roots: &roots},
+		Manifest: bundle.Manifest{Roots: &[]string{"eval"}},
 	}
 
-	regoArgs = append(regoArgs,
+	regoArgs := []func(*rego.Rego){
 		rego.StoreReadAST(true),
 		rego.Metrics(l.metrics),
 		rego.ParsedQuery(query),
 		rego.ParsedBundle("regal_eval_params", &dataBundle),
 		rego.Function2(builtins.RegalParseModuleMeta, builtins.RegalParseModule),
 		rego.Function1(builtins.RegalLastMeta, builtins.RegalLast),
-	)
+	}
 
 	if l.debugMode && l.printHook == nil {
 		l.printHook = topdown.NewPrintHook(os.Stderr)